From f08b59332e5a4e50ff9ca0010eb200c78b2a462a Mon Sep 17 00:00:00 2001
From: Remi Gacogne <remi.gacogne@powerdns.com>
Date: Tue, 22 Jun 2021 18:04:54 +0200
Subject: [PATCH] rec: Make sure that we pass the SOA along the NSEC(3) proof
 for DS queries

If the client is requesting a DS that does not exist, we need to
provide the SOA (+RRSIGs) along with the NSEC(3) proof (+RRSIGs)
and we might not have it if we picked up the proof from a delegation,
in which case we need to keep on to do the actual DS query.
It used to work before 4.5.0 because the zone cuts determination
code was requesting the DS records before doing any resolution, so we
would get the denial and the SOA at the very beginning and not replace
it on a delegation because we knew the zone was Insecure at that point.
Note that we still want to use the "no SOA denial" for internal zone
cuts computation since we don't care about the SOA at that point,
and that saves quite some outgoing queries.
---
 pdns/syncres.cc | 43 +++++++++++++++++++++++++++++++++----------
 pdns/syncres.hh |  3 +++
 2 files changed, 36 insertions(+), 10 deletions(-)

diff --git a/pdns/syncres.cc b/pdns/syncres.cc
index aabf85c2e9..07af513891 100644
--- a/pdns/syncres.cc
+++ b/pdns/syncres.cc
@@ -143,6 +143,13 @@ int SyncRes::beginResolve(const DNSName &qname, const QType qtype, QClass qclass
   else if(qclass!=QClass::IN)
     return -1;
 
+  if (qtype == QType::DS) {
+    d_externalDSQuery = qname;
+  }
+  else {
+    d_externalDSQuery.clear();
+  }
+
   set<GetBestNSAnswer> beenthere;
   int res=doResolve(qname, qtype, ret, depth, beenthere, state);
   d_queryValidationState = state;
@@ -1633,6 +1640,11 @@ static void reapRecordsFromNegCacheEntryForValidation(tcache_t& tcache, const ve
   }
 }
 
+static bool negativeCacheEntryHasSOA(const NegCache::NegCacheEntry& ne)
+{
+  return !ne.authoritySOA.records.empty();
+}
+
 static void reapRecordsForValidation(std::map<QType, CacheEntry>& entries, const vector<DNSRecord>& records)
 {
   for (const auto& rec : records) {
@@ -1743,15 +1755,23 @@ bool SyncRes::doCacheCheck(const DNSName &qname, const DNSName& authname, bool w
     if (qtype != QType::DS || ne.d_qtype.getCode() || ne.d_auth != qname ||
         g_negCache->get(qname, qtype, d_now, ne, true))
     {
-      res = RCode::NXDomain;
-      sttl = ne.d_ttd - d_now.tv_sec;
-      giveNegative = true;
-      cachedState = ne.d_validationState;
-      if (ne.d_qtype.getCode()) {
-        LOG(prefix<<qname<<": "<<qtype.toString()<<" is negatively cached via '"<<ne.d_auth<<"' for another "<<sttl<<" seconds"<<endl);
-        res = RCode::NoError;
-      } else {
-        LOG(prefix<<qname<<": Entire name '"<<qname<<"' is negatively cached via '"<<ne.d_auth<<"' for another "<<sttl<<" seconds"<<endl);
+      /* Careful! If the client is asking for a DS that does not exist, we need to provide the SOA along with the NSEC(3) proof
+         and we might not have it if we picked up the proof from a delegation, in which case we need to keep on to do the actual DS
+         query. */
+      if (qtype == QType::DS && ne.d_qtype.getCode() && !d_externalDSQuery.empty() && qname == d_externalDSQuery && !negativeCacheEntryHasSOA(ne)) {
+        giveNegative = false;
+      }
+      else {
+        res = RCode::NXDomain;
+        sttl = ne.d_ttd - d_now.tv_sec;
+        giveNegative = true;
+        cachedState = ne.d_validationState;
+        if (ne.d_qtype.getCode()) {
+          LOG(prefix<<qname<<": "<<qtype.toString()<<" is negatively cached via '"<<ne.d_auth<<"' for another "<<sttl<<" seconds"<<endl);
+          res = RCode::NoError;
+        } else {
+          LOG(prefix<<qname<<": Entire name '"<<qname<<"' is negatively cached via '"<<ne.d_auth<<"' for another "<<sttl<<" seconds"<<endl);
+        }
       }
     }
   } else if (s_hardenNXD != HardenNXD::No && !qname.isRoot() && !wasForwardedOrAuthZone) {
@@ -3643,7 +3663,10 @@ bool SyncRes::processRecords(const std::string& prefix, const DNSName& qname, co
               g_negCache->add(ne);
             }
 
-            if (qtype == QType::DS && qname == newauth) {
+            /* Careful! If the client is asking for a DS that does not exist, we need to provide the SOA along with the NSEC(3) proof
+               and we might not have it if we picked up the proof from a delegation, in which case we need to keep on to do the actual DS
+               query. */
+            if (qtype == QType::DS && qname == newauth && (d_externalDSQuery.empty() || qname != d_externalDSQuery)) {
               /* we are actually done! */
               negindic = true;
               negIndicHasSignatures = !ne.authoritySOA.signatures.empty() || !ne.DNSSECRecords.signatures.empty();
diff --git a/pdns/syncres.hh b/pdns/syncres.hh
index 3a5e71a76d..6285f6c87b 100644
--- a/pdns/syncres.hh
+++ b/pdns/syncres.hh
@@ -896,6 +896,9 @@ private:
   boost::optional<const boost::uuids::uuid&> d_initialRequestId;
   asyncresolve_t d_asyncResolve{nullptr};
   struct timeval d_now;
+  /* if the client is asking for a DS that does not exist, we need to provide the SOA along with the NSEC(3) proof
+     and we might not have it if we picked up the proof from a delegation */
+  DNSName d_externalDSQuery;
   string d_prefix;
   vState d_queryValidationState{vState::Indeterminate};
 
-- 
2.47.2