From: Remi Gacogne Date: Thu, 5 Oct 2017 15:20:15 +0000 (+0200) Subject: rec: Don't negcache entries for longer than their RRSIG validity X-Git-Tag: rec-4.1.0-rc1~5^2 X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=dbbef467a2a5533d78fca25564cce532cff3d85e;p=pdns 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 9961fb138..5c82bb41d 100644 --- 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& 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(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 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& srcmask, boost::optional context, std::shared_ptr 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 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 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& srcmask, boost::optional context, std::shared_ptr 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 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 cached; + vector> 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 "<& rrsig) +{ + uint32_t res = 0; + if (now < rrsig->d_sigexpire) { + res = static_cast(rrsig->d_sigexpire) - now; + } + return res; +} + static const set 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 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& records, NegCache::NegCacheEntry& ne) { +static void harvestNXRecords(const vector& 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& 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&ret, const vector& 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& records, const lowestTTD = min(lowestTTD, static_cast(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(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< getZoneCuts(const DNSName& begin, const DNSName& en return ret; } +bool isRRSIGNotExpired(const time_t now, const shared_ptr sig) +{ + return sig->d_siginception <= now && sig->d_sigexpire >= now; +} + static bool checkSignatureWithKey(time_t now, const shared_ptr sig, const shared_ptr key, const std::string& msg) { bool result = false; @@ -357,7 +362,7 @@ static bool checkSignatureWithKey(time_t now, const shared_ptrd_siginception <= now && sig->d_sigexpire >= now) { + if(isRRSIGNotExpired(now, sig)) { std::shared_ptr dke = shared_ptr(DNSCryptoKeyEngine::makeFromPublicKeyString(key->d_algorithm, key->d_key)); result = dke->verify(msg, sig->d_signature); LOG("signature by key with tag "<d_tag<<" and algorithm "<d_algorithm)<<" was " << (result ? "" : "NOT ")<<"valid"< >& signatures); bool denialProvesNoDelegation(const DNSName& zone, const std::vector& dsrecords); +bool isRRSIGNotExpired(const time_t now, const shared_ptr sig);