From: Otto Moerbeek <otto.moerbeek@open-xchange.com>
Date: Tue, 23 Jun 2020 09:11:03 +0000 (+0200)
Subject: Draft to study alternative way to do "skip cname check" for DS and
X-Git-Tag: dnsdist-1.5.0-rc4~10^2~1
X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=9c3afd3b2134fd8f6ab5ff6bdbea0141f3319452;p=thirdparty%2Fpdns.git

Draft to study alternative way to do "skip cname check" for DS and
DNSKEY records.

The current code effectively disables lookup of cached CNAME results
for zone cut computations, which results in more queries than needed.

Should fix #9266 or at least give more insight.

Needs critical eyes badly!
---

diff --git a/pdns/recursordist/test-syncres_cc9.cc b/pdns/recursordist/test-syncres_cc9.cc
index 241e6c5fd0..c0a9f0d832 100644
--- a/pdns/recursordist/test-syncres_cc9.cc
+++ b/pdns/recursordist/test-syncres_cc9.cc
@@ -78,7 +78,7 @@ BOOST_AUTO_TEST_CASE(test_dnssec_validation_from_cname_cache_secure)
   for (const auto& record : ret) {
     BOOST_CHECK(record.d_type == QType::CNAME || record.d_type == QType::A || record.d_type == QType::RRSIG);
   }
-  BOOST_CHECK_EQUAL(queriesCount, 5U);
+  BOOST_CHECK_EQUAL(queriesCount, 4U);
 }
 
 BOOST_AUTO_TEST_CASE(test_dnssec_validation_from_cname_cache_insecure)
@@ -232,7 +232,7 @@ BOOST_AUTO_TEST_CASE(test_dnssec_validation_from_cname_cache_bogus)
     BOOST_CHECK(record.d_type == QType::CNAME || record.d_type == QType::A);
     BOOST_CHECK_EQUAL(record.d_ttl, SyncRes::s_maxbogusttl);
   }
