rec: Keep a cached, valid entry over a fresher Bogus one

It turns out to be quite difficult to make us accept a record that
we already have in cache, thanks to sanitization, but let's make
sure that we will not replace a valid entry with a Bogus one if that
happens.
It might happen for SOA records, and for DS records when the TTL of
the corresponding NS records is shorter than the TTL of the DS.

diff --git a/pdns/recursor_cache.cc b/pdns/recursor_cache.cc
index 8961eb8dbe3efaea33133b4c03dc50e0abefae93..0735f256dfe6ddeb57c825782764e5a5b05ab8b1 100644 (file)
--- a/pdns/recursor_cache.cc
+++ b/pdns/recursor_cache.cc
@@ -405,7 +405,6 @@ void MemRecursorCache::replace(time_t now, const DNSName &qname, const QType& qt
   ce.d_qtype=qt.getCode();
   ce.d_signatures=signatures;
   ce.d_authorityRecs=authorityRecs;
-  ce.d_state=state;
   
   //  cerr<<"asked to store "<< (qname.empty() ? "EMPTY" : qname.toString()) <<"|"+qt.getName()<<" -> '";
   //  cerr<<(content.empty() ? string("EMPTY CONTENT")  : content.begin()->d_content->getZoneRepresentation())<<"', auth="<<auth<<", ce.auth="<<ce.d_auth;
@@ -421,6 +420,16 @@ void MemRecursorCache::replace(time_t now, const DNSName &qname, const QType& qt
     }
   }
 
+  if (auth) {
+    /* only if the new entry is auth, we don't want to keep non-auth entry while we have a auth one */
+    if (vStateIsBogus(state) && (!vStateIsBogus(ce.d_state) && ce.d_state != vState::Indeterminate) && ce.d_ttd > now) {
+      /* the new entry is Bogus, the existing one is not and is still valid, let's keep the existing one */
+      return;
+    }
+  }
+
+  ce.d_state = state;
+
   // refuse any attempt to *raise* the TTL of auth NS records, as it would make it possible
   // for an auth to keep a "ghost" zone alive forever, even after the delegation is gone from
   // the parent

diff --git a/pdns/recursordist/test-recursorcache_cc.cc b/pdns/recursordist/test-recursorcache_cc.cc
index ab18ae9d489cef886217cbdc73baf23a11e5a4f5..a96edf020d9faf912e9c1f788ff98aa5a0db17ae 100644 (file)
--- a/pdns/recursordist/test-recursorcache_cc.cc
+++ b/pdns/recursordist/test-recursorcache_cc.cc
@@ -269,6 +269,31 @@ BOOST_AUTO_TEST_CASE(test_RecursorCacheSimple)
     BOOST_REQUIRE_EQUAL(retrieved.size(), 1U);
     BOOST_CHECK_EQUAL(getRR<ARecordContent>(retrieved.at(0))->getCA().toString(), dr2Content.toString());
 
+    // wipe everything
+    MRC.doWipeCache(DNSName("."), true);
+    BOOST_CHECK_EQUAL(MRC.size(), 0U);
+    records.clear();
+
+    // insert Secure record
+    records.push_back(dr2);
+    MRC.replace(now, power, QType(QType::A), records, signatures, authRecords, true, boost::none, boost::none, vState::Secure);
+    BOOST_CHECK_EQUAL(MRC.size(), 1U);
+    vState retrievedState = vState::Indeterminate;
+    bool wasAuth = false;
+    BOOST_CHECK_EQUAL(MRC.get(now, power, QType(QType::A), false, &retrieved, ComboAddress("127.0.0.1"), boost::none, nullptr, nullptr, nullptr, &retrievedState, &wasAuth), (ttd - now));
+    BOOST_CHECK_EQUAL(retrieved.size(), 1U);
+    BOOST_CHECK_EQUAL(vStateToString(retrievedState), vStateToString(vState::Secure));
+    BOOST_CHECK_EQUAL(wasAuth, true);
+    // try to replace that with a Bogus record
+    MRC.replace(now, power, QType(QType::A), records, signatures, authRecords, true, boost::none, boost::none, vState::BogusNoRRSIG);
+    BOOST_CHECK_EQUAL(MRC.size(), 1U);
+    retrievedState = vState::Indeterminate;
+    wasAuth = false;
+    BOOST_CHECK_EQUAL(MRC.get(now, power, QType(QType::A), false, &retrieved, ComboAddress("127.0.0.1"), boost::none, nullptr, nullptr, nullptr, &retrievedState, &wasAuth), (ttd - now));
+    BOOST_CHECK_EQUAL(retrieved.size(), 1U);
+    BOOST_CHECK_EQUAL(vStateToString(retrievedState), vStateToString(vState::Secure));
+    BOOST_CHECK_EQUAL(wasAuth, true);
+
     /**** Most specific netmask tests ****/
 
     // wipe everything

diff --git a/pdns/recursordist/test-syncres_cc9.cc b/pdns/recursordist/test-syncres_cc9.cc
index 5ad9d78e9b8e694fd346b27ea92aa11f0175467e..e0806f12417009d02e4aa7099df91199accde0e7 100644 (file)
--- a/pdns/recursordist/test-syncres_cc9.cc
+++ b/pdns/recursordist/test-syncres_cc9.cc
@@ -951,6 +951,87 @@ BOOST_AUTO_TEST_CASE(test_cname_plus_authority_ns_ttl)
   BOOST_CHECK_EQUAL(wasAuth, false);
 }
 
