dnsdist: Proper HTTP response for timeouts over DoH

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
@@ -563,11 +564,14 @@ try {
 
         int oldFD = ids->origFD.exchange(-1);
         if (oldFD == origFD) {
+          ids->du = nullptr;
           /* we only decrement the outstanding counter if the value was not
              altered in the meantime, which would mean that the state has been actively reused
              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;
         }
 
         if(dh->tc && g_truncateTC) {
@@ -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->response = 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++;

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)

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);
@@ -198,7 +218,7 @@ static int processDOHQuery(DOHUnit* du)
     }
 
     if (result == ProcessQueryResult::SendAnswer) {
-      du->query = std::string(reinterpret_cast<char*>(dq.dh), dq.len);
+      du->response = std::string(reinterpret_cast<char*>(dq.dh), dq.len);
       send(du->rsock, &du, sizeof(du), 0);
       return 0;
     }
@@ -213,21 +233,32 @@ 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;
+      /* either it was already -1 so no DOH unit to handle,
+         or someone handled it before us */
+      oldDU = nullptr;
     }
     else {
       ++ss->reuseds;
       ++g_stats.downstreamTimeouts;
+      ids->du = nullptr;
+      handleDOHTimeout(oldDU);
     }
 
+    ids->du = du;
+
     ids->cs = &cs;
     ids->origID = dh->id;
     setIDStateFromDNSQuestion(*ids, dq, std::move(qname));
@@ -238,8 +269,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 {
@@ -565,8 +596,8 @@ static void on_dnsdist(h2o_socket_t *listener, const char *err)
     //    struct dnsheader* dh = (struct dnsheader*)du->query.c_str();
     //    cout<<"Attempt to send out "<<du->query.size()<<" bytes over https, TC="<<dh->tc<<", RCODE="<<dh->rcode<<", qtype="<<du->qtype<<", req="<<(void*)du->req<<endl;
 
-    du->req->res.content_length = du->query.size();
-    h2o_send_inline(du->req, du->query.c_str(), du->query.size());
+    du->req->res.content_length = du->response.size();
+    h2o_send_inline(du->req, du->response.c_str(), du->response.size());
   }
   else {
     switch(du->status_code) {
@@ -588,6 +619,7 @@ static void on_dnsdist(h2o_socket_t *listener, const char *err)
 
     ++dsc->df->d_errorresponses;
   }
+
   delete du;
 }
 
@@ -771,3 +803,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 */

pdns/doh.hh

@@ -55,6 +55,7 @@ struct st_h2o_req_t;
 struct DOHUnit
 {
   std::string query;
+  std::string response;
   ComboAddress remote;
   ComboAddress dest;
   st_h2o_req_t* req{nullptr};
@@ -74,3 +75,5 @@ struct DOHUnit
 };
 
 #endif /* HAVE_DNS_OVER_HTTPS  */
+
+void handleDOHTimeout(DOHUnit* oldDU);