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 241e6c5fd04f35bd3223255b66a21b1742f53011..c0a9f0d832b99ebef178c30fb3306790fa6b3641 100644 (file)
--- 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 f69d73d8605c7a8387d3f21b871c31df7b4b297e..8d032a19b5f971ba2ecc81f752adb7adfa12b819 100644 (file)
--- 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 fe2084be20975c267dbbf73d79d73a805ff37f01..6acfcea2f8868fb11f381a0abbbb37e749d3b472 100644 (file)
--- 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};