]> git.ipfire.org Git - thirdparty/pdns.git/commitdiff
dnsdist: Send a NotImp answer on empty (qdcount=0) queries 9991/head
authorRemi Gacogne <remi.gacogne@powerdns.com>
Tue, 19 Jan 2021 15:33:21 +0000 (16:33 +0100)
committerRemi Gacogne <remi.gacogne@powerdns.com>
Tue, 19 Jan 2021 15:33:21 +0000 (16:33 +0100)
pdns/dnsdist-console.cc
pdns/dnsdist-lua.cc
pdns/dnsdist-tcp.cc
pdns/dnsdist.cc
pdns/dnsdistdist/docs/reference/config.rst
pdns/dnsdistdist/doh.cc
regression-tests.dnsdist/test_Advanced.py
regression-tests.dnsdist/test_Basics.py

index 5183140487d88bd2b4643dada3fdaf4822c68194..19ec4a3dafb7a111a6afc5a415d70f176320c444 100644 (file)
@@ -535,6 +535,7 @@ const std::vector<ConsoleKeyword> g_consoleKeywords{
   { "setDefaultBPFFilter", true, "filter", "When used at configuration time, the corresponding BPFFilter will be attached to every bind" },
   { "setDynBlocksAction", true, "action", "set which action is performed when a query is blocked. Only DNSAction.Drop (the default) and DNSAction.Refused are supported" },
   { "setDynBlocksPurgeInterval", true, "sec", "set how often the expired dynamic block entries should be removed" },
+  { "setDropEmptyQueries", true, "drop", "Whether to drop empty queries right away instead of sending a NOTIMP response" },
   { "SetECSAction", true, "v4[, v6]", "Set the ECS prefix and prefix length sent to backends to an arbitrary value" },
   { "setECSOverride", true, "bool", "whether to override an existing EDNS Client Subnet value in the query" },
   { "setECSSourcePrefixV4", true, "prefix-length", "the EDNS Client Subnet prefix-length used for IPv4 queries" },
index 3ae46a3521854db969aa4999ffa48a00c7cb9d71..bb783da26fbf5bba4838f1d4c631bfccb5a1f72d 100644 (file)
@@ -2475,6 +2475,7 @@ static void setupLuaConfig(LuaContext& luaCtx, bool client, bool configCheck)
       });
 
     luaCtx.writeFunction("setAllowEmptyResponse", [](bool allow) { g_allowEmptyResponse=allow; });