-  BOOST_CHECK_EQUAL(queriesCount, 5U);
+  BOOST_CHECK_EQUAL(queriesCount, 4U);
 
   ret.clear();
   /* and a third time to make sure that the validation status (and TTL!)
@@ -246,7 +246,7 @@ BOOST_AUTO_TEST_CASE(test_dnssec_validation_from_cname_cache_bogus)
     BOOST_CHECK(record.d_type == QType::CNAME || record.d_type == QType::A);
     BOOST_CHECK_EQUAL(record.d_ttl, SyncRes::s_maxbogusttl);
   }
-  BOOST_CHECK_EQUAL(queriesCount, 5U);
+  BOOST_CHECK_EQUAL(queriesCount, 4U);
 }
 
 BOOST_AUTO_TEST_CASE(test_dnssec_validation_additional_without_rrsig)
diff --git a/pdns/syncres.cc b/pdns/syncres.cc
index f69d73d860..8d032a19b5 100644
--- a/pdns/syncres.cc
+++ b/pdns/syncres.cc
@@ -850,7 +850,7 @@ int SyncRes::doResolveNoQNameMinimization(const DNSName &qname, const QType &qty
       }
     }
 
-    if(!d_skipCNAMECheck && doCNAMECacheCheck(qname, qtype, ret, depth, res, state, wasAuthZone, wasForwardRecurse)) { // will reroute us if needed
+    if(doCNAMECacheCheck(qname, qtype, ret, depth, res, state, wasAuthZone, wasForwardRecurse)) { // will reroute us if needed
       d_wasOutOfBand = wasAuthZone;
       // Do not set *fromCache; res does not reflect the final result in all cases
       return res;
@@ -1238,6 +1238,10 @@ bool SyncRes::doCNAMECacheCheck(const DNSName &qname, const QType &qtype, vector
     return false;
   }
 
+  if (qtype == QType::DS || qtype == QType::DNSKEY) {
+    return true;
+  }
+  
   for(auto const &record : cset) {
     if (record.d_class != QClass::IN) {
       continue;
@@ -1573,6 +1577,7 @@ bool SyncRes::doCacheCheck(const DNSName &qname, const DNSName& authname, bool w
   uint32_t ttl=0;
   uint32_t capTTL = std::numeric_limits<uint32_t>::max();
   bool wasCachedAuth;
+
   if(s_RC->get(d_now.tv_sec, sqname, sqt, !wasForwardRecurse && d_requireAuthData, &cset, d_cacheRemote, d_routingTag, d_doDNSSEC ? &signatures : nullptr, d_doDNSSEC ? &authorityRecs : nullptr, &d_wasVariable, &cachedState, &wasCachedAuth) > 0) {
 
     LOG(prefix<<sqname<<": Found cache hit for "<<sqt.getName()<<": ");
@@ -2052,15 +2057,11 @@ vState SyncRes::getDSRecords(const DNSName& zone, dsmap_t& ds, bool taOnly, unsi
     return result;
   }
 
-  bool oldSkipCNAME = d_skipCNAMECheck;
-  d_skipCNAMECheck = true;
-
   std::set<GetBestNSAnswer> beenthere;
   std::vector<DNSRecord> dsrecords;
 
   vState state = Indeterminate;
   int rcode = doResolve(zone, QType(QType::DS), dsrecords, depth + 1, beenthere, state);
-  d_skipCNAMECheck = oldSkipCNAME;
 
   if (rcode == RCode::NoError || (rcode == RCode::NXDomain && !bogusOnNXD)) {
     uint8_t bestDigestType = 0;
@@ -2211,9 +2212,6 @@ void SyncRes::computeZoneCuts(const DNSName& begin, const DNSName& end, unsigned
   DNSName qname(end);
   std::vector<string> labelsToAdd = begin.makeRelative(end).getRawLabels();
 
-  bool oldSkipCNAME = d_skipCNAMECheck;
-  d_skipCNAMECheck = true;
-
   while(qname != begin) {
     if (labelsToAdd.empty())
       break;
@@ -2269,8 +2267,6 @@ void SyncRes::computeZoneCuts(const DNSName& begin, const DNSName& end, unsigned
     }
   }
 
-  d_skipCNAMECheck = oldSkipCNAME;
-
   LOG(d_prefix<<": list of cuts from "<<begin<<" to "<<end<<endl);
   for (const auto& cut : d_cutStates) {
     if (cut.first.isRoot() || (begin.isPartOf(cut.first) && cut.first.isPartOf(end))) {
@@ -2333,11 +2329,7 @@ vState SyncRes::getDNSKeys(const DNSName& signer, skeyset_t& keys, unsigned int
   LOG(d_prefix<<"Retrieving DNSKeys for "<<signer<<endl);
 
   vState state = Indeterminate;
-  /* following CNAME might lead to us to the wrong DNSKEY */
-  bool oldSkipCNAME = d_skipCNAMECheck;
-  d_skipCNAMECheck = true;
   int rcode = doResolve(signer, QType(QType::DNSKEY), records, depth + 1, beenthere, state);
-  d_skipCNAMECheck = oldSkipCNAME;
 
   if (rcode == RCode::NoError) {
     if (state == Secure) {
diff --git a/pdns/syncres.hh b/pdns/syncres.hh
index fe2084be20..6acfcea2f8 100644
--- a/pdns/syncres.hh
+++ b/pdns/syncres.hh
@@ -699,11 +699,6 @@ public:
     return d_now;
   }
 
-  void setSkipCNAMECheck(bool skip = false)
-  {
-    d_skipCNAMECheck = skip;
-  }
-
   void setQuerySource(const ComboAddress& requestor, boost::optional<const EDNSSubnetOpts&> incomingECS);
 
 #ifdef HAVE_PROTOBUF
@@ -903,7 +898,6 @@ private:
   bool d_DNSSECValidationRequested{false};
   bool d_doEDNS0{true};
   bool d_requireAuthData{true};
-  bool d_skipCNAMECheck{false};
   bool d_updatingRootNS{false};
   bool d_wantsRPZ{true};
   bool d_wasOutOfBand{false};