dnsdist: Fix the wrong RD and CD flags being cached, causing misses

We used to restore the RD and CD flags from the initial query before
inserting the response into the cache. That would cause an issue
if the flags had been altered, for example via SetNoRecurseAction,
as the cache lookup is done _after_ the actions have been applied
and thus after the flags altered.
If the initial query had the RD bit set, and thus was cleared by the
rule, the response would have been inserted with the RD bit restored,
and no lookup would then succeed because it would be done with the
bit cleared.
This commit fixes the insertion to use the RD and CD bits as set in
the response before restoring them, and restores the RD and CD bits
after a cache hit as well, to ensure that:
- cache lookups are done after the rules are applied
- cache insertions are done before the flags are restored

diff --git a/pdns/dnsdist.cc b/pdns/dnsdist.cc
index 920fc78bb8e48954e61c680e5926800933b2ade5..8f3e34b114e17d5aa73d662ed32d16d94c7a469a 100644 (file)
--- a/pdns/dnsdist.cc
+++ b/pdns/dnsdist.cc
@@ -292,6 +292,11 @@ static void restoreFlags(struct dnsheader* dh, uint16_t origFlags)
   *flags |= origFlags;
 }
 
+static uint16_t getRDAndCDFlagsFromDNSHeader(const struct dnsheader* dh)
+{
+  return static_cast<uint16_t>((dh->rd << FLAGS_RD_OFFSET) + (dh->cd << FLAGS_CD_OFFSET));
+}
+
 static bool fixUpQueryTurnedResponse(DNSQuestion& dq, const uint16_t origFlags)
 {
   restoreFlags(dq.getHeader(), origFlags);
@@ -445,6 +450,9 @@ bool processResponse(PacketBuffer& response, LocalStateHolder<vector<DNSDistResp
     return false;
   }
 
+  /* We need to get the flags for the packet cache before restoring the original ones, otherwise it might not match later queries */
+  const auto cacheFlags = getRDAndCDFlagsFromDNSHeader(dr.getHeader());
+
   bool zeroScope = false;
   if (!fixUpResponse(response, *dr.qname, dr.origFlags, dr.ednsAdded, dr.ecsAdded, dr.useZeroScope ? &zeroScope : nullptr)) {
     return false;
@@ -463,7 +471,7 @@ bool processResponse(PacketBuffer& response, LocalStateHolder<vector<DNSDistResp
       zeroScope = false;
     }
     // if zeroScope, pass the pre-ECS hash-key and do not pass the subnet to the cache
-    dr.packetCache->insert(zeroScope ? dr.cacheKeyNoECS : dr.cacheKey, zeroScope ? boost::none : dr.subnet, dr.origFlags, dr.dnssecOK, *dr.qname, dr.qtype, dr.qclass, response, receivedOverUDP, dr.getHeader()->rcode, dr.tempFailureTTL);
+    dr.packetCache->insert(zeroScope ? dr.cacheKeyNoECS : dr.cacheKey, zeroScope ? boost::none : dr.subnet, cacheFlags, dr.dnssecOK, *dr.qname, dr.qtype, dr.qclass, response, receivedOverUDP, dr.getHeader()->rcode, dr.tempFailureTTL);
   }
 
 #ifdef HAVE_DNSCRYPT
@@ -1238,6 +1246,8 @@ ProcessQueryResult processQuery(DNSQuestion& dq, ClientState& cs, LocalHolders&
     if (dq.packetCache && !dq.skipCache) {
       if (dq.packetCache->get(dq, dq.getHeader()->id, &dq.cacheKey, dq.subnet, dq.dnssecOK, !dq.overTCP() || dq.getProtocol() == DNSQuestion::Protocol::DoH, allowExpired)) {
 
+        restoreFlags(dq.getHeader(), dq.origFlags);
+
         if (!prepareOutgoingResponse(holders, cs, dq, true)) {
           return ProcessQueryResult::Drop;
         }

diff --git a/regression-tests.dnsdist/test_Caching.py b/regression-tests.dnsdist/test_Caching.py
index e1a4e2b76ad7a53f849e9f6fc14ba5ecd44520a1..f5a6d4683813a8f81fd8df989c73be200104a404 100644 (file)
--- a/regression-tests.dnsdist/test_Caching.py
+++ b/regression-tests.dnsdist/test_Caching.py
@@ -851,7 +851,6 @@ class TestTempFailureCacheTTLAction(DNSDistTest):
         self.assertTrue(receivedResponse)
         self.assertEqual(receivedResponse, response)
 
-
 class TestCachingWithExistingEDNS(DNSDistTest):
 
     _config_template = """
@@ -2530,3 +2529,67 @@ class TestCachingScopeZeroButNoSubnetcheck(DNSDistTest):
             receivedQuery.id = expectedQuery2.id
             self.checkMessageEDNSWithECS(expectedQuery2, receivedQuery)
             self.checkMessageNoEDNS(receivedResponse, response)
+
+class TestCachingAlteredHeader(DNSDistTest):
+
+    _config_template = """
+    pc = newPacketCache(100)
+    getPool(""):setCache(pc)
+    addAction("cache-set-rd.tests.powerdns.com.", SetNoRecurseAction())
+    newServer{address="127.0.0.1:%d"}
+    """
+
+    def testCachingAlteredHeader(self):
+        """
+        Cache: The header has been altered via a rule
+        """
+        name = 'cache-set-rd.tests.powerdns.com.'
+        query = dns.message.make_query(name, 'A', 'IN')
+        # the query reaching the backend will never have the RD flag set
+        expectedQuery = dns.message.make_query(name, 'A', 'IN')
+        expectedQuery.flags &= ~dns.flags.RD
+        response = dns.message.make_response(query)
+        response.flags &= ~dns.flags.RD
+        rrset = dns.rrset.from_text(name,
+                                    60,
+                                    dns.rdataclass.IN,
+                                    dns.rdatatype.A,
+                                    '192.0.2.1')
+        response.answer.append(rrset)
+
+        # first query has RD=1
+        query.flags |= dns.flags.RD
+        expectedResponse = dns.message.make_response(query)
+        rrset = dns.rrset.from_text(name,
+                                    60,
+                                    dns.rdataclass.IN,
+                                    dns.rdatatype.A,
+                                    '192.0.2.1')
+        expectedResponse.answer.append(rrset)
+
+        (receivedQuery, receivedResponse) = self.sendUDPQuery(query, response)
+        self.assertTrue(receivedQuery)
+        self.assertTrue(receivedResponse)
+        receivedQuery.id = expectedQuery.id
+        self.assertEqual(expectedQuery, receivedQuery)
+        self.assertEqual(receivedResponse, expectedResponse)
+
+        # next query should hit the cache
+        (receivedQuery, receivedResponse) = self.sendUDPQuery(query, response=None, useQueue=False)
+        self.assertFalse(receivedQuery)
+        self.assertTrue(receivedResponse)
+        self.assertEqual(receivedResponse, expectedResponse)
+
+        # same query with RD=0, should hit the cache as well
+        query.flags &= ~dns.flags.RD
+        expectedResponse = dns.message.make_response(query)
+        rrset = dns.rrset.from_text(name,
+                                    60,
+                                    dns.rdataclass.IN,
+                                    dns.rdatatype.A,
+                                    '192.0.2.1')
+        expectedResponse.answer.append(rrset)
+        (receivedQuery, receivedResponse) = self.sendUDPQuery(query, response=None, useQueue=False)
+        self.assertFalse(receivedQuery)
+        self.assertTrue(receivedResponse)
+        self.assertEqual(receivedResponse, expectedResponse)