From: Remi Gacogne Date: Mon, 16 Aug 2021 10:51:15 +0000 (+0200) Subject: dnsdist: Fix the wrong RD and CD flags being cached, causing misses X-Git-Tag: dnsdist-1.7.0-alpha1~63^2~1 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=dbadb4d272a3317407e6bc934f55c2d41a87c0ac;p=thirdparty%2Fpdns.git 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 920fc78bb8..8f3e34b114 100644 --- 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((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, LocalStateHolderinsert(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 e1a4e2b76a..f5a6d46838 100644 --- 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)