dohertj2/ScadaBridge
1
0
Fork 0
Code Issues 2 Pull Requests Actions Packages Projects Releases Wiki Activity

fix(centralui): per-IP spray ceiling on LoginThrottle (arch-review 2026-08-19 P6-9)

Browse Source 
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.
This commit is contained in:
Joseph Doherty
2026-08-19 14:54:25 -04:00
parent 561582747e
commit 013257ec7c
5 changed files with 314 additions and 54 deletions
Download Patch File Download Diff File Expand all files Collapse all files
tests/ZB.MOM.WW.ScadaBridge.Security.Tests/CookieSessionValidatorTests.cs
+45
View File
@@ -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
tests/ZB.MOM.WW.ScadaBridge.Security.Tests/LoginThrottleTests.cs
+115 -1
View File
@@ -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"));
}
}