From: Remi Gacogne <remi.gacogne@powerdns.com>
Date: Wed, 8 Sep 2021 09:11:53 +0000 (+0200)
Subject: rec: Fix the aggressive cache returning duplicated NSEC3 records
X-Git-Tag: dnsdist-1.7.0-alpha1~28^2
X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=refs%2Fpull%2F10701%2Fhead;p=thirdparty%2Fpdns.git

rec: Fix the aggressive cache returning duplicated NSEC3 records

No need to include the same record twice when it provides, at the same
time, a proof that the closest encloser exists and that the next closer
does not, and/or that the wildcard does not exist either.
This happens right away in a zone with a single record, like reported
by Matt Nordhoff, but it might happen in other cases as well.
---

diff --git a/pdns/recursordist/aggressive_nsec.cc b/pdns/recursordist/aggressive_nsec.cc
index 9368e4a865..2f332b6395 100644
--- a/pdns/recursordist/aggressive_nsec.cc
+++ b/pdns/recursordist/aggressive_nsec.cc
@@ -725,8 +725,14 @@ bool AggressiveNSECCache::getNSEC3Denial(time_t now, std::shared_ptr<LockGuarded
 
   addToRRSet(now, soaSet, soaSignatures, zone, doDNSSEC, ret);
   addRecordToRRSet(now, closestNSEC3.d_owner, QType::NSEC3, closestNSEC3.d_ttd - now, closestNSEC3.d_record, closestNSEC3.d_signatures, doDNSSEC, ret);
-  addRecordToRRSet(now, nextCloserEntry.d_owner, QType::NSEC3, nextCloserEntry.d_ttd - now, nextCloserEntry.d_record, nextCloserEntry.d_signatures, doDNSSEC, ret);
-  addRecordToRRSet(now, wcEntry.d_owner, QType::NSEC3, wcEntry.d_ttd - now, wcEntry.d_record, wcEntry.d_signatures, doDNSSEC, ret);
+
+  /* no need to include the same NSEC3 twice */
+  if (nextCloserEntry.d_owner != closestNSEC3.d_owner) {
+    addRecordToRRSet(now, nextCloserEntry.d_owner, QType::NSEC3, nextCloserEntry.d_ttd - now, nextCloserEntry.d_record, nextCloserEntry.d_signatures, doDNSSEC, ret);
+  }
+  if (wcEntry.d_owner != closestNSEC3.d_owner && wcEntry.d_owner != nextCloserEntry.d_owner) {
+    addRecordToRRSet(now, wcEntry.d_owner, QType::NSEC3, wcEntry.d_ttd - now, wcEntry.d_record, wcEntry.d_signatures, doDNSSEC, ret);
+  }
 
   LOG("Found valid NSEC3s covering the requested name and type!" << endl);
   ++d_nsec3Hits;
diff --git a/pdns/recursordist/test-aggressive_nsec_cc.cc b/pdns/recursordist/test-aggressive_nsec_cc.cc
index e507145a28..51693050c6 100644
--- a/pdns/recursordist/test-aggressive_nsec_cc.cc
+++ b/pdns/recursordist/test-aggressive_nsec_cc.cc
@@ -774,13 +774,13 @@ BOOST_AUTO_TEST_CASE(test_aggressive_nsec3_nodata_wildcard)
           addRecordToLW(res, DNSName("powerdns.com."), QType::SOA, "powerdns.com. powerdns.com. 2017032301 10800 3600 604800 3600", DNSResourceRecord::AUTHORITY, 3600);
           addRRSIG(keys, res->d_records, DNSName("powerdns.com."), 300);
           /* first the closest encloser */
-          addNSEC3UnhashedRecordToLW(DNSName("powerdns.com."), DNSName("powerdns.com."), "whatever", {QType::A, QType::TXT, QType::RRSIG}, 600, res->d_records, 10);
+          addNSEC3NoDataNarrowRecordToLW(DNSName("powerdns.com."), DNSName("powerdns.com."), {QType::A, QType::TXT, QType::RRSIG}, 600, res->d_records, 10);
           addRRSIG(keys, res->d_records, DNSName("powerdns.com."), 300);
           /* then the next closer */
-          addNSEC3UnhashedRecordToLW(DNSName("+.powerdns.com."), DNSName("powerdns.com."), "v", {QType::RRSIG}, 600, res->d_records, 10);
+          addNSEC3NarrowRecordToLW(DNSName("a.powerdns.com."), DNSName("powerdns.com."), {QType::RRSIG}, 600, res->d_records, 10);
           addRRSIG(keys, res->d_records, DNSName("powerdns.com."), 300);
           /* a wildcard applies but does not have this type */
-          addNSEC3UnhashedRecordToLW(DNSName("*.powerdns.com."), DNSName("powerdns.com."), "whatever", {QType::TXT, QType::RRSIG}, 600, res->d_records, 10);
+          addNSEC3NoDataNarrowRecordToLW(DNSName("*.powerdns.com."), DNSName("powerdns.com."), {QType::TXT, QType::RRSIG}, 600, res->d_records, 10);
           addRRSIG(keys, res->d_records, DNSName("powerdns.com"), 300, false, boost::none, DNSName("*.powerdns.com"));
           return LWResult::Result::Success;
         }
