From: Remi Gacogne <remi.gacogne@powerdns.com>
Date: Mon, 3 May 2021 13:00:04 +0000 (+0200)
Subject: rec: Only add the NSEC and RRSIG records once in wildcard NODATA answers
X-Git-Tag: dnsdist-1.7.0-alpha0~8^2
X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=refs%2Fpull%2F10350%2Fhead;p=thirdparty%2Fpdns.git

rec: Only add the NSEC and RRSIG records once in wildcard NODATA answers

For wildcard-expanded answers we need to collect the proof that the
exact name does not exist and add them to the response. We also
collect that proof for negative answers.
When the answer is a wildcard-expanded NODATA, we only need to collect
them once, not twice.
---

diff --git a/pdns/recursordist/test-syncres_cc5.cc b/pdns/recursordist/test-syncres_cc5.cc
index 7f83dccdfd..b6230b98e2 100644
--- a/pdns/recursordist/test-syncres_cc5.cc
+++ b/pdns/recursordist/test-syncres_cc5.cc
@@ -1825,6 +1825,60 @@ BOOST_AUTO_TEST_CASE(test_dnssec_validation_wildcard_expanded_onto_itself)
   BOOST_REQUIRE_EQUAL(ret.size(), 4U);
 }
 
+BOOST_AUTO_TEST_CASE(test_dnssec_validation_wildcard_expanded_onto_itself_nodata)
+{
+  std::unique_ptr<SyncRes> sr;
+  initSR(sr, true);
+
+  setDNSSECValidation(sr, DNSSECMode::ValidateAll);
+
+  primeHints();
+  const DNSName target("*.powerdns.com.");
+  testkeysset_t keys;
+
+  auto luaconfsCopy = g_luaconfs.getCopy();
+  luaconfsCopy.dsAnchors.clear();
+  generateKeyMaterial(g_rootdnsname, DNSSECKeeper::ECDSA256, DNSSECKeeper::DIGEST_SHA256, keys, luaconfsCopy.dsAnchors);
+  generateKeyMaterial(DNSName("com."), DNSSECKeeper::ECDSA256, DNSSECKeeper::DIGEST_SHA256, keys, luaconfsCopy.dsAnchors);
+  generateKeyMaterial(DNSName("powerdns.com."), DNSSECKeeper::ECDSA256, DNSSECKeeper::DIGEST_SHA256, keys, luaconfsCopy.dsAnchors);
+
+  g_luaconfs.setState(luaconfsCopy);
+
+  sr->setAsyncCallback([target, keys](const ComboAddress& ip, const DNSName& domain, int type, bool doTCP, bool sendRDQuery, int EDNS0Level, struct timeval* now, boost::optional<Netmask>& srcmask, boost::optional<const ResolveContext&> context, LWResult* res, bool* chained) {
+    if (type == QType::DS || type == QType::DNSKEY) {
+      if (domain == target) {
+        const auto auth = DNSName("powerdns.com.");
+        /* we don't want a cut there */
+        setLWResult(res, 0, true, false, true);
+        addRecordToLW(res, auth, QType::SOA, "foo. bar. 2017032800 1800 900 604800 86400", DNSResourceRecord::AUTHORITY, 86400);
+        addRRSIG(keys, res->d_records, auth, 300);
+        /* add a NSEC denying the DS */
+        std::set<uint16_t> types = {QType::NSEC};
+        addNSECRecordToLW(domain, DNSName("z") + domain, types, 600, res->d_records);
+        addRRSIG(keys, res->d_records, auth, 300);
+        return LWResult::Result::Success;
+      }
+      return genericDSAndDNSKEYHandler(res, domain, domain, type, keys);
+    }
+    else {
+      setLWResult(res, 0, true, false, true);
+      addRecordToLW(res, domain, QType::SOA, "powerdns.com. bar. 2017032800 1800 900 604800 86400", DNSResourceRecord::AUTHORITY, 86400);
+      addRRSIG(keys, res->d_records, DNSName("powerdns.com."), 300);
+      /* add the proof that the exact name does exist but that this type does not */
+      addNSECRecordToLW(DNSName("*.powerdns.com."), DNSName("\\000.*.powerdns.com."), {QType::AAAA, QType::NSEC, QType::RRSIG}, 600, res->d_records);
+      addRRSIG(keys, res->d_records, DNSName("powerdns.com"), 300, false, boost::none, DNSName("*.powerdns.com"));
+      return LWResult::Result::Success;
+    }
+  });
+
+  vector<DNSRecord> ret;
+  int res = sr->beginResolve(target, QType(QType::A), QClass::IN, ret);
+  BOOST_CHECK_EQUAL(res, RCode::NoError);
+  BOOST_CHECK_EQUAL(sr->getValidationState(), vState::Secure);
+  /* SOA + RRSIG, NSEC + RRSIG */
+  BOOST_REQUIRE_EQUAL(ret.size(), 4U);
+}
+
 BOOST_AUTO_TEST_CASE(test_dnssec_validation_wildcard_like_expanded_from_wildcard)
 {
   std::unique_ptr<SyncRes> sr;
diff --git a/pdns/syncres.cc b/pdns/syncres.cc
index d3c57be009..103bfa73af 100644
--- a/pdns/syncres.cc
+++ b/pdns/syncres.cc
@@ -3530,8 +3530,9 @@ bool SyncRes::processRecords(const std::string& prefix, const DNSName& qname, co
     }
     /* if we have a positive answer synthesized from a wildcard, we need to
        return the corresponding NSEC/NSEC3 records from the AUTHORITY section
-       proving that the exact name did not exist */
-    else if (gatherWildcardProof && (rec.d_type == QType::RRSIG || rec.d_type == QType::NSEC || rec.d_type == QType::NSEC3) && rec.d_place == DNSResourceRecord::AUTHORITY) {
+       proving that the exact name did not exist.
+       Except if this is a NODATA answer because then we will gather the NXNSEC records later */
+    else if (gatherWildcardProof && !negindic && (rec.d_type == QType::RRSIG || rec.d_type == QType::NSEC || rec.d_type == QType::NSEC3) && rec.d_place == DNSResourceRecord::AUTHORITY) {
       ret.push_back(rec); // enjoy your DNSSEC
     }
     // for ANY answers we *must* have an authoritative answer, unless we are forwarding recursively
@@ -3596,7 +3597,7 @@ bool SyncRes::processRecords(const std::string& prefix, const DNSName& qname, co
       } else if (rec.d_type == QType::RRSIG && qname.isPartOf(rec.d_name)) {
         auto rrsig = getRR<RRSIGRecordContent>(rec);
         if (rrsig != nullptr && rrsig->d_type == QType::DNAME) {
-           ret.push_back(rec);
+          ret.push_back(rec);
         }
       }
     }