Merge pull request #14977 from rgacogne/ddist19-backport-14967

dnsdist-1.9.x: Backport of #14967 -  Fix ECS zero-scope with incoming DoH queries

pdns/dnsdist-idstate.hh

@@ -155,8 +155,8 @@ struct InternalQueryState
   int32_t d_streamID{-1}; // 4
   uint32_t cacheKey{0}; // 4
   uint32_t cacheKeyNoECS{0}; // 4
-  // DoH-only */
-  uint32_t cacheKeyUDP{0}; // 4
+  // DoH-only: if we received a TC=1 answer, we had to retry over TCP and thus we need the TCP cache key */
+  uint32_t cacheKeyTCP{0}; // 4
   uint32_t ttlCap{0}; // cap the TTL _after_ inserting into the packet cache // 4
   int backendFD{-1}; // 4
   int delayMsec{0};

pdns/dnsdist.cc

@@ -579,10 +579,12 @@ bool processResponseAfterRules(PacketBuffer& response, const std::vector<DNSDist
       zeroScope = false;
     }
     uint32_t cacheKey = dr.ids.cacheKey;
-    if (dr.ids.protocol == dnsdist::Protocol::DoH && dr.ids.forwardedOverUDP) {
-      cacheKey = dr.ids.cacheKeyUDP;
+    if (dr.ids.protocol == dnsdist::Protocol::DoH && !dr.ids.forwardedOverUDP) {
+      cacheKey = dr.ids.cacheKeyTCP;
+      // disable zeroScope in that case, as we only have the "no-ECS" cache key for UDP
+      zeroScope = false;
     }
-    else if (zeroScope) {
+    if (zeroScope) {
       // if zeroScope, pass the pre-ECS hash-key and do not pass the subnet to the cache
       cacheKey = dr.ids.cacheKeyNoECS;
     }
@@ -1441,6 +1443,10 @@ ProcessQueryResult processQueryAfterRules(DNSQuestion& dq, LocalHolders& holders
     const auto& policy = poolPolicy != nullptr ? *poolPolicy : *(holders.policy);
     const auto servers = serverPool->getServers();
     selectedBackend = policy.getSelectedBackend(*servers, dq);
+    bool willBeForwardedOverUDP = !dq.overTCP() || dq.ids.protocol == dnsdist::Protocol::DoH;
+    if (selectedBackend && selectedBackend->isTCPOnly()) {
+      willBeForwardedOverUDP = false;
+    }
 
     uint32_t allowExpired = selectedBackend ? 0 : g_staleCacheEntriesTTL;
 
@@ -1453,7 +1459,7 @@ ProcessQueryResult processQueryAfterRules(DNSQuestion& dq, LocalHolders& holders
       // we need ECS parsing (parseECS) to be true so we can be sure that the initial incoming query did not have an existing
       // ECS option, which would make it unsuitable for the zero-scope feature.
       if (dq.ids.packetCache && !dq.ids.skipCache && (!selectedBackend || !selectedBackend->d_config.disableZeroScope) && dq.ids.packetCache->isECSParsingEnabled()) {
-        if (dq.ids.packetCache->get(dq, dq.getHeader()->id, &dq.ids.cacheKeyNoECS, dq.ids.subnet, dq.ids.dnssecOK, !dq.overTCP(), allowExpired, false, true, false)) {
+        if (dq.ids.packetCache->get(dq, dq.getHeader()->id, &dq.ids.cacheKeyNoECS, dq.ids.subnet, dq.ids.dnssecOK, willBeForwardedOverUDP, allowExpired, false, true, false)) {
 
           vinfolog("Packet cache hit for query for %s|%s from %s (%s, %d bytes)", dq.ids.qname.toLogString(), QType(dq.ids.qtype).toString(), dq.ids.origRemote.toStringWithPort(), dq.ids.protocol.toString(), dq.getData().size());
 
@@ -1479,14 +1485,11 @@ ProcessQueryResult processQueryAfterRules(DNSQuestion& dq, LocalHolders& holders
     }
 
     if (dq.ids.packetCache && !dq.ids.skipCache) {
-      bool forwardedOverUDP = !dq.overTCP();
-      if (selectedBackend && selectedBackend->isTCPOnly()) {
-        forwardedOverUDP = false;
-      }
-
-      /* we do not record a miss for queries received over DoH and forwarded over TCP
+      /* First lookup, which takes into account how the protocol over which the query will be forwarded.
+         For DoH, this lookup is done with the protocol set to TCP but we will retry over UDP below,
+         therefore we do not record a miss for queries received over DoH and forwarded over TCP
          yet, as we will do a second-lookup */
-      if (dq.ids.packetCache->get(dq, dq.getHeader()->id, &dq.ids.cacheKey, dq.ids.subnet, dq.ids.dnssecOK, forwardedOverUDP, allowExpired, false, true, dq.ids.protocol != dnsdist::Protocol::DoH || forwardedOverUDP)) {
+      if (dq.ids.packetCache->get(dq, dq.getHeader()->id, dq.ids.protocol == dnsdist::Protocol::DoH ? &dq.ids.cacheKeyTCP : &dq.ids.cacheKey, dq.ids.subnet, dq.ids.dnssecOK, dq.ids.protocol != dnsdist::Protocol::DoH && willBeForwardedOverUDP, allowExpired, false, true, dq.ids.protocol != dnsdist::Protocol::DoH || !willBeForwardedOverUDP)) {
 
         dnsdist::PacketMangling::editDNSHeaderFromPacket(dq.getMutableData(), [flags=dq.ids.origFlags](dnsheader& header) {
           restoreFlags(&header, flags);
@@ -1503,9 +1506,10 @@ ProcessQueryResult processQueryAfterRules(DNSQuestion& dq, LocalHolders& holders
         ++dq.ids.cs->responses;
         return ProcessQueryResult::SendAnswer;
       }
-      else if (dq.ids.protocol == dnsdist::Protocol::DoH && !forwardedOverUDP) {
-        /* do a second-lookup for UDP responses, but we do not want TC=1 answers */
-        if (dq.ids.packetCache->get(dq, dq.getHeader()->id, &dq.ids.cacheKeyUDP, dq.ids.subnet, dq.ids.dnssecOK, true, allowExpired, false, false, true)) {
+      if (dq.ids.protocol == dnsdist::Protocol::DoH && willBeForwardedOverUDP) {
+        /* do a second-lookup for responses received over UDP, but we do not want TC=1 answers */
+        /* we need to be careful to keep the existing cache-key (TCP) */
+        if (dq.ids.packetCache->get(dq, dq.getHeader()->id, &dq.ids.cacheKey, dq.ids.subnet, dq.ids.dnssecOK, true, allowExpired, false, false, true)) {
           if (!prepareOutgoingResponse(holders, *dq.ids.cs, dq, true)) {
             return ProcessQueryResult::Drop;
           }

regression-tests.dnsdist/test_Caching.py

@@ -2396,6 +2396,12 @@ def testCacheCollisionWithECSParsing(self):
 
 class TestCachingScopeZero(DNSDistTest):
 
+    _serverKey = 'server.key'
+    _serverCert = 'server.chain'
+    _serverName = 'tls.tests.dnsdist.org'
+    _caCert = 'ca.pem'
+    _dohServerPort = pickAvailablePort()
+    _dohBaseURL = ("https://%s:%d/" % (_serverName, _dohServerPort))
     _config_template = """
     -- Be careful to enable ECS parsing in the packet cache, otherwise scope zero is disabled
     pc = newPacketCache(100, {maxTTL=86400, minTTL=1, temporaryFailureTTL=60, staleTTL=60, dontAge=false, numberOfShards=1, deferrableInsertLock=true, maxNegativeTTL=3600, parseECS=true})
@@ -2406,7 +2412,11 @@ class TestCachingScopeZero(DNSDistTest):
     -- to unset it using rules before the first cache lookup)
     addAction(RDRule(), SetECSAction("192.0.2.1/32"))
     addAction(RDRule(), SetNoRecurseAction())
+
+    -- test the DoH special case (query received over TCP, forwarded over UDP)
+    addDOHLocal("127.0.0.1:%d", "%s", "%s", { "/" }, {library='nghttp2'})
     """
+    _config_params = ['_testServerPort', '_dohServerPort', '_serverCert', '_serverKey']
 
     def testScopeZero(self):
         """
@@ -2582,6 +2592,38 @@ def testNoECS(self):
             self.checkMessageEDNSWithECS(expectedQuery2, receivedQuery)
             self.checkMessageNoEDNS(receivedResponse, response)
 
+    def testScopeZeroIncomingDoH(self):
+        """
+        Cache: Test the scope-zero feature with a query received over DoH, backend returns a scope of zero
+        """
+        ttl = 600
+        name = 'scope-zero-incoming-doh.cache.tests.powerdns.com.'
+        query = dns.message.make_query(name, 'AAAA', 'IN')
+        query.flags &= ~dns.flags.RD
+        ecso = clientsubnetoption.ClientSubnetOption('127.0.0.0', 24)
+        expectedQuery = dns.message.make_query(name, 'AAAA', 'IN', use_edns=True, options=[ecso], payload=4096)
+        expectedQuery.flags &= ~dns.flags.RD
+        ecsoResponse = clientsubnetoption.ClientSubnetOption('127.0.0.1', 24, 0)
+        expectedResponse = dns.message.make_response(query)
+        scopedResponse = dns.message.make_response(query)
+        scopedResponse.use_edns(edns=True, payload=4096, options=[ecsoResponse])
+        rrset = dns.rrset.from_text(name,
+                                    ttl,
+                                    dns.rdataclass.IN,
+                                    dns.rdatatype.AAAA,
+                                    '::1')
+        scopedResponse.answer.append(rrset)
+        expectedResponse.answer.append(rrset)
+
+        (receivedQuery, receivedResponse) = self.sendDOHQueryWrapper(query, scopedResponse)
+        receivedQuery.id = expectedQuery.id
+        self.checkMessageEDNSWithECS(expectedQuery, receivedQuery)
+        self.checkMessageNoEDNS(receivedResponse, expectedResponse)
+
+        # next query should hit the cache
+        (receivedQuery, receivedResponse) = self.sendDOHQueryWrapper(query, response=None, useQueue=False)
+        self.checkMessageNoEDNS(receivedResponse, expectedResponse)
+
 class TestCachingScopeZeroButNoSubnetcheck(DNSDistTest):
 
     _config_template = """