diff --git a/pdns/recursordist/test-syncres_cc.cc b/pdns/recursordist/test-syncres_cc.cc
index 6a46150639..ba81b99920 100644
--- a/pdns/recursordist/test-syncres_cc.cc
+++ b/pdns/recursordist/test-syncres_cc.cc
@@ -431,6 +431,17 @@ void addNSEC3UnhashedRecordToLW(const DNSName& domain, const DNSName& zone, cons
   addNSEC3RecordToLW(DNSName(toBase32Hex(hashed)) + zone, next, salt, iterations, types, ttl, records, optOut);
 }
 
+/* Proves a NODATA (name exists, type does not) but the next owner name is right behind, so it should not prove anything else unless we are very unlucky */
+void addNSEC3NoDataNarrowRecordToLW(const DNSName& domain, const DNSName& zone, const std::set<uint16_t>& types, uint32_t ttl, std::vector<DNSRecord>& records, unsigned int iterations, bool optOut)
+{
+  static const std::string salt = "deadbeef";
+  std::string hashed = hashQNameWithSalt(salt, iterations, domain);
+  std::string hashedNext(hashed);
+  incrementHash(hashedNext);
+
+  addNSEC3RecordToLW(DNSName(toBase32Hex(hashed)) + zone, hashedNext, salt, iterations, types, ttl, records, optOut);
+}
+
 void addNSEC3NarrowRecordToLW(const DNSName& domain, const DNSName& zone, const std::set<uint16_t>& types, uint32_t ttl, std::vector<DNSRecord>& records, unsigned int iterations, bool optOut)
 {
   static const std::string salt = "deadbeef";
diff --git a/pdns/recursordist/test-syncres_cc.hh b/pdns/recursordist/test-syncres_cc.hh
index dbd05f7c98..f33abd77cf 100644
--- a/pdns/recursordist/test-syncres_cc.hh
+++ b/pdns/recursordist/test-syncres_cc.hh
@@ -59,8 +59,13 @@ void addNSECRecordToLW(const DNSName& domain, const DNSName& next, const std::se
 
 void addNSEC3RecordToLW(const DNSName& hashedName, const std::string& hashedNext, const std::string& salt, unsigned int iterations, const std::set<uint16_t>& types, uint32_t ttl, std::vector<DNSRecord>& records, bool optOut = false);
 
+/* Proves a NODATA (name exists, type does not) */
 void addNSEC3UnhashedRecordToLW(const DNSName& domain, const DNSName& zone, const std::string& next, const std::set<uint16_t>& types, uint32_t ttl, std::vector<DNSRecord>& records, unsigned int iterations = 10, bool optOut = false);
 
+/* Proves a NODATA (name exists, type does not) and the next owner name is right behind, so it should not prove anything else unless we are very unlucky */
+void addNSEC3NoDataNarrowRecordToLW(const DNSName& domain, const DNSName& zone, const std::set<uint16_t>& types, uint32_t ttl, std::vector<DNSRecord>& records, unsigned int iterations = 10, bool optOut = false);
+
+/* Proves a NXDOMAIN (name does not exist) with the owner name right before, and the next name right after, so it should not prove anything else unless we are very unlucky */
 void addNSEC3NarrowRecordToLW(const DNSName& domain, const DNSName& zone, const std::set<uint16_t>& types, uint32_t ttl, std::vector<DNSRecord>& records, unsigned int iterations = 10, bool OptOut = false);
 
 void generateKeyMaterial(const DNSName& name, unsigned int algo, uint8_t digest, testkeysset_t& keys);