dnsdist: Proper HTTP response for timeouts over DoH

diff --git a/pdns/dnsdist.cc b/pdns/dnsdist.cc
index babcc9972878f14790ffd9da249722fd7001c79c..b8ab70b9b3f440cffa58e88d7ceab4c6ab1b401f 100644 (file)
--- a/pdns/dnsdist.cc
+++ b/pdns/dnsdist.cc
@@ -546,9 +546,10 @@ try {
         IDState* ids = &dss->idStates[queryId];
         int origFD = ids->origFD;
 
-        if(origFD < 0 && ids->du == nullptr) // duplicate
+        if(origFD < 0) // duplicate
           continue;
 
+        auto du = ids->du;
         /* setting age to 0 to prevent the maintainer thread from
            cleaning this IDS while we process the response.
            We have already a copy of the origFD, so it would
@@ -568,7 +569,10 @@ try {
              and the other thread has not incremented the outstanding counter, so we don't
              want it to be decremented twice. */
           --dss->outstanding;  // you'd think an attacker could game this, but we're using connected socket
+        } else {
+          du = nullptr;
         }
+        ids->du = nullptr;
 
         if(dh->tc && g_truncateTC) {
           truncateTC(response, &responseLen, responseSize, consumed);
@@ -588,15 +592,15 @@ try {
         }
 
         if (ids->cs && !ids->cs->muted) {
-          if (ids->du) {
+          if (du) {
 #ifdef HAVE_DNS_OVER_HTTPS
             // DoH query
-            ids->du->query = std::string(response, responseLen);
-            if (send(ids->du->rsock, &ids->du, sizeof(ids->du), 0) != sizeof(ids->du)) {
-              delete ids->du;
+            du->query = std::string(response, responseLen);
+            if (send(du->rsock, &du, sizeof(du), 0) != sizeof(du)) {
+              delete du;
             }
 #endif /* HAVE_DNS_OVER_HTTPS */
-            ids->du = nullptr;
+            du = nullptr;
           }
           else {
             ComboAddress empty;
@@ -1564,16 +1568,25 @@ static void processUDPQuery(ClientState& cs, LocalHolders& holders, const struct
     unsigned int idOffset = (ss->idOffset++) % ss->idStates.size();
     IDState* ids = &ss->idStates[idOffset];
     ids->age = 0;
-    ids->du = nullptr;
+    DOHUnit* du = nullptr;
+
+    if (ids->origFD != -1) {
+      du = ids->du;
+    }
 
     int oldFD = ids->origFD.exchange(cs.udpFD);
     if(oldFD < 0) {
-      // if we are reusing, no change in outstanding
       ++ss->outstanding;
+      /* either it was already -1 so no DOH unit to handle,
+         or someone handled it before us */
+      du = nullptr;
     }
     else {
+      // if we are reusing, no change in outstanding
       ++ss->reuseds;
       ++g_stats.downstreamTimeouts;
+      ids->du = nullptr;
+      handleDOHTimeout(du);
     }
 
     ids->cs = &cs;
@@ -2064,11 +2077,14 @@ static void healthChecksThread()
              so the sooner the better any way since we _will_
              decrement it.
           */
+          auto oldDU = ids.du;
+
           if (ids.origFD.exchange(-1) != origFD) {
             /* this state has been altered in the meantime,
                don't go anywhere near it */
             continue;
           }
+          handleDOHTimeout(oldDU);
           ids.du = nullptr;
           ids.age = 0;
           dss->reuseds++;

diff --git a/pdns/dnsdistdist/Makefile.am b/pdns/dnsdistdist/Makefile.am
index 2112afa53e4ebdb14dac3a8ccea60cbd1774493e..71bd409c5e4263e1819a492602c1274471ce8c4e 100644 (file)
--- a/pdns/dnsdistdist/Makefile.am
+++ b/pdns/dnsdistdist/Makefile.am
@@ -134,7 +134,7 @@ dnsdist_SOURCES = \
        dnsname.cc dnsname.hh \
        dnsparser.hh dnsparser.cc \
        dnswriter.cc dnswriter.hh \
-       doh.hh \
+       doh.hh doh.cc \
        dolog.hh \
        ednsoptions.cc ednsoptions.hh \
        ednscookies.cc ednscookies.hh \
@@ -207,7 +207,6 @@ endif
 endif
 
 if HAVE_DNS_OVER_HTTPS
-dnsdist_SOURCES += doh.cc
 
 if HAVE_LIBH2OEVLOOP
 dnsdist_LDADD += $(LIBH2OEVLOOP_LIBS)

diff --git a/pdns/dnsdistdist/doh.cc b/pdns/dnsdistdist/doh.cc
index a8cf9070662ba89d77229e2fac17ebff13ef60fe..8608922b91a4e2f064ee2de08ee314662e1015e6 100644 (file)
--- a/pdns/dnsdistdist/doh.cc
+++ b/pdns/dnsdistdist/doh.cc
@@ -1,3 +1,7 @@
+#include "config.h"
+#include "doh.hh"
+
+#ifdef HAVE_DNS_OVER_HTTPS
 #define H2O_USE_EPOLL 1
 
 #include <errno.h>
@@ -122,6 +126,22 @@ struct DOHServerConfig
   int dohresponsepair[2]{-1,-1};
 };
 
+void handleDOHTimeout(DOHUnit* oldDU)
+{
+  if (oldDU == nullptr) {
+    return;
+  }
+
+/* we are about to erase an existing DU */
+  oldDU->error = true;
+  oldDU->status_code = 502;
+
+  if (send(oldDU->rsock, &oldDU, sizeof(oldDU), 0) != sizeof(oldDU)) {
+    delete oldDU;
+    oldDU = nullptr;
+  }
+}
+
 static void on_socketclose(void *data)
 {
   DOHAcceptContext* ctx = reinterpret_cast<DOHAcceptContext*>(data);
@@ -213,21 +233,31 @@ static int processDOHQuery(DOHUnit* du)
       return -1;
     }
 
+    ComboAddress dest = du->dest;
     unsigned int idOffset = (ss->idOffset++) % ss->idStates.size();
     IDState* ids = &ss->idStates[idOffset];
     ids->age = 0;
-    ids->du = du;
+    DOHUnit* oldDU = nullptr;
+    if (ids->origFD != -1) {
+      oldDU = ids->du;
+    }
 
-    int oldFD = ids->origFD.exchange(cs.udpFD);
-    if(oldFD < 0) {
+    int oldFD = ids->origFD.exchange(0);
+    if (oldFD < 0) {
       // if we are reusing, no change in outstanding
       ++ss->outstanding;
     }
     else {
       ++ss->reuseds;
       ++g_stats.downstreamTimeouts;
+      /* origFD is not -1 anymore, we can't touch it */
+      oldDU = nullptr;
     }
 
+    handleDOHTimeout(oldDU);
+
+    ids->du = du;
+
     ids->cs = &cs;
     ids->origID = dh->id;
     setIDStateFromDNSQuestion(*ids, dq, std::move(qname));
@@ -238,8 +268,8 @@ static int processDOHQuery(DOHUnit* du)
        We need to keep track of which one it is since we may
        want to use the real but not the listening addr to reply.
     */
-    if (du->dest.sin4.sin_family != 0) {
-      ids->origDest = du->dest;
+    if (dest.sin4.sin_family != 0) {
+      ids->origDest = dest;
       ids->destHarvested = true;
     }
     else {
@@ -588,6 +618,7 @@ static void on_dnsdist(h2o_socket_t *listener, const char *err)
 
     ++dsc->df->d_errorresponses;
   }
+
   delete du;
 }
 
@@ -771,3 +802,11 @@ catch(const std::exception& e) {
 catch(...) {
   throw runtime_error("DOH thread failed to launch");
 }
+
+#else /* HAVE_DNS_OVER_HTTPS */
+
+void handleDOHTimeout(DOHUnit* oldDU)
+{
+}
+
+#endif /* HAVE_DNS_OVER_HTTPS */

diff --git a/pdns/doh.hh b/pdns/doh.hh
index 29f4adcc94f5db8c852b73333cbe9c6dfdcb8e38..7d411d25f8f1c3ac67b7b0da405ec0bdc497c3eb 100644 (file)
--- a/pdns/doh.hh
+++ b/pdns/doh.hh
@@ -74,3 +74,5 @@ struct DOHUnit
 };
 
 #endif /* HAVE_DNS_OVER_HTTPS  */
+
+void handleDOHTimeout(DOHUnit* oldDU);