From 860d5e8ea7662dbb2d463aea60b3cda310b7cec4 Mon Sep 17 00:00:00 2001
From: Remi Gacogne <remi.gacogne@powerdns.com>
Date: Fri, 5 May 2017 19:02:40 +0200
Subject: [PATCH] rec: Fix handling on DS denial during referral

---
 pdns/recursordist/test-syncres_cc.cc | 37 ++++++++++++++--------------
 pdns/syncres.cc                      | 27 ++++++++++----------
 pdns/syncres.hh                      |  2 +-
 pdns/validate.cc                     |  2 +-
 4 files changed, 34 insertions(+), 34 deletions(-)

diff --git a/pdns/recursordist/test-syncres_cc.cc b/pdns/recursordist/test-syncres_cc.cc
index 040d460f7..cbb1687b1 100644
--- a/pdns/recursordist/test-syncres_cc.cc
+++ b/pdns/recursordist/test-syncres_cc.cc
@@ -3790,7 +3790,6 @@ BOOST_AUTO_TEST_CASE(test_dnssec_insecure_unknown_ds_digest) {
   auto luaconfsCopy = g_luaconfs.getCopy();
   luaconfsCopy.dsAnchors.clear();
   luaconfsCopy.dsAnchors[g_rootdnsname].insert(drc);
-  cerr<<"inserted DS for root with tag "<<drc.d_tag<<" and algo "<<drc.d_algorithm<<endl;
   g_luaconfs.setState(luaconfsCopy);
 
   size_t queriesCount = 0;
@@ -4507,8 +4506,8 @@ BOOST_AUTO_TEST_CASE(test_dnssec_secure_to_insecure) {
   BOOST_CHECK_EQUAL(sr->getValidationState(), Insecure);
   BOOST_REQUIRE_EQUAL(ret.size(), 1);
   BOOST_CHECK(ret[0].d_type == QType::A);
-  /* only 5 because no DNSKEY query for powerdns.com (insecure) */
-  BOOST_CHECK_EQUAL(queriesCount, 5);
+  /* only 4 because no DS query for powerdns.com (DS denial in referral), and then no DNSKEY query either (insecure) */
+  BOOST_CHECK_EQUAL(queriesCount, 4);
 
   /* again, to test the cache */
   ret.clear();
@@ -4517,7 +4516,7 @@ BOOST_AUTO_TEST_CASE(test_dnssec_secure_to_insecure) {
   BOOST_CHECK_EQUAL(sr->getValidationState(), Insecure);
   BOOST_REQUIRE_EQUAL(ret.size(), 1);
   BOOST_CHECK(ret[0].d_type == QType::A);
-  BOOST_CHECK_EQUAL(queriesCount, 5);
+  BOOST_CHECK_EQUAL(queriesCount, 4);
 }
 
 BOOST_AUTO_TEST_CASE(test_dnssec_secure_to_insecure_nodata) {
@@ -4603,8 +4602,8 @@ BOOST_AUTO_TEST_CASE(test_dnssec_secure_to_insecure_nodata) {
   BOOST_CHECK_EQUAL(res, RCode::NoError);
   BOOST_CHECK_EQUAL(sr->getValidationState(), Insecure);
   BOOST_REQUIRE_EQUAL(ret.size(), 1);
-  /* only 5 because no DNSKEY query for powerdns.com (insecure) */
-  BOOST_CHECK_EQUAL(queriesCount, 5);
+  /* same as above */
+  BOOST_CHECK_EQUAL(queriesCount, 4);
 
   /* again, to test the cache */
   ret.clear();
@@ -4612,12 +4611,12 @@ BOOST_AUTO_TEST_CASE(test_dnssec_secure_to_insecure_nodata) {
   BOOST_CHECK_EQUAL(res, RCode::NoError);
   BOOST_CHECK_EQUAL(sr->getValidationState(), Insecure);
   BOOST_REQUIRE_EQUAL(ret.size(), 1);
-  BOOST_CHECK_EQUAL(queriesCount, 5);
+  BOOST_CHECK_EQUAL(queriesCount, 4);
 }
 
 BOOST_AUTO_TEST_CASE(test_dnssec_secure_to_insecure_cname) {
   std::unique_ptr<SyncRes> sr;
-  initSR(sr, true, true);
+  initSR(sr, true);
 
   g_dnssecmode = DNSSECMode::ValidateAll;
 
@@ -4710,7 +4709,7 @@ BOOST_AUTO_TEST_CASE(test_dnssec_secure_to_insecure_cname) {
   BOOST_CHECK_EQUAL(res, RCode::NoError);
   BOOST_CHECK_EQUAL(sr->getValidationState(), Insecure);
   BOOST_REQUIRE_EQUAL(ret.size(), 3);
-  BOOST_CHECK_EQUAL(queriesCount, 9);
+  BOOST_CHECK_EQUAL(queriesCount, 8);
 
   /* again, to test the cache */
   ret.clear();
@@ -4718,7 +4717,7 @@ BOOST_AUTO_TEST_CASE(test_dnssec_secure_to_insecure_cname) {
   BOOST_CHECK_EQUAL(res, RCode::NoError);
   BOOST_CHECK_EQUAL(sr->getValidationState(), Insecure);
   BOOST_REQUIRE_EQUAL(ret.size(), 3);
-  BOOST_CHECK_EQUAL(queriesCount, 9);
+  BOOST_CHECK_EQUAL(queriesCount, 8);
 }
 
 BOOST_AUTO_TEST_CASE(test_dnssec_insecure_ta) {
@@ -4807,8 +4806,8 @@ BOOST_AUTO_TEST_CASE(test_dnssec_insecure_ta) {
   /* We got a RRSIG */
   BOOST_REQUIRE_EQUAL(ret.size(), 2);
   BOOST_CHECK(ret[0].d_type == QType::A);
-  /* only 5 because no DNSKEY query for com (insecure) */
-  BOOST_CHECK_EQUAL(queriesCount, 5);
+  /* only 4 because no DNSKEY query for com (insecure) */
+  BOOST_CHECK_EQUAL(queriesCount, 4);
 
   /* again, to test the cache */
   ret.clear();
@@ -4817,7 +4816,7 @@ BOOST_AUTO_TEST_CASE(test_dnssec_insecure_ta) {
   BOOST_CHECK_EQUAL(sr->getValidationState(), Secure);
   BOOST_REQUIRE_EQUAL(ret.size(), 2);
   BOOST_CHECK(ret[0].d_type == QType::A);
-  BOOST_CHECK_EQUAL(queriesCount, 5);
+  BOOST_CHECK_EQUAL(queriesCount, 4);
 }
 
 BOOST_AUTO_TEST_CASE(test_dnssec_insecure_ta_norrsig) {
@@ -4906,8 +4905,8 @@ BOOST_AUTO_TEST_CASE(test_dnssec_insecure_ta_norrsig) {
   /* No RRSIG */
   BOOST_REQUIRE_EQUAL(ret.size(), 1);
   BOOST_CHECK(ret[0].d_type == QType::A);
-  /* only 4 because no DNSKEY query for com (insecure) and no RRSIG meaning no DNSKEY for powerdns.com */
-  BOOST_CHECK_EQUAL(queriesCount, 4);
+  /* only 3 because no DNSKEY query for com (insecure) and no RRSIG meaning no DNSKEY for powerdns.com */
+  BOOST_CHECK_EQUAL(queriesCount, 3);
 
   /* again, to test the cache */
   ret.clear();
@@ -4916,13 +4915,13 @@ BOOST_AUTO_TEST_CASE(test_dnssec_insecure_ta_norrsig) {
   BOOST_CHECK_EQUAL(sr->getValidationState(), Bogus);
   BOOST_REQUIRE_EQUAL(ret.size(), 1);
   BOOST_CHECK(ret[0].d_type == QType::A);
-  BOOST_CHECK_EQUAL(queriesCount, 4);
+  BOOST_CHECK_EQUAL(queriesCount, 3);
 }
 
 #if 0
 BOOST_AUTO_TEST_CASE(test_dnssec_secure_to_insecure_hidden_cut) {
   std::unique_ptr<SyncRes> sr;
-  initSR(sr, true, true, true);
+  initSR(sr, true, true);
 
   g_dnssecLOG = true;
   g_dnssecmode = DNSSECMode::ValidateAll;
@@ -4984,8 +4983,8 @@ BOOST_AUTO_TEST_CASE(test_dnssec_secure_to_insecure_hidden_cut) {
         else if (ip == ComboAddress("192.0.2.1:53")) {
           setLWResult(res, 0, false, false, true);
           addRecordToLW(res, domain, QType::NS, "ns.gov.nl.ca.", DNSResourceRecord::AUTHORITY, 3600);
-          /* no DS */
-          addNSECRecordToLW(domain, DNSName("gow.nl.ca."), { QType::NS }, 600, res->d_records);
+          /* denial of DS FOR nl.ca while sending a referral for gov.nl.ca !! */
+          addNSECRecordToLW(DNSName("nl.ca"), DNSName("nm.ca."), { QType::NS }, 600, res->d_records);
           addRRSIG(keys, res->d_records, DNSName("ca."), 300);
           addRecordToLW(res, "ns.gov.nl.ca.", QType::A, "192.0.2.2", DNSResourceRecord::ADDITIONAL, 3600);
           return 1;
diff --git a/pdns/syncres.cc b/pdns/syncres.cc
index 2b94aa536..72e67709b 100644
--- a/pdns/syncres.cc
+++ b/pdns/syncres.cc
@@ -1682,7 +1682,7 @@ RCode::rcodes_ SyncRes::updateCacheFromRecords(unsigned int depth, LWResult& lwr
   return RCode::NoError;
 }
 
-void SyncRes::getDenialValidationState(NegCache::NegCacheEntry& ne, vState& state, const dState expectedState)
+void SyncRes::getDenialValidationState(NegCache::NegCacheEntry& ne, vState& state, const dState expectedState, bool allowOptOut)
 {
   ne.d_validationState = state;
 
@@ -1690,9 +1690,11 @@ void SyncRes::getDenialValidationState(NegCache::NegCacheEntry& ne, vState& stat
     cspmap_t csp = harvestCSPFromNE(ne);
     dState res = getDenial(csp, ne.d_name, ne.d_qtype.getCode());
     if (res != expectedState) {
-      if (ne.d_qtype.getCode() == QType::DS && res == OPTOUT) {
-        LOG("Invalid denial found for "<<ne.d_name<<", retuning Insecure"<<endl);
-        ne.d_validationState = Insecure;
+      if (res == OPTOUT && allowOptOut) {
+        LOG("OPT-out denial found for "<<ne.d_name<<", retuning Insecure"<<endl);
+        ne.d_validationState = Secure;
+        updateValidationState(state, Insecure);
+        return;
       }
       else {
         LOG("Invalid denial found for "<<ne.d_name<<", retuning Bogus"<<endl);
@@ -1726,7 +1728,7 @@ bool SyncRes::processRecords(const std::string& prefix, const DNSName& qname, co
         ne.d_qtype = QType(0); // this encodes 'whole record'
         ne.d_auth = rec.d_name;
         harvestNXRecords(lwr.d_records, ne);
-        getDenialValidationState(ne, state, NXDOMAIN);
+        getDenialValidationState(ne, state, NXDOMAIN, false);
         t_sstorage.negcache.add(ne);
         if(s_rootNXTrust && ne.d_auth.isRoot() && auth.isRoot()) {
           ne.d_name = ne.d_name.getLastLabel();
@@ -1774,25 +1776,24 @@ bool SyncRes::processRecords(const std::string& prefix, const DNSName& qname, co
     else if(rec.d_place==DNSResourceRecord::AUTHORITY && rec.d_type==QType::DS && qname.isPartOf(rec.d_name)) {
       LOG(prefix<<qname<<": got DS record '"<<rec.d_name<<"' -> '"<<rec.d_content->getZoneRepresentation()<<"'"<<endl);
     }
-    else if(qtype == QType::DS && (rec.d_type==QType::NSEC || rec.d_type==QType::NSEC3)) {
+    else if(realreferral && rec.d_place==DNSResourceRecord::AUTHORITY && (rec.d_type==QType::NSEC || rec.d_type==QType::NSEC3) && newauth.isPartOf(auth)) {
       /* we might have received a denial of the DS, let's check */
       if (state == Secure) {
         NegCache::NegCacheEntry ne;
         ne.d_auth = auth;
         ne.d_ttd = d_now.tv_sec + rec.d_ttl;
-        ne.d_name = qname;
-        ne.d_qtype = qtype;
+        ne.d_name = newauth;
+        ne.d_qtype = QType::DS;
         harvestNXRecords(lwr.d_records, ne);
         cspmap_t csp = harvestCSPFromNE(ne);
-        dState denialState = getDenial(csp, qname, qtype.getCode());
+        dState denialState = getDenial(csp, newauth, QType::DS);
         if (denialState == NXQTYPE || denialState == OPTOUT) {
-          LOG(prefix<<qname<<": got negative indication of DS record for '"<<qname<<endl);
+          ne.d_validationState = Secure;
           rec.d_ttl = min(s_maxnegttl, rec.d_ttl);
-          ret.push_back(rec);
+          LOG(prefix<<qname<<": got negative indication of DS record for '"<<newauth<<endl);
           if(!wasVariable()) {
             t_sstorage.negcache.add(ne);
           }
-          negindic = true;
         }
       }
     }
@@ -1813,7 +1814,7 @@ bool SyncRes::processRecords(const std::string& prefix, const DNSName& qname, co
           ne.d_name = qname;
           ne.d_qtype = qtype;
           harvestNXRecords(lwr.d_records, ne);
-          getDenialValidationState(ne, state, NXQTYPE);
+          getDenialValidationState(ne, state, NXQTYPE, qtype == QType::DS);
           if(qtype.getCode()) {  // prevents us from blacking out a whole domain
             t_sstorage.negcache.add(ne);
           }
diff --git a/pdns/syncres.hh b/pdns/syncres.hh
index 64c6bfd2e..b1b3c0c05 100644
--- a/pdns/syncres.hh
+++ b/pdns/syncres.hh
@@ -739,7 +739,7 @@ private:
   vState validateDNSKeys(const DNSName& zone, const std::vector<DNSRecord>& dnskeys, const std::vector<std::shared_ptr<RRSIGRecordContent> >& signatures, unsigned int depth);
   vState getDSRecords(const DNSName& zone, dsmap_t& ds, bool onlyTA, unsigned int depth);
   vState getDNSKeys(const DNSName& signer, skeyset_t& keys, unsigned int depth);
-  void getDenialValidationState(NegCache::NegCacheEntry& ne, vState& state, dState expectedState);
+  void getDenialValidationState(NegCache::NegCacheEntry& ne, vState& state, const dState expectedState, bool allowOptOut);
   vState getTA(const DNSName& zone, dsmap_t& ds);
   vState getValidationStatus(const DNSName& subdomain, unsigned int depth);
 
diff --git a/pdns/validate.cc b/pdns/validate.cc
index eee32ce55..f5b6de0e6 100644
--- a/pdns/validate.cc
+++ b/pdns/validate.cc
@@ -112,7 +112,7 @@ dState getDenial(const cspmap_t &validrrsets, const DNSName& qname, const uint16
           return NXDOMAIN;
         }
 
-        LOG("Did not cover us, start="<<v.first.first<<", us="<<toBase32Hex(h)<<", end="<<toBase32Hex(nsec3->d_nexthash)<<endl);
+        LOG("Did not cover us ("<<qname<<"), start="<<v.first.first<<", us="<<toBase32Hex(h)<<", end="<<toBase32Hex(nsec3->d_nexthash)<<endl);
       }
     }
   }
-- 
2.40.0