From 013257ec7cc3526278aac3a4866ab734da3a93ef Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Wed, 19 Aug 2026 14:54:25 -0400 Subject: [PATCH] fix(centralui): per-IP spray ceiling on LoginThrottle (arch-review 2026-08-19 P6-9) LoginThrottle keyed failures only as {username}|{ip}, so a password spray from one IP against many distinct usernames never accumulated MaxLoginFailuresPerWindow failures on any single pair and was never throttled, despite the class doc claiming spray protection. Add a secondary per-IP counter (SecurityOptions.PerIpMaxFailures, default 25, validated strictly greater than MaxLoginFailuresPerWindow) that shares the existing fixed window/lockout machinery but tracks failures by IP alone. IsLockedOut now refuses a bind when either the per-pair or the per-IP counter is locked. RecordSuccess clears only the per-pair counter (never the per-IP one), so a spray that stumbles onto one valid account cannot use that success to reset its IP budget and keep spraying; window expiry is the only relief for the per-IP counter. Per-pair behavior for normal (non-spray) flows is unchanged. Updates the class doc and Component-Security.md to describe the two-tier model instead of the single-tier spray claim. --- docs/requirements/Component-Security.md | 1 + .../LoginThrottle.cs | 180 ++++++++++++------ .../SecurityOptions.cs | 26 +++ .../CookieSessionValidatorTests.cs | 45 +++++ .../LoginThrottleTests.cs | 116 ++++++++++- 5 files changed, 314 insertions(+), 54 deletions(-) diff --git a/docs/requirements/Component-Security.md b/docs/requirements/Component-Security.md index c8a1f28f..92049438 100644 --- a/docs/requirements/Component-Security.md +++ b/docs/requirements/Component-Security.md @@ -209,6 +209,7 @@ To blunt online password-spray and brute-force against the directory, every LDAP - **Scope — every LDAP-bind surface**: the Central UI interactive login (`POST /auth/login`) and CLI token issue (`POST /auth/token`), plus the HTTP-Basic management/CLI surfaces fronted by `ManagementAuthenticator` — `POST /management`, the audit REST endpoints, **and the debug-stream hub (`DebugStreamHub.OnConnectedAsync` delegates its whole credential path to `ManagementAuthenticator.AuthenticateAsync`, closing the round-2 N1 gap where the hub bound LDAP directly and bypassed the throttle)**. No bind happens outside these paths. The dev/test `DisableLogin` bypass sits **above** the throttle: a login-disabled deployment never binds, so it is never throttled. - **Key and window**: counters are keyed per `{username}|{IP}` pair (lower-cased username; the IP half is the **forwarded-header-resolved client IP** when `ScadaBridge:Security:ForwardedHeaders` names a trusted proxy, otherwise the raw connection peer). A fixed window of `LoginFailureWindowMinutes` (default 5) opens on the first failure; reaching `MaxLoginFailuresPerWindow` failures (default 5) within it locks that key out for `LoginLockoutMinutes` (default 5). A successful bind clears the key; an expired window resets the count. Setting `MaxLoginFailuresPerWindow` to `0` disables throttling entirely. +- **Per-IP spray ceiling (arch-review 2026-08-19 P6-9).** The per-pair key alone is blind to a *spray* — one attacker IP trying one password against many distinct usernames never reaches `MaxLoginFailuresPerWindow` on any single pair — so a secondary counter keyed on the IP alone, same window/lockout but a higher ceiling (`PerIpMaxFailures`, default 25, validated strictly greater than `MaxLoginFailuresPerWindow`), locks out *new* attempts from that IP once it is reached; a successful bind does **not** reset the per-IP counter (only window/lockout expiry does), so a spray that stumbles onto one valid account cannot ride that success to keep spraying. - **Proxy topologies and the username-lockout DoS.** The key is deliberately `{username}|{IP}`, not `{username}` alone: keying on username alone would let any network-adjacent actor lock any operator out of a SCADA control surface with five wrong passwords, repeatable indefinitely. That isolation only exists when the throttle sees real client IPs — behind a proxy **without** ForwardedHeaders trust, all clients collapse onto the proxy's IP and the DoS returns. Mitigations: (a) the shipped Traefik topologies enable trusted-proxy ForwardedHeaders (so per-IP isolation is real in the documented deployment); (b) the lockout window is short by default (`LoginLockoutMinutes` = 5) and never touches the directory account itself; (c) residual risk — an attacker spraying from their *own* IP locking out a victim username *at that IP only* — is the throttle working as designed. A success-path bypass ("a correct password unlocks") was considered and rejected: it would let an attacker who has the password bypass the lockout entirely, and it converts the throttle into a password oracle during the lockout window. - **Behaviour when locked**: the surface refuses the bind without contacting LDAP. The Basic-Auth surfaces (`ManagementAuthenticator`) and `POST /auth/token` return HTTP `429` with `code = "AUTH_THROTTLED"`; `POST /auth/login` redirects to `/login` with a "Too many failed attempts. Try again later." message. - **Best-effort, per-node**: the throttle is in-memory with no external state. The two central nodes maintain independent counters (a spray hitting both simply burns its budget on each). Memory is bounded — writes prune expired keys and cap the tracked-key count. This is a rate-limit guard, not an account-lockout policy; it never disables the directory account itself. diff --git a/src/ZB.MOM.WW.ScadaBridge.Security/LoginThrottle.cs b/src/ZB.MOM.WW.ScadaBridge.Security/LoginThrottle.cs index 164d6b65..129b6a69 100644 --- a/src/ZB.MOM.WW.ScadaBridge.Security/LoginThrottle.cs +++ b/src/ZB.MOM.WW.ScadaBridge.Security/LoginThrottle.cs @@ -4,7 +4,8 @@ using Microsoft.Extensions.Options; namespace ZB.MOM.WW.ScadaBridge.Security; /// -/// Fixed-window LDAP-bind failure lockout, keyed per {username}|{ip}. +/// Fixed-window LDAP-bind failure lockout, layering a per-{username}|{ip} budget with a +/// secondary, coarser per-IP budget. /// /// /// @@ -17,27 +18,51 @@ namespace ZB.MOM.WW.ScadaBridge.Security; /// determined spray hitting both nodes still burns its budget twice as fast on each). /// /// +/// Two tiers, same clock. The primary counter is keyed per {username}|{ip} pair +/// and stops a brute-force attempt against one account. On its own that counter is blind to a +/// spray — one attacker IP trying one password against many distinct usernames never +/// accumulates failures on any single +/// pair. A secondary counter keyed on the IP alone closes that gap: it uses the same fixed +/// window () and lockout duration +/// () but a higher ceiling +/// (), so a shared NAT gateway absorbing a +/// handful of ordinary typos across a few users stays under it while a spray does not. +/// refuses a bind attempt when EITHER counter for the pair's IP is +/// currently locked out. +/// +/// +/// Success-path semantics are asymmetric by design. clears +/// only the per-pair counter, never the per-IP counter — a spray that happens to guess (or +/// already knows) one valid account on the sprayed IP must not be able to reset the IP's +/// failure budget and keep spraying indefinitely. The per-IP counter's only relief is window +/// expiry (or lockout expiry once armed); this is the cheapest safe choice and intentionally +/// does not distinguish "this IP's failures are all from one confused legitimate user" from +/// "this IP just found one valid account mid-spray" — the throttle cannot tell those apart. +/// +/// /// The counter uses a fixed window: the first failure opens a window of -/// ; reaching -/// failures inside that window locks -/// the key out for . A successful bind or an -/// expired window resets the counter. Setting -/// to 0 disables throttling entirely. +/// ; reaching the tier's ceiling +/// ( per pair, +/// per IP) inside that window locks the key out +/// for . An expired window resets the count. +/// Setting to 0 disables both tiers +/// entirely. /// /// /// Registered as a singleton () so the /// counters are shared across all requests on a node. Thread-safe via /// and atomic entry swaps. Memory is bounded: -/// each write opportunistically prunes expired entries, and if the map still exceeds -/// the oldest-windowed entries are dropped — so a spray across a -/// large IP/username space cannot grow the map without limit. +/// each write opportunistically prunes expired entries in both maps, and if either map still +/// exceeds the oldest-windowed entries are dropped — so a spray +/// across a large IP/username space cannot grow either map without limit. /// /// public sealed class LoginThrottle { /// - /// Hard cap on the number of tracked {username}|{ip} keys. Above this the - /// oldest-windowed entries are dropped on write to bound memory under a spray attack. + /// Hard cap on the number of tracked keys in EITHER the per-pair ({username}|{ip}) + /// or per-IP map. Above this the oldest-windowed entries are dropped on write to bound + /// memory under a spray attack. /// public const int MaxTrackedKeys = 10_000; @@ -48,16 +73,23 @@ public sealed class LoginThrottle /// window/lockout expiry directly and never depends on pruning. internal const int PruneEveryNFailures = 64; - /// Diagnostics/test accessor: number of currently tracked keys. + /// Diagnostics/test accessor: number of currently tracked per-pair keys. internal int TrackedKeyCount => _entries.Count; + /// Diagnostics/test accessor: number of currently tracked per-IP keys. + internal int TrackedIpCount => _ipEntries.Count; + private int _failureWrites; private readonly TimeProvider _clock; private readonly IOptions _options; + /// Primary tier: one entry per {username}|{ip} pair. private readonly ConcurrentDictionary _entries = new(StringComparer.Ordinal); + /// Secondary tier: one entry per IP, catching a spray across many usernames. + private readonly ConcurrentDictionary _ipEntries = new(StringComparer.Ordinal); + /// /// One tracked key's state: the failure , the /// that anchors the fixed failure window, and — once the @@ -76,26 +108,36 @@ public sealed class LoginThrottle /// /// Returns true when the given + pair is - /// currently locked out and a bind must be refused without contacting LDAP. Never mutates - /// state (an expired lockout simply reads as unlocked; the entry is cleared on the next write). + /// currently locked out, OR itself is locked out under the secondary + /// spray ceiling, and a bind must be refused without contacting LDAP. Never mutates state + /// (an expired lockout simply reads as unlocked; the entry is cleared on the next write). /// /// The username being authenticated. /// The client IP address of the bind attempt. - /// true when the key is currently locked out; otherwise false. + /// true when the pair or its IP is currently locked out; otherwise false. public bool IsLockedOut(string username, string ip) { var opts = _options.Value; if (opts.MaxLoginFailuresPerWindow <= 0) return false; // throttling disabled + var now = _clock.GetUtcNow(); + + if (_ipEntries.TryGetValue(IpKey(ip), out var ipEntry) + && ipEntry.LockedUntil is { } ipUntil && ipUntil > now) + { + return true; + } + if (!_entries.TryGetValue(Key(username, ip), out var entry)) return false; - return entry.LockedUntil is { } until && until > _clock.GetUtcNow(); + return entry.LockedUntil is { } until && until > now; } /// /// Records a failed bind for the given + . - /// Opens or advances the fixed window and, on reaching the configured threshold, arms the - /// lockout. No-op when throttling is disabled. + /// Opens or advances the fixed window for BOTH the per-pair and the per-IP counter and, on + /// either reaching its configured threshold, arms that counter's lockout. No-op when + /// throttling is disabled. /// /// The username that failed authentication. /// The client IP address of the failed bind attempt. @@ -107,12 +149,53 @@ public sealed class LoginThrottle var now = _clock.GetUtcNow(); var window = TimeSpan.FromMinutes(opts.LoginFailureWindowMinutes); var lockout = TimeSpan.FromMinutes(opts.LoginLockoutMinutes); - var key = Key(username, ip); - _entries.AddOrUpdate( + RecordFailureIn(_entries, Key(username, ip), opts.MaxLoginFailuresPerWindow, window, lockout, now); + RecordFailureIn(_ipEntries, IpKey(ip), opts.PerIpMaxFailures, window, lockout, now); + + // Amortized prune (R2 N8): every Nth failure, or immediately when either map is over + // the hard cap — the cap is the memory-safety invariant and must never wait for the + // cadence. + if (Interlocked.Increment(ref _failureWrites) % PruneEveryNFailures == 0 + || _entries.Count > MaxTrackedKeys || _ipEntries.Count > MaxTrackedKeys) + { + Prune(now); + } + } + + /// + /// Records a successful bind for the given + + /// , clearing any accumulated failure count and lockout for that PAIR + /// only. Deliberately does not touch the IP's secondary counter — see the class + /// remarks on success-path asymmetry: resetting the per-IP counter on any successful bind + /// would let a spray that stumbles onto (or already knows) one valid account on the + /// sprayed IP keep spraying indefinitely. The per-IP counter's only relief is window/lockout + /// expiry. + /// + /// The username that authenticated successfully. + /// The client IP address of the successful bind attempt. + public void RecordSuccess(string username, string ip) + { + _entries.TryRemove(Key(username, ip), out _); + } + + /// + /// Records one failure against in , opening or + /// advancing its fixed window and arming its lockout once is + /// reached. Shared by the per-pair and per-IP tiers — only the map, key, and ceiling differ. + /// + private static void RecordFailureIn( + ConcurrentDictionary map, + string key, + int maxFailures, + TimeSpan window, + TimeSpan lockout, + DateTimeOffset now) + { + map.AddOrUpdate( key, // First observed failure for this key: open a fresh window. - _ => Arm(new Entry(1, now, null), opts, now, lockout), + _ => Arm(new Entry(1, now, null), maxFailures, now, lockout), (_, existing) => { // A live lockout stands until it expires — further failures don't extend it. @@ -124,68 +207,59 @@ public sealed class LoginThrottle // If the current window has lapsed (or a prior lockout has expired), restart it. if (existing.LockedUntil is not null || now - existing.WindowStart >= window) { - return Arm(new Entry(1, now, null), opts, now, lockout); + return Arm(new Entry(1, now, null), maxFailures, now, lockout); } - return Arm(existing with { Count = existing.Count + 1 }, opts, now, lockout); + return Arm(existing with { Count = existing.Count + 1 }, maxFailures, now, lockout); }); - - // Amortized prune (R2 N8): every Nth failure, or immediately when over the hard - // cap — the cap is the memory-safety invariant and must never wait for the cadence. - if (Interlocked.Increment(ref _failureWrites) % PruneEveryNFailures == 0 - || _entries.Count > MaxTrackedKeys) - { - Prune(now); - } } - /// - /// Records a successful bind for the given + - /// , clearing any accumulated failure count and lockout for that key. - /// - /// The username that authenticated successfully. - /// The client IP address of the successful bind attempt. - public void RecordSuccess(string username, string ip) - { - _entries.TryRemove(Key(username, ip), out _); - } - - /// Arms the lockout on if it has reached the threshold. - private static Entry Arm(Entry entry, SecurityOptions opts, DateTimeOffset now, TimeSpan lockout) => - entry.Count >= opts.MaxLoginFailuresPerWindow + /// Arms the lockout on if it has reached . + private static Entry Arm(Entry entry, int maxFailures, DateTimeOffset now, TimeSpan lockout) => + entry.Count >= maxFailures ? entry with { LockedUntil = now + lockout } : entry; /// /// Opportunistic memory management on write: drop entries whose window has fully expired - /// (no active lockout), and if the map still exceeds evict the - /// oldest-windowed entries so a spray across many keys cannot grow the map unbounded. + /// (no active lockout) in BOTH maps, and if either map still exceeds + /// evict its oldest-windowed entries so a spray across many + /// keys cannot grow either map unbounded. /// private void Prune(DateTimeOffset now) { var opts = _options.Value; var window = TimeSpan.FromMinutes(opts.LoginFailureWindowMinutes); - foreach (var kvp in _entries) + PruneMap(_entries, window, now); + PruneMap(_ipEntries, window, now); + } + + private static void PruneMap(ConcurrentDictionary map, TimeSpan window, DateTimeOffset now) + { + foreach (var kvp in map) { var e = kvp.Value; var lockActive = e.LockedUntil is { } until && until > now; if (!lockActive && now - e.WindowStart >= window) { - _entries.TryRemove(kvp); + map.TryRemove(kvp); } } - if (_entries.Count <= MaxTrackedKeys) return; + if (map.Count <= MaxTrackedKeys) return; // Still over the cap after pruning expired entries — evict the oldest-windowed keys. - var excess = _entries.Count - MaxTrackedKeys; - foreach (var kvp in _entries.OrderBy(k => k.Value.WindowStart).Take(excess)) + var excess = map.Count - MaxTrackedKeys; + foreach (var kvp in map.OrderBy(k => k.Value.WindowStart).Take(excess)) { - _entries.TryRemove(kvp); + map.TryRemove(kvp); } } private static string Key(string username, string ip) => $"{(username ?? string.Empty).ToLowerInvariant()}|{ip ?? string.Empty}"; + + /// Key for the secondary, IP-only tier — deliberately no username component. + private static string IpKey(string ip) => ip ?? string.Empty; } diff --git a/src/ZB.MOM.WW.ScadaBridge.Security/SecurityOptions.cs b/src/ZB.MOM.WW.ScadaBridge.Security/SecurityOptions.cs index 2c7d6649..d77f490d 100644 --- a/src/ZB.MOM.WW.ScadaBridge.Security/SecurityOptions.cs +++ b/src/ZB.MOM.WW.ScadaBridge.Security/SecurityOptions.cs @@ -124,6 +124,18 @@ public class SecurityOptions /// failures within the window. Default: 5. /// public int LoginLockoutMinutes { get; set; } = 5; + + /// + /// Secondary, IP-only failure ceiling layered on top of the per-{username}|{ip} + /// budget () — closes the password-spray gap where + /// one attacker IP tries one password against many distinct usernames, never accumulating + /// failures on any single pair. Reuses the same + /// window and + /// lockout duration, just counted per IP instead of per pair. Set higher than the per-pair + /// ceiling (validated by ) so a handful of ordinary + /// typos behind a shared NAT gateway don't trip it. Default: 25. + /// + public int PerIpMaxFailures { get; set; } = 25; } /// @@ -186,6 +198,20 @@ public sealed class SecurityOptionsValidator : Microsoft.Extensions.Options.IVal $"({options.LoginLockoutMinutes}) must be non-negative."); } + // SECURITY: the per-IP spray ceiling must sit strictly above the per-pair ceiling. + // If it didn't, a single unlucky (or malicious) username/IP pair could trip the + // IP-wide lockout before its own per-pair budget is exhausted, locking out every + // OTHER username sharing that IP (e.g. a NAT gateway) on account of one pair's + // failures — defeating the "don't punish shared IPs for a couple of typos" intent. + if (options.PerIpMaxFailures <= options.MaxLoginFailuresPerWindow) + { + return Microsoft.Extensions.Options.ValidateOptionsResult.Fail( + $"{nameof(SecurityOptions.PerIpMaxFailures)} " + + $"({options.PerIpMaxFailures}) must be strictly greater than " + + $"{nameof(SecurityOptions.MaxLoginFailuresPerWindow)} " + + $"({options.MaxLoginFailuresPerWindow})."); + } + return Microsoft.Extensions.Options.ValidateOptionsResult.Success; } } diff --git a/tests/ZB.MOM.WW.ScadaBridge.Security.Tests/CookieSessionValidatorTests.cs b/tests/ZB.MOM.WW.ScadaBridge.Security.Tests/CookieSessionValidatorTests.cs index 783f6d35..c21b1d41 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.Security.Tests/CookieSessionValidatorTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.Security.Tests/CookieSessionValidatorTests.cs @@ -681,6 +681,51 @@ public class SecurityOptionsValidatorConfigGuardTests Assert.True(result.Failed); Assert.Contains(nameof(SecurityOptions.RoleRefreshThresholdMinutes), result.FailureMessage); } + + // ── arch-review 2026-08-19 P6-9: PerIpMaxFailures must exceed the per-pair ceiling ── + + [Fact] + public void Validate_DefaultPerIpMaxFailures_Succeeds() + { + var result = new SecurityOptionsValidator().Validate(name: null, ValidOptions()); + Assert.True(result.Succeeded); + } + + [Fact] + public void Validate_PerIpMaxFailuresEqualsPerPairCeiling_Fails() + { + var options = ValidOptions(); + options.PerIpMaxFailures = options.MaxLoginFailuresPerWindow; // 5 == 5 + + var result = new SecurityOptionsValidator().Validate(name: null, options); + + Assert.True(result.Failed); + Assert.Contains(nameof(SecurityOptions.PerIpMaxFailures), result.FailureMessage); + Assert.Contains(nameof(SecurityOptions.MaxLoginFailuresPerWindow), result.FailureMessage); + } + + [Fact] + public void Validate_PerIpMaxFailuresBelowPerPairCeiling_Fails() + { + var options = ValidOptions(); + options.PerIpMaxFailures = 3; // < default MaxLoginFailuresPerWindow (5) + + var result = new SecurityOptionsValidator().Validate(name: null, options); + + Assert.True(result.Failed); + Assert.Contains(nameof(SecurityOptions.PerIpMaxFailures), result.FailureMessage); + } + + [Fact] + public void Validate_PerIpMaxFailuresOneAbovePerPairCeiling_Succeeds() + { + var options = ValidOptions(); + options.PerIpMaxFailures = options.MaxLoginFailuresPerWindow + 1; // tightest valid config + + var result = new SecurityOptionsValidator().Validate(name: null, options); + + Assert.True(result.Succeeded); + } } #endregion diff --git a/tests/ZB.MOM.WW.ScadaBridge.Security.Tests/LoginThrottleTests.cs b/tests/ZB.MOM.WW.ScadaBridge.Security.Tests/LoginThrottleTests.cs index 91ed4b1c..999ed128 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.Security.Tests/LoginThrottleTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.Security.Tests/LoginThrottleTests.cs @@ -136,9 +136,123 @@ public class LoginThrottleTests var (throttle, clock) = Create(); throttle.RecordFailure("stale", "10.0.0.1"); clock.Advance(TimeSpan.FromMinutes(10)); // window fully expired + // Distinct IPs (not just distinct usernames) so this stays a pure per-pair prune-cadence + // test — hammering PruneEveryNFailures (64) failures from a SINGLE IP would itself trip + // the P6-9 per-IP spray ceiling (default 25) and lock "10.0.0.1" out, which would make + // the IsLockedOut("stale", "10.0.0.1") assertion below fail for an unrelated reason. for (var i = 0; i < LoginThrottle.PruneEveryNFailures; i++) - throttle.RecordFailure($"fresh{i}", "10.0.0.1"); // cadence tick fires within these + throttle.RecordFailure($"fresh{i}", $"10.0.1.{i}"); // cadence tick fires within these Assert.False(throttle.IsLockedOut("stale", "10.0.0.1")); Assert.True(throttle.TrackedKeyCount <= LoginThrottle.PruneEveryNFailures); // "stale" swept } + + // ── arch-review 2026-08-19 P6-9: per-IP spray ceiling ────────────────────── + + [Fact] + public void Spray_OnePasswordAcrossManyUsernamesFromOneIp_TripsThePerIpCeiling() + { + // THE FAIL-UNFIXED PROOF: against the old per-{username}|{ip}-only throttle, one + // failure each across PerIpMaxFailures distinct usernames from a single IP never + // reaches MaxLoginFailuresPerWindow (5) on any one pair, so the next attempt from + // that IP — even against a brand-new username — sails through un-throttled. With the + // fix, the shared per-IP counter accumulates across usernames and locks the IP out. + var clock = new TestTimeProvider(Start); + var options = new SecurityOptions { PerIpMaxFailures = 10 }; + var throttle = Create(clock, options); + + // One failure each across the first (ceiling - 1) usernames: no individual pair ever + // gets near the per-pair ceiling (5), and the IP hasn't tipped over yet either. + for (var i = 0; i < options.PerIpMaxFailures - 1; i++) + { + throttle.RecordFailure($"user{i}", "10.0.0.1"); + Assert.False(throttle.IsLockedOut($"user{i}", "10.0.0.1")); + } + + // The PerIpMaxFailures-th failure — against yet another brand-new username — tips the + // shared IP counter over the ceiling. + throttle.RecordFailure("user-last", "10.0.0.1"); + + // Now EVERY username from that IP is refused, including ones never tried before. + Assert.True(throttle.IsLockedOut("brand-new-user", "10.0.0.1")); + } + + [Fact] + public void Spray_DistinctIpsAreUnaffectedByOneIpsLockout() + { + var clock = new TestTimeProvider(Start); + var options = new SecurityOptions { PerIpMaxFailures = 10 }; + var throttle = Create(clock, options); + + for (var i = 0; i < options.PerIpMaxFailures; i++) + throttle.RecordFailure($"user{i}", "10.0.0.1"); + + Assert.True(throttle.IsLockedOut("anyone", "10.0.0.1")); + Assert.False(throttle.IsLockedOut("anyone", "10.0.0.2")); // untouched IP + Assert.False(throttle.IsLockedOut("user0", "10.0.0.2")); + } + + [Fact] + public void PerPairLockout_StillFiresBeforeThePerIpCeiling_NormalFlowUnchanged() + { + // Byte-compatible per-pair behavior: five failures against ONE pair still locks that + // pair out well below the (much higher) default per-IP ceiling. + var clock = new TestTimeProvider(Start); + var throttle = Create(clock); + + for (var i = 0; i < 5; i++) throttle.RecordFailure("alice", "10.0.0.1"); + + Assert.True(throttle.IsLockedOut("alice", "10.0.0.1")); + Assert.False(throttle.IsLockedOut("bob", "10.0.0.1")); // IP itself is nowhere near its ceiling + } + + [Fact] + public void NatFriendly_AFewFailuresAcrossAFewUsers_StaysUnderThePerIpCeiling() + { + // A shared NAT gateway where a handful of users mistype their passwords must not be + // punished: a couple of failures each across a few distinct usernames stays well under + // the default PerIpMaxFailures (25). + var clock = new TestTimeProvider(Start); + var throttle = Create(clock); // default PerIpMaxFailures = 25 + + foreach (var user in new[] { "alice", "bob", "carol", "dave" }) + for (var i = 0; i < 3; i++) // 4 users x 3 failures = 12 < 25, and < 5 per pair + throttle.RecordFailure(user, "10.0.0.1"); + + foreach (var user in new[] { "alice", "bob", "carol", "dave" }) + Assert.False(throttle.IsLockedOut(user, "10.0.0.1")); + Assert.False(throttle.IsLockedOut("eve", "10.0.0.1")); + } + + [Fact] + public void PerIpLockout_ReleasesOnWindowExpiry() + { + var clock = new TestTimeProvider(Start); + var options = new SecurityOptions { PerIpMaxFailures = 10 }; + var throttle = Create(clock, options); + + for (var i = 0; i < options.PerIpMaxFailures; i++) + throttle.RecordFailure($"user{i}", "10.0.0.1"); + Assert.True(throttle.IsLockedOut("brand-new-user", "10.0.0.1")); + + clock.Advance(TimeSpan.FromMinutes(6)); // > default LoginLockoutMinutes (5) + Assert.False(throttle.IsLockedOut("brand-new-user", "10.0.0.1")); + } + + [Fact] + public void RecordSuccess_DoesNotResetThePerIpCounter() + { + // A spray that stumbles onto (or already knows) one valid account on the sprayed IP + // must not be able to reset the IP's failure budget via that success and keep spraying. + var clock = new TestTimeProvider(Start); + var options = new SecurityOptions { PerIpMaxFailures = 10 }; + var throttle = Create(clock, options); + + for (var i = 0; i < options.PerIpMaxFailures - 1; i++) + throttle.RecordFailure($"user{i}", "10.0.0.1"); // one short of the IP ceiling + + throttle.RecordSuccess("valid-user", "10.0.0.1"); // a hit mid-spray + + throttle.RecordFailure("user-last", "10.0.0.1"); // the IP ceiling is still reached + Assert.True(throttle.IsLockedOut("another-new-user", "10.0.0.1")); + } }