rec: Don't negcache entries for longer than their RRSIG validity

diff --git a/pdns/recursordist/test-syncres_cc.cc b/pdns/recursordist/test-syncres_cc.cc
index 9961fb1381af61018c28d362fff80f8cb9dc018e..5c82bb41de2c426fc3ca321989858316535c0724 100644 (file)
--- a/pdns/recursordist/test-syncres_cc.cc
+++ b/pdns/recursordist/test-syncres_cc.cc
@@ -278,10 +278,10 @@ static bool addRRSIG(const testkeysset_t& keys, std::vector<DNSRecord>& records,
   }
 
   DNSRecord rec;
+  rec.d_type = QType::RRSIG;
   rec.d_place = records[recordsCount-1].d_place;
   rec.d_name = records[recordsCount-1].d_name;
-  rec.d_type = QType::RRSIG;
-  rec.d_ttl = sigValidity;
+  rec.d_ttl = records[recordsCount-1].d_ttl;
 
   rec.d_content = std::make_shared<RRSIGRecordContent>(rrc);
   records.push_back(rec);
@@ -428,8 +428,8 @@ static int genericDSAndDNSKEYHandler(LWResult* res, const DNSName& domain, DNSNa
 
   if (type == QType::DNSKEY) {
     setLWResult(res, 0, true, false, true);
-    addDNSKEY(keys, auth, 300, res->d_records);
-    addRRSIG(keys, res->d_records, auth, 300);
+    addDNSKEY(keys, domain, 300, res->d_records);
+    addRRSIG(keys, res->d_records, domain, 300);
     return 1;
   }
 
@@ -7303,6 +7303,136 @@ BOOST_AUTO_TEST_CASE(test_nsec3_insecure_delegation_denial) {
   BOOST_CHECK_EQUAL(denialState, INSECURE);
 }
 
+BOOST_AUTO_TEST_CASE(test_dnssec_rrsig_negcache_validity) {
+  std::unique_ptr<SyncRes> sr;
+  const time_t now = time(nullptr);
+  initSR(sr, true);
+
+  setDNSSECValidation(sr, DNSSECMode::ValidateAll);
+
+  primeHints();
+  const DNSName target("com.");
+  testkeysset_t keys;
+
+  auto luaconfsCopy = g_luaconfs.getCopy();
+  luaconfsCopy.dsAnchors.clear();
+  generateKeyMaterial(g_rootdnsname, DNSSECKeeper::ECDSA256, DNSSECKeeper::SHA256, keys, luaconfsCopy.dsAnchors);
+  generateKeyMaterial(DNSName("com."), DNSSECKeeper::ECDSA256, DNSSECKeeper::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, std::shared_ptr<RemoteLogger> outgoingLogger, LWResult* res) {
+      queriesCount++;
+
+      DNSName auth = domain;
+      auth.chopOff();
+
+      if (type == QType::DS || type == QType::DNSKEY) {
+        return genericDSAndDNSKEYHandler(res, domain, auth, type, keys);
+      }
+      else {
+        setLWResult(res, RCode::NoError, true, false, true);
+        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, domain, 300);
+        addNSECRecordToLW(domain, DNSName("z."), { QType::NSEC, QType::RRSIG }, 600, res->d_records);
+        addRRSIG(keys, res->d_records, domain, 1);
+        return 1;
+      }
+
+      return 0;
+    });
+
+  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(), Secure);
+  BOOST_REQUIRE_EQUAL(ret.size(), 4);
+  BOOST_CHECK_EQUAL(queriesCount, 4);
+
+  /* check that the entry has not been negatively cached for longer than the RRSIG validity */
+  NegCache::NegCacheEntry ne;
+  BOOST_CHECK_EQUAL(SyncRes::t_sstorage.negcache.size(), 1);
+  BOOST_REQUIRE_EQUAL(SyncRes::t_sstorage.negcache.get(target, QType(QType::A), sr->getNow(), ne), true);
+  BOOST_CHECK_EQUAL(ne.d_ttd, now + 1);
+  BOOST_CHECK_EQUAL(ne.authoritySOA.records.size(), 1);
+  BOOST_CHECK_EQUAL(ne.authoritySOA.signatures.size(), 1);
+  BOOST_CHECK_EQUAL(ne.DNSSECRecords.records.size(), 1);
+  BOOST_CHECK_EQUAL(ne.DNSSECRecords.signatures.size(), 1);
+
+  /* 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(), Secure);
+  BOOST_REQUIRE_EQUAL(ret.size(), 4);
+  BOOST_CHECK_EQUAL(queriesCount, 4);
+}
+
+BOOST_AUTO_TEST_CASE(test_dnssec_rrsig_cache_validity) {
+  std::unique_ptr<SyncRes> sr;
+  const time_t now = time(nullptr);
+  initSR(sr, true);
+
+  setDNSSECValidation(sr, DNSSECMode::ValidateAll);
+
+  primeHints();
+  const DNSName target("com.");
+  const ComboAddress targetAddr("192.0.2.42");
+  testkeysset_t keys;
+
+  auto luaconfsCopy = g_luaconfs.getCopy();
+  luaconfsCopy.dsAnchors.clear();
+  generateKeyMaterial(g_rootdnsname, DNSSECKeeper::ECDSA256, DNSSECKeeper::SHA256, keys, luaconfsCopy.dsAnchors);
+  generateKeyMaterial(DNSName("com."), DNSSECKeeper::ECDSA256, DNSSECKeeper::SHA256, keys);
+  g_luaconfs.setState(luaconfsCopy);
+
+  size_t queriesCount = 0;
+
+  sr->setAsyncCallback([target,targetAddr,&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, std::shared_ptr<RemoteLogger> outgoingLogger, LWResult* res) {
+      queriesCount++;
+
+      DNSName auth = domain;
+      auth.chopOff();
+
+      if (type == QType::DS || type == QType::DNSKEY) {
+        return genericDSAndDNSKEYHandler(res, domain, auth, type, keys);
+      }
+      else {
+        setLWResult(res, RCode::NoError, true, false, true);
+        addRecordToLW(res, domain, QType::A, targetAddr.toString(), DNSResourceRecord::ANSWER, 3600);
+        addRRSIG(keys, res->d_records, domain, 1);
+        return 1;
+      }
+
+      return 0;
+    });
+
+  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(), Secure);
+  BOOST_REQUIRE_EQUAL(ret.size(), 2);
+  BOOST_CHECK_EQUAL(queriesCount, 4);
+
+  /* check that the entry has not been cached for longer than the RRSIG validity */
+  const ComboAddress who;
+  vector<DNSRecord> cached;
+  vector<std::shared_ptr<RRSIGRecordContent>> signatures;
+  BOOST_REQUIRE_EQUAL(t_RC->get(now, target, QType(QType::A), true, &cached, who, &signatures), 1);
+  BOOST_REQUIRE_EQUAL(cached.size(), 1);
+  BOOST_REQUIRE_EQUAL(signatures.size(), 1);
+  BOOST_CHECK_EQUAL((cached[0].d_ttl - now), 1);
+
+  /* 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(), Secure);
+  BOOST_REQUIRE_EQUAL(ret.size(), 2);
+  BOOST_CHECK_EQUAL(queriesCount, 4);
+}
+
 /*
 // cerr<<"asyncresolve called to ask "<<ip.toStringWithPort()<<" about "<<domain.toString()<<" / "<<QType(type).getName()<<" over "<<(doTCP ? "TCP" : "UDP")<<" (rd: "<<sendRDQuery<<", EDNS0 level: "<<EDNS0Level<<")"<<endl;
 

diff --git a/pdns/syncres.cc b/pdns/syncres.cc
index 79253ecbca2cb73705e79b7c5abcba3d8523b91d..e964ffc833c4aa404d383c09fb0fce141994df98 100644 (file)
--- a/pdns/syncres.cc
+++ b/pdns/syncres.cc
@@ -1117,6 +1117,15 @@ static bool magicAddrMatch(const QType& query, const QType& answer)
   return answer.getCode() == QType::A || answer.getCode() == QType::AAAA;
 }
 
+static uint32_t getRRSIGTTL(const time_t now, const std::shared_ptr<RRSIGRecordContent>& rrsig)
+{
+  uint32_t res = 0;
+  if (now < rrsig->d_sigexpire) {
+    res = static_cast<uint32_t>(rrsig->d_sigexpire) - now;
+  }
+  return res;
+}
+
 static const set<uint16_t> nsecTypes = {QType::NSEC, QType::NSEC3};
 
 /* Fills the authoritySOA and DNSSECRecords fields from ne with those found in the records
@@ -1124,7 +1133,7 @@ static const set<uint16_t> nsecTypes = {QType::NSEC, QType::NSEC3};
  * \param records The records to parse for the authority SOA and NSEC(3) records
  * \param ne      The NegCacheEntry to be filled out (will not be cleared, only appended to
  */
