Merge pull request #9991 from rgacogne/ddist-notimp-empty-queries

dnsdist: Send a NotImp answer on empty (qdcount=0) queries

pdns/dnsdist-console.cc

@@ -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" },

pdns/dnsdist-lua.cc

@@ -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) {

pdns/dnsdist-tcp.cc

@@ -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;

pdns/dnsdist.cc

@@ -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;
 
@@ -1093,7 +1094,9 @@ bool checkQueryHeaders(const struct dnsheader* dh)
 
   if (dh->qdcount == 0) {
     ++g_stats.emptyQueries;
-    return false;
+    if (g_dropEmptyQueries) {
+      return false;
+    }
   }
 
   if (dh->rd) {
@@ -1306,11 +1309,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;
@@ -1331,7 +1344,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) {

pdns/dnsdistdist/docs/reference/config.rst

@@ -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

pdns/dnsdistdist/doh.cc

@@ -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();
     }

regression-tests.dnsdist/test_Advanced.py

@@ -55,6 +55,7 @@ def testAdvancedAllowDropped(self):
         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 @@ def testAdvancedLuaFFIUpdate(self):
             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)

regression-tests.dnsdist/test_Basics.py

@@ -422,3 +422,17 @@ def testAddActionDNSNames(self):
                 (_, 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)