From: Remi Gacogne <remi.gacogne@powerdns.com>
Date: Tue, 21 Nov 2017 09:42:43 +0000 (+0100)
Subject: rec: Cache Secure validation state when inserting negcache entries
X-Git-Tag: auth-4.1.0~23^2
X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=b25712fd08331ec8a34e99b144a0797b180b6762;p=pdns

rec: Cache Secure validation state when inserting negcache entries

Fix a bug that prevented Secure negative cache entries to be marked
as such when they were first inserted, marking them as Indeterminate
instead. This would require us to validate them a second time for no
valid reason.
---

diff --git a/pdns/recursordist/test-syncres_cc.cc b/pdns/recursordist/test-syncres_cc.cc
index 465bc364c..c5ed98d7f 100644
--- a/pdns/recursordist/test-syncres_cc.cc
+++ b/pdns/recursordist/test-syncres_cc.cc
@@ -8295,6 +8295,7 @@ BOOST_AUTO_TEST_CASE(test_dnssec_rrsig_negcache_validity) {
   BOOST_CHECK_EQUAL(SyncRes::t_sstorage.negcache.size(), 1);
   BOOST_REQUIRE_EQUAL(SyncRes::t_sstorage.negcache.get(target, QType(QType::A), sr->getNow(), ne), true);
   BOOST_CHECK_EQUAL(ne.d_ttd, now + 1);
+  BOOST_CHECK_EQUAL(ne.d_validationState, Secure);
   BOOST_CHECK_EQUAL(ne.authoritySOA.records.size(), 1);
   BOOST_CHECK_EQUAL(ne.authoritySOA.signatures.size(), 1);
   BOOST_CHECK_EQUAL(ne.DNSSECRecords.records.size(), 1);
@@ -8845,6 +8846,15 @@ BOOST_AUTO_TEST_CASE(test_dnssec_validation_from_negcache_secure) {
   BOOST_CHECK_EQUAL(sr->getValidationState(), Indeterminate);
   BOOST_REQUIRE_EQUAL(ret.size(), 4);
   BOOST_CHECK_EQUAL(queriesCount, 1);
+  /* check that the entry has not been negatively cached */
+  NegCache::NegCacheEntry ne;
+  BOOST_CHECK_EQUAL(SyncRes::t_sstorage.negcache.size(), 1);
+  BOOST_REQUIRE_EQUAL(SyncRes::t_sstorage.negcache.get(target, QType(QType::A), sr->getNow(), ne), true);
+  BOOST_CHECK_EQUAL(ne.d_validationState, Indeterminate);
+  BOOST_CHECK_EQUAL(ne.authoritySOA.records.size(), 1);
+  BOOST_CHECK_EQUAL(ne.authoritySOA.signatures.size(), 1);
+  BOOST_CHECK_EQUAL(ne.DNSSECRecords.records.size(), 1);
+  BOOST_CHECK_EQUAL(ne.DNSSECRecords.signatures.size(), 1);
 
   ret.clear();
   /* second one _does_ require validation */
@@ -8854,6 +8864,13 @@ BOOST_AUTO_TEST_CASE(test_dnssec_validation_from_negcache_secure) {
   BOOST_CHECK_EQUAL(sr->getValidationState(), Secure);
   BOOST_REQUIRE_EQUAL(ret.size(), 4);
   BOOST_CHECK_EQUAL(queriesCount, 4);
+  BOOST_CHECK_EQUAL(SyncRes::t_sstorage.negcache.size(), 1);
+  BOOST_REQUIRE_EQUAL(SyncRes::t_sstorage.negcache.get(target, QType(QType::A), sr->getNow(), ne), true);
+  BOOST_CHECK_EQUAL(ne.d_validationState, Secure);
+  BOOST_CHECK_EQUAL(ne.authoritySOA.records.size(), 1);
+  BOOST_CHECK_EQUAL(ne.authoritySOA.signatures.size(), 1);
+  BOOST_CHECK_EQUAL(ne.DNSSECRecords.records.size(), 1);
+  BOOST_CHECK_EQUAL(ne.DNSSECRecords.signatures.size(), 1);
 }
 
 BOOST_AUTO_TEST_CASE(test_dnssec_validation_from_negcache_insecure) {
@@ -8904,6 +8921,15 @@ BOOST_AUTO_TEST_CASE(test_dnssec_validation_from_negcache_insecure) {
   BOOST_CHECK_EQUAL(sr->getValidationState(), Indeterminate);
   BOOST_REQUIRE_EQUAL(ret.size(), 1);
   BOOST_CHECK_EQUAL(queriesCount, 1);
+  /* check that the entry has not been negatively cached */
+  NegCache::NegCacheEntry ne;
+  BOOST_CHECK_EQUAL(SyncRes::t_sstorage.negcache.size(), 1);
+  BOOST_REQUIRE_EQUAL(SyncRes::t_sstorage.negcache.get(target, QType(QType::A), sr->getNow(), ne), true);
+  BOOST_CHECK_EQUAL(ne.d_validationState, Indeterminate);
+  BOOST_CHECK_EQUAL(ne.authoritySOA.records.size(), 1);
+  BOOST_CHECK_EQUAL(ne.authoritySOA.signatures.size(), 0);
+  BOOST_CHECK_EQUAL(ne.DNSSECRecords.records.size(), 0);
+  BOOST_CHECK_EQUAL(ne.DNSSECRecords.signatures.size(), 0);
 
   ret.clear();
   /* second one _does_ require validation */
@@ -8913,6 +8939,12 @@ BOOST_AUTO_TEST_CASE(test_dnssec_validation_from_negcache_insecure) {
   BOOST_CHECK_EQUAL(sr->getValidationState(), Insecure);
   BOOST_REQUIRE_EQUAL(ret.size(), 1);
   BOOST_CHECK_EQUAL(queriesCount, 1);
+  BOOST_REQUIRE_EQUAL(SyncRes::t_sstorage.negcache.get(target, QType(QType::A), sr->getNow(), ne), true);
+  BOOST_CHECK_EQUAL(ne.d_validationState, Insecure);
+  BOOST_CHECK_EQUAL(ne.authoritySOA.records.size(), 1);
+  BOOST_CHECK_EQUAL(ne.authoritySOA.signatures.size(), 0);
+  BOOST_CHECK_EQUAL(ne.DNSSECRecords.records.size(), 0);
+  BOOST_CHECK_EQUAL(ne.DNSSECRecords.signatures.size(), 0);
 }
 
 BOOST_AUTO_TEST_CASE(test_dnssec_validation_from_negcache_bogus) {
@@ -8967,6 +8999,14 @@ BOOST_AUTO_TEST_CASE(test_dnssec_validation_from_negcache_bogus) {
   BOOST_CHECK_EQUAL(sr->getValidationState(), Indeterminate);
   BOOST_REQUIRE_EQUAL(ret.size(), 2);
   BOOST_CHECK_EQUAL(queriesCount, 1);
+  NegCache::NegCacheEntry ne;
+  BOOST_CHECK_EQUAL(SyncRes::t_sstorage.negcache.size(), 1);
+  BOOST_REQUIRE_EQUAL(SyncRes::t_sstorage.negcache.get(target, QType(QType::A), sr->getNow(), ne), true);
+  BOOST_CHECK_EQUAL(ne.d_validationState, Indeterminate);
+  BOOST_CHECK_EQUAL(ne.authoritySOA.records.size(), 1);
+  BOOST_CHECK_EQUAL(ne.authoritySOA.signatures.size(), 1);
+  BOOST_CHECK_EQUAL(ne.DNSSECRecords.records.size(), 0);
+  BOOST_CHECK_EQUAL(ne.DNSSECRecords.signatures.size(), 0);
 
   ret.clear();
   /* second one _does_ require validation */
@@ -8976,6 +9016,12 @@ BOOST_AUTO_TEST_CASE(test_dnssec_validation_from_negcache_bogus) {
   BOOST_CHECK_EQUAL(sr->getValidationState(), Bogus);
   BOOST_REQUIRE_EQUAL(ret.size(), 2);
   BOOST_CHECK_EQUAL(queriesCount, 4);
+  BOOST_REQUIRE_EQUAL(SyncRes::t_sstorage.negcache.get(target, QType(QType::A), sr->getNow(), ne), true);
+  BOOST_CHECK_EQUAL(ne.d_validationState, Bogus);
+  BOOST_CHECK_EQUAL(ne.authoritySOA.records.size(), 1);
+  BOOST_CHECK_EQUAL(ne.authoritySOA.signatures.size(), 1);
+  BOOST_CHECK_EQUAL(ne.DNSSECRecords.records.size(), 0);
+  BOOST_CHECK_EQUAL(ne.DNSSECRecords.signatures.size(), 0);
 }
 
 BOOST_AUTO_TEST_CASE(test_lowercase_outgoing) {
diff --git a/pdns/syncres.cc b/pdns/syncres.cc
index 5c026511a..570198165 100644
--- a/pdns/syncres.cc
+++ b/pdns/syncres.cc
@@ -1747,7 +1747,7 @@ void SyncRes::computeZoneCuts(const DNSName& begin, const DNSName& end, unsigned
     }
     else {
       /* remove the temporary cut */
-      LOG(d_prefix<<qname<<": removing cut state for "<<qname<<", was "<<vStates[d_cutStates[qname]]<<endl);
+      LOG(d_prefix<<qname<<": removing cut state for "<<qname<<endl);
       d_cutStates.erase(qname);
     }
   }
@@ -2081,7 +2081,10 @@ RCode::rcodes_ SyncRes::updateCacheFromRecords(unsigned int depth, LWResult& lwr
 
 void SyncRes::updateDenialValidationState(NegCache::NegCacheEntry& ne, vState& state, const dState denialState, const dState expectedState, bool allowOptOut)
 {
-  if (denialState != expectedState) {
+  if (denialState == expectedState) {
+    ne.d_validationState = Secure;
+  }
+  else {
     if (denialState == OPTOUT && allowOptOut) {
       LOG(d_prefix<<"OPT-out denial found for "<<ne.d_name<<endl);
       ne.d_validationState = Secure;