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 e5b454e2d25a6466b407ca19eb60fec78a62cf0a..61cd873bb9de2f0625eeda8a90d980234c6c6803 100644 (file)
--- 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 <cinttypes>
+#include <climits>
 
 #include "aggressive_nsec.hh"
 #include "cachecleaner.hh"
@@ -28,6 +29,7 @@
 #include "validate.hh"
 
 std::unique_ptr<AggressiveNSECCache> 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<MemRecursorCache> 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<NSEC3RecordContent>& 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<NSEC3RecordContent>& 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<std::shared_ptr<RRSIGRecordContent>>& 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 e8fe05a774c603ec17c6427d54e2d76f34a50f8c..64b23ea3a4e961f4928ac074a874683c9315c228 100644 (file)
--- 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 51693050c66c48c279e7b7a55fb8837f10312f72..7e0058efca6c215db30dee1c51b7009f5f8a9a6e 100644 (file)
--- 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<SyncRes> sr;
   initSR(sr, true);
+  AggressiveNSECCache::s_maxNSEC3CommonPrefix = 159;
   g_aggressiveNSECCache = make_unique<AggressiveNSECCache>(10000);
 
   setDNSSECValidation(sr, DNSSECMode::ValidateAll);
@@ -706,6 +707,7 @@ BOOST_AUTO_TEST_CASE(test_aggressive_nsec3_nodata_wildcard)
 {
   std::unique_ptr<SyncRes> sr;
   initSR(sr, true);
+  AggressiveNSECCache::s_maxNSEC3CommonPrefix = 159;
   g_aggressiveNSECCache = make_unique<AggressiveNSECCache>(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<AggressiveNSECCache>(10000);
   g_recCache = std::make_unique<MemRecursorCache>();
 
@@ -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<AggressiveNSECCache>(10000);
   g_recCache = std::make_unique<MemRecursorCache>();
 

diff --git a/pdns/recursordist/test-syncres_cc.cc b/pdns/recursordist/test-syncres_cc.cc
index f866d75880deb2a82324cc885c97d8b0355eda51..a9058e15ae40424bd460bead274b180349765c73 100644 (file)
--- 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";