Don't cache non-auth rrsets if a Bogus rrset was found in the answer

Signed-off-by: Otto Moerbeek <otto.moerbeek@open-xchange.com>

diff --git a/pdns/recursordist/syncres.cc b/pdns/recursordist/syncres.cc
index 2ccc4607ee6d6b9211a61f81d077353b7893ab13..30f0b31d07c81d47f22431627bad6513c3435fb0 100644 (file)
--- a/pdns/recursordist/syncres.cc
+++ b/pdns/recursordist/syncres.cc
@@ -2226,7 +2226,7 @@ bool SyncRes::canUseRecords(const std::string& prefix, const DNSName& qname, con
     LOG(prefix << qname << ": Cannot use " << name << '/' << qtype << " records from cache: Bogus" << endl);
     return false;
   }
-  // We could validate Indeterminete authoritative records here.
+  // We could validate Indeterminate authoritative records here.
   return true;
 }
 
@@ -2249,8 +2249,7 @@ void SyncRes::getBestNSFromCache(const DNSName& qname, const QType qtype, vector
     *flawedNSSet = false;
 
     vState state{vState::Indeterminate};
-    if (bool isAuth = false; g_recCache->get(d_now.tv_sec, subdomain, QType::NS, flags, &nsVector, d_cacheRemote, d_routingTag, nullptr, nullptr, nullptr, nullptr, &isAuth) > 0 &&
-        canUseRecords(prefix, qname, subdomain, QType::NS, state)) {
+    if (bool isAuth = false; g_recCache->get(d_now.tv_sec, subdomain, QType::NS, flags, &nsVector, d_cacheRemote, d_routingTag, nullptr, nullptr, nullptr, &state, &isAuth) > 0 && canUseRecords(prefix, qname, subdomain, QType::NS, state)) {
       if (s_maxnsperresolve > 0 && nsVector.size() > s_maxnsperresolve) {
         vector<DNSRecord> selected;
         selected.reserve(s_maxnsperresolve);
@@ -4325,9 +4324,6 @@ void SyncRes::sanitizeRecords(const std::string& prefix, LWResult& lwr, const DN
         }
         soaInAuth = true;
       }
-      if (!haveAnswers && lwr.d_rcode == RCode::NoError) {
-        acceptDelegation = true;
-      }
     }
     /* dealing with records in additional */
     else if (rec->d_place == DNSResourceRecord::ADDITIONAL) {
@@ -4340,6 +4336,10 @@ void SyncRes::sanitizeRecords(const std::string& prefix, LWResult& lwr, const DN
     }
   } // end of first loop, handled answer and most of authority section
 
+  if (!haveAnswers && lwr.d_rcode == RCode::NoError) {
+    acceptDelegation = true;
+  }
+
   sanitizeRecordsPass2(prefix, lwr, qname, qtype, auth, allowedAnswerNames, allowedAdditionals, cnameSeen, acceptDelegation && !soaInAuth, skipvec, skipCount);
 }
 
@@ -4653,6 +4653,7 @@ RCode::rcodes_ SyncRes::updateCacheFromRecords(unsigned int depth, const string&
     }
   }
 
