From 1eed7f4572d396b544756cf93be2297db23483b3 Mon Sep 17 00:00:00 2001
From: Remi Gacogne <remi.gacogne@powerdns.com>
Date: Tue, 13 Jul 2021 14:55:47 +0200
Subject: [PATCH] rec: Fix the NSEC3 ancestor check for DS in the aggressive
 cache

---
 pdns/recursordist/aggressive_nsec.cc         | 13 ++++++++++++-
 pdns/recursordist/test-aggressive_nsec_cc.cc |  4 ++--
 2 files changed, 14 insertions(+), 3 deletions(-)

diff --git a/pdns/recursordist/aggressive_nsec.cc b/pdns/recursordist/aggressive_nsec.cc
index 370750d403..41b095cf26 100644
--- a/pdns/recursordist/aggressive_nsec.cc
+++ b/pdns/recursordist/aggressive_nsec.cc
@@ -545,6 +545,8 @@ bool AggressiveNSECCache::getNSEC3Denial(time_t now, std::shared_ptr<AggressiveN
     }
 
     const DNSName signer = getSigner(exactNSEC3.d_signatures);
+    /* here we need to allow an ancestor NSEC3 proving that a DS does not exist as it is an
+       exact match for the name */
     if (type != QType::DS && isNSEC3AncestorDelegation(signer, exactNSEC3.d_owner, nsec3)) {
       /* RFC 6840 section 4.1 "Clarifications on Nonexistence Proofs":
          Ancestor delegation NSEC or NSEC3 RRs MUST NOT be used to assume
@@ -581,7 +583,9 @@ bool AggressiveNSECCache::getNSEC3Denial(time_t now, std::shared_ptr<AggressiveN
       }
 
       const DNSName signer = getSigner(closestNSEC3.d_signatures);
-      if (type != QType::DS && isNSEC3AncestorDelegation(signer, closestNSEC3.d_owner, nsec3)) {
+      /* This time we do not allow any ancestor NSEC3, as if the closest encloser is a delegation
+         NS we know nothing about the names in the child zone. */
+      if (isNSEC3AncestorDelegation(signer, closestNSEC3.d_owner, nsec3)) {
         /* RFC 6840 section 4.1 "Clarifications on Nonexistence Proofs":
            Ancestor delegation NSEC or NSEC3 RRs MUST NOT be used to assume
            nonexistence of any RRs below that zone cut, which include all RRs at
@@ -630,6 +634,8 @@ bool AggressiveNSECCache::getNSEC3Denial(time_t now, std::shared_ptr<AggressiveN
     return false;
   }
 
+  /* An ancestor NSEC3 would be fine here, since it does prove that there is no delegation at the next closer
+     name (we don't insert opt-out NSEC3s into the cache). */
   DNSName wildcard(g_wildcarddnsname + closestEncloser);
   auto wcHash = toBase32Hex(hashQNameWithSalt(salt, iterations, wildcard));
   LOG("Looking for a NSEC3 covering the wildcard " << wildcard << " (" << wcHash << ")" << endl);
@@ -650,6 +656,8 @@ bool AggressiveNSECCache::getNSEC3Denial(time_t now, std::shared_ptr<AggressiveN
     }
 
     const DNSName wcSigner = getSigner(wcEntry.d_signatures);
+    /* It's an exact match for the wildcard, so it does exist. If we are looking for a DS
+       an ancestor NSEC3 is fine, otherwise it does not prove anything. */
     if (type != QType::DS && isNSEC3AncestorDelegation(wcSigner, wcEntry.d_owner, nsec3)) {
       /* RFC 6840 section 4.1 "Clarifications on Nonexistence Proofs":
          Ancestor delegation NSEC or NSEC3 RRs MUST NOT be used to assume
@@ -681,6 +689,8 @@ bool AggressiveNSECCache::getNSEC3Denial(time_t now, std::shared_ptr<AggressiveN
       return false;
     }
 
+    /* We have a NSEC3 proving that the wildcard does not exist. An ancestor NSEC3 would be fine here, since it does prove
+       that there is no delegation at the wildcard name (we don't insert opt-out NSEC3s into the cache). */
     res = RCode::NXDomain;
   }
 
@@ -731,6 +741,7 @@ bool AggressiveNSECCache::getDenial(time_t now, const DNSName& name, const QType
   }
 
   LOG(": found a possible NSEC at " << entry.d_owner << " ");
+  // note that matchesNSEC() takes care of ruling out ancestor NSECs for us
   auto denial = matchesNSEC(name, type.getCode(), entry.d_owner, content, entry.d_signatures);
   if (denial == dState::NODENIAL || denial == dState::INCONCLUSIVE) {
     LOG(" but it does no cover us" << endl);
diff --git a/pdns/recursordist/test-aggressive_nsec_cc.cc b/pdns/recursordist/test-aggressive_nsec_cc.cc
index 3cfec2f5ac..aa52826972 100644
--- a/pdns/recursordist/test-aggressive_nsec_cc.cc
+++ b/pdns/recursordist/test-aggressive_nsec_cc.cc
@@ -388,7 +388,7 @@ BOOST_AUTO_TEST_CASE(test_aggressive_nsec_ancestor)
   /* now we query other2.sub.powerdns.com, we should NOT be able to use the NSEC3s we have
      to prove that the name does not exist */
   ret.clear();
-  res = sr->beginResolve(DNSName("4.sub.powerdns.com"), QType(QType::A), QClass::IN, ret);
+  res = sr->beginResolve(DNSName("4.sub.powerdns.com"), QType(QType::DS), QClass::IN, ret);
   BOOST_CHECK_EQUAL(res, RCode::NoError);
   BOOST_CHECK_EQUAL(sr->getValidationState(), vState::Insecure);
   BOOST_REQUIRE_EQUAL(ret.size(), 1U);
@@ -918,7 +918,7 @@ BOOST_AUTO_TEST_CASE(test_aggressive_nsec3_ancestor)
   /* now we query other2.sub.powerdns.com, we should NOT be able to use the NSEC3s we have
      to prove that the name does not exist */
   ret.clear();
-  res = sr->beginResolve(DNSName("4.sub.powerdns.com"), QType(QType::A), QClass::IN, ret);
+  res = sr->beginResolve(DNSName("4.sub.powerdns.com"), QType(QType::DS), QClass::IN, ret);
   BOOST_CHECK_EQUAL(res, RCode::NoError);
   BOOST_CHECK_EQUAL(sr->getValidationState(), vState::Insecure);
   BOOST_REQUIRE_EQUAL(ret.size(), 1U);
-- 
2.47.3