dnsdist: Subnets excluded from dynamic rules should not count towards thresholds

Until now we only looked at whether a subnet was excluded from dynamic rules
when deciding to insert a block. This introduced an issue when the dynamic
rules were configured to group clients into subnets via the `setMasks` directive,
because then queries received from an excluded client were still counted towards
the thresholds for the final subnet. For example, when grouping IPv4 clients
into `/24` subnets and excluding `192.0.2.1`, we would end up blocking the
whole `192.0.2.0/24` subnet if the number of queries or responses received
from `192.0.2.1` were over the threshold.
From now on excluded subnets will no longer count toward the thresholds.

Signed-off-by: Remi Gacogne <remi.gacogne@powerdns.com>

diff --git a/pdns/dnsdistdist/dnsdist-dynblocks.cc b/pdns/dnsdistdist/dnsdist-dynblocks.cc
index 9657df36ee3137b7242c10df6cb19c1f7b99b8ad..daac40778e524c298e6a4781024b0607a00aa98d 100644 (file)
--- a/pdns/dnsdistdist/dnsdist-dynblocks.cc
+++ b/pdns/dnsdistdist/dnsdist-dynblocks.cc
@@ -401,6 +401,10 @@ void DynBlockRulesGroup::processQueryRules(counts_t& counts, const struct timesp
       bool typeRuleMatches = checkIfQueryTypeMatches(ringEntry);
 
       if (qRateMatches || typeRuleMatches) {
+        if (d_excludedSubnets.match(ringEntry.requestor)) {
+          continue;
+        }
+
         auto& entry = counts[AddressAndPortRange(ringEntry.requestor, ringEntry.requestor.isIPv4() ? d_v4Mask : d_v6Mask, d_portMask)];
         if (qRateMatches) {
           ++entry.queries;
@@ -466,11 +470,20 @@ void DynBlockRulesGroup::processResponseRules(counts_t& counts, StatNode& root,
         continue;
       }
 
+      bool suffixMatchRuleMatches = d_suffixMatchRule.matches(ringEntry.when);
+      if (suffixMatchRuleMatches) {
+        const bool hit = ringEntry.isACacheHit();
+        root.submit(ringEntry.name, ((ringEntry.dh.rcode == 0 && ringEntry.usec == std::numeric_limits<uint32_t>::max()) ? -1 : ringEntry.dh.rcode), ringEntry.size, hit, std::nullopt, g_rings.getSamplingRate());
+      }
+
+      if (d_excludedSubnets.match(ringEntry.requestor)) {
+        continue;
+      }
+
       auto& entry = counts[AddressAndPortRange(ringEntry.requestor, ringEntry.requestor.isIPv4() ? d_v4Mask : d_v6Mask, d_portMask)];
       ++entry.responses;
 
       bool respRateMatches = d_respRateRule.matches(ringEntry.when);
-      bool suffixMatchRuleMatches = d_suffixMatchRule.matches(ringEntry.when);
       bool rcodeRuleMatches = checkIfResponseCodeMatches(ringEntry);
       bool respCacheMissRatioRuleMatches = d_respCacheMissRatioRule.matches(ringEntry.when);
 
@@ -483,11 +496,6 @@ void DynBlockRulesGroup::processResponseRules(counts_t& counts, StatNode& root,
       if (respCacheMissRatioRuleMatches && !ringEntry.isACacheHit()) {
         ++entry.cacheMisses;
       }
-
-      if (suffixMatchRuleMatches) {
-        const bool hit = ringEntry.isACacheHit();
-        root.submit(ringEntry.name, ((ringEntry.dh.rcode == 0 && ringEntry.usec == std::numeric_limits<uint32_t>::max()) ? -1 : ringEntry.dh.rcode), ringEntry.size, hit, std::nullopt, g_rings.getSamplingRate());
-      }
     }
   }
 }

diff --git a/pdns/dnsdistdist/dnsdist-settings-definitions.yml b/pdns/dnsdistdist/dnsdist-settings-definitions.yml
index 4ff176673197234fdd1c4eb6aa642b6d1fc9b1c8..1f0110092c0bd29184108b453f0a8106f6eaabb7 100644 (file)
--- a/pdns/dnsdistdist/dnsdist-settings-definitions.yml
+++ b/pdns/dnsdistdist/dnsdist-settings-definitions.yml
@@ -692,10 +692,16 @@ dynamic_rules:
       type: "u8"
       default: "32"
       description: "Number of bits to keep for IPv4 addresses"
+      changes:
+        - version: "2.1.0"
+          content: "Queries and corresponding responses coming from an excluded (``excluded_ranges``) client no longer count towards the thresholds for the aggregated subnet the client belongs to"
     - name: "mask_ipv6"
       type: "u8"
       default: "64"
       description: "Number of bits to keep for IPv6 addresses. In some scenarios it might make sense to block a whole /64 IPv6 range instead of a single address, for example"
+      changes:
+        - version: "2.1.0"
+          content: "Queries and corresponding responses coming from an excluded (``excluded_ranges``) client no longer count towards the thresholds for the aggregated subnet the client belongs to"
     - name: "mask_port"
       type: u8
       default: "0"

diff --git a/pdns/dnsdistdist/docs/reference/config.rst b/pdns/dnsdistdist/docs/reference/config.rst
index 7f44a8f71eef5ba64f0898a24d7757d4b1edc315..2ba425bc33b2758c9c69b7ab1f101d7b36cd88cb 100644 (file)
--- a/pdns/dnsdistdist/docs/reference/config.rst
+++ b/pdns/dnsdistdist/docs/reference/config.rst
@@ -1883,6 +1883,9 @@ faster than the existing rules.
 
     .. versionadded:: 1.7.0
 
+    .. versionchanged:: 2.1.0
+      Queries and corresponding responses coming from an excluded (see :meth:`DynBlockRulesGroup::excludeRange`) client no longer count towards the thresholds for the aggregated subnet the client belongs to.
+
     Set the number of bits to keep in the IP address when inserting a block. The default is 32 for IPv4 and 128 for IPv6, meaning
     that only the exact address is blocked, but in some scenarios it might make sense to block a whole /64 IPv6 range instead of a
     single address, for example.

diff --git a/pdns/dnsdistdist/test-dnsdistdynblocks_hh.cc b/pdns/dnsdistdist/test-dnsdistdynblocks_hh.cc
index 12bb2c7fd0cbbeada1283e8f09087a6fd29ef8a2..ae5062bab224ce17995ee38d26e281db8b5eb823 100644 (file)
--- a/pdns/dnsdistdist/test-dnsdistdynblocks_hh.cc
+++ b/pdns/dnsdistdist/test-dnsdistdynblocks_hh.cc
@@ -196,6 +196,86 @@ BOOST_FIXTURE_TEST_CASE(test_DynBlockRulesGroup_QueryRate, TestFixture)
   }
 }
 
