From: Remi Gacogne Date: Mon, 11 May 2026 14:41:03 +0000 (+0200) Subject: dnsdist: Fix TCP rate-limiting ban expiry (introduced in f960b7d8d98911c717ee7dfeb4dc... X-Git-Tag: auth-5.1.0~72^2~4 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=1c936163797206ef81f441575099489c5fbf8e31;p=thirdparty%2Fpdns.git dnsdist: Fix TCP rate-limiting ban expiry (introduced in f960b7d8d98911c717ee7dfeb4dc6475ce98d135) Signed-off-by: Remi Gacogne --- diff --git a/pdns/dnsdistdist/dnsdist-concurrent-connections.cc b/pdns/dnsdistdist/dnsdist-concurrent-connections.cc index 43a4d29c85..6ff4a0f980 100644 --- a/pdns/dnsdistdist/dnsdist-concurrent-connections.cc +++ b/pdns/dnsdistdist/dnsdist-concurrent-connections.cc @@ -220,7 +220,7 @@ IncomingConcurrentTCPConnectionsManager::NewConnectionResult IncomingConcurrentT }; auto checkConnectionAllowed = [now, from, maxConnsPerClient, threshold, tcpRate, tlsNewRate, tlsResumedRate, interval, isTLS, &immutable, &getProtocol](const ClientEntry& entry) { - if (entry.d_bannedUntil != 0 && entry.d_bannedUntil >= now) { + if (entry.d_bannedUntil != 0 && entry.d_bannedUntil >= *now) { VERBOSESLOG(infolog("Refusing %s connection from %s: banned", getProtocol(), from.toStringWithPort()), dnsdist::logging::getTopLogger("concurrent-connections-manager")->info(Logr::Info, "Refusing connection", "reason", Logging::Loggable("banned"), "protocol", Logging::Loggable(getProtocol()), "client.address", Logging::Loggable(from))); return NewConnectionResult::Denied; @@ -336,7 +336,9 @@ void IncomingConcurrentTCPConnectionsManager::accountClosedTCPConnection(const C } editEntryIfPresent(from, [](const ClientEntry& entry) { auto& count = entry.d_concurrentConnections; - count--; + if (count > 0) { + count--; + } }); } diff --git a/pdns/dnsdistdist/test-dnsdist-concurrent-connections.cc b/pdns/dnsdistdist/test-dnsdist-concurrent-connections.cc index ce1694d1e0..ad7bd84733 100644 --- a/pdns/dnsdistdist/test-dnsdist-concurrent-connections.cc +++ b/pdns/dnsdistdist/test-dnsdist-concurrent-connections.cc @@ -32,18 +32,35 @@ BOOST_AUTO_TEST_SUITE(test_dnsdist_concurrent_connections) -BOOST_AUTO_TEST_CASE(test_Below_Rate) +static void initConfiguration(uint64_t maxTCPConnectionsRatePerClient, uint64_t tcpConnectionsRatePerClientInterval, uint64_t maxTCPConnectionsPerClient, uint32_t banDuration) { - const uint64_t maxTCPConnectionsRatePerClient = 10U; - const uint64_t tcpConnectionsRatePerClientInterval = 5U; - const uint64_t maxTCPConnectionsPerClient = 1U; dnsdist::configuration::updateImmutableConfiguration([&](dnsdist::configuration::ImmutableConfiguration& config) { config.d_maxTCPConnectionsPerClient = maxTCPConnectionsPerClient; config.d_maxTCPConnectionsRatePerClient = maxTCPConnectionsRatePerClient; config.d_tcpConnectionsRatePerClientInterval = tcpConnectionsRatePerClientInterval; + config.d_tcpBanDurationForExceedingTCPTLSRate = banDuration; }); +} - dnsdist::IncomingConcurrentTCPConnectionsManager::clear(); +struct TestFixture +{ + TestFixture() + { + dnsdist::IncomingConcurrentTCPConnectionsManager::clear(); + } + ~TestFixture() + { + dnsdist::IncomingConcurrentTCPConnectionsManager::clear(); + } +}; + +BOOST_FIXTURE_TEST_CASE(test_Below_Rate, TestFixture) +{ + const uint64_t maxTCPConnectionsRatePerClient = 10U; + const uint64_t tcpConnectionsRatePerClientInterval = 5U; + const uint64_t maxTCPConnectionsPerClient = 1U; + const uint32_t banDuration = 10U; + initConfiguration(maxTCPConnectionsRatePerClient, tcpConnectionsRatePerClientInterval, maxTCPConnectionsPerClient, banDuration); time_t now = time(nullptr); /* simulate a client sending up to maxTCPConnectionsRatePerClient every second, for 120 seconds (so 2 buckets) */ @@ -61,16 +78,13 @@ BOOST_AUTO_TEST_CASE(test_Below_Rate) } } -BOOST_AUTO_TEST_CASE(test_Below_Rate_Skipping_Bucket) +BOOST_FIXTURE_TEST_CASE(test_Below_Rate_Skipping_Bucket, TestFixture) { const uint64_t maxTCPConnectionsRatePerClient = 10U; const uint64_t tcpConnectionsRatePerClientInterval = 5U; const uint64_t maxTCPConnectionsPerClient = 1U; - dnsdist::configuration::updateImmutableConfiguration([&](dnsdist::configuration::ImmutableConfiguration& config) { - config.d_maxTCPConnectionsPerClient = maxTCPConnectionsPerClient; - config.d_maxTCPConnectionsRatePerClient = maxTCPConnectionsRatePerClient; - config.d_tcpConnectionsRatePerClientInterval = tcpConnectionsRatePerClientInterval; - }); + const uint32_t banDuration = 10U; + initConfiguration(maxTCPConnectionsRatePerClient, tcpConnectionsRatePerClientInterval, maxTCPConnectionsPerClient, banDuration); dnsdist::IncomingConcurrentTCPConnectionsManager::clear(); time_t now = time(nullptr); @@ -107,16 +121,13 @@ BOOST_AUTO_TEST_CASE(test_Below_Rate_Skipping_Bucket) } } -BOOST_AUTO_TEST_CASE(test_Above_Rate_After_Being_Below) +BOOST_FIXTURE_TEST_CASE(test_Above_Rate_After_Being_Below, TestFixture) { const uint64_t maxTCPConnectionsRatePerClient = 10U; const uint64_t tcpConnectionsRatePerClientInterval = 5U; const uint64_t maxTCPConnectionsPerClient = 1U; - dnsdist::configuration::updateImmutableConfiguration([&](dnsdist::configuration::ImmutableConfiguration& config) { - config.d_maxTCPConnectionsPerClient = maxTCPConnectionsPerClient; - config.d_maxTCPConnectionsRatePerClient = maxTCPConnectionsRatePerClient; - config.d_tcpConnectionsRatePerClientInterval = tcpConnectionsRatePerClientInterval; - }); + const uint32_t banDuration = 10U; + initConfiguration(maxTCPConnectionsRatePerClient, tcpConnectionsRatePerClientInterval, maxTCPConnectionsPerClient, banDuration); dnsdist::IncomingConcurrentTCPConnectionsManager::clear(); time_t now = time(nullptr); @@ -162,19 +173,19 @@ BOOST_AUTO_TEST_CASE(test_Above_Rate_After_Being_Below) /* the last one */ auto result = dnsdist::IncomingConcurrentTCPConnectionsManager::accountNewTCPConnection(client, false, false, now); BOOST_REQUIRE(result == dnsdist::IncomingConcurrentTCPConnectionsManager::NewConnectionResult::Denied); - dnsdist::IncomingConcurrentTCPConnectionsManager::accountClosedTCPConnection(client); + + /* check that the ban properly expires (takes a while to go below the rate, though, because we opened a lot of connections during the last minute ) */ + result = dnsdist::IncomingConcurrentTCPConnectionsManager::accountNewTCPConnection(client, false, false, now + std::max(tcpConnectionsRatePerClientInterval * 60U, static_cast(banDuration)) + 1U); + BOOST_REQUIRE(result == dnsdist::IncomingConcurrentTCPConnectionsManager::NewConnectionResult::Allowed); } -BOOST_AUTO_TEST_CASE(test_Above_Max_Concurrent_Connections) +BOOST_FIXTURE_TEST_CASE(test_Above_Max_Concurrent_Connections, TestFixture) { const uint64_t maxTCPConnectionsRatePerClient = 10U; const uint64_t tcpConnectionsRatePerClientInterval = 5U; const uint64_t maxTCPConnectionsPerClient = 1U; - dnsdist::configuration::updateImmutableConfiguration([&](dnsdist::configuration::ImmutableConfiguration& config) { - config.d_maxTCPConnectionsPerClient = maxTCPConnectionsPerClient; - config.d_maxTCPConnectionsRatePerClient = maxTCPConnectionsRatePerClient; - config.d_tcpConnectionsRatePerClientInterval = tcpConnectionsRatePerClientInterval; - }); + const uint32_t banDuration = 10U; + initConfiguration(maxTCPConnectionsRatePerClient, tcpConnectionsRatePerClientInterval, maxTCPConnectionsPerClient, banDuration); dnsdist::IncomingConcurrentTCPConnectionsManager::clear(); const time_t now = time(nullptr); @@ -190,18 +201,22 @@ BOOST_AUTO_TEST_CASE(test_Above_Max_Concurrent_Connections) auto result = dnsdist::IncomingConcurrentTCPConnectionsManager::accountNewTCPConnection(client, false, false); BOOST_REQUIRE(result == dnsdist::IncomingConcurrentTCPConnectionsManager::NewConnectionResult::Denied); BOOST_REQUIRE(dnsdist::IncomingConcurrentTCPConnectionsManager::isClientOverThreshold(client)); + + /* now check that we are correctly allowed once at least one existing connections has been closed */ + dnsdist::IncomingConcurrentTCPConnectionsManager::accountClosedTCPConnection(client); + result = dnsdist::IncomingConcurrentTCPConnectionsManager::accountNewTCPConnection(client, false, false); + BOOST_REQUIRE(result == dnsdist::IncomingConcurrentTCPConnectionsManager::NewConnectionResult::Allowed); + dnsdist::IncomingConcurrentTCPConnectionsManager::accountClosedTCPConnection(client); + BOOST_REQUIRE(!dnsdist::IncomingConcurrentTCPConnectionsManager::isClientOverThreshold(client)); } -BOOST_AUTO_TEST_CASE(test_Above_Max_Connection_Rate) +BOOST_FIXTURE_TEST_CASE(test_Above_Max_Connection_Rate, TestFixture) { const uint64_t maxTCPConnectionsRatePerClient = 10U; const uint64_t tcpConnectionsRatePerClientInterval = 5U; const uint64_t maxTCPConnectionsPerClient = 1U; - dnsdist::configuration::updateImmutableConfiguration([&](dnsdist::configuration::ImmutableConfiguration& config) { - config.d_maxTCPConnectionsPerClient = maxTCPConnectionsPerClient; - config.d_maxTCPConnectionsRatePerClient = maxTCPConnectionsRatePerClient; - config.d_tcpConnectionsRatePerClientInterval = tcpConnectionsRatePerClientInterval; - }); + const uint32_t banDuration = 10U; + initConfiguration(maxTCPConnectionsRatePerClient, tcpConnectionsRatePerClientInterval, maxTCPConnectionsPerClient, banDuration); dnsdist::IncomingConcurrentTCPConnectionsManager::clear(); const time_t now = time(nullptr); @@ -217,6 +232,10 @@ BOOST_AUTO_TEST_CASE(test_Above_Max_Connection_Rate) /* now go over the top */ auto result = dnsdist::IncomingConcurrentTCPConnectionsManager::accountNewTCPConnection(client, false, false, now); BOOST_REQUIRE(result == dnsdist::IncomingConcurrentTCPConnectionsManager::NewConnectionResult::Denied); + + /* check that the ban properly expires (takes a while to go below the rate) */ + result = dnsdist::IncomingConcurrentTCPConnectionsManager::accountNewTCPConnection(client, false, false, now + std::max(60U, banDuration) + 1U); + BOOST_REQUIRE(result == dnsdist::IncomingConcurrentTCPConnectionsManager::NewConnectionResult::Allowed); } BOOST_AUTO_TEST_SUITE_END();