From: Otto Moerbeek <otto.moerbeek@open-xchange.com>
Date: Mon, 4 Nov 2019 15:57:29 +0000 (+0100)
Subject: Less aggressive 8020: by default only cut at NXDOMAIN if the entry is Secure.
X-Git-Tag: dnsdist-1.4.0~17^2~5
X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=d40a915bacb8bf80fdadcebf4490b53491ce4db3;p=thirdparty%2Fpdns.git

Less aggressive 8020: by default only cut at NXDOMAIN if the entry is Secure.
We might want to explicitly validate Inderminate records if needed.
That code is not written yet.
---

diff --git a/pdns/pdns_recursor.cc b/pdns/pdns_recursor.cc
index 3f1a5166fd..5e1d794324 100644
--- a/pdns/pdns_recursor.cc
+++ b/pdns/pdns_recursor.cc
@@ -3956,7 +3956,17 @@ static int serviceMain(int argc, char*argv[])
   SyncRes::s_ecscachelimitttl = ::arg().asNum("ecs-cache-limit-ttl");
 
   SyncRes::s_qnameminimization = ::arg().mustDo("qname-minimization");
-  SyncRes::s_hardenNXD = ::arg().mustDo("nothing-below-nxdomain");
+
+  SyncRes::s_hardenNXD = SyncRes::HardenNXD::DNSSEC;
+  string value = ::arg()["nothing-below-nxdomain"];
+  if (value == "yes") {
+    SyncRes::s_hardenNXD = SyncRes::HardenNXD::Yes;
+  } else if (value == "no") {
+    SyncRes::s_hardenNXD = SyncRes::HardenNXD::No;
+  } else if (value != "dnssec") {
+    g_log << Logger::Error << "Unknown nothing-below-nxdomain mode: " << value << endl;
+    exit(1);
+  }
 
   if (!::arg().isEmpty("ecs-scope-zero-address")) {
     ComboAddress scopeZero(::arg()["ecs-scope-zero-address"]);
@@ -4697,8 +4707,9 @@ int main(int argc, char **argv)
     ::arg().set("public-suffix-list-file", "Path to the Public Suffix List file, if any")="";
     ::arg().set("distribution-load-factor", "The load factor used when PowerDNS is distributing queries to worker threads")="0.0";
     ::arg().setSwitch("qname-minimization", "Use Query Name Minimization")="no";
-    ::arg().setSwitch("nothing-below-nxdomain", "When an NXDOMAIN exists in cache for a name with fewer labels than the qname, send NXDOMAIN without doing a lookup (see RFC 8020)")="yes";
+    ::arg().setSwitch("nothing-below-nxdomain", "When an NXDOMAIN exists in cache for a name with fewer labels than the qname, send NXDOMAIN without doing a lookup (see RFC 8020)")="dnssec";
     ::arg().set("max-generate-steps", "Maximum number of $GENERATE steps when loading a zone from a file")="0";
+
 #ifdef NOD_ENABLED
     ::arg().set("new-domain-tracking", "Track newly observed domains (i.e. never seen before).")="no";
     ::arg().set("new-domain-log", "Log newly observed domains.")="yes";
diff --git a/pdns/recursordist/test-syncres_cc.cc b/pdns/recursordist/test-syncres_cc.cc
index 46ee34258a..8223bdca09 100644
--- a/pdns/recursordist/test-syncres_cc.cc
+++ b/pdns/recursordist/test-syncres_cc.cc
@@ -129,7 +129,7 @@ void initSR(bool debug)
   SyncRes::s_ecsipv6cachelimit = 56;
   SyncRes::s_ecscachelimitttl = 0;
   SyncRes::s_rootNXTrust = true;
-  SyncRes::s_hardenNXD = true;
+  SyncRes::s_hardenNXD = SyncRes::HardenNXD::DNSSEC;
   SyncRes::s_minimumTTL = 0;
   SyncRes::s_minimumECSTTL = 0;
   SyncRes::s_serverID = "PowerDNS Unit Tests Server ID";
diff --git a/pdns/recursordist/test-syncres_cc2.cc b/pdns/recursordist/test-syncres_cc2.cc
index e42a45442e..42c2743ba5 100644
--- a/pdns/recursordist/test-syncres_cc2.cc
+++ b/pdns/recursordist/test-syncres_cc2.cc
@@ -418,6 +418,7 @@ BOOST_AUTO_TEST_CASE(test_root_nx_dont_trust) {
 BOOST_AUTO_TEST_CASE(test_rfc8020_nothing_underneath) {
   std::unique_ptr<SyncRes> sr;
   initSR(sr);
+  SyncRes::s_hardenNXD = SyncRes::HardenNXD::Yes;
 
   primeHints();
 
@@ -448,71 +449,72 @@ BOOST_AUTO_TEST_CASE(test_rfc8020_nothing_underneath) {
   vector<DNSRecord> ret;
   int res = sr->beginResolve(target1, QType(QType::A), QClass::IN, ret);
   BOOST_CHECK_EQUAL(res, RCode::NXDomain);
-  BOOST_CHECK_EQUAL(ret.size(), 1);
-  BOOST_CHECK_EQUAL(queriesCount, 2);
-  BOOST_CHECK_EQUAL(SyncRes::getNegCacheSize(), 1);
+  BOOST_CHECK_EQUAL(ret.size(), 1U);
+  BOOST_CHECK_EQUAL(queriesCount, 2U);
+  BOOST_CHECK_EQUAL(SyncRes::getNegCacheSize(), 1U);
 
   ret.clear();
   res = sr->beginResolve(target2, QType(QType::A), QClass::IN, ret);
   BOOST_CHECK_EQUAL(res, RCode::NXDomain);
-  BOOST_CHECK_EQUAL(ret.size(), 1);
-  BOOST_CHECK_EQUAL(queriesCount, 2);
-  BOOST_CHECK_EQUAL(SyncRes::getNegCacheSize(), 1);
+  BOOST_CHECK_EQUAL(ret.size(), 1U);
+  BOOST_CHECK_EQUAL(queriesCount, 2U);
+  BOOST_CHECK_EQUAL(SyncRes::getNegCacheSize(), 1U);
 
   ret.clear();
   res = sr->beginResolve(target3, QType(QType::A), QClass::IN, ret);
   BOOST_CHECK_EQUAL(res, RCode::NXDomain);
-  BOOST_CHECK_EQUAL(ret.size(), 1);
-  BOOST_CHECK_EQUAL(queriesCount, 2);
-  BOOST_CHECK_EQUAL(SyncRes::getNegCacheSize(), 1);
+  BOOST_CHECK_EQUAL(ret.size(), 1U);
+  BOOST_CHECK_EQUAL(queriesCount, 2U);
+  BOOST_CHECK_EQUAL(SyncRes::getNegCacheSize(), 1U);
 
   ret.clear();
   res = sr->beginResolve(target4, QType(QType::A), QClass::IN, ret);
   BOOST_CHECK_EQUAL(res, RCode::NXDomain);
-  BOOST_CHECK_EQUAL(ret.size(), 1);
-  BOOST_CHECK_EQUAL(queriesCount, 2);
-  BOOST_CHECK_EQUAL(SyncRes::getNegCacheSize(), 1);
+  BOOST_CHECK_EQUAL(ret.size(), 1U);
+  BOOST_CHECK_EQUAL(queriesCount, 2U);
+  BOOST_CHECK_EQUAL(SyncRes::getNegCacheSize(), 1U);
 
   // Now test without RFC 8020 to see the cache and query count grow
-  SyncRes::s_hardenNXD = false;
+  SyncRes::s_hardenNXD = SyncRes::HardenNXD::No;
 
   // Already cached
   ret.clear();
   res = sr->beginResolve(target1, QType(QType::A), QClass::IN, ret);
   BOOST_CHECK_EQUAL(res, RCode::NXDomain);
-  BOOST_CHECK_EQUAL(ret.size(), 1);
-  BOOST_CHECK_EQUAL(queriesCount, 2);
-  BOOST_CHECK_EQUAL(SyncRes::getNegCacheSize(), 1);
+  BOOST_CHECK_EQUAL(ret.size(), 1U);
+  BOOST_CHECK_EQUAL(queriesCount, 2U);
+  BOOST_CHECK_EQUAL(SyncRes::getNegCacheSize(), 1U);
 
   // New query
   ret.clear();
   res = sr->beginResolve(target2, QType(QType::A), QClass::IN, ret);
   BOOST_CHECK_EQUAL(res, RCode::NXDomain);
-  BOOST_CHECK_EQUAL(ret.size(), 1);
-  BOOST_CHECK_EQUAL(queriesCount, 3);
-  BOOST_CHECK_EQUAL(SyncRes::getNegCacheSize(), 2);
+  BOOST_CHECK_EQUAL(ret.size(), 1U);
+  BOOST_CHECK_EQUAL(queriesCount, 3U);
+  BOOST_CHECK_EQUAL(SyncRes::getNegCacheSize(), 2U);
 
   ret.clear();
   res = sr->beginResolve(target3, QType(QType::A), QClass::IN, ret);
   BOOST_CHECK_EQUAL(res, RCode::NXDomain);
-  BOOST_CHECK_EQUAL(ret.size(), 1);
-  BOOST_CHECK_EQUAL(queriesCount, 4);
-  BOOST_CHECK_EQUAL(SyncRes::getNegCacheSize(), 3);
+  BOOST_CHECK_EQUAL(ret.size(), 1U);
+  BOOST_CHECK_EQUAL(queriesCount, 4U);
+  BOOST_CHECK_EQUAL(SyncRes::getNegCacheSize(), 3U);
 
   ret.clear();
   res = sr->beginResolve(target4, QType(QType::A), QClass::IN, ret);
   BOOST_CHECK_EQUAL(res, RCode::NXDomain);
-  BOOST_CHECK_EQUAL(ret.size(), 1);
-  BOOST_CHECK_EQUAL(queriesCount, 5);
-  BOOST_CHECK_EQUAL(SyncRes::getNegCacheSize(), 4);
+  BOOST_CHECK_EQUAL(ret.size(), 1U);
+  BOOST_CHECK_EQUAL(queriesCount, 5U);
+  BOOST_CHECK_EQUAL(SyncRes::getNegCacheSize(), 4U);
 
   // reset
-  SyncRes::s_hardenNXD = true;
+  SyncRes::s_hardenNXD = SyncRes::HardenNXD::DNSSEC;
 }
 
 BOOST_AUTO_TEST_CASE(test_rfc8020_nodata) {
   std::unique_ptr<SyncRes> sr;
   initSR(sr);
+  SyncRes::s_hardenNXD = SyncRes::HardenNXD::Yes;
 
   primeHints();
 
@@ -556,35 +558,36 @@ BOOST_AUTO_TEST_CASE(test_rfc8020_nodata) {
   vector<DNSRecord> ret;
   int res = sr->beginResolve(target1, QType(QType::TXT), QClass::IN, ret);
   BOOST_CHECK_EQUAL(res, RCode::NoError);
-  BOOST_CHECK_EQUAL(ret.size(), 1);
-  BOOST_CHECK_EQUAL(queriesCount, 2);
-  BOOST_CHECK_EQUAL(SyncRes::getNegCacheSize(), 1);
+  BOOST_CHECK_EQUAL(ret.size(), 1U);
+  BOOST_CHECK_EQUAL(queriesCount, 2U);
+  BOOST_CHECK_EQUAL(SyncRes::getNegCacheSize(), 1U);
 
   ret.clear();
   res = sr->beginResolve(target1, QType(QType::A), QClass::IN, ret);
   BOOST_CHECK_EQUAL(res, RCode::NoError);
-  BOOST_CHECK_EQUAL(ret.size(), 1);
-  BOOST_CHECK_EQUAL(queriesCount, 3);
-  BOOST_CHECK_EQUAL(SyncRes::getNegCacheSize(), 1);
+  BOOST_CHECK_EQUAL(ret.size(), 1U);
+  BOOST_CHECK_EQUAL(queriesCount, 3U);
+  BOOST_CHECK_EQUAL(SyncRes::getNegCacheSize(), 1U);
 
   ret.clear();
   res = sr->beginResolve(target2, QType(QType::A), QClass::IN, ret);
   BOOST_CHECK_EQUAL(res, RCode::NXDomain);
-  BOOST_CHECK_EQUAL(ret.size(), 1);
-  BOOST_CHECK_EQUAL(queriesCount, 4);
-  BOOST_CHECK_EQUAL(SyncRes::getNegCacheSize(), 2);
+  BOOST_CHECK_EQUAL(ret.size(), 1U);
+  BOOST_CHECK_EQUAL(queriesCount, 4U);
+  BOOST_CHECK_EQUAL(SyncRes::getNegCacheSize(), 2U);
 
   ret.clear();
   res = sr->beginResolve(target3, QType(QType::A), QClass::IN, ret);
   BOOST_CHECK_EQUAL(res, RCode::NXDomain);
-  BOOST_CHECK_EQUAL(ret.size(), 1);
-  BOOST_CHECK_EQUAL(queriesCount, 4);
-  BOOST_CHECK_EQUAL(SyncRes::getNegCacheSize(), 2);
+  BOOST_CHECK_EQUAL(ret.size(), 1U);
+  BOOST_CHECK_EQUAL(queriesCount, 4U);
+  BOOST_CHECK_EQUAL(SyncRes::getNegCacheSize(), 2U);
 }
 
 BOOST_AUTO_TEST_CASE(test_rfc8020_nodata_bis) {
   std::unique_ptr<SyncRes> sr;
   initSR(sr);
+  SyncRes::s_hardenNXD = SyncRes::HardenNXD::Yes;
 
   primeHints();
 
@@ -628,30 +631,30 @@ BOOST_AUTO_TEST_CASE(test_rfc8020_nodata_bis) {
   vector<DNSRecord> ret;
   int res = sr->beginResolve(target1, QType(QType::TXT), QClass::IN, ret);
   BOOST_CHECK_EQUAL(res, RCode::NoError);
-  BOOST_CHECK_EQUAL(ret.size(), 1);
-  BOOST_CHECK_EQUAL(queriesCount, 2);
-  BOOST_CHECK_EQUAL(SyncRes::getNegCacheSize(), 1);
+  BOOST_CHECK_EQUAL(ret.size(), 1U);
+  BOOST_CHECK_EQUAL(queriesCount, 2U);
+  BOOST_CHECK_EQUAL(SyncRes::getNegCacheSize(), 1U);
 
   ret.clear();
   res = sr->beginResolve(target1, QType(QType::A), QClass::IN, ret);
   BOOST_CHECK_EQUAL(res, RCode::NoError);
-  BOOST_CHECK_EQUAL(ret.size(), 1);
-  BOOST_CHECK_EQUAL(queriesCount, 3);
-  BOOST_CHECK_EQUAL(SyncRes::getNegCacheSize(), 1);
+  BOOST_CHECK_EQUAL(ret.size(), 1U);
+  BOOST_CHECK_EQUAL(queriesCount, 3U);
+  BOOST_CHECK_EQUAL(SyncRes::getNegCacheSize(), 1U);
 
   ret.clear();
   res = sr->beginResolve(target2, QType(QType::TXT), QClass::IN, ret);
   BOOST_CHECK_EQUAL(res, RCode::NXDomain);
-  BOOST_CHECK_EQUAL(ret.size(), 1);
-  BOOST_CHECK_EQUAL(queriesCount, 4);
-  BOOST_CHECK_EQUAL(SyncRes::getNegCacheSize(), 2);
+  BOOST_CHECK_EQUAL(ret.size(), 1U);
+  BOOST_CHECK_EQUAL(queriesCount, 4U);
+  BOOST_CHECK_EQUAL(SyncRes::getNegCacheSize(), 2U);
 
   ret.clear();
   res = sr->beginResolve(target3, QType(QType::TXT), QClass::IN, ret);
   BOOST_CHECK_EQUAL(res, RCode::NXDomain);
-  BOOST_CHECK_EQUAL(ret.size(), 1);
-  BOOST_CHECK_EQUAL(queriesCount, 4);
-  BOOST_CHECK_EQUAL(SyncRes::getNegCacheSize(), 2);
+  BOOST_CHECK_EQUAL(ret.size(), 1U);
+  BOOST_CHECK_EQUAL(queriesCount, 4U);
+  BOOST_CHECK_EQUAL(SyncRes::getNegCacheSize(), 2U);
 }
 
 BOOST_AUTO_TEST_CASE(test_skip_negcache_for_variable_response) {
diff --git a/pdns/syncres.cc b/pdns/syncres.cc
index ae18dd8abe..e2aa148a48 100644
--- a/pdns/syncres.cc
+++ b/pdns/syncres.cc
@@ -88,7 +88,7 @@ bool SyncRes::s_nopacketcache;
 bool SyncRes::s_rootNXTrust;
 bool SyncRes::s_noEDNS;
 bool SyncRes::s_qnameminimization;
-bool SyncRes::s_hardenNXD;
+SyncRes::HardenNXD SyncRes::s_hardenNXD;
 
 #define LOG(x) if(d_lm == Log) { g_log <<Logger::Warning << x; } else if(d_lm == Store) { d_trace << x; }
 
@@ -1414,19 +1414,27 @@ bool SyncRes::doCacheCheck(const DNSName &qname, const DNSName& authname, bool w
         LOG(prefix<<qname<<": Entire name '"<<qname<<" is negatively cached via '"<<ne->d_auth<<"' for another "<<sttl<<" seconds"<<endl);
       }
     }
-  } else if (s_hardenNXD && !qname.isRoot() && !wasForwardedOrAuthZone) {
+  } else if (s_hardenNXD != HardenNXD::No && !qname.isRoot() && !wasForwardedOrAuthZone) {
     auto labels = qname.getRawLabels();
     DNSName negCacheName(g_rootdnsname);
     negCacheName.prependRawLabel(labels.back());
     labels.pop_back();
     while(!labels.empty()) {
       if (t_sstorage.negcache.get(negCacheName, QType(0), d_now, &ne, true)) {
-        res = RCode::NXDomain;
-        sttl = ne->d_ttd - d_now.tv_sec;
-        giveNegative = true;
-        cachedState = ne->d_validationState;
-        LOG(prefix<<qname<<": Name '"<<negCacheName<<"' and below, is negatively cached via '"<<ne->d_auth<<"' for another "<<sttl<<" seconds"<<endl);
-        break;
+        if (ne->d_validationState == Indeterminate && validationEnabled()) {
+          // LOG(prefix << negCacheName <<  " negatively cached and Indeterminate, trying to validate NXDOMAIN" << endl);
+          // ...
+          // And get the updated ne struct
+          //t_sstorage.negcache.get(negCacheName, QType(0), d_now, &ne, true);
+        }
+        if (s_hardenNXD == HardenNXD::Yes || ne->d_validationState == Secure) {
+          res = RCode::NXDomain;
+          sttl = ne->d_ttd - d_now.tv_sec;
+          giveNegative = true;
+          cachedState = ne->d_validationState;
+          LOG(prefix<<qname<<": Name '"<<negCacheName<<"' and below, is negatively cached via '"<<ne->d_auth<<"' for another "<<sttl<<" seconds"<<endl);
+          break;
+        }
       }
       negCacheName.prependRawLabel(labels.back());
       labels.pop_back();
diff --git a/pdns/syncres.hh b/pdns/syncres.hh
index f72a2cdb50..77af72ba6b 100644
--- a/pdns/syncres.hh
+++ b/pdns/syncres.hh
@@ -285,7 +285,9 @@ public:
     time_t modeSetAt;
   };
 
-    //! This represents a number of decaying Ewmas, used to store performance per nameserver-name.
+  enum class HardenNXD { No, DNSSEC, Yes };
+  
+  //! This represents a number of decaying Ewmas, used to store performance per nameserver-name.
   /** Modelled to work mostly like the underlying DecayingEwma. After you've called get,
       d_best is filled out with the best address for this collection */
   struct DecayingEwmaCollection
@@ -752,7 +754,7 @@ public:
   static bool s_rootNXTrust;
   static bool s_nopacketcache;
   static bool s_qnameminimization;
-  static bool s_hardenNXD;
+  static HardenNXD s_hardenNXD;
 
   std::unordered_map<std::string,bool> d_discardedPolicies;
   DNSFilterEngine::Policy d_appliedPolicy;