From: Remi Gacogne Date: Fri, 13 Dec 2024 14:45:31 +0000 (+0100) Subject: dnsdist: Fix ECS zero-scope with incoming DoH queries X-Git-Tag: dnsdist-2.0.0-alpha0~2^2 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=refs%2Fpull%2F14967%2Fhead;p=thirdparty%2Fpdns.git 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 259c55e92a..48fdeefc30 100644 --- 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 75ec0dd29a..758e0b7dad 100644 --- 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 = 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 f9421812b3..0f0f1195b4 100644 --- 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 = """