diff --git a/.claude/rules/data-plane-auth.md b/.claude/rules/data-plane-auth.md index ce69f08be..d59a8ef0a 100644 --- a/.claude/rules/data-plane-auth.md +++ b/.claude/rules/data-plane-auth.md @@ -240,8 +240,10 @@ Two things to know before relying on any of this: keyed cert-thumbprint-else-peer-IP. `OpcUaApplicationHost.DisableSdkAuthenticationLockout` zeroes `MaxFailedAuthenticationAttempts` on built AND loaded configs; `FailedCredentialLockout` (`OpcUa:CredentialLockout`, 5 / 60 s / 300 s) counts and refuses UserName activations only, keyed thumbprint+peer together. Do not re-enable the - SDK setting. The check RESERVES an in-flight slot (`TryBegin`/`Complete`/`Abandon`) — a check and a separate - record let a parallel burst through; recorded + in-flight ≥ max ⇒ refused. An expired lockout resets the count. On + SDK setting. The check takes a verification SLOT (`BeginAsync`/`Complete`/`Abandon`) — a check and a + separate record let a parallel burst through. At most `max − failures` verify at once; the rest WAIT (never refused + for parallelism — ScadaBridge opens many keyed sessions from one IP), refused only on lockout or, uncounted, after + `SlotWaitSeconds`. An expired lockout resets the count. On `None` endpoints the key is effectively IP-only (NAT neighbours share it, UserName only). `docs/security.md` §"Failed-credential lockout". - **Keys reach a driver-only node inside the deployment artifact** (stage 4). `ConfigComposer` snapshots diff --git a/docs/Configuration.md b/docs/Configuration.md index f100852c7..23f2e0e55 100644 --- a/docs/Configuration.md +++ b/docs/Configuration.md @@ -76,6 +76,7 @@ Until #678.6 the overlay never loaded on a node carrying a `cluster-*` role (the | `PeerApplicationUris` | list of string | `[]` (empty) | Partner node `ApplicationUri`s published in `Server.ServerArray` for redundancy discovery. See [`Redundancy.md`](Redundancy.md). | | `CredentialLockout:MaxFailedAttempts` | int | `5` | Failed UserName (API-key) activations from one client before its UserName activations are refused `BadUserAccessDenied` (#833). `≤ 0` disables. **Never refuses Anonymous.** See [`security.md`](security.md#failed-credential-lockout-opcuacredentiallockout). | | `CredentialLockout:FailureWindowSeconds` | int | `60` | A failure more than this long after the client's previous one starts a fresh count. Must be `> 0` while the lockout is enabled (validated). | +| `CredentialLockout:SlotWaitSeconds` | int | `10` | How long a UserName activation waits for one of its client's verification slots (at most `MaxFailedAttempts − failures` verify at once; the rest queue) before that one attempt fails `BadTimeout`, uncounted. Must be `> 0` while enabled (validated). | | `CredentialLockout:LockoutSeconds` | int | `300` | How long a locked-out client's UserName activations are refused; an expired lockout starts a fresh count. Must be `> 0` while the lockout is enabled (validated). | > **Transport security profiles** (the values in `EnabledSecurityProfiles` — `None`, `Basic256Sha256Sign`, `Basic256Sha256SignAndEncrypt`) and the PKI trust flow are documented in full in [`security.md`](security.md). This page does not duplicate them. diff --git a/docs/security.md b/docs/security.md index 247a824f2..b2fa3e8e4 100644 --- a/docs/security.md +++ b/docs/security.md @@ -338,19 +338,22 @@ The host now sets `MaxFailedAuthenticationAttempts = 0` on every configuration i always starts a fresh count**, whatever `FailureWindowSeconds` is; a successful UserName activation clears the count. **Anonymous activations never consult it**, from any client. The first refusal of each lockout episode is logged at Information, the rest at Debug. -- **Parallel activations are held to the threshold.** The check reserves an in-flight slot under the same lock, - and a client whose recorded failures plus in-progress UserName activations already reach - `MaxFailedAttempts` is refused (`BadUserAccessDenied`, "too many authentication attempts in progress"), so a - burst cannot slip past the check before its first failure is recorded. ⚠️ The same rule caps a **legitimate** - client at `MaxFailedAttempts` concurrent UserName activations (the check is milliseconds, so this only bites a - burst of simultaneous session opens from one key). +- **Parallel activations wait — they are never refused for parallelism.** A client gets at most + `MaxFailedAttempts − recordedFailures` verification **slots**; a UserName activation beyond that **queues** (FIFO) + until one frees, so no more than `MaxFailedAttempts` unverified guesses are ever in progress-or-failed per window, + while a legitimate consumer opening many keyed sessions at once (e.g. a ScadaBridge site node behind one NAT IP on a + `None` endpoint) just waits milliseconds and is admitted. A waiter is refused only when the key actually **locks + out** (`BadUserAccessDenied`, without reaching the authenticator), or after `SlotWaitSeconds` (default 10) + with `BadTimeout` — **not** counted as a credential failure. A cancelled activation leaves the queue and frees + any slot it held. - **Keyed on certificate thumbprint *and* peer address together.** That separates clients only when they present a certificate. ⚠️ On a `None` endpoint clients commonly present none, so the key is **effectively the IP alone**: NAT neighbours there **do** share one count (for UserName activations only — never for anonymous ones). When the transport reports no peer address at all, every certificate-less client shares the single key `-@-`. (Observed on docker-dev: every host client arrives as `-@192.168.97.1`.) -- **Validated at host start:** with `MaxFailedAttempts > 0`, both `LockoutSeconds` and `FailureWindowSeconds` must be - `> 0` (`OpcUaApplicationHostOptionsValidator`); `MaxFailedAttempts <= 0` disables the lockout. +- **Validated at host start:** with `MaxFailedAttempts > 0`, `LockoutSeconds`, `FailureWindowSeconds` and + `SlotWaitSeconds` must all be `> 0` (`OpcUaApplicationHostOptionsValidator`); `MaxFailedAttempts <= 0` disables + the lockout. - Every denial counts, including a key-store timeout or miss-budget refusal (as the SDK's did) — a ConfigDb outage can therefore lock a keyed client out for up to `LockoutSeconds` after recovery. - Bounded at 4096 tracked clients (expired entries are pruned; a client arriving at a full table is not diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.Host/Configuration/OpcUaApplicationHostOptionsValidator.cs b/src/Server/ZB.MOM.WW.OtOpcUa.Host/Configuration/OpcUaApplicationHostOptionsValidator.cs index c33afe3a3..c51ed0d40 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.Host/Configuration/OpcUaApplicationHostOptionsValidator.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.Host/Configuration/OpcUaApplicationHostOptionsValidator.cs @@ -34,8 +34,9 @@ public sealed class OpcUaApplicationHostOptionsValidator : OptionsValidatorBase< // nothing, a zero FailureWindowSeconds would never accumulate. MaxFailedAttempts <= 0 disables it. var lockout = o.CredentialLockout; builder.RequireThat(lockout is null || lockout.MaxFailedAttempts <= 0 - || (lockout.LockoutSeconds > 0 && lockout.FailureWindowSeconds > 0), - "OpcUa:CredentialLockout:LockoutSeconds and OpcUa:CredentialLockout:FailureWindowSeconds must both be " + || (lockout.LockoutSeconds > 0 && lockout.FailureWindowSeconds > 0 && lockout.SlotWaitSeconds > 0), + "OpcUa:CredentialLockout:LockoutSeconds, OpcUa:CredentialLockout:FailureWindowSeconds and " + + "OpcUa:CredentialLockout:SlotWaitSeconds must all be " + "greater than 0 while OpcUa:CredentialLockout:MaxFailedAttempts is greater than 0 (set " + "MaxFailedAttempts to 0 to disable the lockout)"); } diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/OpcUaApplicationHost.cs b/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/OpcUaApplicationHost.cs index 62402e731..61f8db9ab 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/OpcUaApplicationHost.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/OpcUaApplicationHost.cs @@ -275,31 +275,35 @@ public sealed class OpcUaApplicationHost : IAsyncDisposable var lockoutKey = tokenType == UserTokenType.UserName ? FailedCredentialLockout.KeyFor(context.ChannelCertificate?.Thumbprint, peer) : null; - if (lockoutKey is not null - && !host._credentialLockout.TryBegin(lockoutKey, out var refusal, out var remaining, out var firstRefusal)) + if (lockoutKey is not null) { - // Logged once per lockout episode at Information; every further refusal at Debug, so a client hammering - // a lockout cannot flood the log (#833 review). - var level = firstRefusal ? LogLevel.Information : LogLevel.Debug; - if (refusal == CredentialLockoutRefusal.LockedOut) + // Waits (bounded) for one of this client's verification slots — parallel activations queue, they are + // never refused for parallelism. Refused only when locked out, or (uncounted) when no slot freed in time. + var decision = await host._credentialLockout.BeginAsync(lockoutKey, ct).ConfigureAwait(false); + if (decision.Verdict == CredentialLockoutVerdict.LockedOut) { - var seconds = (long)Math.Ceiling(remaining.TotalSeconds); - host._logger.Log(level, + // Logged once per lockout episode at Information; every further refusal at Debug, so a client + // hammering a lockout cannot flood the log (#833 review). + var seconds = (long)Math.Ceiling(decision.Remaining.TotalSeconds); + host._logger.Log(decision.FirstRefusal ? LogLevel.Information : LogLevel.Debug, "OpcUaApplicationHost: UserName activation refused — client {Client} is locked out for {Seconds}s more", lockoutKey, seconds); return AuthenticationResult.Reject(ServiceResult.Create(StatusCodes.BadUserAccessDenied, $"Too many failed authentication attempts. Try again in {seconds} seconds.")); } - host._logger.Log(level, - "OpcUaApplicationHost: UserName activation refused — client {Client} already has as many failed plus " - + "in-progress attempts as OpcUa:CredentialLockout allows", - lockoutKey); - return AuthenticationResult.Reject(ServiceResult.Create(StatusCodes.BadUserAccessDenied, - "Too many authentication attempts in progress. Try again shortly.")); + if (decision.Verdict == CredentialLockoutVerdict.SlotWaitTimedOut) + { + host._logger.LogWarning( + "OpcUaApplicationHost: UserName activation for client {Client} timed out waiting for a " + + "verification slot (OpcUa:CredentialLockout:SlotWaitSeconds); not counted as a failure", + lockoutKey); + return AuthenticationResult.Reject(ServiceResult.Create(StatusCodes.BadTimeout, + "Timed out waiting to verify the credential. Try again.")); + } } - // The slot TryBegin reserved is released on every path — completed with the outcome, or abandoned + // The slot BeginAsync granted is released on every path — completed with the outcome, or abandoned // (uncounted) when the activation was cancelled or threw. var completed = false; try diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/Security/FailedCredentialLockout.cs b/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/Security/FailedCredentialLockout.cs index 5a3d27304..24fffa4dc 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/Security/FailedCredentialLockout.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/Security/FailedCredentialLockout.cs @@ -7,7 +7,7 @@ namespace ZB.MOM.WW.OtOpcUa.OpcUaServer.Security; /// /// The defaults are the OPC Foundation SDK 2.0.1 SessionManager lockout this replaces: 5 failures, /// each inside 60 s of the previous one, lock the client out for 5 minutes. When enabled -/// ( > 0) both durations must be positive — enforced at host start by +/// ( > 0) every duration must be positive — enforced at host start by /// OpcUaApplicationHostOptionsValidator. /// public sealed class OpcUaCredentialLockoutOptions @@ -20,24 +20,37 @@ public sealed class OpcUaCredentialLockoutOptions /// How long a locked-out client's UserName activations are refused, in seconds. public int LockoutSeconds { get; set; } = 300; -} - -/// Why refused an activation. -public enum CredentialLockoutRefusal -{ - /// Not refused. - None, - - /// The client is locked out. - LockedOut, /// - /// The client's recorded failures plus its activations still being checked already reach the threshold, so - /// one more concurrent attempt could exceed it before any of them has been recorded. + /// How long a UserName activation waits for one of its client's verification slots before that one attempt + /// is failed (BadTimeout, NOT counted as a credential failure). Only reached when a client has more + /// verifications in progress than minus its recorded failures; a verification + /// is milliseconds, so in practice this bounds a pathological store stall. Kept well under the SDK's + /// operation timeout so the client gets this answer rather than a transport timeout. /// - TooManyInFlight, + public int SlotWaitSeconds { get; set; } = 10; } +/// The outcome of . +public enum CredentialLockoutVerdict +{ + /// A verification slot is held — authenticate, then call . + Proceed, + + /// The client is locked out; refuse without authenticating. + LockedOut, + + /// No slot freed within the wait; fail this one attempt, uncounted. + SlotWaitTimedOut, +} + +/// What decided. +/// Proceed, locked out, or timed out waiting. +/// Time left on the lockout when . +/// True for the first refusal of a lockout episode (log it at Information, the rest at Debug). +public readonly record struct CredentialLockoutDecision( + CredentialLockoutVerdict Verdict, TimeSpan Remaining, bool FirstRefusal); + /// /// Brute-force lockout for UserName (API-key) activations, scoped so it can never refuse an /// anonymous activation (lmxopcua#833). @@ -69,16 +82,22 @@ public enum CredentialLockoutRefusal /// window); a successful UserName activation clears the client's count. /// /// -/// Concurrency. The authenticate call is awaited between the check and the outcome, so a check and a -/// separate record would let a burst of parallel activations all pass before the first failure lands. -/// therefore reserves an in-flight slot under the same lock as the check, and -/// refuses while recorded failures + in-flight attempts already reach the threshold; -/// releases the slot and records the outcome. +/// Concurrency — parallelism waits, it is never refused. The authenticate call is awaited between the +/// check and the outcome, so a check and a separate record would let a parallel burst of guesses all pass +/// before the first failure lands. therefore hands out at most +/// MaxFailedAttempts − recordedFailures verification slots per key; an activation beyond that +/// waits (FIFO) for a slot, and / release it and wake the +/// next waiter. So no more than MaxFailedAttempts unverified guesses are ever in progress-or-failed +/// per window, while a legitimate client opening many keyed sessions at once (a ScadaBridge site node behind +/// one NAT IP) just queues for milliseconds. Waiters are refused only when the key actually locks out (they +/// never reach the authenticator) or after +/// (uncounted); a cancelled waiter is removed from the queue. /// /// -/// Memory is bounded at : expired, idle entries are pruned when it fills, -/// and a client that still finds it full is not tracked (it can only evade by rotating certificates or -/// addresses, which it could do against the SDK lockout too; API-key secrets are not guessable). +/// Memory is bounded at : entries with no failures, slots or waiters are +/// pruned when it fills, and a client that still finds it full is not tracked (it can only evade by +/// rotating certificates or addresses, which it could do against the SDK lockout too; API-key secrets are +/// not guessable). /// /// public sealed class FailedCredentialLockout @@ -111,85 +130,95 @@ public sealed class FailedCredentialLockout $"{(string.IsNullOrEmpty(certificateThumbprint) ? "-" : certificateThumbprint)}@{peer?.ToString() ?? "-"}"; /// - /// Checks and, when it may proceed, reserves an in-flight slot that the caller MUST - /// release with (only when this returns ). + /// Acquires a verification slot for , waiting for one when the client already has as + /// many in progress as it may. On the caller MUST release the + /// slot with or . /// /// The client key from . - /// Why it was refused, when this returns false. - /// The time left on the lockout when refused . - /// - /// True for the first refusal of this lockout episode (so the caller logs it once at Information and the - /// rest at Debug). - /// - /// when the activation may be authenticated. - public bool TryBegin(string key, out CredentialLockoutRefusal refusal, out TimeSpan remaining, out bool firstRefusal) + /// Cancels the wait; a cancelled waiter is dequeued and the exception propagates. + /// The decision. + public async ValueTask BeginAsync(string key, CancellationToken ct = default) { - refusal = CredentialLockoutRefusal.None; - remaining = TimeSpan.Zero; - firstRefusal = false; - if (!Enabled) return true; - var now = _time.GetUtcNow(); + if (!Enabled) return new(CredentialLockoutVerdict.Proceed, TimeSpan.Zero, false); + + Waiter waiter; + Entry entry; lock (_gate) { - if (!_entries.TryGetValue(key, out var entry)) + var now = _time.GetUtcNow(); + if (!_entries.TryGetValue(key, out var existing)) { if (_entries.Count >= MaxTrackedClients) Prune(now); - if (_entries.Count >= MaxTrackedClients) return true; // untracked: nothing to reserve - entry = new Entry(); - _entries[key] = entry; + // Untracked when still full: it gets no slot accounting, as before (see remarks). + if (_entries.Count >= MaxTrackedClients) return new(CredentialLockoutVerdict.Proceed, TimeSpan.Zero, false); + existing = new Entry(); + _entries[key] = existing; } + entry = existing; Refresh(entry, now); - if (entry.LockedUntil > now) + if (TryLockedOut(entry, now, out var refused)) return refused; + if (entry.Waiters.Count == 0 && HasFreeSlot(entry)) { - refusal = CredentialLockoutRefusal.LockedOut; - remaining = entry.LockedUntil - now; - firstRefusal = !entry.RefusalLogged; - entry.RefusalLogged = true; - return false; + entry.InFlight++; + return new(CredentialLockoutVerdict.Proceed, TimeSpan.Zero, false); } - if (entry.Failures + entry.InFlight >= _options.MaxFailedAttempts) + waiter = new Waiter(); + entry.Waiters.AddLast(waiter.Node = new LinkedListNode(waiter)); + } + + var wait = TimeSpan.FromSeconds(Math.Max(1, _options.SlotWaitSeconds)); + try + { + return await waiter.Completion.Task.WaitAsync(wait, _time, ct).ConfigureAwait(false); + } + catch (Exception ex) when (ex is TimeoutException or OperationCanceledException) + { + lock (_gate) { - refusal = CredentialLockoutRefusal.TooManyInFlight; - firstRefusal = !entry.RefusalLogged; - entry.RefusalLogged = true; - return false; + if (waiter.Completion.Task.IsCompletedSuccessfully) + { + // Decided in the same instant the wait gave up. A timeout takes the decision as made; a + // cancellation hands a granted slot back (waking the next waiter) so nothing leaks. + var decided = waiter.Completion.Task.Result; + if (ex is not OperationCanceledException) return decided; + if (decided.Verdict == CredentialLockoutVerdict.Proceed) ReleaseCore(key, entry, outcome: null); + } + else + { + entry.Waiters.Remove(waiter.Node!); + waiter.Completion.TrySetCanceled(); + RemoveIfIdle(key, entry); + } } - entry.InFlight++; - return true; + if (ex is OperationCanceledException) throw; + return new(CredentialLockoutVerdict.SlotWaitTimedOut, TimeSpan.Zero, false); } } - /// Releases the slot reserved and records the activation's outcome. + /// Releases a slot from and records the activation's outcome. /// The client key. /// Whether the activation was admitted. /// when this failure locked the client out. public bool Complete(string key, bool succeeded) { if (!Enabled) return false; - var now = _time.GetUtcNow(); lock (_gate) { - if (!_entries.TryGetValue(key, out var entry)) return false; // was untracked at TryBegin - if (entry.InFlight > 0) entry.InFlight--; - return succeeded ? RecordSuccessCore(key, entry) : RecordFailureCore(entry, now); + return _entries.TryGetValue(key, out var entry) && ReleaseCore(key, entry, succeeded); } } - /// Releases a reserved slot without recording an outcome (the activation was cancelled or threw). + /// Releases a slot without recording an outcome (the activation was cancelled or threw). /// The client key. public void Abandon(string key) { if (!Enabled) return; lock (_gate) { - if (_entries.TryGetValue(key, out var entry) && entry.InFlight > 0) - { - entry.InFlight--; - if (IsIdle(entry, _time.GetUtcNow())) _entries.Remove(key); - } + if (_entries.TryGetValue(key, out var entry)) ReleaseCore(key, entry, outcome: null); } } @@ -212,7 +241,7 @@ public sealed class FailedCredentialLockout } } - /// Records one failed UserName activation with no reservation (test seam / direct callers). + /// Records one failed UserName activation with no slot (test seam / direct callers). /// The client key. /// when this failure locked the client out. public bool RecordFailure(string key) @@ -228,17 +257,22 @@ public sealed class FailedCredentialLockout entry = new Entry(); _entries[key] = entry; } - return RecordFailureCore(entry, now); + var locked = RecordFailureCore(entry, now); + Pump(entry, now); + return locked; } } - /// Clears a client's count after a successful UserName activation (no reservation). + /// Clears a client's count after a successful UserName activation (no slot). /// The client key. public void RecordSuccess(string key) { lock (_gate) { - if (_entries.TryGetValue(key, out var entry)) RecordSuccessCore(key, entry); + if (!_entries.TryGetValue(key, out var entry)) return; + ClearCount(entry); + Pump(entry, _time.GetUtcNow()); + RemoveIfIdle(key, entry); } } @@ -248,16 +282,65 @@ public sealed class FailedCredentialLockout get { lock (_gate) return _entries.Count; } } - private bool RecordSuccessCore(string key, Entry entry) + /// How many activations are waiting for a slot on (test observation). + internal int WaitingCount(string key) { - if (entry.InFlight == 0) _entries.Remove(key); - else + lock (_gate) return _entries.TryGetValue(key, out var e) ? e.Waiters.Count : 0; + } + + private bool HasFreeSlot(Entry entry) => entry.Failures + entry.InFlight < _options.MaxFailedAttempts; + + private bool TryLockedOut(Entry entry, DateTimeOffset now, out CredentialLockoutDecision refused) + { + refused = default; + if (entry.LockedUntil <= now) return false; + refused = new(CredentialLockoutVerdict.LockedOut, entry.LockedUntil - now, !entry.RefusalLogged); + entry.RefusalLogged = true; + return true; + } + + /// Releases one slot, records (null = uncounted) and wakes waiters. + private bool ReleaseCore(string key, Entry entry, bool? outcome) + { + var now = _time.GetUtcNow(); + if (entry.InFlight > 0) entry.InFlight--; + var lockedNow = false; + if (outcome == true) ClearCount(entry); + else if (outcome == false) lockedNow = RecordFailureCore(entry, now); + Pump(entry, now); + RemoveIfIdle(key, entry); + return lockedNow; + } + + /// + /// Hands freed slots to waiters in order — or, when the key is locked out, refuses every waiter (they never + /// reach the authenticator). Called under the gate after anything that can free a slot or lock the key. + /// + private void Pump(Entry entry, DateTimeOffset now) + { + Refresh(entry, now); + while (entry.Waiters.First is { } first) { - entry.Failures = 0; - entry.LockedUntil = default; - entry.RefusalLogged = false; + if (TryLockedOut(entry, now, out var refused)) + { + entry.Waiters.RemoveFirst(); + first.Value.Completion.TrySetResult(refused); + continue; + } + + if (!HasFreeSlot(entry)) return; + entry.Waiters.RemoveFirst(); + entry.InFlight++; + if (!first.Value.Completion.TrySetResult(new(CredentialLockoutVerdict.Proceed, TimeSpan.Zero, false))) + entry.InFlight--; // a waiter that already gave up never takes the slot } - return false; + } + + private static void ClearCount(Entry entry) + { + entry.Failures = 0; + entry.LockedUntil = default; + entry.RefusalLogged = false; } private bool RecordFailureCore(Entry entry, DateTimeOffset now) @@ -281,9 +364,7 @@ public sealed class FailedCredentialLockout { if (entry.LockedUntil != default && entry.LockedUntil <= now) { - entry.Failures = 0; - entry.LockedUntil = default; - entry.RefusalLogged = false; + ClearCount(entry); } else if (entry.LockedUntil == default && entry.Failures > 0 && now - entry.LastFailure > TimeSpan.FromSeconds(Math.Max(0, _options.FailureWindowSeconds))) @@ -293,19 +374,34 @@ public sealed class FailedCredentialLockout } } + private void RemoveIfIdle(string key, Entry entry) + { + if (IsIdle(entry, _time.GetUtcNow())) _entries.Remove(key); + } + private void Prune(DateTimeOffset now) { foreach (var key in _entries.Where(e => IsIdle(e.Value, now)).Select(e => e.Key).ToList()) _entries.Remove(key); } + /// Idle = nothing to remember: no failures, no lockout, no slot held and nobody waiting. private bool IsIdle(Entry entry, DateTimeOffset now) { Refresh(entry, now); - return entry.InFlight == 0 && entry.Failures == 0 && entry.LockedUntil == default; + return entry.InFlight == 0 && entry.Waiters.Count == 0 && entry.Failures == 0 && entry.LockedUntil == default; } - /// One client's failure count, in-flight reservations and lockout. Mutated only under the gate. + /// One activation waiting for a slot. + private sealed class Waiter + { + public readonly TaskCompletionSource Completion = + new(TaskCreationOptions.RunContinuationsAsynchronously); + + public LinkedListNode? Node; + } + + /// One client's failure count, slots, waiters and lockout. Mutated only under the gate. private sealed class Entry { public int Failures; @@ -313,5 +409,6 @@ public sealed class FailedCredentialLockout public DateTimeOffset LastFailure; public DateTimeOffset LockedUntil; public bool RefusalLogged; + public readonly LinkedList Waiters = new(); } } diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.Host.IntegrationTests/OpcUaApplicationHostOptionsValidatorTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.Host.IntegrationTests/OpcUaApplicationHostOptionsValidatorTests.cs index a7484189b..ead7d38cb 100644 --- a/tests/Server/ZB.MOM.WW.OtOpcUa.Host.IntegrationTests/OpcUaApplicationHostOptionsValidatorTests.cs +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.Host.IntegrationTests/OpcUaApplicationHostOptionsValidatorTests.cs @@ -79,4 +79,17 @@ public sealed class OpcUaApplicationHostOptionsValidatorTests result.Succeeded.ShouldBe(valid); if (!valid) result.Failures!.ShouldContain(f => f.Contains("OpcUa:CredentialLockout:LockoutSeconds")); } + + /// #833 review: the slot wait must be positive while the lockout is enabled. + [Fact] + public void Credential_lockout_slot_wait_must_be_positive_while_enabled() + { + var result = Sut.Validate(null, new OpcUaApplicationHostOptions + { + CredentialLockout = new ZB.MOM.WW.OtOpcUa.OpcUaServer.Security.OpcUaCredentialLockoutOptions { SlotWaitSeconds = 0 }, + }); + + result.Failed.ShouldBeTrue(); + result.Failures!.ShouldContain(f => f.Contains("OpcUa:CredentialLockout:SlotWaitSeconds")); + } } diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.IntegrationTests/CredentialLockoutWireTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.IntegrationTests/CredentialLockoutWireTests.cs index 9519f4447..342c67c33 100644 --- a/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.IntegrationTests/CredentialLockoutWireTests.cs +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.IntegrationTests/CredentialLockoutWireTests.cs @@ -59,6 +59,29 @@ public sealed class CredentialLockoutWireTests auth.UserNameCalls.ShouldBe(callsBefore, "a locked-out client's credential is refused without being checked"); } + /// + /// #833 review — one client opening more keyed sessions at once than MaxFailedAttempts (a ScadaBridge site + /// node behind one NAT IP) is never refused for parallelism: the extra activations wait for a slot. + /// + [Fact] + public async Task Parallel_valid_activations_beyond_the_threshold_all_succeed() + { + var ct = TestContext.Current.CancellationToken; + var auth = new CountingAuthenticator(); + await using var rig = await Rig.StartAsync(auth, new OpcUaCredentialLockoutOptions { MaxFailedAttempts = 2 }, ct); + + var sessions = await Task.WhenAll(Enumerable.Range(0, 8).Select(_ => rig.ConnectAsync(GoodKey, ct))); + try + { + sessions.ShouldAllBe(s => s.Connected); + auth.UserNameCalls.ShouldBe(8); + } + finally + { + foreach (var s in sessions) s.Dispose(); + } + } + [Fact] public async Task A_disabled_lockout_admits_a_valid_key_after_any_number_of_failures() { diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/FailedCredentialLockoutTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/FailedCredentialLockoutTests.cs index 077d77bab..1bd619e8f 100644 --- a/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/FailedCredentialLockoutTests.cs +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/FailedCredentialLockoutTests.cs @@ -102,52 +102,166 @@ public sealed class FailedCredentialLockoutTests lockout.TrackedCount.ShouldBe(1, "expired entries are pruned to make room"); } + private static CancellationToken Ct => TestContext.Current.CancellationToken; + /// - /// #833 review — the authenticate call is awaited between the check and the outcome. A burst of parallel - /// activations that all check before any failure is recorded must still be held to the threshold: the check - /// reserves an in-flight slot, so only MaxFailedAttempts may be in progress at once. + /// #833 review — a legitimate client opening many keyed sessions at once (a ScadaBridge site node behind one + /// NAT IP) must never be refused for parallelism: at most MaxFailedAttempts verify concurrently, the rest + /// WAIT for a slot, and all 20 succeed. /// [Fact] - public async Task A_parallel_burst_is_held_to_the_threshold_before_any_failure_is_recorded() + public async Task An_all_valid_parallel_burst_queues_for_slots_and_every_activation_succeeds() { - var lockout = new FailedCredentialLockout(new OpcUaCredentialLockoutOptions { MaxFailedAttempts = 3 }, new ManualClock()); - const int burst = 20; - using var allChecked = new CountdownEvent(burst); - var release = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); - var admitted = 0; + var lockout = new FailedCredentialLockout(new OpcUaCredentialLockoutOptions { MaxFailedAttempts = 5 }, new ManualClock()); + var concurrent = 0; + var maxConcurrent = 0; + var proceeded = 0; - var attempts = Enumerable.Range(0, burst).Select(attempt => Task.Run(async () => + await Task.WhenAll(Enumerable.Range(0, 20).Select(attempt => Task.Run(async () => { - var ok = lockout.TryBegin(Client, out _, out _, out _); - if (ok) Interlocked.Increment(ref admitted); - allChecked.Signal(); - await release.Task; // every attempt checks before any outcome lands — the race window - if (ok) lockout.Complete(Client, succeeded: false); - }, TestContext.Current.CancellationToken)).ToArray(); + var decision = await lockout.BeginAsync(Client, Ct); + decision.Verdict.ShouldBe(CredentialLockoutVerdict.Proceed); + Interlocked.Increment(ref proceeded); + var now = Interlocked.Increment(ref concurrent); + InterlockedMax(ref maxConcurrent, now); + await Task.Delay(20, Ct); // the authenticate call + Interlocked.Decrement(ref concurrent); + lockout.Complete(Client, succeeded: true); + }, Ct))); - allChecked.Wait(TimeSpan.FromSeconds(10), TestContext.Current.CancellationToken).ShouldBeTrue(); + proceeded.ShouldBe(20); + maxConcurrent.ShouldBe(5, "exactly MaxFailedAttempts verify at once — no more, and the cap is actually reached"); + lockout.TrackedCount.ShouldBe(0, "nothing left behind once the burst drains"); + } + + /// + /// #833 review — the security half: a parallel burst of guesses that all arrive before any failure is + /// recorded still reaches the authenticator only MaxFailedAttempts times; once those fail, every waiter is + /// refused as locked out without being authenticated. + /// + [Fact] + public async Task An_all_bad_parallel_burst_authenticates_only_max_attempts_and_refuses_the_waiters_as_locked_out() + { + var lockout = new FailedCredentialLockout(new OpcUaCredentialLockoutOptions { MaxFailedAttempts = 5 }, new ManualClock()); + var release = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + var authenticated = 0; + var lockedOut = 0; + + var attempts = Enumerable.Range(0, 20).Select(attempt => Task.Run(async () => + { + var decision = await lockout.BeginAsync(Client, Ct); + if (decision.Verdict == CredentialLockoutVerdict.LockedOut) + { + Interlocked.Increment(ref lockedOut); + return; + } + + decision.Verdict.ShouldBe(CredentialLockoutVerdict.Proceed); + Interlocked.Increment(ref authenticated); + await release.Task; // hold the slots until all 15 others are queued behind them + lockout.Complete(Client, succeeded: false); + }, Ct)).ToArray(); + + await WaitUntilAsync(() => lockout.WaitingCount(Client) == 15); + authenticated.ShouldBe(5); release.SetResult(); await Task.WhenAll(attempts); - admitted.ShouldBe(3, "only MaxFailedAttempts activations may be checked at once"); - lockout.IsLockedOut(Client, out _).ShouldBeTrue("the three failures lock the client"); + authenticated.ShouldBe(5, "no waiter may reach the authenticator once the key locks out"); + lockedOut.ShouldBe(15); + lockout.IsLockedOut(Client, out _).ShouldBeTrue(); + lockout.WaitingCount(Client).ShouldBe(0); + } + + /// After a recorded failure the client has one slot fewer: capacity is MaxFailedAttempts − failures. + [Fact] + public async Task Recorded_failures_shrink_the_number_of_concurrent_verifications() + { + var lockout = new FailedCredentialLockout(new OpcUaCredentialLockoutOptions { MaxFailedAttempts = 3 }, new ManualClock()); + lockout.RecordFailure(Client); + lockout.RecordFailure(Client); + + (await lockout.BeginAsync(Client, Ct)).Verdict.ShouldBe(CredentialLockoutVerdict.Proceed); + var second = lockout.BeginAsync(Client, Ct).AsTask(); + await WaitUntilAsync(() => lockout.WaitingCount(Client) == 1); + second.IsCompleted.ShouldBeFalse("one failure short of the threshold leaves exactly one slot"); + + lockout.Complete(Client, succeeded: true); // a success clears the count and hands the slot on + (await second).Verdict.ShouldBe(CredentialLockoutVerdict.Proceed); + lockout.Complete(Client, succeeded: true); + lockout.TrackedCount.ShouldBe(0); + } + + /// A cancelled waiter is dequeued and leaves nothing behind (no leaked slot, entry prunable). + [Fact] + public async Task A_cancelled_waiter_is_removed_and_leaks_nothing() + { + var lockout = new FailedCredentialLockout(new OpcUaCredentialLockoutOptions { MaxFailedAttempts = 1 }, new ManualClock()); + (await lockout.BeginAsync(Client, Ct)).Verdict.ShouldBe(CredentialLockoutVerdict.Proceed); + + using var cts = CancellationTokenSource.CreateLinkedTokenSource(Ct); + var waiting = lockout.BeginAsync(Client, cts.Token).AsTask(); + await WaitUntilAsync(() => lockout.WaitingCount(Client) == 1); + cts.Cancel(); + await Should.ThrowAsync(waiting); + lockout.WaitingCount(Client).ShouldBe(0); + + lockout.Complete(Client, succeeded: true); + lockout.TrackedCount.ShouldBe(0, "the held slot and the cancelled waiter are both gone"); + (await lockout.BeginAsync(Client, Ct)).Verdict.ShouldBe(CredentialLockoutVerdict.Proceed); + lockout.Abandon(Client); + lockout.TrackedCount.ShouldBe(0); + } + + /// A waiter that never gets a slot times out, uncounted — the client is not one failure closer. + [Fact] + public async Task A_slot_wait_timeout_fails_that_attempt_without_counting_it() + { + var lockout = new FailedCredentialLockout( + new OpcUaCredentialLockoutOptions { MaxFailedAttempts = 1, SlotWaitSeconds = 1 }, new ManualClock()); + (await lockout.BeginAsync(Client, Ct)).Verdict.ShouldBe(CredentialLockoutVerdict.Proceed); + + (await lockout.BeginAsync(Client, Ct)).Verdict.ShouldBe(CredentialLockoutVerdict.SlotWaitTimedOut); + lockout.WaitingCount(Client).ShouldBe(0); + + lockout.Complete(Client, succeeded: true); + lockout.TrackedCount.ShouldBe(0, "the timed-out attempt recorded no failure"); } [Fact] - public void In_flight_attempts_are_refused_as_TooManyInFlight_and_released_slots_free_up() + public async Task Only_the_first_refusal_of_a_lockout_episode_is_flagged_for_Information_logging() { - var lockout = new FailedCredentialLockout(new OpcUaCredentialLockoutOptions { MaxFailedAttempts = 2 }, new ManualClock()); - lockout.TryBegin(Client, out _, out _, out _).ShouldBeTrue(); - lockout.TryBegin(Client, out _, out _, out _).ShouldBeTrue(); - lockout.TryBegin(Client, out var refusal, out _, out var first).ShouldBeFalse(); - refusal.ShouldBe(CredentialLockoutRefusal.TooManyInFlight); - first.ShouldBeTrue(); + var clock = new ManualClock(); + var lockout = new FailedCredentialLockout(new OpcUaCredentialLockoutOptions { MaxFailedAttempts = 1, LockoutSeconds = 10 }, clock); + lockout.RecordFailure(Client).ShouldBeTrue(); - lockout.Abandon(Client); // a cancelled activation frees its slot, uncounted - lockout.Complete(Client, succeeded: true); // a success clears the count - lockout.TryBegin(Client, out _, out _, out _).ShouldBeTrue(); - lockout.Complete(Client, succeeded: true); - lockout.TrackedCount.ShouldBe(0); + var r1 = await lockout.BeginAsync(Client, Ct); + r1.Verdict.ShouldBe(CredentialLockoutVerdict.LockedOut); + r1.FirstRefusal.ShouldBeTrue(); + (await lockout.BeginAsync(Client, Ct)).FirstRefusal.ShouldBeFalse(); + + clock.Advance(TimeSpan.FromSeconds(11)); + (await lockout.BeginAsync(Client, Ct)).Verdict.ShouldBe(CredentialLockoutVerdict.Proceed); + lockout.Complete(Client, succeeded: false).ShouldBeTrue("a new episode"); + (await lockout.BeginAsync(Client, Ct)).FirstRefusal.ShouldBeTrue("each lockout episode logs its first refusal once"); + } + + private static void InterlockedMax(ref int target, int value) + { + int seen; + while ((seen = Volatile.Read(ref target)) < value && Interlocked.CompareExchange(ref target, value, seen) != seen) + { + } + } + + private static async Task WaitUntilAsync(Func condition) + { + var deadline = DateTime.UtcNow + TimeSpan.FromSeconds(10); + while (!condition()) + { + DateTime.UtcNow.ShouldBeLessThan(deadline, "condition not reached in time"); + await Task.Delay(5, Ct); + } } /// @@ -170,26 +284,6 @@ public sealed class FailedCredentialLockoutTests lockout.RecordFailure(Client).ShouldBeTrue(); } - [Fact] - public void Only_the_first_refusal_of_a_lockout_episode_is_flagged_for_Information_logging() - { - var clock = new ManualClock(); - var lockout = new FailedCredentialLockout(new OpcUaCredentialLockoutOptions { MaxFailedAttempts = 1, LockoutSeconds = 10 }, clock); - lockout.RecordFailure(Client).ShouldBeTrue(); - - lockout.TryBegin(Client, out var r1, out _, out var first1).ShouldBeFalse(); - r1.ShouldBe(CredentialLockoutRefusal.LockedOut); - first1.ShouldBeTrue(); - lockout.TryBegin(Client, out _, out _, out var first2).ShouldBeFalse(); - first2.ShouldBeFalse(); - - clock.Advance(TimeSpan.FromSeconds(11)); - lockout.TryBegin(Client, out _, out _, out _).ShouldBeTrue(); - lockout.Complete(Client, succeeded: false).ShouldBeTrue("a new episode"); - lockout.TryBegin(Client, out _, out _, out var first3).ShouldBeFalse(); - first3.ShouldBeTrue("each lockout episode logs its first refusal once"); - } - [Fact] public void The_host_switches_the_sdk_lockout_off() {