Failure to retrieve DNSKEYs of an Insecure zone should not be fatal.

This issue happens if a record set is signed even though the zone
itself is Insecure. Syncres then tries to retrieve DNSKEYs and a
timeout on that would lead to an ImmediateServFailException.

Only throw exception later in validateRecordsWithSigs, after checking
zone cuts, when we are sure the zone is Secure.

(cherry picked from commit 6dc8b0b2c6fb2e628356f8dc5c5de4dfd919ec5d)

diff --git a/pdns/recursordist/test-syncres_cc5.cc b/pdns/recursordist/test-syncres_cc5.cc
index b6230b98e2175bcee51394e777335f3a940bd403..1af1d8719665d6a79168973a13f7d4fa586d7199 100644 (file)
--- a/pdns/recursordist/test-syncres_cc5.cc
+++ b/pdns/recursordist/test-syncres_cc5.cc
@@ -2179,12 +2179,12 @@ BOOST_AUTO_TEST_CASE(test_dnssec_secure_servfail_ds)
   }
 }
 
-BOOST_AUTO_TEST_CASE(test_dnssec_secure_servfail_dnskey)
+static void dnssec_secure_servfail_dnskey(DNSSECMode mode, vState expectedValidationResult)
 {
   std::unique_ptr<SyncRes> sr;
   initSR(sr, true);
 
-  setDNSSECValidation(sr, DNSSECMode::ValidateAll);
+  setDNSSECValidation(sr, mode);
 
   primeHints();
   const DNSName target("powerdns.com.");
@@ -2289,4 +2289,121 @@ BOOST_AUTO_TEST_CASE(test_dnssec_secure_servfail_dnskey)
   }
 }
 
+BOOST_AUTO_TEST_CASE(test_dnssec_secure_servfail_dnskey)
+{
+  dnssec_secure_servfail_dnskey(DNSSECMode::ValidateAll, vState::Indeterminate);
+  dnssec_secure_servfail_dnskey(DNSSECMode::Off, vState::Indeterminate);
+}
+
+// Same test as above but powerdns.com is now Insecure according to parent, so failure to retrieve DNSSKEYs
+// should be mostly harmless.
+static void dnssec_secure_servfail_dnskey_insecure(DNSSECMode mode, vState expectedValidationResult)
+{
+  std::unique_ptr<SyncRes> sr;
+  initSR(sr, true);
+
+  setDNSSECValidation(sr, mode);
+
+  primeHints();
+  const DNSName target("powerdns.com.");
+  const ComboAddress targetAddr("192.0.2.42");
+
+  // We use two sets of keys, as powerdns.com is Insecure according to parent but returns signed results,
+  // triggering a (failing) DNSKEY retrieval.
+  testkeysset_t keys;
+  testkeysset_t pdnskeys;
+
+  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, pdnskeys);
+
+  g_luaconfs.setState(luaconfsCopy);
+
+  size_t queriesCount = 0;
+
+  /* make sure that the signature inception and validity times are computed
+     based on the SyncRes time, not the current one, in case the function
+     takes too long. */
+
+  const time_t fixedNow = sr->getNow().tv_sec;
+
+  sr->setAsyncCallback([target, targetAddr, &queriesCount, keys, pdnskeys, fixedNow](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++;
+
+    DNSName auth = domain;
+    if (domain == target) {
+      auth = DNSName("powerdns.com.");
+    }
+
+    if (type == QType::DNSKEY && domain == DNSName("powerdns.com.")) {
+      /* time out */
+      return LWResult::Result::Timeout;
+    }
+
+    if (type == QType::DS || type == QType::DNSKEY) {
+      // This one does not know about pdnskeys, so it will declare powerdns.com as Insecure
+      return genericDSAndDNSKEYHandler(res, domain, auth, type, keys, true, fixedNow);
+    }
+
+    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, false, boost::none, boost::none, fixedNow);
+      addRecordToLW(res, "a.gtld-servers.com.", QType::A, "192.0.2.1", DNSResourceRecord::ADDITIONAL, 3600);
+      return LWResult::Result::Success;
+    }
+
+    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, false, boost::none, boost::none, fixedNow);
+        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, auth, QType::NS, "ns1.powerdns.com.", DNSResourceRecord::AUTHORITY, 3600);
+        addDS(auth, 300, res->d_records, keys);
+        addRRSIG(keys, res->d_records, DNSName("com."), 300, false, boost::none, boost::none, fixedNow);
+        addRecordToLW(res, "ns1.powerdns.com.", QType::A, "192.0.2.2", DNSResourceRecord::ADDITIONAL, 3600);
+      }
+      return LWResult::Result::Success;
+    }
+
+    if (ip == ComboAddress("192.0.2.2:53")) {
+      if (type == QType::NS) {
+        setLWResult(res, 0, true, false, true);
+        addRecordToLW(res, domain, QType::NS, "ns1.powerdns.com.");
+        addRRSIG(pdnskeys, res->d_records, auth, 300, false, boost::none, boost::none, fixedNow);
+        addRecordToLW(res, "ns1.powerdns.com.", QType::A, "192.0.2.2", DNSResourceRecord::ADDITIONAL, 3600);
+        addRRSIG(pdnskeys, res->d_records, auth, 300);
+      }
+      else {
+        setLWResult(res, RCode::NoError, true, false, true);
+        addRecordToLW(res, domain, QType::A, targetAddr.toString(), DNSResourceRecord::ANSWER, 3600);
+        addRRSIG(pdnskeys, res->d_records, auth, 300, false, boost::none, boost::none, fixedNow);
+      }
+      return LWResult::Result::Success;
+    }
+
+    return LWResult::Result::Timeout;
+  });
+
+  vector<DNSRecord> ret;
+  auto res = sr->beginResolve(target, QType(QType::A), QClass::IN, ret);
+  BOOST_CHECK_EQUAL(res, 0);
+  BOOST_CHECK_EQUAL(sr->getValidationState(), expectedValidationResult);
+  BOOST_REQUIRE_EQUAL(ret.size(), 2U);
+}
+
+BOOST_AUTO_TEST_CASE(test_dnssec_secure_servfail_dnskey_insecure)
+{
+  dnssec_secure_servfail_dnskey_insecure(DNSSECMode::ValidateAll, vState::Insecure);
+  dnssec_secure_servfail_dnskey_insecure(DNSSECMode::Off, vState::Insecure);
+}
+
 BOOST_AUTO_TEST_SUITE_END()

