dnsdist: Fix ECS zero-scope with incoming DoH queries

The zero-scope feature involves a first cache lookup before the ECS
information has been added to the query, then on a miss a second,
regular lookup is done. When we get a response from the backend that
contains an ECS scope set to 0, we can insert it into the cache in a
way that allows using it for all clients, but we must be careful to
use the key that was computed during the first lookup, and not the
second one.
Incoming DoH queries make that even more interesting because while
they are received over TCP, they are initially forwarded to the
backend over UDP but can be retried over TCP if a TC=1 answer is
received. In that case we must be very careful not to insert the
answer into the cache using the wrong protocol, as we don't want to
serve a TC=1 answer to a client contacting us over TCP, for example.
The computation of the cache key and protocol was unfortunately broken
for the incoming query received over DoH, forwarded over UDP and
response has a zero scope case. This commit fixes it.

diff --git a/pdns/dnsdistdist/dnsdist-idstate.hh b/pdns/dnsdistdist/dnsdist-idstate.hh
index 259c55e92a15cdda8ba67fe07593b98ec935b99b..48fdeefc305655b50d0c74ea32da8475f52a22f2 100644 (file)
--- a/pdns/dnsdistdist/dnsdist-idstate.hh
+++ b/pdns/dnsdistdist/dnsdist-idstate.hh
@@ -157,8 +157,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};

diff --git a/pdns/dnsdistdist/dnsdist.cc b/pdns/dnsdistdist/dnsdist.cc
index 75ec0dd29a637ebac000c43a1b8c4f69fa5d18d8..758e0b7dadada0ef1dec4ee24affd0cc629a4cab 100644 (file)
--- a/pdns/dnsdistdist/dnsdist.cc
+++ b/pdns/dnsdistdist/dnsdist.cc
@@ -501,14 +501,15 @@ bool processResponseAfterRules(PacketBuffer& response, DNSResponse& dnsResponse,
       zeroScope = false;
     }
     uint32_t cacheKey = dnsResponse.ids.cacheKey;