+  bool seenBogusRRSet = false;
   for (auto tCacheEntry = tcache.begin(); tCacheEntry != tcache.end(); ++tCacheEntry) {
 
     if (tCacheEntry->second.records.empty()) { // this happens when we did store signatures, but passed on the records themselves
@@ -4757,6 +4758,7 @@ RCode::rcodes_ SyncRes::updateCacheFromRecords(unsigned int depth, const string&
     }
 
     if (vStateIsBogus(recordState)) {
+      seenBogusRRSet = true;
       /* this is a TTD by now, be careful */
       for (auto& record : tCacheEntry->second.records) {
         auto newval = std::min(record.d_ttl, static_cast<uint32_t>(s_maxbogusttl + d_now.tv_sec));
@@ -4777,7 +4779,11 @@ RCode::rcodes_ SyncRes::updateCacheFromRecords(unsigned int depth, const string&
     if (tCacheEntry->first.type != QType::NSEC3 && (tCacheEntry->first.type == QType::DS || tCacheEntry->first.type == QType::NS || tCacheEntry->first.type == QType::A || tCacheEntry->first.type == QType::AAAA || isAA || wasForwardRecurse)) {
 
       bool doCache = true;
-      if (tCacheEntry->first.place == DNSResourceRecord::ANSWER && ednsmask) {
+      if (!isAA && seenBogusRRSet) {
+        LOG(prefix << qname << ": Not caching non-authoritative rrsets received with Bogus answer" << endl);
+        doCache = false;
+      }
+      if (doCache && tCacheEntry->first.place == DNSResourceRecord::ANSWER && ednsmask) {
         const bool isv4 = ednsmask->isIPv4();
         if ((isv4 && s_ecsipv4nevercache) || (!isv4 && s_ecsipv6nevercache)) {
           doCache = false;

diff --git a/pdns/recursordist/test-syncres_cc4.cc b/pdns/recursordist/test-syncres_cc4.cc
index 5ab63d1067219c4eafa36d07d257b5064fe2baa3..daf52d2c4c51dc3f06fdb88a46ce2b842033d114 100644 (file)
--- a/pdns/recursordist/test-syncres_cc4.cc
+++ b/pdns/recursordist/test-syncres_cc4.cc
@@ -1992,7 +1992,7 @@ BOOST_AUTO_TEST_CASE(test_dnssec_bogus_bad_algo)
 BOOST_AUTO_TEST_CASE(test_dnssec_bogus_unsigned_ds)
 {
   std::unique_ptr<SyncRes> sr;
-  initSR(sr, true);
+  initSR(sr, true, false);
 
   setDNSSECValidation(sr, DNSSECMode::ValidateAll);
 
@@ -2052,7 +2052,7 @@ BOOST_AUTO_TEST_CASE(test_dnssec_bogus_unsigned_ds)
   BOOST_CHECK_EQUAL(res, RCode::NoError);
   BOOST_CHECK_EQUAL(sr->getValidationState(), vState::BogusNoRRSIG);
   BOOST_REQUIRE_EQUAL(ret.size(), 2U);
-  BOOST_CHECK_EQUAL(queriesCount, 3U);
+  BOOST_CHECK_EQUAL(queriesCount, 4U);
 
   /* again, to test the cache */
   ret.clear();
@@ -2060,7 +2060,7 @@ BOOST_AUTO_TEST_CASE(test_dnssec_bogus_unsigned_ds)
   BOOST_CHECK_EQUAL(res, RCode::NoError);
   BOOST_CHECK_EQUAL(sr->getValidationState(), vState::BogusNoRRSIG);
   BOOST_REQUIRE_EQUAL(ret.size(), 2U);
-  BOOST_CHECK_EQUAL(queriesCount, 3U);
+  BOOST_CHECK_EQUAL(queriesCount, 4U);
 
   /* now we ask directly for the DS */
   ret.clear();
@@ -2068,7 +2068,7 @@ BOOST_AUTO_TEST_CASE(test_dnssec_bogus_unsigned_ds)
   BOOST_CHECK_EQUAL(res, RCode::NoError);
   BOOST_CHECK_EQUAL(sr->getValidationState(), vState::BogusNoRRSIG);
   BOOST_REQUIRE_EQUAL(ret.size(), 1U);
-  BOOST_CHECK_EQUAL(queriesCount, 3U);
+  BOOST_CHECK_EQUAL(queriesCount, 4U);
 }
 
 BOOST_AUTO_TEST_CASE(test_dnssec_bogus_unsigned_ds_direct)

diff --git a/regression-tests.recursor/ghost-1/expected_result b/regression-tests.recursor/ghost-1/expected_result
index bc000e8412804af40cb54a21e83b683b9c3e8f5e..cde45420211e488806627278206fff3ed68505cd 100644 (file)
--- a/regression-tests.recursor/ghost-1/expected_result
+++ b/regression-tests.recursor/ghost-1/expected_result
@@ -4,8 +4,8 @@ Reply to question for qname='a.www.1.ghost.example.net.', qtype=A
 0      b.www.1.ghost.example.net.      3600    IN      A       192.0.2.7
 Rcode: 0 (No Error), RD: 1, QR: 1, TC: 0, AA: 0, opcode: 0
 Reply to question for qname='b.www.1.ghost.example.net.', qtype=A
-0      c.www.1.ghost.example.net.      3600    IN      A       192.0.2.7
-Rcode: 0 (No Error), RD: 1, QR: 1, TC: 0, AA: 0, opcode: 0
+1      ghost.example.net.      3600    IN      SOA     ns.example.net. hostmaster.example.net. 1 3600 1800 1209600 300
+Rcode: 3 (Non-Existent domain), RD: 1, QR: 1, TC: 0, AA: 0, opcode: 0
 Reply to question for qname='c.www.1.ghost.example.net.', qtype=A
 1      ghost.example.net.      3600    IN      SOA     ns.example.net. hostmaster.example.net. 1 3600 1800 1209600 300
 Rcode: 3 (Non-Existent domain), RD: 1, QR: 1, TC: 0, AA: 0, opcode: 0

diff --git a/regression-tests.recursor/ghost-2/expected_result b/regression-tests.recursor/ghost-2/expected_result
index 7a2e7325d91693592ca94c572c7f552ca2f48b79..e17693e0734400f1a91a9ebdd759599dfcc4ad9d 100644 (file)
--- a/regression-tests.recursor/ghost-2/expected_result
+++ b/regression-tests.recursor/ghost-2/expected_result
@@ -4,8 +4,8 @@ Reply to question for qname='a.www.2.ghost.example.net.', qtype=A
 0      b.www.2.ghost.example.net.      3600    IN      A       192.0.2.8
 Rcode: 0 (No Error), RD: 1, QR: 1, TC: 0, AA: 0, opcode: 0
 Reply to question for qname='b.www.2.ghost.example.net.', qtype=A
-0      c.www.2.ghost.example.net.      3600    IN      A       192.0.2.8
-Rcode: 0 (No Error), RD: 1, QR: 1, TC: 0, AA: 0, opcode: 0
+1      ghost.example.net.      3600    IN      SOA     ns.example.net. hostmaster.example.net. 1 3600 1800 1209600 300
+Rcode: 3 (Non-Existent domain), RD: 1, QR: 1, TC: 0, AA: 0, opcode: 0
 Reply to question for qname='c.www.2.ghost.example.net.', qtype=A
 1      ghost.example.net.      3600    IN      SOA     ns.example.net. hostmaster.example.net. 1 3600 1800 1209600 300
 Rcode: 3 (Non-Existent domain), RD: 1, QR: 1, TC: 0, AA: 0, opcode: 0