-static void harvestNXRecords(const vector<DNSRecord>& records, NegCache::NegCacheEntry& ne) {
+static void harvestNXRecords(const vector<DNSRecord>& records, NegCache::NegCacheEntry& ne, const time_t now, uint32_t* lowestTTL) {
   for(const auto& rec : records) {
     if(rec.d_place != DNSResourceRecord::AUTHORITY)
       // RFC 4035 section 3.1.3. indicates that NSEC records MUST be placed in
@@ -1138,19 +1147,33 @@ static void harvestNXRecords(const vector<DNSRecord>& records, NegCache::NegCach
       if(rrsig) {
         if(rrsig->d_type == QType::SOA) {
           ne.authoritySOA.signatures.push_back(rec);
+          if (lowestTTL && isRRSIGNotExpired(now, rrsig)) {
+            *lowestTTL = min(*lowestTTL, rec.d_ttl);
+            *lowestTTL = min(*lowestTTL, getRRSIGTTL(now, rrsig));
+          }
         }
         if(nsecTypes.count(rrsig->d_type)) {
           ne.DNSSECRecords.signatures.push_back(rec);
+          if (lowestTTL && isRRSIGNotExpired(now, rrsig)) {
+            *lowestTTL = min(*lowestTTL, rec.d_ttl);
+            *lowestTTL = min(*lowestTTL, getRRSIGTTL(now, rrsig));
+          }
         }
       }
       continue;
     }
     if(rec.d_type == QType::SOA) {
       ne.authoritySOA.records.push_back(rec);
+      if (lowestTTL) {
+        *lowestTTL = min(*lowestTTL, rec.d_ttl);
+      }
       continue;
     }
     if(nsecTypes.count(rec.d_type)) {
       ne.DNSSECRecords.records.push_back(rec);
+      if (lowestTTL) {
+        *lowestTTL = min(*lowestTTL, rec.d_ttl);
+      }
       continue;
     }
   }
@@ -1178,7 +1201,7 @@ static cspmap_t harvestCSPFromNE(const NegCache::NegCacheEntry& ne)
 static void addNXNSECS(vector<DNSRecord>&ret, const vector<DNSRecord>& records)
 {
   NegCache::NegCacheEntry ne;
-  harvestNXRecords(records, ne);
+  harvestNXRecords(records, ne, 0, nullptr);
   ret.insert(ret.end(), ne.authoritySOA.signatures.begin(), ne.authoritySOA.signatures.end());
   ret.insert(ret.end(), ne.DNSSECRecords.records.begin(), ne.DNSSECRecords.records.end());
   ret.insert(ret.end(), ne.DNSSECRecords.signatures.begin(), ne.DNSSECRecords.signatures.end());
@@ -1282,8 +1305,8 @@ uint32_t SyncRes::computeLowestTTD(const std::vector<DNSRecord>& records, const
     lowestTTD = min(lowestTTD, static_cast<uint32_t>(signaturesTTL + d_now.tv_sec));
 
     for(const auto& sig : signatures) {
-      if (sig->d_siginception <= d_now.tv_sec && sig->d_sigexpire > d_now.tv_sec) {
-        // we don't decrement d_sigexpire by 'now' because we actually want a TTD, not a TTL */
+      if (isRRSIGNotExpired(d_now.tv_sec, sig)) {
+        // we don't decerement d_sigexpire by 'now' because we actually want a TTD, not a TTL */
         lowestTTD = min(lowestTTD, static_cast<uint32_t>(sig->d_sigexpire));
       }
     }
@@ -1948,11 +1971,13 @@ bool SyncRes::processRecords(const std::string& prefix, const DNSName& qname, co
 
       NegCache::NegCacheEntry ne;
 
-      ne.d_ttd = d_now.tv_sec + rec.d_ttl;
+      uint32_t lowestTTL = rec.d_ttl;
       ne.d_name = qname;
       ne.d_qtype = QType(0); // this encodes 'whole record'
       ne.d_auth = rec.d_name;
-      harvestNXRecords(lwr.d_records, ne);
+      harvestNXRecords(lwr.d_records, ne, d_now.tv_sec, &lowestTTL);
+      ne.d_ttd = d_now.tv_sec + lowestTTL;
+
       getDenialValidationState(ne, state, NXDOMAIN, false, false);
 
       if(!wasVariable()) {
@@ -2011,15 +2036,17 @@ bool SyncRes::processRecords(const std::string& prefix, const DNSName& qname, co
       if (state == Secure) {
         NegCache::NegCacheEntry ne;
         ne.d_auth = auth;
-        ne.d_ttd = d_now.tv_sec + rec.d_ttl;
         ne.d_name = newauth;
         ne.d_qtype = QType::DS;
-        harvestNXRecords(lwr.d_records, ne);
+        rec.d_ttl = min(s_maxnegttl, rec.d_ttl);
+        uint32_t lowestTTL = rec.d_ttl;
+        harvestNXRecords(lwr.d_records, ne, d_now.tv_sec, &lowestTTL);
+
         cspmap_t csp = harvestCSPFromNE(ne);
         dState denialState = getDenial(csp, newauth, QType::DS, true);
         if (denialState == NXQTYPE || denialState == OPTOUT || denialState == INSECURE) {
+          ne.d_ttd = lowestTTL + d_now.tv_sec;
           ne.d_validationState = Secure;
-          rec.d_ttl = min(s_maxnegttl, rec.d_ttl);
           LOG(prefix<<qname<<": got negative indication of DS record for '"<<newauth<<"'"<<endl);
 
           auto cut = d_cutStates.find(newauth);
@@ -2058,10 +2085,12 @@ bool SyncRes::processRecords(const std::string& prefix, const DNSName& qname, co
 
         NegCache::NegCacheEntry ne;
         ne.d_auth = rec.d_name;
-        ne.d_ttd = d_now.tv_sec + rec.d_ttl;
+        uint32_t lowestTTL = rec.d_ttl;
         ne.d_name = qname;
         ne.d_qtype = qtype;
-        harvestNXRecords(lwr.d_records, ne);
+        harvestNXRecords(lwr.d_records, ne, d_now.tv_sec, &lowestTTL);
+        ne.d_ttd = d_now.tv_sec + lowestTTL;
+
         getDenialValidationState(ne, state, NXQTYPE, qtype == QType::DS, false);
 
         if(!wasVariable()) {

diff --git a/pdns/validate.cc b/pdns/validate.cc
index 46a148dded219d7ec682711953cedb907ca51351..a84600ed33707831cdb9fe343b93bc8acc2fbd18 100644 (file)
--- a/pdns/validate.cc
+++ b/pdns/validate.cc
@@ -349,6 +349,11 @@ static const vector<DNSName> getZoneCuts(const DNSName& begin, const DNSName& en
   return ret;
 }
 
+bool isRRSIGNotExpired(const time_t now, const shared_ptr<RRSIGRecordContent> sig)
+{
+  return sig->d_siginception <= now && sig->d_sigexpire >= now;
+}
+
 static bool checkSignatureWithKey(time_t now, const shared_ptr<RRSIGRecordContent> sig, const shared_ptr<DNSKEYRecordContent> key, const std::string& msg)
 {
   bool result = false;
@@ -357,7 +362,7 @@ static bool checkSignatureWithKey(time_t now, const shared_ptr<RRSIGRecordConten
        - The validator's notion of the current time MUST be less than or equal to the time listed in the RRSIG RR's Expiration field.
        - The validator's notion of the current time MUST be greater than or equal to the time listed in the RRSIG RR's Inception field.
     */
-    if(sig->d_siginception <= now && sig->d_sigexpire >= now) {
+    if(isRRSIGNotExpired(now, sig)) {
       std::shared_ptr<DNSCryptoKeyEngine> dke = shared_ptr<DNSCryptoKeyEngine>(DNSCryptoKeyEngine::makeFromPublicKeyString(key->d_algorithm, key->d_key));
       result = dke->verify(msg, sig->d_signature);
       LOG("signature by key with tag "<<sig->d_tag<<" and algorithm "<<DNSSECKeeper::algorithm2name(sig->d_algorithm)<<" was " << (result ? "" : "NOT ")<<"valid"<<endl);

diff --git a/pdns/validate.hh b/pdns/validate.hh
index e09bcd56175b7e1c3f75edf189d694d794d6d86f..85399b52bf320b9326b7b17f2035ca640ea36e5e 100644 (file)
--- a/pdns/validate.hh
+++ b/pdns/validate.hh
@@ -75,3 +75,4 @@ dState getDenial(const cspmap_t &validrrsets, const DNSName& qname, const uint16
 bool isSupportedDS(const DSRecordContent& ds);
 DNSName getSigner(const std::vector<std::shared_ptr<RRSIGRecordContent> >& signatures);
 bool denialProvesNoDelegation(const DNSName& zone, const std::vector<DNSRecord>& dsrecords);
+bool isRRSIGNotExpired(const time_t now, const shared_ptr<RRSIGRecordContent> sig);