+    luaCtx.writeFunction("setDropEmptyQueries", [](bool drop) { extern bool g_dropEmptyQueries; g_dropEmptyQueries = drop; });
 
 #if defined(HAVE_LIBSSL) && defined(HAVE_OCSP_BASIC_SIGN)
     luaCtx.writeFunction("generateOCSPResponse", [client](const std::string& certFile, const std::string& caCert, const std::string& caKey, const std::string& outFile, int ndays, int nmin) {
index fbbd3281b7bef7ca577e39559f39b4ed2898d509..fb9e33bac0366f13ac992da72edde08a1e03499e 100644 (file)
@@ -528,9 +528,23 @@ static IOState handleQuery(std::shared_ptr<IncomingTCPConnectionState>& state, c
     return state->sendResponse(state, now, std::move(response));
   }
 
-  const auto* dh = reinterpret_cast<dnsheader*>(state->d_buffer.data());
-  if (!checkQueryHeaders(dh)) {
-    return IOState::NeedRead;
+  {
+    /* this pointer will be invalidated the second the buffer is resized, don't hold onto it! */
+    auto* dh = reinterpret_cast<dnsheader*>(state->d_buffer.data());
+    if (!checkQueryHeaders(dh)) {
+      return IOState::NeedRead;
+    }
+
+    if (dh->qdcount == 0) {
+      TCPResponse response;
+      dh->rcode = RCode::NotImp;
+      dh->qr = true;
+      response.d_selfGenerated = true;
+      response.d_buffer = std::move(state->d_buffer);
+      state->d_state = IncomingTCPConnectionState::State::idle;
+      ++state->d_currentQueriesCount;
+      return state->sendResponse(state, now, std::move(response));
+    }
   }
 
   uint16_t qtype, qclass;
@@ -558,7 +572,7 @@ static IOState handleQuery(std::shared_ptr<IncomingTCPConnectionState>& state, c
   }
 
   // the buffer might have been invalidated by now
-  dh = dq.getHeader();
+  const dnsheader* dh = dq.getHeader();
   if (result == ProcessQueryResult::SendAnswer) {
     TCPResponse response;
     response.d_selfGenerated = true;
index 59c4aecf9ca1024af7d56a958c00430dfe0b6042..d7930a294d7ede7aa064daeb34471e1f55e98245 100644 (file)
@@ -141,6 +141,7 @@ int g_udpTimeout{2};
 bool g_servFailOnNoPolicy{false};
 bool g_truncateTC{false};
 bool g_fixupCase{false};
+bool g_dropEmptyQueries{false};
 
 std::set<std::string> g_capabilitiesToRetain;
 
@@ -1092,7 +1093,9 @@ bool checkQueryHeaders(const struct dnsheader* dh)
 
   if (dh->qdcount == 0) {
     ++g_stats.emptyQueries;
-    return false;
+    if (g_dropEmptyQueries) {
+      return false;
+    }
   }
 
   if (dh->rd) {
@@ -1305,11 +1308,21 @@ static void processUDPQuery(ClientState& cs, LocalHolders& holders, const struct
       return;
     }
 
-    struct dnsheader* dh = reinterpret_cast<struct dnsheader*>(query.data());
-    queryId = ntohs(dh->id);
+    {
+      /* this pointer will be invalidated the second the buffer is resized, don't hold onto it! */
+      struct dnsheader* dh = reinterpret_cast<struct dnsheader*>(query.data());
+      queryId = ntohs(dh->id);
 
-    if (!checkQueryHeaders(dh)) {
-      return;
+      if (!checkQueryHeaders(dh)) {
+        return;
+      }
+
+      if (dh->qdcount == 0) {
+        dh->rcode = RCode::NotImp;
+        dh->qr = true;
+        sendUDPResponse(cs.udpFD, query, 0, dest, remote);
+        return;
+      }
     }
 
     uint16_t qtype, qclass;
@@ -1330,7 +1343,7 @@ static void processUDPQuery(ClientState& cs, LocalHolders& holders, const struct
     }
 
     // the buffer might have been invalidated by now (resized)
-    dh = dq.getHeader();
+    struct dnsheader* dh = dq.getHeader();
     if (result == ProcessQueryResult::SendAnswer) {
 #if defined(HAVE_RECVMMSG) && defined(HAVE_SENDMMSG) && defined(MSG_WAITFORONE)
       if (dq.delayMsec == 0 && responsesVect != nullptr) {
index 1b2839005b4aa1723ec3b5adf9955dd0210f3d0d..7ce970272020f71a7d7a4c3e619d55d53dbb60f0 100644 (file)
@@ -150,7 +150,7 @@ Listen Sockets
   * ``ocspResponses``: list - List of files containing OCSP responses, in the same order than the certificates and keys, that will be used to provide OCSP stapling responses.
   * ``minTLSVersion``: str - Minimum version of the TLS protocol to support. Possible values are 'tls1.0', 'tls1.1', 'tls1.2' and 'tls1.3'. Default is to require at least TLS 1.0.
   * ``numberOfTicketsKeys``: int - The maximum number of tickets keys to keep in memory at the same time. Only one key is marked as active and used to encrypt new tickets while the remaining ones can still be used to decrypt existing tickets after a rotation. Default to 5.
-  * ``ticketKeyFile``: str - The path to a file from where TLS tickets keys should be loaded, to support RFC 5077. These keys should be rotated often and never written to persistent storage to preserve forward secrecy. The default is to generate a random key. dnsdist supports several tickets keys to be able to decrypt existing sessions after the rotation.
+  * ``ticketKeyFile``: str - The path to a file from where TLS tickets keys should be loaded, to support :rfc:`5077`. These keys should be rotated often and never written to persistent storage to preserve forward secrecy. The default is to generate a random key. dnsdist supports several tickets keys to be able to decrypt existing sessions after the rotation.
   * ``ticketsKeysRotationDelay``: int - Set the delay before the TLS tickets key is rotated, in seconds. Default is 43200 (12h).
   * ``sessionTimeout``: int - Set the TLS session lifetime in seconds, this is used both for TLS ticket lifetime and for sessions kept in memory.
   * ``sessionTickets``: bool - Whether session resumption via session tickets is enabled. Default is true, meaning tickets are enabled.
@@ -196,7 +196,7 @@ Listen Sockets
   * ``ciphers``: str - The TLS ciphers to use. The exact format depends on the provider used. When the OpenSSL provider is used, ciphers for TLS 1.3 must be specified via ``ciphersTLS13``.
   * ``ciphersTLS13``: str - The ciphers to use for TLS 1.3, when the OpenSSL provider is used. When the GnuTLS provider is used, ``ciphers`` applies regardless of the TLS protocol and this setting is not used.
   * ``numberOfTicketsKeys``: int - The maximum number of tickets keys to keep in memory at the same time, if the provider supports it (GnuTLS doesn't, OpenSSL does). Only one key is marked as active and used to encrypt new tickets while the remaining ones can still be used to decrypt existing tickets after a rotation. Default to 5.
-  * ``ticketKeyFile``: str - The path to a file from where TLS tickets keys should be loaded, to support RFC 5077. These keys should be rotated often and never written to persistent storage to preserve forward secrecy. The default is to generate a random key. The OpenSSL provider supports several tickets keys to be able to decrypt existing sessions after the rotation, while the GnuTLS provider only supports one key.
+  * ``ticketKeyFile``: str - The path to a file from where TLS tickets keys should be loaded, to support :rfc:`5077`. These keys should be rotated often and never written to persistent storage to preserve forward secrecy. The default is to generate a random key. The OpenSSL provider supports several tickets keys to be able to decrypt existing sessions after the rotation, while the GnuTLS provider only supports one key.
   * ``ticketsKeysRotationDelay``: int - Set the delay before the TLS tickets key is rotated, in seconds. Default is 43200 (12h).
   * ``sessionTimeout``: int - Set the TLS session lifetime in seconds, this is used both for TLS ticket lifetime and for sessions kept in memory.
   * ``sessionTickets``: bool - Whether session resumption via session tickets is enabled. Default is true, meaning tickets are enabled.
@@ -1536,6 +1536,14 @@ Other functions
 
   Set to true (defaults to false) to allow empty responses (qdcount=0) with a NoError or NXDomain rcode (default) from backends. dnsdist drops these responses by default because it can't match them against the initial query since they don't contain the qname, qtype and qclass, and therefore the risk of collision is much higher than with regular responses.
 
+.. function:: setDropEmptyQueries(drop)
+
+  .. versionadded:: 1.6.0
+
+  Set to true (defaults to false) to drop empty queries (qdcount=0) right away with a NotImp rcode. dnsdist used to drop these queries by default because most rules and existing Lua code expects a query to have a qname, qtype and qclass. However :rfc:`7873` uses these queries to request a server cookie, and :rfc:`8906` as a conformance test, so answering these queries with NotImp is much better than not answering at all.
+
+  :param bool drop: Whether to drop these queries (defaults to false)
+
 .. function:: setProxyProtocolMaximumPayloadSize(size)
 
   .. versionadded:: 1.6.0
index 7b06c8225dc65f4f395a638ac193afeb50460d92..e1e9c53c96180c372b5024c0b37e8cffb6700b4d 100644 (file)
@@ -216,6 +216,27 @@ struct DOHServerConfig
   int dohresponsepair[2]{-1,-1};
 };
 
+
+static void sendDoHUnitToTheMainThread(DOHUnit* du, const char* description)
+{
+  /* increase the ref counter before sending the pointer */
+  du->get();
+
+  static_assert(sizeof(du) <= PIPE_BUF, "Writes up to PIPE_BUF are guaranteed not to be interleaved and to either fully succeed or fail");
+  ssize_t sent = write(du->rsock, &du, sizeof(du));
+  if (sent != sizeof(du)) {
+    if (errno == EAGAIN || errno == EWOULDBLOCK) {
+      ++g_stats.dohResponsePipeFull;
+      vinfolog("Unable to pass a %s to the DoH worker thread because the pipe is full", description);
+    }
+    else {
+      vinfolog("Unable to pass a %s to the DoH worker thread because we couldn't write to the pipe: %s", description, stringerror());
+    }
+
+    du->release();
+  }
+}
+
 /* This function is called from other threads than the main DoH one,
    instructing it to send a 502 error to the client */
 void handleDOHTimeout(DOHUnit* oldDU)
@@ -227,22 +248,7 @@ void handleDOHTimeout(DOHUnit* oldDU)
 /* we are about to erase an existing DU */
   oldDU->status_code = 502;
 
-  /* increase the ref counter before sending the pointer */
-  oldDU->get();
-
-  static_assert(sizeof(oldDU) <= PIPE_BUF, "Writes up to PIPE_BUF are guaranteed not to be interleaved and to either fully succeed or fail");
-  ssize_t sent = write(oldDU->rsock, &oldDU, sizeof(oldDU));
-  if (sent != sizeof(oldDU)) {
-    if (errno == EAGAIN || errno == EWOULDBLOCK) {
-      ++g_stats.dohResponsePipeFull;
-      vinfolog("Unable to pass a DoH timeout to the DoH worker thread because the pipe is full");
-    }
-    else {
-      vinfolog("Unable to pass a DoH timeout to the DoH worker thread because we couldn't write to the pipe: %s", stringerror());
-    }
-
-    oldDU->release();
-  }
+  sendDoHUnitToTheMainThread(oldDU, "DoH timeout");
 
   oldDU->release();
   oldDU = nullptr;
@@ -445,6 +451,16 @@ static int processDOHQuery(DOHUnit* du)
         return -1; // drop
       }
 
+      if (dh->qdcount == 0) {
+        dh->rcode = RCode::NotImp;
+        dh->qr = true;
+        du->response = std::move(du->query);
+
+        sendDoHUnitToTheMainThread(du, "DoH self-answered response");
+
+        return 0;
+      }
+
       queryId = ntohs(dh->id);
     }
 
@@ -468,23 +484,9 @@ static int processDOHQuery(DOHUnit* du)
       if (du->response.empty()) {
         du->response = std::move(du->query);
       }
-      /* increase the ref counter before sending the pointer */
-      du->get();
 
-      static_assert(sizeof(du) <= PIPE_BUF, "Writes up to PIPE_BUF are guaranteed not to be interleaved and to either fully succeed or fail");
+      sendDoHUnitToTheMainThread(du, "DoH self-answered response");
 
-      ssize_t sent = write(du->rsock, &du, sizeof(du));
-      if (sent != sizeof(du)) {
-        if (errno == EAGAIN || errno == EWOULDBLOCK) {
-          ++g_stats.dohResponsePipeFull;
-          vinfolog("Unable to pass a DoH self-answered response to the DoH worker thread because the pipe is full");
-        }
-        else {
-          vinfolog("Unable to pass a DoH self-answered to the DoH worker thread because we couldn't write to the pipe: %s", stringerror());
-        }
-
-        du->release();
-      }
       return 0;
     }
 
@@ -1105,24 +1107,9 @@ static void dnsdistclient(int qsock)
 
       if (processDOHQuery(du) < 0) {
         du->status_code = 500;
-        /* increase the ref count before sending the pointer */
-        du->get();
-
-        static_assert(sizeof(du) <= PIPE_BUF, "Writes up to PIPE_BUF are guaranteed not to be interleaved and to either fully succeed or fail");
-
-        ssize_t sent = write(du->rsock, &du, sizeof(du));
-        if (sent != sizeof(du)) {
-          if (errno == EAGAIN || errno == EWOULDBLOCK) {
-            ++g_stats.dohResponsePipeFull;
-            vinfolog("Unable to pass a DoH internal error to the DoH worker thread because the pipe is full");
-          }
-          else {
-            vinfolog("Unable to pass a DoH internal error to the DoH worker thread because we couldn't write to the pipe: %s", stringerror());
-          }
-
-          // XXX but now what - will h2o time this out for us?
-          du->release();
-        }
+
+        sendDoHUnitToTheMainThread(du, "DoH internal error");
+        // XXX if we failed to send it to the main thread, now what - will h2o eventually time this out for us
       }
       du->release();
     }
index 089b8551d5d9b8802bfb586ccf8b08e3595a86cd..943efa186b41ff7d5960afe2e2695128d61891a5 100644 (file)
@@ -55,6 +55,7 @@ class TestAdvancedAllow(DNSDistTest):
         for method in ("sendUDPQuery", "sendTCPQuery"):
             sender = getattr(self, method)
             (_, receivedResponse) = sender(query, response=None, useQueue=False)
+            self.assertEquals(receivedResponse, None)
 
 class TestAdvancedFixupCase(DNSDistTest):
 
@@ -2128,3 +2129,22 @@ class TestAdvancedLuaFFI(DNSDistTest):
             sender = getattr(self, method)
             (_, receivedResponse) = sender(query, response=None, useQueue=False)
             self.assertEquals(receivedResponse, response)
+
+class TestAdvancedDropEmptyQueries(DNSDistTest):
+
+    _config_template = """
+    setDropEmptyQueries(true)
+    newServer{address="127.0.0.1:%s"}
+    """
+
+    def testAdvancedDropEmptyQueries(self):
+        """
+        Advanced: Drop empty queries
+        """
+        name = 'drop-empty-queries.advanced.tests.powerdns.com.'
+        query = dns.message.Message()
+
+        for method in ("sendUDPQuery", "sendTCPQuery"):
+            sender = getattr(self, method)
+            (_, receivedResponse) = sender(query, response=None, useQueue=False)
+            self.assertEquals(receivedResponse, None)
index 3e12cfc18eb5b887815a15c619c4ebe62c2e8c4f..07138e25abbadd4980c2331cdaa8cc2ff637dd7e 100644 (file)
@@ -422,3 +422,17 @@ class TestBasics(DNSDistTest):
                 (_, receivedResponse) = sender(query, response=None, useQueue=False)
                 self.assertEquals(receivedResponse, expectedResponse)
 
+    def testEmptyQueries(self):
+        """
+        Basic: NotImp on empty queries
+        """
+        name = 'notimp-empty-queries.basic.tests.powerdns.com.'
+        query = dns.message.Message()
+
+        response = dns.message.make_response(query)
+        response.set_rcode(dns.rcode.NOTIMP)
+
+        for method in ("sendUDPQuery", "sendTCPQuery"):
+            sender = getattr(self, method)
+            (_, receivedResponse) = sender(query, response=None, useQueue=False)
+            self.assertEquals(receivedResponse, response)