From: Otto Moerbeek Date: Mon, 23 Jan 2023 15:55:14 +0000 (+0100) Subject: rec: backport 12395 to rec-4.8.x: When the stale function is triggered, wrong data... X-Git-Tag: rec-4.8.2~2^2 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=refs%2Fpull%2F12457%2Fhead;p=thirdparty%2Fpdns.git rec: backport 12395 to rec-4.8.x: When the stale function is triggered, wrong data can be returned from negcache and record cache Backport of #12395 --- diff --git a/pdns/recursordist/negcache.cc b/pdns/recursordist/negcache.cc index cf3b76fe8f..b929cf76a9 100644 --- a/pdns/recursordist/negcache.cc +++ b/pdns/recursordist/negcache.cc @@ -238,6 +238,26 @@ size_t NegCache::wipe(const DNSName& name, bool subtree) return ret; } +size_t NegCache::wipe(const DNSName& qname, QType qtype) +{ + size_t ret = 0; + auto& map = getMap(qname); + auto content = map.lock(); + auto range = content->d_map.equal_range(std::tie(qname)); + auto i = range.first; + while (i != range.second) { + if (i->d_qtype == QType::ENT || i->d_qtype == qtype) { + i = content->d_map.erase(i); + ++ret; + --map.d_entriesCount; + } + else { + ++i; + } + } + return ret; +} + /*! * Clear the negative cache */ diff --git a/pdns/recursordist/negcache.hh b/pdns/recursordist/negcache.hh index 01a4e402b6..f80c28e7a2 100644 --- a/pdns/recursordist/negcache.hh +++ b/pdns/recursordist/negcache.hh @@ -96,6 +96,7 @@ public: void clear(); size_t doDump(int fd, size_t maxCacheEntries); size_t wipe(const DNSName& name, bool subtree = false); + size_t wipe(const DNSName& name, QType qtype); size_t size() const; private: diff --git a/pdns/recursordist/test-syncres_cc10.cc b/pdns/recursordist/test-syncres_cc10.cc index 8603b6ea66..e416956328 100644 --- a/pdns/recursordist/test-syncres_cc10.cc +++ b/pdns/recursordist/test-syncres_cc10.cc @@ -1331,6 +1331,303 @@ BOOST_AUTO_TEST_CASE(test_servestale_neg) BOOST_CHECK_EQUAL(lookupCount, 2U); } +BOOST_AUTO_TEST_CASE(test_servestale_neg_to_available) +{ + std::unique_ptr sr; + initSR(sr); + MemRecursorCache::s_maxServedStaleExtensions = 1440; + NegCache::s_maxServedStaleExtensions = 1440; + + primeHints(); + + const DNSName target("powerdns.com."); + + std::set downServers; + size_t downCount = 0; + size_t lookupCount = 0; + bool negLookup = true; + + const int theTTL = 5; + const int negTTL = 60; + + sr->setAsyncCallback([&downServers, &downCount, &lookupCount, &negLookup, target](const ComboAddress& ip, const DNSName& domain, int type, bool doTCP, bool sendRDQuery, int EDNS0Level, struct timeval* now, boost::optional& srcmask, boost::optional context, LWResult* res, bool* chained) { + /* this will cause issue with qname minimization if we ever implement it */ + if (downServers.find(ip) != downServers.end()) { + downCount++; + return LWResult::Result::Timeout; + } + + if (isRootServer(ip)) { + setLWResult(res, 0, false, false, true); + addRecordToLW(res, "com.", QType::NS, "a.gtld-servers.net.", DNSResourceRecord::AUTHORITY); + addRecordToLW(res, "a.gtld-servers.net.", QType::A, "192.0.2.1", DNSResourceRecord::ADDITIONAL); + addRecordToLW(res, "a.gtld-servers.net.", QType::AAAA, "2001:DB8::1", DNSResourceRecord::ADDITIONAL); + return LWResult::Result::Success; + } + else if (ip == ComboAddress("192.0.2.1:53") || ip == ComboAddress("[2001:DB8::1]:53")) { + setLWResult(res, 0, false, false, true); + addRecordToLW(res, "powerdns.com.", QType::NS, "pdns-public-ns1.powerdns.com.", DNSResourceRecord::AUTHORITY, theTTL); + addRecordToLW(res, "powerdns.com.", QType::NS, "pdns-public-ns2.powerdns.com.", DNSResourceRecord::AUTHORITY, theTTL); + addRecordToLW(res, "pdns-public-ns1.powerdns.com.", QType::A, "192.0.2.2", DNSResourceRecord::ADDITIONAL, theTTL); + addRecordToLW(res, "pdns-public-ns1.powerdns.com.", QType::AAAA, "2001:DB8::2", DNSResourceRecord::ADDITIONAL, theTTL); + addRecordToLW(res, "pdns-public-ns2.powerdns.com.", QType::A, "192.0.2.3", DNSResourceRecord::ADDITIONAL, theTTL); + addRecordToLW(res, "pdns-public-ns2.powerdns.com.", QType::AAAA, "2001:DB8::3", DNSResourceRecord::ADDITIONAL, theTTL); + return LWResult::Result::Success; + } + else if (ip == ComboAddress("192.0.2.2:53") || ip == ComboAddress("192.0.2.3:53") || ip == ComboAddress("[2001:DB8::2]:53") || ip == ComboAddress("[2001:DB8::3]:53")) { + if (negLookup) { + setLWResult(res, 0, true, false, true); + addRecordToLW(res, "powerdns.com.", QType::SOA, "pdns-public-ns1.powerdns.com. pieter\\.lexis.powerdns.com. 2017032301 10800 3600 604800 60", DNSResourceRecord::AUTHORITY, negTTL); + lookupCount++; + return LWResult::Result::Success; + } + else { + setLWResult(res, 0, true, false, true); + addRecordToLW(res, target, QType::A, "192.0.2.4", DNSResourceRecord::ANSWER, theTTL); + lookupCount++; + return LWResult::Result::Success; + } + } + else { + return LWResult::Result::Timeout; + } + }); + + time_t now = time(nullptr); + + vector ret; + int res = sr->beginResolve(target, QType(QType::A), QClass::IN, ret); + BOOST_CHECK_EQUAL(res, RCode::NoError); + BOOST_REQUIRE_EQUAL(ret.size(), 1U); + BOOST_CHECK_EQUAL(ret[0].d_name, target); + BOOST_CHECK(ret[0].d_type == QType::SOA); + BOOST_CHECK_EQUAL(downCount, 0U); + BOOST_CHECK_EQUAL(lookupCount, 1U); + + downServers.insert(ComboAddress("192.0.2.2:53")); + downServers.insert(ComboAddress("192.0.2.3:53")); + downServers.insert(ComboAddress("[2001:DB8::2]:53")); + downServers.insert(ComboAddress("[2001:DB8::3]:53")); + + sr->setNow(timeval{now + negTTL + 1, 0}); + + // record is expired, so serve stale should kick in + ret.clear(); + res = sr->beginResolve(target, QType(QType::A), QClass::IN, ret); + BOOST_CHECK_EQUAL(res, RCode::NoError); + BOOST_REQUIRE_EQUAL(ret.size(), 1U); + BOOST_CHECK(ret[0].d_type == QType::SOA); + BOOST_CHECK_EQUAL(ret[0].d_name, target); + BOOST_CHECK_EQUAL(downCount, 4U); + BOOST_CHECK_EQUAL(lookupCount, 1U); + + // Again, no lookup as the record is marked stale + ret.clear(); + res = sr->beginResolve(target, QType(QType::A), QClass::IN, ret); + BOOST_CHECK_EQUAL(res, RCode::NoError); + BOOST_REQUIRE_EQUAL(ret.size(), 1U); + BOOST_CHECK(ret[0].d_type == QType::SOA); + BOOST_CHECK_EQUAL(ret[0].d_name, target); + BOOST_CHECK_EQUAL(downCount, 4U); + BOOST_CHECK_EQUAL(lookupCount, 1U); + + // Again, no lookup as the record is marked stale but as the TTL has passed a task should have been pushed + sr->setNow(timeval{now + 2 * (negTTL + 1), 0}); + ret.clear(); + res = sr->beginResolve(target, QType(QType::A), QClass::IN, ret); + BOOST_CHECK_EQUAL(res, RCode::NoError); + BOOST_REQUIRE_EQUAL(ret.size(), 1U); + BOOST_CHECK(ret[0].d_type == QType::SOA); + BOOST_CHECK_EQUAL(ret[0].d_name, target); + BOOST_CHECK_EQUAL(downCount, 4U); + BOOST_CHECK_EQUAL(lookupCount, 1U); + + BOOST_REQUIRE_EQUAL(getTaskSize(), 1U); + auto task = taskQueuePop(); + BOOST_CHECK(task.d_qname == target); + BOOST_CHECK_EQUAL(task.d_qtype, QType::A); + + // Now simulate a succeeding task execution an record has become available + negLookup = false; + sr->setNow(timeval{now + 3 * (negTTL + 1), 0}); + downServers.clear(); + sr->setRefreshAlmostExpired(true); + ret.clear(); + res = sr->beginResolve(target, QType(QType::A), QClass::IN, ret); + BOOST_CHECK_EQUAL(res, RCode::NoError); + BOOST_REQUIRE_EQUAL(ret.size(), 1U); + BOOST_CHECK(ret[0].d_type == QType::A); + BOOST_CHECK_EQUAL(ret[0].d_name, target); + BOOST_CHECK_EQUAL(downCount, 4U); + BOOST_CHECK_EQUAL(lookupCount, 2U); + + // And again, result should come from cache + sr->setRefreshAlmostExpired(false); + ret.clear(); + res = sr->beginResolve(target, QType(QType::A), QClass::IN, ret); + BOOST_CHECK_EQUAL(res, RCode::NoError); + BOOST_REQUIRE_EQUAL(ret.size(), 1U); + BOOST_CHECK(ret[0].d_type == QType::A); + BOOST_CHECK_EQUAL(ret[0].d_name, target); + BOOST_CHECK_EQUAL(downCount, 4U); + BOOST_CHECK_EQUAL(lookupCount, 2U); +} + +BOOST_AUTO_TEST_CASE(test_servestale_cname_to_nxdomain) +{ + std::unique_ptr sr; + initSR(sr); + MemRecursorCache::s_maxServedStaleExtensions = 1440; + NegCache::s_maxServedStaleExtensions = 1440; + + primeHints(); + + const DNSName target("www.powerdns.com."); + const DNSName auth("powerdns.com."); + + std::set downServers; + size_t downCount = 0; + size_t lookupCount = 0; + bool cnameOK = true; + + const int theTTL = 5; + const int negTTL = 60; + + sr->setAsyncCallback([&downServers, &downCount, &lookupCount, &cnameOK, target, auth](const ComboAddress& ip, const DNSName& domain, int type, bool doTCP, bool sendRDQuery, int EDNS0Level, struct timeval* now, boost::optional& srcmask, boost::optional context, LWResult* res, bool* chained) { + /* this will cause issue with qname minimization if we ever implement it */ + if (downServers.find(ip) != downServers.end()) { + downCount++; + return LWResult::Result::Timeout; + } + + if (isRootServer(ip)) { + setLWResult(res, 0, false, false, true); + addRecordToLW(res, "com.", QType::NS, "a.gtld-servers.net.", DNSResourceRecord::AUTHORITY); + addRecordToLW(res, "a.gtld-servers.net.", QType::A, "192.0.2.1", DNSResourceRecord::ADDITIONAL); + addRecordToLW(res, "a.gtld-servers.net.", QType::AAAA, "2001:DB8::1", DNSResourceRecord::ADDITIONAL); + return LWResult::Result::Success; + } + else if (ip == ComboAddress("192.0.2.1:53") || ip == ComboAddress("[2001:DB8::1]:53")) { + setLWResult(res, 0, false, false, true); + addRecordToLW(res, "powerdns.com.", QType::NS, "pdns-public-ns1.powerdns.com.", DNSResourceRecord::AUTHORITY, theTTL); + addRecordToLW(res, "powerdns.com.", QType::NS, "pdns-public-ns2.powerdns.com.", DNSResourceRecord::AUTHORITY, theTTL); + addRecordToLW(res, "pdns-public-ns1.powerdns.com.", QType::A, "192.0.2.2", DNSResourceRecord::ADDITIONAL, theTTL); + addRecordToLW(res, "pdns-public-ns1.powerdns.com.", QType::AAAA, "2001:DB8::2", DNSResourceRecord::ADDITIONAL, theTTL); + addRecordToLW(res, "pdns-public-ns2.powerdns.com.", QType::A, "192.0.2.3", DNSResourceRecord::ADDITIONAL, theTTL); + addRecordToLW(res, "pdns-public-ns2.powerdns.com.", QType::AAAA, "2001:DB8::3", DNSResourceRecord::ADDITIONAL, theTTL); + return LWResult::Result::Success; + } + else if (ip == ComboAddress("192.0.2.2:53") || ip == ComboAddress("192.0.2.3:53") || ip == ComboAddress("[2001:DB8::2]:53") || ip == ComboAddress("[2001:DB8::3]:53")) { + if (cnameOK) { + setLWResult(res, 0, true, false, true); + addRecordToLW(res, target, QType::CNAME, "cname.powerdns.com.", DNSResourceRecord::ANSWER, 5); + addRecordToLW(res, DNSName("cname.powerdns.com"), QType::A, "192.0.2.4", DNSResourceRecord::ANSWER, theTTL); + lookupCount++; + return LWResult::Result::Success; + } + else { + setLWResult(res, RCode::NXDomain, true, false, true); + addRecordToLW(res, auth, QType::SOA, "pdns-public-ns1.powerdns.com. pieter\\.lexis.powerdns.com. 2017032301 10800 3600 604800 60", DNSResourceRecord::AUTHORITY, negTTL); + lookupCount++; + return LWResult::Result::Success; + } + } + else { + return LWResult::Result::Timeout; + } + }); + + time_t now = time(nullptr); + + vector ret; + int res = sr->beginResolve(target, QType(QType::A), QClass::IN, ret); + BOOST_REQUIRE_EQUAL(ret.size(), 2U); + BOOST_CHECK(ret[0].d_type == QType::CNAME); + BOOST_CHECK(ret[1].d_type == QType::A); + BOOST_CHECK_EQUAL(ret[0].d_name, target); + BOOST_CHECK_EQUAL(ret[1].d_name, DNSName("cname.powerdns.com")); + BOOST_CHECK_EQUAL(downCount, 0U); + BOOST_CHECK_EQUAL(lookupCount, 2U); + + downServers.insert(ComboAddress("192.0.2.2:53")); + downServers.insert(ComboAddress("192.0.2.3:53")); + downServers.insert(ComboAddress("[2001:DB8::2]:53")); + downServers.insert(ComboAddress("[2001:DB8::3]:53")); + + sr->setNow(timeval{now + theTTL + 1, 0}); + + // record is expired, so serve stale should kick in + ret.clear(); + res = sr->beginResolve(target, QType(QType::A), QClass::IN, ret); + BOOST_CHECK_EQUAL(res, RCode::NoError); + BOOST_REQUIRE_EQUAL(ret.size(), 2U); + BOOST_CHECK(ret[0].d_type == QType::CNAME); + BOOST_CHECK(ret[1].d_type == QType::A); + BOOST_CHECK_EQUAL(ret[0].d_name, target); + BOOST_CHECK_EQUAL(ret[1].d_name, DNSName("cname.powerdns.com")); + BOOST_CHECK_EQUAL(downCount, 4U); + BOOST_CHECK_EQUAL(lookupCount, 2U); + + // Again, no lookup as the record is marked stale + ret.clear(); + res = sr->beginResolve(target, QType(QType::A), QClass::IN, ret); + BOOST_CHECK_EQUAL(res, RCode::NoError); + BOOST_REQUIRE_EQUAL(ret.size(), 2U); + BOOST_CHECK(ret[0].d_type == QType::CNAME); + BOOST_CHECK(ret[1].d_type == QType::A); + BOOST_CHECK_EQUAL(ret[0].d_name, target); + BOOST_CHECK_EQUAL(ret[1].d_name, DNSName("cname.powerdns.com")); + BOOST_CHECK_EQUAL(downCount, 4U); + BOOST_CHECK_EQUAL(lookupCount, 2U); + + // Again, no lookup as the record is marked stale but as the TTL has passed a task should have been pushed + sr->setNow(timeval{now + 2 * (theTTL + 1), 0}); + ret.clear(); + res = sr->beginResolve(target, QType(QType::A), QClass::IN, ret); + BOOST_CHECK_EQUAL(res, RCode::NoError); + BOOST_REQUIRE_EQUAL(ret.size(), 2U); + BOOST_CHECK(ret[0].d_type == QType::CNAME); + BOOST_CHECK(ret[1].d_type == QType::A); + BOOST_CHECK_EQUAL(ret[0].d_name, target); + BOOST_CHECK_EQUAL(ret[1].d_name, DNSName("cname.powerdns.com")); + BOOST_CHECK_EQUAL(downCount, 4U); + BOOST_CHECK_EQUAL(lookupCount, 2U); + + BOOST_REQUIRE_EQUAL(getTaskSize(), 2U); + auto task = taskQueuePop(); + BOOST_CHECK(task.d_qname == target); + BOOST_CHECK_EQUAL(task.d_qtype, QType::CNAME); + task = taskQueuePop(); + BOOST_CHECK(task.d_qname == DNSName("cname.powerdns.com")); + BOOST_CHECK_EQUAL(task.d_qtype, QType::A); + + // Now simulate a succeeding task execution and NxDomain on explicit CNAME result becomes available + cnameOK = false; + sr->setNow(timeval{now + 3 * (theTTL + 1), 0}); + downServers.clear(); + sr->setRefreshAlmostExpired(true); + + ret.clear(); + res = sr->beginResolve(target, QType(QType::CNAME), QClass::IN, ret); + BOOST_CHECK_EQUAL(res, RCode::NXDomain); + BOOST_REQUIRE_EQUAL(ret.size(), 1U); + BOOST_CHECK(ret[0].d_type == QType::SOA); + BOOST_CHECK_EQUAL(ret[0].d_name, auth); + BOOST_CHECK_EQUAL(downCount, 4U); + BOOST_CHECK_EQUAL(lookupCount, 3U); + + // And again, result should come from cache + sr->setRefreshAlmostExpired(false); + ret.clear(); + res = sr->beginResolve(target, QType(QType::CNAME), QClass::IN, ret); + BOOST_CHECK_EQUAL(res, RCode::NXDomain); + BOOST_REQUIRE_EQUAL(ret.size(), 1U); + BOOST_CHECK(ret[0].d_type == QType::SOA); + BOOST_CHECK_EQUAL(ret[0].d_name, auth); + BOOST_CHECK_EQUAL(downCount, 4U); + BOOST_CHECK_EQUAL(lookupCount, 3U); +} + BOOST_AUTO_TEST_CASE(test_glued_referral_additional_update) { // Test that additional records update the cache diff --git a/pdns/syncres.cc b/pdns/syncres.cc index 54602efabf..c01a6ff1c9 100644 --- a/pdns/syncres.cc +++ b/pdns/syncres.cc @@ -4598,6 +4598,12 @@ RCode::rcodes_ SyncRes::updateCacheFromRecords(unsigned int depth, LWResult& lwr } g_recCache->replace(d_now.tv_sec, i->first.name, i->first.type, i->second.records, i->second.signatures, authorityRecs, i->first.type == QType::DS ? true : isAA, auth, i->first.place == DNSResourceRecord::ANSWER ? ednsmask : boost::none, d_routingTag, recordState, remoteIP, d_refresh); + // Delete potential negcache entry. When a record recovers with serve-stale the negcache entry can cause the wrong entry to + // be served, as negcache entries are checked before record cache entries + if (NegCache::s_maxServedStaleExtensions > 0) { + g_negCache->wipe(i->first.name, i->first.type); + } + if (g_aggressiveNSECCache && needWildcardProof && recordState == vState::Secure && i->first.place == DNSResourceRecord::ANSWER && i->first.name == qname && !i->second.signatures.empty() && !d_routingTag && !ednsmask) { /* we have an answer synthesized from a wildcard and aggressive NSEC is enabled, we need to store the wildcard in its non-expanded form in the cache to be able to synthesize wildcard answers later */ @@ -4764,6 +4770,11 @@ bool SyncRes::processRecords(const std::string& prefix, const DNSName& qname, co */ if (newtarget.empty() && putInNegCache) { g_negCache->add(ne); + // doCNAMECacheCheck() checks record cache and does not look into negcache. That means that an old record might be found if + // serve-stale is active. Avoid that by explicitly zapping that CNAME record. + if (qtype == QType::CNAME && MemRecursorCache::s_maxServedStaleExtensions > 0) { + g_recCache->doWipeCache(qname, false, qtype); + } if (s_rootNXTrust && ne.d_auth.isRoot() && auth.isRoot() && lwr.d_aabit) { ne.d_name = ne.d_name.getLastLabel(); g_negCache->add(ne);