+BOOST_AUTO_TEST_CASE(test_bogus_does_not_replace_secure_in_the_cache)
+{
+  std::unique_ptr<SyncRes> sr;
+  initSR(sr, true);
+
+  setDNSSECValidation(sr, DNSSECMode::ValidateAll);
+
+  primeHints();
+
+  testkeysset_t keys;
+  auto luaconfsCopy = g_luaconfs.getCopy();
+  luaconfsCopy.dsAnchors.clear();
+  generateKeyMaterial(g_rootdnsname, DNSSECKeeper::ECDSA256, DNSSECKeeper::DIGEST_SHA256, keys, luaconfsCopy.dsAnchors);
+  generateKeyMaterial(DNSName("com."), DNSSECKeeper::ECDSA256, DNSSECKeeper::DIGEST_SHA256, keys);
+  generateKeyMaterial(DNSName("powerdns.com."), DNSSECKeeper::ECDSA256, DNSSECKeeper::DIGEST_SHA256, keys);
+  g_luaconfs.setState(luaconfsCopy);
+
+  sr->setAsyncCallback([keys](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) {
+
+    if (type == QType::DS || type == QType::DNSKEY) {
+      if (domain == DNSName("cname.powerdns.com.")) {
+        return genericDSAndDNSKEYHandler(res, domain, domain, type, keys, false /* no cut */);
+      }
+      else {
+        return genericDSAndDNSKEYHandler(res, domain, domain, type, keys);
+      }
+    }
+
+    if (isRootServer(ip)) {
+      setLWResult(res, 0, false, false, true);
+      addRecordToLW(res, "powerdns.com.", QType::NS, "a.gtld-servers.com.", DNSResourceRecord::AUTHORITY, 3600);
+      addDS(DNSName("powerdns.com."), 300, res->d_records, keys);
+      addRRSIG(keys, res->d_records, DNSName("com."), 300);
+      addRecordToLW(res, "a.gtld-servers.com.", QType::A, "192.0.2.1", DNSResourceRecord::ADDITIONAL, 3600);
+      return LWResult::Result::Success;
+    }
+    else {
+      setLWResult(res, 0, true, false, true);
+      if (domain == DNSName("powerdns.com.") && type == QType::A) {
+        addRecordToLW(res, domain, QType::A, "192.0.2.1");
+        addRRSIG(keys, res->d_records, DNSName("powerdns.com."), 300);
+        addRecordToLW(res, domain, QType::SOA, "foo. bar. 2017032800 1800 900 604800 86400");
+        addRRSIG(keys, res->d_records, DNSName("powerdns.com."), 300);
+      }
+      else if (domain == DNSName("powerdns.com.") && type == QType::AAAA) {
+        addRecordToLW(res, domain, QType::AAAA, "2001:db8::1");
+        addRRSIG(keys, res->d_records, DNSName("powerdns.com."), 300);
+          addRecordToLW(res, domain, QType::SOA, "foo. bar. 2017032800 1800 900 604800 86400");
+        /* no RRSIG this time! */
+      }
+
+      return LWResult::Result::Success;
+    }
+  });
+
+  const time_t now = sr->getNow().tv_sec;
+
+  vector<DNSRecord> ret;
+  int res = sr->beginResolve(DNSName("powerdns.com."), QType(QType::A), QClass::IN, ret);
+  BOOST_CHECK_EQUAL(res, RCode::NoError);
+  BOOST_REQUIRE_EQUAL(ret.size(), 3U);
+
+  const ComboAddress who;
+  vector<DNSRecord> cached;
+  bool wasAuth = false;
+  vState retrievedState = vState::Insecure;
+  BOOST_CHECK_GT(g_recCache->get(now, DNSName("powerdns.com."), QType(QType::SOA), true, &cached, who, boost::none, nullptr, nullptr, nullptr, &retrievedState, &wasAuth), 0);
+  BOOST_CHECK_EQUAL(vStateToString(retrievedState), vStateToString(vState::Secure));
+  BOOST_CHECK_EQUAL(wasAuth, true);
+
+  ret.clear();
+  res = sr->beginResolve(DNSName("powerdns.com."), QType(QType::AAAA), QClass::IN, ret);
+  BOOST_CHECK_EQUAL(res, RCode::NoError);
+  BOOST_REQUIRE_EQUAL(ret.size(), 2U);
+
+  cached.clear();
+  BOOST_CHECK_GT(g_recCache->get(now, DNSName("powerdns.com."), QType(QType::SOA), true, &cached, who, boost::none, nullptr, nullptr, nullptr, &retrievedState, &wasAuth), 0);
+  BOOST_CHECK_EQUAL(vStateToString(retrievedState), vStateToString(vState::Secure));
+  BOOST_CHECK_EQUAL(wasAuth, true);
+}
+
 BOOST_AUTO_TEST_CASE(test_records_sanitization_general)
 {
   std::unique_ptr<SyncRes> sr;