From: Otto Moerbeek Date: Wed, 21 Dec 2022 13:43:39 +0000 (+0100) Subject: Only store NSEC3 records in aggressive cache if we expect them to be effective. X-Git-Tag: dnsdist-1.8.0-rc1~37^2~5 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=dcf64a6e0f82a25c3259383552409d5f9b8892c1;p=thirdparty%2Fpdns.git Only store NSEC3 records in aggressive cache if we expect them to be effective. The aggressive cache is not very effective for large NSEC3 domains: each NSEC3 record only covers a relatively small amount of random names. We only want to put NSEC3 records in the aggresive cache if there is a decent chance a hash will hit it. Hashes are random wrt the corresponding names. So look at the bitwise common prefix of the owner and next, and do not store the record if the common prefix is long. We also might want to introduce a switch to completely forget the aggressive cache for the NSEC3 case. --- diff --git a/pdns/recursordist/aggressive_nsec.cc b/pdns/recursordist/aggressive_nsec.cc index e5b454e2d2..61cd873bb9 100644 --- a/pdns/recursordist/aggressive_nsec.cc +++ b/pdns/recursordist/aggressive_nsec.cc @@ -20,6 +20,7 @@ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. */ #include +#include #include "aggressive_nsec.hh" #include "cachecleaner.hh" @@ -28,6 +29,7 @@ #include "validate.hh" std::unique_ptr g_aggressiveNSECCache{nullptr}; +uint8_t AggressiveNSECCache::s_maxNSEC3CommonPrefix = AggressiveNSECCache::s_default_maxNSEC3CommonPrefix; /* this is defined in syncres.hh and we are not importing that here */ extern std::unique_ptr g_recCache; @@ -226,21 +228,40 @@ static bool isMinimallyCoveringNSEC(const DNSName& owner, const std::shared_ptr< return true; } -// This function name is somewhat misleading. It only returns true if the nextHash is ownerHash+2, as is common -// in minimally covering NXDOMAINs (i.e. the NSEC3 covers hash[deniedname]-1 .. hash[deniedname]+2. -// Minimally covering NSEC3s for NODATA tend to be ownerHash+1, because they need to prove the name, so they -// can tell us what types are in the bitmap for that name. For those names, this function returns false. -// This is on purpose because NODATA denials actually do contain useful information we can reuse later - -// specifically, the type bitmap for a name that does exist. -static bool isMinimallyCoveringNSEC3(const DNSName& owner, const std::shared_ptr& nsec) +static size_t computeCommonPrefix(const string& one, const string& two) { - std::string ownerHash(owner.getStorage().c_str(), owner.getStorage().size()); - const std::string& nextHash = nsec->d_nexthash; - - incrementHash(ownerHash); - incrementHash(ownerHash); + size_t length = 0; + const auto bound = std::min(one.length(), two.length()); + + for (size_t i = 0; i < bound; i++) { + const auto byte1 = one.at(i); + const auto byte2 = two.at(i); + // shortcut + if (byte1 == byte2) { + length += CHAR_BIT; + continue; + } + // bytes differ, lets look at the bits + for (ssize_t j = CHAR_BIT - 1; j >= 0; j--) { + const auto bit1 = byte1 & (1 << j); + const auto bit2 = byte2 & (1 << j); + if (bit1 != bit2) { + return length; + } + length++; + } + } + return length; +} - return ownerHash == nextHash; +// If the NSEC3 hashes have a long common prefix, they deny only a small subset of all possible hashes +// So don't take the trouble to store those. +static bool isSmallCoveringNSEC3(const DNSName& owner, const std::shared_ptr& nsec) +{ + std::string ownerHash(fromBase32Hex(owner.getRawLabel(0))); + const std::string& nextHash = nsec->d_nexthash; + auto commonPrefix = computeCommonPrefix(ownerHash, nextHash); + return commonPrefix > AggressiveNSECCache::s_maxNSEC3CommonPrefix; } void AggressiveNSECCache::insertNSEC(const DNSName& zone, const DNSName& owner, const DNSRecord& record, const std::vector>& signatures, bool nsec3) @@ -293,8 +314,8 @@ void AggressiveNSECCache::insertNSEC(const DNSName& zone, const DNSName& owner, return; } - if (isMinimallyCoveringNSEC3(owner, content)) { - /* not accepting minimally covering answers since they only deny one name */ + if (isSmallCoveringNSEC3(owner, content)) { + /* not accepting small covering answers since they only deny a small subset */ return; } diff --git a/pdns/recursordist/aggressive_nsec.hh b/pdns/recursordist/aggressive_nsec.hh index e8fe05a774..64b23ea3a4 100644 --- a/pdns/recursordist/aggressive_nsec.hh +++ b/pdns/recursordist/aggressive_nsec.hh @@ -40,6 +40,9 @@ using namespace ::boost::multi_index; class AggressiveNSECCache { public: + static const uint8_t s_default_maxNSEC3CommonPrefix = 10; + static uint8_t s_maxNSEC3CommonPrefix; + AggressiveNSECCache(uint64_t entries) : d_maxEntries(entries) { diff --git a/pdns/recursordist/test-aggressive_nsec_cc.cc b/pdns/recursordist/test-aggressive_nsec_cc.cc index 51693050c6..7e0058efca 100644 --- a/pdns/recursordist/test-aggressive_nsec_cc.cc +++ b/pdns/recursordist/test-aggressive_nsec_cc.cc @@ -514,6 +514,7 @@ BOOST_AUTO_TEST_CASE(test_aggressive_nsec3_nxdomain) { std::unique_ptr sr; initSR(sr, true); + AggressiveNSECCache::s_maxNSEC3CommonPrefix = 159; g_aggressiveNSECCache = make_unique(10000); setDNSSECValidation(sr, DNSSECMode::ValidateAll); @@ -706,6 +707,7 @@ BOOST_AUTO_TEST_CASE(test_aggressive_nsec3_nodata_wildcard) { std::unique_ptr sr; initSR(sr, true); + AggressiveNSECCache::s_maxNSEC3CommonPrefix = 159; g_aggressiveNSECCache = make_unique(10000); setDNSSECValidation(sr, DNSSECMode::ValidateAll); @@ -1222,6 +1224,7 @@ BOOST_AUTO_TEST_CASE(test_aggressive_nsec3_rollover) { /* test that we don't compare a hash using the wrong (former) salt or iterations count in case of a rollover, or when different servers use different parameters */ + AggressiveNSECCache::s_maxNSEC3CommonPrefix = 159; auto cache = make_unique(10000); g_recCache = std::make_unique(); @@ -1524,6 +1527,7 @@ BOOST_AUTO_TEST_CASE(test_aggressive_nsec_ancestor_cases) BOOST_AUTO_TEST_CASE(test_aggressive_nsec3_ancestor_cases) { + AggressiveNSECCache::s_maxNSEC3CommonPrefix = 159; auto cache = make_unique(10000); g_recCache = std::make_unique(); diff --git a/pdns/recursordist/test-syncres_cc.cc b/pdns/recursordist/test-syncres_cc.cc index f866d75880..a9058e15ae 100644 --- a/pdns/recursordist/test-syncres_cc.cc +++ b/pdns/recursordist/test-syncres_cc.cc @@ -211,6 +211,8 @@ void initSR(bool debug) g_maxNSEC3Iterations = 2500; g_aggressiveNSECCache.reset(); + AggressiveNSECCache::s_maxNSEC3CommonPrefix = AggressiveNSECCache::s_default_maxNSEC3CommonPrefix; + taskQueueClear(); ::arg().set("version-string", "string reported on version.pdns or version.bind") = "PowerDNS Unit Tests";