Merge pull request #17556 from karelbilek/kb/doh-fix-doh

dnsdist: TCP multiplexer: update should reset ttd

Author: rgacogne

pdns/dnsdistdist/tcpiohandler-mplexer.hh

@@ -74,37 +74,6 @@ public:
     return result;
   }
 
-  void add(IOState iostate, FDMultiplexer::callbackfunc_t callback, FDMultiplexer::funcparam_t callbackData, std::optional<struct timeval> ttd)
-  {
-    DEBUGLOG("in " << __PRETTY_FUNCTION__ << " for fd " << d_fd << ", last state was " << getState() << ", adding " << (int)iostate);
-    if (iostate == IOState::NeedRead) {
-      if (isWaitingForRead()) {
-        if (ttd) {
-          /* let's update the TTD ! */
-          d_mplexer.setReadTTD(d_fd, *ttd, /* we pass 0 here because we already have a TTD */ 0);
-        }
-        return;
-      }
-
-      d_mplexer.addReadFD(d_fd, callback, callbackData, ttd ? &*ttd : nullptr);
-      DEBUGLOG(__PRETTY_FUNCTION__ << ": add read FD " << d_fd);
-      d_isWaitingForRead = true;
-    }
-    else if (iostate == IOState::NeedWrite) {
-      if (isWaitingForWrite()) {
-        if (ttd) {
-          /* let's update the TTD ! */
-          d_mplexer.setWriteTTD(d_fd, *ttd, /* we pass 0 here because we already have a TTD */ 0);
-        }
-        return;
-      }
-
-      d_mplexer.addWriteFD(d_fd, callback, callbackData, ttd ? &*ttd : nullptr);
-      DEBUGLOG(__PRETTY_FUNCTION__ << ": add write FD " << d_fd);
-      d_isWaitingForWrite = true;
-    }
-  }
-
   void update(IOState iostate, FDMultiplexer::callbackfunc_t callback = FDMultiplexer::callbackfunc_t(), FDMultiplexer::funcparam_t callbackData = boost::any(), std::optional<struct timeval> ttd = std::nullopt)
   {
     DEBUGLOG("in " << __PRETTY_FUNCTION__ << " for fd " << d_fd << ", last state was " << getState() << " , new state is " << (int)iostate);
@@ -125,6 +94,9 @@ public:
           /* let's update the TTD ! */
           d_mplexer.setReadTTD(d_fd, *ttd, /* we pass 0 here because we already have a TTD */ 0);
         }
+        else {
+          d_mplexer.resetReadTTD(d_fd);
+        }
         return;
       }
 
@@ -146,6 +118,9 @@ public:
           /* let's update the TTD ! */
           d_mplexer.setWriteTTD(d_fd, *ttd, /* we pass 0 here because we already have a TTD */ 0);
         }
+        else {
+          d_mplexer.resetWriteTTD(d_fd);
+        }
         return;
       }
 

pdns/mplexer.hh

@@ -185,6 +185,18 @@ public:
     d_readCallbacks.replace(it, newEntry);
   }
 
+  void resetReadTTD(int fd)
+  {
+    const auto& it = d_readCallbacks.find(fd);
+    if (it == d_readCallbacks.end()) {
+      throw FDMultiplexerException("attempt to timestamp fd not in the multiplexer");
+    }
+
+    auto newEntry = *it;
+    memset(&newEntry.d_ttd, 0, sizeof(newEntry.d_ttd));
+    d_readCallbacks.replace(it, newEntry);
+  }
+
   void setWriteTTD(int fd, struct timeval tv, int timeout)
   {
     const auto& it = d_writeCallbacks.find(fd);
@@ -198,6 +210,18 @@ public:
     d_writeCallbacks.replace(it, newEntry);
   }
 
+  void resetWriteTTD(int fd)
+  {
+    const auto& it = d_writeCallbacks.find(fd);
+    if (it == d_writeCallbacks.end()) {
+      throw FDMultiplexerException("attempt to timestamp fd not in the multiplexer");
+    }
+
+    auto newEntry = *it;
+    memset(&newEntry.d_ttd, 0, sizeof(newEntry.d_ttd));
+    d_writeCallbacks.replace(it, newEntry);
+  }
+
   void alterFDToRead(int fd, callbackfunc_t toDo, const funcparam_t& parameter = funcparam_t(), const struct timeval* ttd = nullptr)
   {
     accountingRemoveFD(d_writeCallbacks, fd);

regression-tests.dnsdist/test_DOH.py

@@ -2374,3 +2374,73 @@ def testDOHWithPaddingWithECS(self):
 
 class TestDOHEDNSPadding(DOHEDNSPadding, DNSDistDOHTest):
     _dohLibrary = "nghttp2"
+
+
+class TestDOHNoIdleTimeoutKeepsConnection(DNSDistDOHTest, DNSDistTest):
+    _serverKey = "server.key"
+    _serverCert = "server.chain"
+    _serverName = "tls.tests.dnsdist.org"
+    _caCert = "ca.pem"
+    _dohServerPort = pickAvailablePort()
+    _dohBaseURL = "https://%s:%d/PowerDNS" % (_serverName, _dohServerPort)
+
+    _config_template = """
+    newServer{address="127.0.0.1:%d"}
+    addDOHLocal("127.0.0.1:%d", "%s", "%s", { "/PowerDNS" }, {idleTimeout = 0})
+    """
+    _config_params = [
+        "_testServerPort",
+        "_dohServerPort",
+        "_serverCert",
+        "_serverKey",
+    ]
+    _verboseMode = True
+
+    def testKeepsConnection(self):
+        """
+        DOH: Keeps connection with idleTimeout
+        """
+        name = "simple.doh.tests.powerdns.com."
+        query = dns.message.make_query(name, "A", "IN")
+        expectedQuery = dns.message.make_query(name, "A", "IN")
+        response = dns.message.make_response(query)
+        rrset = dns.rrset.from_text(name, 3600, dns.rdataclass.IN, dns.rdatatype.A, "127.0.0.1")
+        response.answer.append(rrset)
+
+        conn = self.openDOHConnection(self._dohServerPort, caFile=self._caCert, timeout=2.0)
+        conn.setopt(pycurl.HTTP_VERSION, pycurl.CURL_HTTP_VERSION_2_PRIOR_KNOWLEDGE)
+        conn.setopt(pycurl.SSL_VERIFYPEER, 1)
+        conn.setopt(pycurl.SSL_VERIFYHOST, 2)
+        conn.setopt(pycurl.CAINFO, self._caCert)
+
+        (receivedQuery, receivedResponse) = self.sendDOHQuery(
+            self._dohServerPort,
+            self._serverName,
+            self._dohBaseURL,
+            query,
+            response=response,
+            caFile=self._caCert,
+            conn=conn,
+        )
+        self.assertTrue(receivedQuery)
+        self.assertTrue(receivedResponse)
+        receivedQuery.id = expectedQuery.id
+        self.assertEqual(expectedQuery, receivedQuery)
+
+        time.sleep(3)
+
+        (receivedQuery, receivedResponse) = self.sendDOHQuery(
+            self._dohServerPort,
+            self._serverName,
+            self._dohBaseURL,
+            query,
+            response=response,
+            caFile=self._caCert,
+            conn=conn,
+        )
+        self.assertTrue(receivedQuery)
+        self.assertTrue(receivedResponse)
+        receivedQuery.id = expectedQuery.id
+        self.assertEqual(expectedQuery, receivedQuery)
+
+        self.assertEqual(conn.getinfo(pycurl.NUM_CONNECTS), 0)