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 95ec2b282a4c5d44663756f34fb09e17d0f33917..987a6aa5a1ab35fb3ac0a2492a66b86d3792f4dd 100644 (file)
--- a/pdns/recursordist/syncres.cc
+++ b/pdns/recursordist/syncres.cc
@@ -2310,7 +2310,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;
 }
 
@@ -2333,8 +2333,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);
@@ -4408,9 +4407,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) {
@@ -4423,6 +4419,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);
 }
 
@@ -4730,6 +4730,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
@@ -4834,6 +4835,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));
@@ -4854,7 +4856,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 7e5db3c57b3d1a6fb998593c45ffc8867dfee272..af671bda4623aea911d5a035329b5a8e3815ff21 100644 (file)
--- a/pdns/recursordist/test-syncres_cc4.cc
+++ b/pdns/recursordist/test-syncres_cc4.cc
@@ -1908,7 +1908,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);
 
@@ -1968,7 +1968,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();
@@ -1976,7 +1976,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();
@@ -1984,7 +1984,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