rec: backport 12395 to rec-4.8.x: When the stale function is triggered, wrong data...

Backport of #12395

diff --git a/pdns/recursordist/negcache.cc b/pdns/recursordist/negcache.cc
index cf3b76fe8fb74b7b976724ca33078136ed27bb80..b929cf76a94a7320306ffd22c177485144144005 100644 (file)
--- 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 01a4e402b665a3ecb6e7362220fe787be56d65e0..f80c28e7a2c29592ccc128543de1824e85c05f4d 100644 (file)
--- 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 8603b6ea6609743e4444eff67f6bb3783cb5c9f0..e41695632842bf6f730318b980d2b661235c9fab 100644 (file)
--- 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<SyncRes> sr;
+  initSR(sr);
+  MemRecursorCache::s_maxServedStaleExtensions = 1440;
+  NegCache::s_maxServedStaleExtensions = 1440;
+
+  primeHints();
+
+  const DNSName target("powerdns.com.");
+
+  std::set<ComboAddress> 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<Netmask>& srcmask, boost::optional<const ResolveContext&> 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<DNSRecord> 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<SyncRes> 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<ComboAddress> 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<Netmask>& srcmask, boost::optional<const ResolveContext&> 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<DNSRecord> 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 54602efabfb1faab8342034540bb249a5f356b34..c01a6ff1c9ae9bd24384e3ae940eeb6586c9a114 100644 (file)
--- 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);