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.

(cherry picked from commit 12af2075a86c11ee5441defbfe6695a609cb6eb4)

diff --git a/pdns/dnsdist-idstate.hh b/pdns/dnsdist-idstate.hh
index d22d274ead892cbbc40745375d0f034682ad0c41..1568618dd134dcb0473d9d0d1eeeae7a8c8128e6 100644 (file)
--- a/pdns/dnsdist-idstate.hh
+++ b/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};

diff --git a/pdns/dnsdist.cc b/pdns/dnsdist.cc
index 96ce8ac0d56ce518ed973e8f46447e5117cc89fa..b7c19b4a621bbd414895a8058c3618aad5795b01 100644 (file)
--- a/pdns/dnsdist.cc
+++ b/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;
           }

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 = """