rec: Fix the gathering of denial proof for wildcard-expanded answers

If somehow the RRSIG indicating that the answer is expanded from a
wildcard (label count smaller than the number of labels in the name)
went _after_ the NSEC we need, we forgot to gather that NSEC.
It might have been an issue for downstream validation (we do gather
them a second time later for our own validation) since the client
would not have received them.

diff --git a/pdns/recursordist/test-syncres_cc5.cc b/pdns/recursordist/test-syncres_cc5.cc
index de48487d396ff7025f34b9ddd470e4885ea4919f..b267b6b21e74a7e4b5ffd2ff5c2572a2d0274a83 100644 (file)
--- a/pdns/recursordist/test-syncres_cc5.cc
+++ b/pdns/recursordist/test-syncres_cc5.cc
@@ -817,6 +817,124 @@ BOOST_AUTO_TEST_CASE(test_dnssec_validation_nsec_wildcard)
   BOOST_CHECK_EQUAL(queriesCount, 9U);
 }
 
+BOOST_AUTO_TEST_CASE(test_dnssec_validation_nsec_wildcard_proof_before_rrsig)
+{
+  /* this tests makes sure that we correctly detect that we need to gather
+     wildcard proof (since the answer is expanded from a wildcard, we need
+     to prove that the target name does not exist) even though the RRSIG which
+     allows us to detect that the answer is an expanded wildcard (from the label
+     count field of the RRSIG) comes _after_ the NSEC
+  */
+  std::unique_ptr<SyncRes> sr;
+  initSR(sr, true);
+
+  setDNSSECValidation(sr, DNSSECMode::ValidateAll);
+
+  primeHints();
+  const DNSName target("www.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);
+  generateKeyMaterial(DNSName("powerdns.com."), DNSSECKeeper::ECDSA256, DNSSECKeeper::DIGEST_SHA256, keys);
+
+  g_luaconfs.setState(luaconfsCopy);
+
+  size_t queriesCount = 0;
+
+  sr->setAsyncCallback([target, &queriesCount, 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) {
+    queriesCount++;
+
+    if (type == QType::DS || type == QType::DNSKEY) {
+      if (type == QType::DS && domain == target) {
+        setLWResult(res, RCode::NoError, true, false, true);
+        addRecordToLW(res, DNSName("powerdns.com."), QType::SOA, "pdns-public-ns1.powerdns.com. pieter\\.lexis.powerdns.com. 2017032301 10800 3600 604800 3600", DNSResourceRecord::AUTHORITY, 3600);
+        addRRSIG(keys, res->d_records, DNSName("powerdns.com."), 300);
+        addNSECRecordToLW(DNSName("www.powerdns.com."), DNSName("wwz.powerdns.com."), {QType::A, QType::NSEC, QType::RRSIG}, 600, res->d_records);
+        addRRSIG(keys, res->d_records, DNSName("powerdns.com"), 300);
+        return LWResult::Result::Success;
+      }
+      else {
+        return genericDSAndDNSKEYHandler(res, domain, domain, type, keys);
+      }
+    }
+    else {
+      if (isRootServer(ip)) {
+        setLWResult(res, 0, false, false, true);
+        addRecordToLW(res, "com.", QType::NS, "a.gtld-servers.com.", DNSResourceRecord::AUTHORITY, 3600);
+        addDS(DNSName("com."), 300, res->d_records, keys);
+        addRRSIG(keys, res->d_records, DNSName("."), 300);
+        addRecordToLW(res, "a.gtld-servers.com.", QType::A, "192.0.2.1", DNSResourceRecord::ADDITIONAL, 3600);
+        return LWResult::Result::Success;
+      }
+      else if (ip == ComboAddress("192.0.2.1:53")) {
+        if (domain == DNSName("com.")) {
+          setLWResult(res, 0, true, false, true);
+          addRecordToLW(res, domain, QType::NS, "a.gtld-servers.com.");
+          addRRSIG(keys, res->d_records, domain, 300);
+          addRecordToLW(res, "a.gtld-servers.com.", QType::A, "192.0.2.1", DNSResourceRecord::ADDITIONAL, 3600);
+          addRRSIG(keys, res->d_records, domain, 300);
+        }
+        else {
+          setLWResult(res, 0, false, false, true);
+          addRecordToLW(res, "powerdns.com.", QType::NS, "ns1.powerdns.com.", DNSResourceRecord::AUTHORITY, 3600);
+          addDS(DNSName("powerdns.com."), 300, res->d_records, keys);
+          addRRSIG(keys, res->d_records, DNSName("com."), 300);
+          addRecordToLW(res, "ns1.powerdns.com.", QType::A, "192.0.2.2", DNSResourceRecord::ADDITIONAL, 3600);
+        }
+        return LWResult::Result::Success;
+      }
+      else if (ip == ComboAddress("192.0.2.2:53")) {
+        setLWResult(res, 0, true, false, true);
+        if (type == QType::NS) {
+          if (domain == DNSName("powerdns.com.")) {
+            addRecordToLW(res, domain, QType::NS, "ns1.powerdns.com.");
+            addRRSIG(keys, res->d_records, DNSName("powerdns.com"), 300);
+            addRecordToLW(res, "ns1.powerdns.com.", QType::A, "192.0.2.2", DNSResourceRecord::ADDITIONAL, 3600);
+            addRRSIG(keys, res->d_records, DNSName("powerdns.com"), 300);
+          }
+          else {
+            addRecordToLW(res, domain, QType::SOA, "pdns-public-ns1.powerdns.com. pieter\\.lexis.powerdns.com. 2017032301 10800 3600 604800 3600", DNSResourceRecord::AUTHORITY, 3600);
+            addRRSIG(keys, res->d_records, DNSName("powerdns.com"), 300);
+            addNSECRecordToLW(DNSName("www.powerdns.com."), DNSName("wwz.powerdns.com."), {QType::A, QType::NSEC, QType::RRSIG}, 600, res->d_records);
+            addRRSIG(keys, res->d_records, DNSName("powerdns.com"), 300);
+          }
+        }
+        else {
+          addRecordToLW(res, domain, QType::A, "192.0.2.42");
+          addRRSIG(keys, res->d_records, DNSName("powerdns.com"), 300, false, boost::none, DNSName("*.powerdns.com"));
+          /* we need to add the proof that this name does not exist, so the wildcard may apply */
+          addNSECRecordToLW(DNSName("a.powerdns.com."), DNSName("wwz.powerdns.com."), {QType::A, QType::NSEC, QType::RRSIG}, 600, res->d_records);
+          addRRSIG(keys, res->d_records, DNSName("powerdns.com"), 300);
+          /* now this is the important part! We are swapping the first RRSIG and the NSEC, to make sure we still gather the NSEC proof that the
+             exact name does not exist even though we have not seen the RRSIG whose label count is smaller than the target name yet */
+          std::swap(res->d_records.at(1), res->d_records.at(3));
+        }
+        return LWResult::Result::Success;
+      }
+    }
+
+    return LWResult::Result::Timeout;
+  });
+
+  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);
+  BOOST_REQUIRE_EQUAL(ret.size(), 4U);
+  BOOST_CHECK_EQUAL(queriesCount, 9U);
+
+  /* again, to test the cache */
+  ret.clear();
+  res = sr->beginResolve(target, QType(QType::A), QClass::IN, ret);
+  BOOST_CHECK_EQUAL(res, RCode::NoError);
+  BOOST_CHECK_EQUAL(sr->getValidationState(), vState::Secure);
+  BOOST_REQUIRE_EQUAL(ret.size(), 4U);
+  BOOST_CHECK_EQUAL(queriesCount, 9U);
+}
+
 BOOST_AUTO_TEST_CASE(test_dnssec_validation_nsec_nodata_nowildcard)
 {
   std::unique_ptr<SyncRes> sr;

diff --git a/pdns/syncres.cc b/pdns/syncres.cc
index 92f674e4de412ba7f3ec07c3f526409e17f9e9f5..b00d15a9284b93fdb8527e18335f29fff3926898 100644 (file)
--- a/pdns/syncres.cc
+++ b/pdns/syncres.cc
@@ -2908,20 +2908,6 @@ RCode::rcodes_ SyncRes::updateCacheFromRecords(unsigned int depth, LWResult& lwr
       isCNAMEAnswer = false;
     }
 
-    /* if we have a positive answer synthesized from a wildcard,
-       we need to store the corresponding NSEC/NSEC3 records proving
-       that the exact name did not exist in the negative cache */
-    if(gatherWildcardProof) {
-      if (nsecTypes.count(rec.d_type)) {
-        authorityRecs.push_back(std::make_shared<DNSRecord>(rec));
-      }
-      else if (rec.d_type == QType::RRSIG) {
-        auto rrsig = getRR<RRSIGRecordContent>(rec);
-        if (rrsig && nsecTypes.count(rrsig->d_type)) {
-          authorityRecs.push_back(std::make_shared<DNSRecord>(rec));
-        }
-      }
-    }
     if (rec.d_type == QType::RRSIG) {
       auto rrsig = getRR<RRSIGRecordContent>(rec);
       if (rrsig) {
@@ -2953,8 +2939,29 @@ RCode::rcodes_ SyncRes::updateCacheFromRecords(unsigned int depth, LWResult& lwr
     }
   }
 
+  /* if we have a positive answer synthesized from a wildcard,
+     we need to store the corresponding NSEC/NSEC3 records proving
+     that the exact name did not exist in the negative cache */
+  if (gatherWildcardProof) {
+    for (const auto& rec : lwr.d_records) {
+      if (rec.d_type == QType::OPT || rec.d_class != QClass::IN) {
+        continue;
+      }
+
+      if (nsecTypes.count(rec.d_type)) {
+        authorityRecs.push_back(std::make_shared<DNSRecord>(rec));
+      }
+      else if (rec.d_type == QType::RRSIG) {
+        auto rrsig = getRR<RRSIGRecordContent>(rec);
+        if (rrsig && nsecTypes.count(rrsig->d_type)) {
+          authorityRecs.push_back(std::make_shared<DNSRecord>(rec));
+        }
+      }
+    }
+  }
+
   // reap all answers from this packet that are acceptable
-  for(auto& rec : lwr.d_records) {
+  for (auto& rec : lwr.d_records) {
     if(rec.d_type == QType::OPT) {
       LOG(prefix<<qname<<": OPT answer '"<<rec.d_name<<"' from '"<<auth<<"' nameservers" <<endl);
       continue;