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.

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 2e37749d3dcc90a5726ed247f9cf69c12dfa7f3a..17721259eea04291ed56f286040ad973615eceb1 100644 (file)
--- a/pdns/syncres.cc
+++ b/pdns/syncres.cc
@@ -3774,7 +3774,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;
@@ -3786,7 +3786,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) {
@@ -3880,9 +3881,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) {
@@ -3893,6 +3895,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 e1ea08718f2e552d7834087aea68e4add6c15a22..cc2d168bcafac8715c6377fb10e1cf04ccc7f6b4 100644 (file)
--- a/pdns/syncres.hh
+++ b/pdns/syncres.hh
@@ -595,7 +595,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);