-    if (dnsResponse.ids.protocol == dnsdist::Protocol::DoH && dnsResponse.ids.forwardedOverUDP) {
-      cacheKey = dnsResponse.ids.cacheKeyUDP;
+    if (dnsResponse.ids.protocol == dnsdist::Protocol::DoH && !dnsResponse.ids.forwardedOverUDP) {
+      cacheKey = dnsResponse.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 = dnsResponse.ids.cacheKeyNoECS;
     }
-
     dnsResponse.ids.packetCache->insert(cacheKey, zeroScope ? boost::none : dnsResponse.ids.subnet, dnsResponse.ids.cacheFlags, dnsResponse.ids.dnssecOK, dnsResponse.ids.qname, dnsResponse.ids.qtype, dnsResponse.ids.qclass, response, dnsResponse.ids.forwardedOverUDP, dnsResponse.getHeader()->rcode, dnsResponse.ids.tempFailureTTL);
 
     const auto& chains = dnsdist::configuration::getCurrentRuntimeConfiguration().d_ruleChains;
@@ -1418,6 +1419,10 @@ ProcessQueryResult processQueryAfterRules(DNSQuestion& dnsQuestion, std::shared_
     std::shared_ptr<ServerPool> serverPool = getPool(dnsQuestion.ids.poolName);
     dnsQuestion.ids.packetCache = serverPool->packetCache;
     selectBackendForOutgoingQuery(dnsQuestion, serverPool, selectedBackend);
+    bool willBeForwardedOverUDP = !dnsQuestion.overTCP() || dnsQuestion.ids.protocol == dnsdist::Protocol::DoH;
+    if (selectedBackend && selectedBackend->isTCPOnly()) {
+      willBeForwardedOverUDP = false;
+    }
 
     uint32_t allowExpired = selectedBackend ? 0 : dnsdist::configuration::getCurrentRuntimeConfiguration().d_staleCacheEntriesTTL;
 
@@ -1430,7 +1435,7 @@ ProcessQueryResult processQueryAfterRules(DNSQuestion& dnsQuestion, std::shared_
       // 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 (dnsQuestion.ids.packetCache && !dnsQuestion.ids.skipCache && (!selectedBackend || !selectedBackend->d_config.disableZeroScope) && dnsQuestion.ids.packetCache->isECSParsingEnabled()) {
-        if (dnsQuestion.ids.packetCache->get(dnsQuestion, dnsQuestion.getHeader()->id, &dnsQuestion.ids.cacheKeyNoECS, dnsQuestion.ids.subnet, dnsQuestion.ids.dnssecOK, !dnsQuestion.overTCP(), allowExpired, false, true, false)) {
+        if (dnsQuestion.ids.packetCache->get(dnsQuestion, dnsQuestion.getHeader()->id, &dnsQuestion.ids.cacheKeyNoECS, dnsQuestion.ids.subnet, dnsQuestion.ids.dnssecOK, willBeForwardedOverUDP, allowExpired, false, true, false)) {
 
           vinfolog("Packet cache hit for query for %s|%s from %s (%s, %d bytes)", dnsQuestion.ids.qname.toLogString(), QType(dnsQuestion.ids.qtype).toString(), dnsQuestion.ids.origRemote.toStringWithPort(), dnsQuestion.ids.protocol.toString(), dnsQuestion.getData().size());
 
@@ -1456,14 +1461,11 @@ ProcessQueryResult processQueryAfterRules(DNSQuestion& dnsQuestion, std::shared_
     }
 
     if (dnsQuestion.ids.packetCache && !dnsQuestion.ids.skipCache) {
-      bool forwardedOverUDP = !dnsQuestion.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 (dnsQuestion.ids.packetCache->get(dnsQuestion, dnsQuestion.getHeader()->id, &dnsQuestion.ids.cacheKey, dnsQuestion.ids.subnet, dnsQuestion.ids.dnssecOK, forwardedOverUDP, allowExpired, false, true, dnsQuestion.ids.protocol != dnsdist::Protocol::DoH || forwardedOverUDP)) {
+      if (dnsQuestion.ids.packetCache->get(dnsQuestion, dnsQuestion.getHeader()->id, dnsQuestion.ids.protocol == dnsdist::Protocol::DoH ? &dnsQuestion.ids.cacheKeyTCP : &dnsQuestion.ids.cacheKey, dnsQuestion.ids.subnet, dnsQuestion.ids.dnssecOK, dnsQuestion.ids.protocol != dnsdist::Protocol::DoH && willBeForwardedOverUDP, allowExpired, false, true, dnsQuestion.ids.protocol != dnsdist::Protocol::DoH || !willBeForwardedOverUDP)) {
 
         dnsdist::PacketMangling::editDNSHeaderFromPacket(dnsQuestion.getMutableData(), [flags = dnsQuestion.ids.origFlags](dnsheader& header) {
           restoreFlags(&header, flags);
@@ -1480,9 +1482,10 @@ ProcessQueryResult processQueryAfterRules(DNSQuestion& dnsQuestion, std::shared_
         ++dnsQuestion.ids.cs->responses;
         return ProcessQueryResult::SendAnswer;
       }
-      if (dnsQuestion.ids.protocol == dnsdist::Protocol::DoH && !forwardedOverUDP) {
-        /* do a second-lookup for UDP responses, but we do not want TC=1 answers */
-        if (dnsQuestion.ids.packetCache->get(dnsQuestion, dnsQuestion.getHeader()->id, &dnsQuestion.ids.cacheKeyUDP, dnsQuestion.ids.subnet, dnsQuestion.ids.dnssecOK, true, allowExpired, false, false, true)) {
+      if (dnsQuestion.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 (dnsQuestion.ids.packetCache->get(dnsQuestion, dnsQuestion.getHeader()->id, &dnsQuestion.ids.cacheKey, dnsQuestion.ids.subnet, dnsQuestion.ids.dnssecOK, true, allowExpired, false, false, true)) {
           if (!prepareOutgoingResponse(*dnsQuestion.ids.cs, dnsQuestion, true)) {
             return ProcessQueryResult::Drop;
           }

diff --git a/regression-tests.dnsdist/test_Caching.py b/regression-tests.dnsdist/test_Caching.py
index f9421812b36327221cd91170712f331a85c287fd..0f0f1195b4778c4c4684c9a37d5e1e05b41d27bb 100644 (file)
--- a/regression-tests.dnsdist/test_Caching.py
+++ b/regression-tests.dnsdist/test_Caching.py
@@ -2396,6 +2396,12 @@ class TestCachingCollisionWithECSParsing(DNSDistTest):
 
 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 @@ class TestCachingScopeZero(DNSDistTest):
             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 = """