diff --git a/pdns/syncres.cc b/pdns/syncres.cc
index 372b6999b7ca73a95113697792bcfa140f3326ca..389eda7b9868b0e147b4024ccab049e3ec8f1aea 100644 (file)
--- a/pdns/syncres.cc
+++ b/pdns/syncres.cc
@@ -2806,7 +2806,7 @@ vState SyncRes::validateDNSKeys(const DNSName& zone, const std::vector<DNSRecord
   return state;
 }
 
-vState SyncRes::getDNSKeys(const DNSName& signer, skeyset_t& keys, unsigned int depth)
+vState SyncRes::getDNSKeys(const DNSName& signer, skeyset_t& keys, bool& servFailOccurred, unsigned int depth)
 {
   std::vector<DNSRecord> records;
   std::set<GetBestNSAnswer> beenthere;
@@ -2818,7 +2818,8 @@ vState SyncRes::getDNSKeys(const DNSName& signer, skeyset_t& keys, unsigned int
   setCacheOnly(oldCacheOnly);
 
   if (rcode == RCode::ServFail) {
-    throw ImmediateServFailException("Server Failure while retrieving DNSKEY records for " + signer.toLogString());
+    servFailOccurred = true;
+    return vState::BogusUnableToGetDNSKEYs;
   }
 
   if (rcode == RCode::NoError) {
@@ -2912,9 +2913,10 @@ vState SyncRes::validateRecordsWithSigs(unsigned int depth, const DNSName& qname
         }
       }
     }
+    bool servFailOccurred = false;
     if (state == vState::Secure) {
       LOG(d_prefix<<"retrieving the DNSKEYs for "<<signer<<endl);
-      state = getDNSKeys(signer, keys, depth);
+      state = getDNSKeys(signer, keys, servFailOccurred, depth);
     }
 
     if (state != vState::Secure) {
@@ -2925,6 +2927,9 @@ vState SyncRes::validateRecordsWithSigs(unsigned int depth, const DNSName& qname
       LOG(d_prefix<<"checking whether we missed a zone cut for "<<signer<<" before returning a Bogus state for "<<name<<"|"<<type.toString()<<endl);
       auto zState = getValidationStatus(signer, false, dsFailed, depth);
       if (zState == vState::Secure) {
+        if (state == vState::BogusUnableToGetDNSKEYs && servFailOccurred) {
+          throw ImmediateServFailException("Server Failure while retrieving DNSKEY records for " + signer.toLogString());
+        }
         /* too bad */
         LOG(d_prefix<<"we are still in a Secure zone, returning "<<vStateToString(state)<<endl);
         return state;

diff --git a/pdns/syncres.hh b/pdns/syncres.hh
index 364f93aee61644f2acf04c84523784d20bccc97a..934e6960a6ca8745de8df52079b8a3d47118e812 100644 (file)
--- a/pdns/syncres.hh
+++ b/pdns/syncres.hh
@@ -885,7 +885,7 @@ private:
   void updateValidationState(vState& state, const vState stateUpdate);
   vState validateRecordsWithSigs(unsigned int depth, const DNSName& qname, const QType qtype, const DNSName& name, const QType type, const std::vector<DNSRecord>& records, const std::vector<std::shared_ptr<RRSIGRecordContent> >& signatures);
   vState validateDNSKeys(const DNSName& zone, const std::vector<DNSRecord>& dnskeys, const std::vector<std::shared_ptr<RRSIGRecordContent> >& signatures, unsigned int depth);
-  vState getDNSKeys(const DNSName& signer, skeyset_t& keys, unsigned int depth);
+  vState getDNSKeys(const DNSName& signer, skeyset_t& keys, bool& servFailOccurred, unsigned int depth);
   dState getDenialValidationState(const NegCache::NegCacheEntry& ne, const dState expectedState, bool referralToUnsigned);
   void updateDenialValidationState(vState& neValidationState, const DNSName& neName, vState& state, const dState denialState, const dState expectedState, bool isDS, unsigned int depth);
   void computeNegCacheValidationStatus(const NegCache::NegCacheEntry& ne, const DNSName& qname, QType qtype, const int res, vState& state, unsigned int depth);