+/* check that even if we group requestors to a /24, excluded IPs will not count toward
+   the group limit */
+BOOST_FIXTURE_TEST_CASE(test_DynBlockRulesGroup_QueryRate_ExcludedSubnets, TestFixture)
+{
+  dnsheader dnsHeader{};
+  memset(&dnsHeader, 0, sizeof(dnsHeader));
+  DNSName qname("rings.powerdns.com.");
+  ComboAddress requestor1("192.0.2.1");
+  ComboAddress requestor2("192.0.2.2");
+  ComboAddress backend("192.0.2.42");
+  uint16_t qtype = QType::AAAA;
+  uint16_t size = 42;
+  dnsdist::Protocol protocol = dnsdist::Protocol::DoUDP;
+  dnsdist::Protocol outgoingProtocol = dnsdist::Protocol::DoUDP;
+  unsigned int responseTime = 0;
+  struct timespec now;
+  gettime(&now);
+  NetmaskTree<DynBlock, AddressAndPortRange> emptyNMG;
+
+  size_t numberOfSeconds = 10;
+  size_t blockDuration = 60;
+  const auto action = DNSAction::Action::Drop;
+  const std::string reason = "Exceeded query rate";
+
+  DynBlockRulesGroup dbrg;
+  dbrg.setQuiet(true);
+  dbrg.setMasks(24, 128, 0);
+  dbrg.excludeRange(Netmask(requestor2, 32));
+
+  {
+    /* block above 50 qps for numberOfSeconds seconds, no warning */
+    DynBlockRulesGroup::DynBlockRule rule(reason, blockDuration, 50, 0, numberOfSeconds, action);
+    dbrg.setQueryRate(std::move(rule));
+  }
+
+  {
+    /* insert just above 50 qps from a given client in the last 10s
+       this should trigger the rule this time */
+    size_t numberOfQueries = (50 * numberOfSeconds) + 1;
+    g_rings.clear();
+    BOOST_CHECK_EQUAL(g_rings.getNumberOfQueryEntries(), 0U);
+    dnsdist::DynamicBlocks::clearClientAddressDynamicRules();
+
+    for (size_t idx = 0; idx < numberOfQueries; idx++) {
+      g_rings.insertQuery(now, requestor1, qname, qtype, size, dnsHeader, protocol);
+      g_rings.insertResponse(now, requestor1, qname, qtype, responseTime, size, dnsHeader, backend, outgoingProtocol);
+    }
+    BOOST_CHECK_EQUAL(g_rings.getNumberOfQueryEntries(), numberOfQueries);
+
+    dbrg.apply(now);
+    BOOST_CHECK_EQUAL(dnsdist::DynamicBlocks::getClientAddressDynamicRules().size(), 1U);
+    BOOST_CHECK(dnsdist::DynamicBlocks::getClientAddressDynamicRules().lookup(requestor1) != nullptr);
+    BOOST_CHECK(dnsdist::DynamicBlocks::getClientAddressDynamicRules().lookup(requestor2) != nullptr);
+    const auto& block = dnsdist::DynamicBlocks::getClientAddressDynamicRules().lookup(requestor1)->second;
+    BOOST_CHECK_EQUAL(block.reason, reason);
+    BOOST_CHECK_EQUAL(static_cast<size_t>(block.until.tv_sec), now.tv_sec + blockDuration);
+    BOOST_CHECK(block.domain.empty());
+    BOOST_CHECK(block.action == action);
+    BOOST_CHECK_EQUAL(block.blocks, 0U);
+    BOOST_CHECK_EQUAL(block.warning, false);
+  }
+
+  {
+    /* now do the same but from an excluded requestor */
+    size_t numberOfQueries = (50 * numberOfSeconds) + 1;
+    g_rings.clear();
+    BOOST_CHECK_EQUAL(g_rings.getNumberOfQueryEntries(), 0U);
+    dnsdist::DynamicBlocks::clearClientAddressDynamicRules();
+
+    for (size_t idx = 0; idx < numberOfQueries; idx++) {
+      g_rings.insertQuery(now, requestor2, qname, qtype, size, dnsHeader, protocol);
+      g_rings.insertResponse(now, requestor2, qname, qtype, responseTime, size, dnsHeader, backend, outgoingProtocol);
+    }
+    BOOST_CHECK_EQUAL(g_rings.getNumberOfQueryEntries(), numberOfQueries);
+
+    dbrg.apply(now);
+    BOOST_CHECK_EQUAL(dnsdist::DynamicBlocks::getClientAddressDynamicRules().size(), 0U);
+  }
+}
+
 BOOST_FIXTURE_TEST_CASE(test_DynBlockRulesGroup_QueryRate_RangeV6, TestFixture)
 {
   /* Check that we correctly group IPv6 addresses from the same /64 subnet into the same

diff --git a/regression-tests.dnsdist/test_DynBlocksGroup.py b/regression-tests.dnsdist/test_DynBlocksGroup.py
index e0ec591147e1f759f313079a7fcede8ddf92f427..08c98388dc01670191556151506bb4ec65144f3b 100644 (file)
--- a/regression-tests.dnsdist/test_DynBlocksGroup.py
+++ b/regression-tests.dnsdist/test_DynBlocksGroup.py
@@ -227,6 +227,61 @@ class TestDynBlockGroupExcludedViaNMG(DynBlocksTest):
         self.assertEqual(query, receivedQuery)
         self.assertEqual(receivedResponse, receivedResponse)
 
+class TestDynBlockGroupExcludedRange(DynBlocksTest):
+
+    _config_template = """
+    local dbr = dynBlockRulesGroup()
+    dbr:setQueryRate(%d, %d, "Exceeded query rate", %d)
+    dbr:setMasks(8, 128, 0)
+    dbr:excludeRange("127.0.0.1/32")
+
+    function maintenance()
+           dbr:apply()
+    end
+
+    newServer{address="127.0.0.1:%d"}
+    """
+
+    def testExcluded(self):
+        """
+        Dyn Blocks (group) : Excluded from the dynamic block rules (range)
+        """
+        name = 'excluded.group.dynblocks.tests.powerdns.com.'
+        query = dns.message.make_query(name, 'A', 'IN')
+        response = dns.message.make_response(query)
+        rrset = dns.rrset.from_text(name,
+                                    60,
+                                    dns.rdataclass.IN,
+                                    dns.rdatatype.A,
+                                    '192.0.2.1')
+        response.answer.append(rrset)
+
+        allowed = 0
+        sent = 0
+        for _ in range((self._dynBlockQPS * self._dynBlockPeriod) + 1):
+            (receivedQuery, receivedResponse) = self.sendUDPQuery(query, response)
+            sent = sent + 1
+            if receivedQuery:
+                receivedQuery.id = query.id
+                self.assertEqual(query, receivedQuery)
+                self.assertEqual(response, receivedResponse)
+                allowed = allowed + 1
+            else:
+                # the query has not reached the responder,
+                # let's clear the response queue
+                self.clearToResponderQueue()
+
+        # we should not have been blocked
+        self.assertEqual(allowed, sent)
+
+        waitForMaintenanceToRun()
+
+        # we should still not be blocked
+        (receivedQuery, receivedResponse) = self.sendUDPQuery(query, response)
+        receivedQuery.id = query.id
+        self.assertEqual(query, receivedQuery)
+        self.assertEqual(receivedResponse, receivedResponse)
+
 class TestDynBlockGroupNoOp(DynBlocksTest):
 
     _config_template = """