diff --git a/Projects/UOContent.Tests/Tests/Network/AutoDenylist/AutoDenylistTests.cs b/Projects/UOContent.Tests/Tests/Network/AutoDenylist/AutoDenylistTests.cs index 19dcd123c..ae926794d 100644 --- a/Projects/UOContent.Tests/Tests/Network/AutoDenylist/AutoDenylistTests.cs +++ b/Projects/UOContent.Tests/Tests/Network/AutoDenylist/AutoDenylistTests.cs @@ -103,6 +103,27 @@ public class AutoDenylistTests Assert.Equal(0, AutoDenylist.RingCount); } + // The ring grows in doublings but is capped at maxEntries, which is not a power of two. Filling exactly + // to it must land on the last slot rather than off the end. + [Fact] + public void Ring_fills_exactly_to_a_non_power_of_two_cap() + { + Reset(maxEntries: 100); + + for (var i = 0; i < 120; i++) + { + AutoDenylist.Hold(IPAddress.Parse($"198.51.100.{i}"), BanReasons.InvalidSeed, Now); + } + + Assert.Equal(100, AutoDenylist.Count); + Assert.Equal(100, AutoDenylist.RingCount); + + // And the whole ring still drains, so no slot was stranded by a wrapped write. + AutoDenylist.Drain(Now + DurationMs + 1); + Assert.Equal(0, AutoDenylist.Count); + Assert.Equal(0, AutoDenylist.RingCount); + } + // Releasing leaves no ring record behind, so a re-detection is not retired by the old one. [Fact] public void Release_then_re_hold_is_not_retired_by_the_stale_record() diff --git a/Projects/UOContent/Network/AutoDenylist/AutoDenylist.cs b/Projects/UOContent/Network/AutoDenylist/AutoDenylist.cs index 4a6f079ea..7b3c44ae4 100644 --- a/Projects/UOContent/Network/AutoDenylist/AutoDenylist.cs +++ b/Projects/UOContent/Network/AutoDenylist/AutoDenylist.cs @@ -96,11 +96,13 @@ public static class AutoDenylist Drain(nowTicks); + var key = address.ToUInt128(); + // Deliberately not refreshed: the first detection sets the expiry and later ones leave it alone. - // That is what keeps insertion order equal to expiry order, which is the whole reason the drain - // can stop at the first live record. A flooder whose hold lapses trips the rate limiter on its - // next attempt -- which runs before the connection filters -- and is held again immediately. - if (!_held.Add(address.ToUInt128())) + // That keeps insertion order equal to expiry order, which is why the drain can stop at the first + // live record. A flooder whose hold lapses trips the rate limiter on its next attempt -- which + // runs ahead of the connection filters -- and is held again. + if (!_held.Add(key)) { return true; } @@ -108,7 +110,7 @@ public static class AutoDenylist // Drain already reclaimed everything reclaimable, so being over now means genuinely full. if (_held.Count > _maxEntries) { - _held.Remove(address.ToUInt128()); + _held.Remove(key); if (!_warnedFull) { @@ -122,7 +124,7 @@ public static class AutoDenylist return false; } - Push(address.ToUInt128(), nowTicks + _durationMs); + Push(key, nowTicks + _durationMs); return true; } @@ -130,8 +132,8 @@ public static class AutoDenylist /// /// The accept-path decision, split out so the policy can be tested without a clock. Drains first: the - /// expiry is not stored beside the membership, so there is nothing to expire on read and a lapsed hold - /// must be retired here rather than left to deny. Costs one array read when nothing has lapsed. + /// expiry lives in the ring, not beside the membership, so a lapsed hold has to be retired here rather + /// than expired on read. One array read when nothing has lapsed. /// internal static bool IsDenied(IPAddress address, long nowTicks) { @@ -168,12 +170,18 @@ public static class AutoDenylist /// internal static void Drain(long nowTicks) { + var before = _ringCount; + // Subtraction, never a direct compare: tick counts wrap. See dev-docs/tick-counts.md. while (_ringCount > 0 && _ringExpiry[_ringHead] - nowTicks <= 0) { _held.Remove(_ringKeys[_ringHead]); _ringHead = _ringHead + 1 == _ringKeys.Length ? 0 : _ringHead + 1; _ringCount--; + } + + if (_ringCount != before) + { _warnedFull = false; } } @@ -198,7 +206,9 @@ public static class AutoDenylist private static void Grow() { - var size = Math.Max(64, _ringKeys.Length * 2); + // Capped at the entry cap: Push only runs below it, so the ring never needs more, and doubling + // past it would reserve roughly twice the slots it can ever use. + var size = Math.Min(Math.Max(64, _ringKeys.Length * 2), _maxEntries); var keys = new UInt128[size]; var expiry = new long[size]; diff --git a/dev-docs/ip-bans-and-allowlists.md b/dev-docs/ip-bans-and-allowlists.md index a57447cb0..c0db9dfa4 100644 --- a/dev-docs/ip-bans-and-allowlists.md +++ b/dev-docs/ip-bans-and-allowlists.md @@ -201,7 +201,7 @@ firewalled off. Shortening the 5s handshake window has been tried and broke real | `blocklist.json` | `enabled` (default `false`), `file`, `reloadInterval`, `reportHits`, `banDuration`, `promoteSuppression` | | `ip-allowlist.json` | `enabled` (default `false`), `files` (wildcards allowed), `reloadInterval` | | `login-allowlist.json` | `enabled`, `file`, `ttl`, `flushInterval`, `escalateAfterStrikes`, `strikeWindow` | -| `auto-denylist.json` | `enabled`, `duration`, `maxEntries` | +| `auto-denylist.json` | `enabled`, `duration`, `maxEntries` (default `324,449` — sized for the floods seen in practice; see the remark on the setting before raising it) | | `crowdsec.json` | `lapiUrl`, `machineId`, `password`, `origin`, `manualBanDuration`, `flushInterval`, `maxQueue` | | `firewall.json` | Admin-curated entries |