diff --git a/.claude/rules/data-plane-auth.md b/.claude/rules/data-plane-auth.md index 3f65d65e1..63579321b 100644 --- a/.claude/rules/data-plane-auth.md +++ b/.claude/rules/data-plane-auth.md @@ -211,7 +211,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. `docs/security.md` §"Failed-credential lockout". + 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 + `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 the `ApiKey` rows — the peppered HMAC only, never a secret, and the pepper is node config that is deliberately *not* in the artifact — and `DeploymentArtifact.ParseDataPlaneAuthority` decodes them diff --git a/docs/Configuration.md b/docs/Configuration.md index 6b17cef46..b2e887300 100644 --- a/docs/Configuration.md +++ b/docs/Configuration.md @@ -75,8 +75,8 @@ Until #678.6 the overlay never loaded on a node carrying a `cluster-*` role (the | `AutoAcceptUntrustedClientCertificates` | bool | `false` | Auto-trust unknown client certs on first connect (dev convenience). Not validated. See [`security.md`](security.md). | | `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. | -| `CredentialLockout:LockoutSeconds` | int | `300` | How long a locked-out client's UserName activations are refused. | +| `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: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 74ab377c0..50d7ab563 100644 --- a/docs/security.md +++ b/docs/security.md @@ -334,10 +334,23 @@ The host now sets `MaxFailedAuthenticationAttempts = 0` on every configuration i - **UserName activations only.** A failed one counts; at `MaxFailedAttempts` (5) within `FailureWindowSeconds` (60) of each other, that client's **UserName** activations are refused `BadUserAccessDenied` for `LockoutSeconds` (300) without the key store being consulted, and a Warning is - logged once. A refusal during the lockout is not counted (it never extends itself); a successful UserName - activation clears the count. **Anonymous activations never consult it**, from any client. -- **Keyed on certificate thumbprint *and* peer address together**, so locking a client out needs both its - certificate and its source address, and NAT neighbours with different certificates do not share a count. + logged once. A refusal during the lockout is not counted (it never extends itself); an **expired lockout + 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). +- **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. - 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 789afca9b..c33afe3a3 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.Host/Configuration/OpcUaApplicationHostOptionsValidator.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.Host/Configuration/OpcUaApplicationHostOptionsValidator.cs @@ -29,5 +29,14 @@ public sealed class OpcUaApplicationHostOptionsValidator : OptionsValidatorBase< // IReadOnlyCollection, so it can't bind to MinCount's IReadOnlyCollection parameter // directly. ToList() bridges to the shared primitive while preserving the count (and message). builder.MinCount(o.EnabledSecurityProfiles?.ToList(), 1, "OpcUa:EnabledSecurityProfiles"); + + // #833 review: an enabled credential lockout needs positive durations — a zero LockoutSeconds would lock + // 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 " + + "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 c5b049966..62402e731 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/OpcUaApplicationHost.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/OpcUaApplicationHost.cs @@ -275,40 +275,62 @@ public sealed class OpcUaApplicationHost : IAsyncDisposable var lockoutKey = tokenType == UserTokenType.UserName ? FailedCredentialLockout.KeyFor(context.ChannelCertificate?.Thumbprint, peer) : null; - if (lockoutKey is not null && host._credentialLockout.IsLockedOut(lockoutKey, out var remaining)) + if (lockoutKey is not null + && !host._credentialLockout.TryBegin(lockoutKey, out var refusal, out var remaining, out var firstRefusal)) { - var seconds = (long)Math.Ceiling(remaining.TotalSeconds); - host._logger.LogInformation( - "OpcUaApplicationHost: UserName activation refused — client {Client} is locked out for {Seconds}s more", - lockoutKey, seconds); + // 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) + { + var seconds = (long)Math.Ceiling(remaining.TotalSeconds); + host._logger.Log(level, + "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 failed authentication attempts. Try again in {seconds} seconds.")); + "Too many authentication attempts in progress. Try again shortly.")); } - // Port zero is only an adapter for the existing IP-budget seam: the SDK exposes no peer port. - await HandleImpersonationAsync(host._userAuthenticator, args, host._logger, host._options.AllowAnonymous, - peer is null ? null : new System.Net.IPEndPoint(peer, 0), ct).ConfigureAwait(false); - var result = ServiceResult.IsBad(args.IdentityValidationError) - ? AuthenticationResult.Reject(args.IdentityValidationError) - : args.Identity is { } identity - ? AuthenticationResult.Accept(identity) - : AuthenticationResult.Reject(new ServiceResult(StatusCodes.BadIdentityTokenRejected)); - - if (lockoutKey is not null) + // The slot TryBegin reserved is released on every path — completed with the outcome, or abandoned + // (uncounted) when the activation was cancelled or threw. + var completed = false; + try { - if (args.Identity is not null && !ServiceResult.IsBad(args.IdentityValidationError)) + // Port zero is only an adapter for the existing IP-budget seam: the SDK exposes no peer port. + await HandleImpersonationAsync(host._userAuthenticator, args, host._logger, host._options.AllowAnonymous, + peer is null ? null : new System.Net.IPEndPoint(peer, 0), ct).ConfigureAwait(false); + var result = ServiceResult.IsBad(args.IdentityValidationError) + ? AuthenticationResult.Reject(args.IdentityValidationError) + : args.Identity is { } identity + ? AuthenticationResult.Accept(identity) + : AuthenticationResult.Reject(new ServiceResult(StatusCodes.BadIdentityTokenRejected)); + + if (lockoutKey is not null) { - host._credentialLockout.RecordSuccess(lockoutKey); - } - else if (host._credentialLockout.RecordFailure(lockoutKey)) - { - host._logger.LogWarning( - "OpcUaApplicationHost: client {Client} locked out of UserName activation for {Seconds}s after " - + "{Attempts} failed attempts (OpcUa:CredentialLockout); anonymous activation is unaffected", - lockoutKey, host._options.CredentialLockout.LockoutSeconds, host._options.CredentialLockout.MaxFailedAttempts); + var admitted = args.Identity is not null && !ServiceResult.IsBad(args.IdentityValidationError); + completed = true; + if (host._credentialLockout.Complete(lockoutKey, admitted)) + { + host._logger.LogWarning( + "OpcUaApplicationHost: client {Client} locked out of UserName activation for {Seconds}s after " + + "{Attempts} failed attempts (OpcUa:CredentialLockout); anonymous activation is unaffected", + lockoutKey, host._options.CredentialLockout.LockoutSeconds, host._options.CredentialLockout.MaxFailedAttempts); + } } + return result; + } + finally + { + if (lockoutKey is not null && !completed) host._credentialLockout.Abandon(lockoutKey); } - return result; } } 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 cecc28ae1..5a3d27304 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/Security/FailedCredentialLockout.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/Security/FailedCredentialLockout.cs @@ -6,7 +6,9 @@ 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. +/// 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 +/// OpcUaApplicationHostOptionsValidator. /// public sealed class OpcUaCredentialLockoutOptions { @@ -20,6 +22,22 @@ public sealed class OpcUaCredentialLockoutOptions 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. + /// + TooManyInFlight, +} + /// /// Brute-force lockout for UserName (API-key) activations, scoped so it can never refuse an /// anonymous activation (lmxopcua#833). @@ -38,18 +56,28 @@ public sealed class OpcUaCredentialLockoutOptions /// MaxFailedAuthenticationAttempts = 0 and enforces this narrower lockout instead. /// /// -/// What it does. Only UserName activations are counted and refused. The key is the presented -/// client certificate thumbprint and the observed peer address together, so locking a victim -/// out needs both its certificate and its source address. Same timing as the SDK: a failure within +/// What it does. Only UserName activations are counted and refused; Anonymous never consults it. +/// The key is the presented client certificate thumbprint and the observed peer address together. +/// ⚠️ That only separates clients that present a certificate: on a None endpoint clients commonly +/// present none, so the key is effectively the IP alone and NAT neighbours there share one count +/// (for UserName activations only — never for anonymous ones); and when the transport reports no peer +/// address, every certificate-less client shares the single key -@-. Timing: a failure within /// of the previous one adds to the /// count; at the client is refused for /// ; a refusal during the lockout is not -/// itself counted (the lockout never extends itself); a successful UserName activation clears the -/// client's count. +/// counted (the lockout never extends itself); an expired lockout starts a fresh count (whatever the +/// window); a successful UserName activation clears the client's count. /// /// -/// Memory is bounded at : expired 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 +/// 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. +/// +/// +/// 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). /// /// @@ -82,10 +110,93 @@ public sealed class FailedCredentialLockout public static string KeyFor(string? certificateThumbprint, System.Net.IPAddress? peer) => $"{(string.IsNullOrEmpty(certificateThumbprint) ? "-" : certificateThumbprint)}@{peer?.ToString() ?? "-"}"; - /// Whether is locked out right now. + /// + /// Checks and, when it may proceed, reserves an in-flight slot that the caller MUST + /// release with (only when this returns ). + /// + /// 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) + { + refusal = CredentialLockoutRefusal.None; + remaining = TimeSpan.Zero; + firstRefusal = false; + if (!Enabled) return true; + var now = _time.GetUtcNow(); + lock (_gate) + { + if (!_entries.TryGetValue(key, out var entry)) + { + if (_entries.Count >= MaxTrackedClients) Prune(now); + if (_entries.Count >= MaxTrackedClients) return true; // untracked: nothing to reserve + entry = new Entry(); + _entries[key] = entry; + } + + Refresh(entry, now); + if (entry.LockedUntil > now) + { + refusal = CredentialLockoutRefusal.LockedOut; + remaining = entry.LockedUntil - now; + firstRefusal = !entry.RefusalLogged; + entry.RefusalLogged = true; + return false; + } + + if (entry.Failures + entry.InFlight >= _options.MaxFailedAttempts) + { + refusal = CredentialLockoutRefusal.TooManyInFlight; + firstRefusal = !entry.RefusalLogged; + entry.RefusalLogged = true; + return false; + } + + entry.InFlight++; + return true; + } + } + + /// Releases the slot reserved 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); + } + } + + /// Releases a reserved 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); + } + } + } + + /// Whether is locked out right now (observation; reserves nothing). /// The client key from . /// The time left on the lockout when this returns true. - /// when the client's UserName activations must be refused. + /// when the client's UserName activations are refused. public bool IsLockedOut(string key, out TimeSpan remaining) { remaining = TimeSpan.Zero; @@ -94,17 +205,14 @@ public sealed class FailedCredentialLockout lock (_gate) { if (!_entries.TryGetValue(key, out var entry)) return false; - if (entry.LockedUntil > now) - { - remaining = entry.LockedUntil - now; - return true; - } - if (IsExpired(entry, now)) _entries.Remove(key); - return false; + Refresh(entry, now); + if (entry.LockedUntil <= now) return false; + remaining = entry.LockedUntil - now; + return true; } } - /// Records one failed UserName activation. + /// Records one failed UserName activation with no reservation (test seam / direct callers). /// The client key. /// when this failure locked the client out. public bool RecordFailure(string key) @@ -113,30 +221,25 @@ public sealed class FailedCredentialLockout var now = _time.GetUtcNow(); lock (_gate) { - if (_entries.TryGetValue(key, out var entry)) - { - if (entry.LockedUntil > now) return false; // a refusal never extends the lockout - entry = IsExpired(entry, now) ? new Entry(1, now, default) : entry with { Failures = entry.Failures + 1, LastFailure = now }; - } - else + if (!_entries.TryGetValue(key, out var entry)) { if (_entries.Count >= MaxTrackedClients) Prune(now); if (_entries.Count >= MaxTrackedClients) return false; - entry = new Entry(1, now, default); + entry = new Entry(); + _entries[key] = entry; } - - if (entry.Failures >= _options.MaxFailedAttempts) - entry = entry with { LockedUntil = now + TimeSpan.FromSeconds(Math.Max(0, _options.LockoutSeconds)) }; - _entries[key] = entry; - return entry.LockedUntil > now; + return RecordFailureCore(entry, now); } } - /// Clears a client's count after a successful UserName activation. + /// Clears a client's count after a successful UserName activation (no reservation). /// The client key. public void RecordSuccess(string key) { - lock (_gate) _entries.Remove(key); + lock (_gate) + { + if (_entries.TryGetValue(key, out var entry)) RecordSuccessCore(key, entry); + } } /// How many clients are tracked (test observation). @@ -145,15 +248,70 @@ public sealed class FailedCredentialLockout get { lock (_gate) return _entries.Count; } } - private bool IsExpired(Entry entry, DateTimeOffset now) => - entry.LockedUntil <= now && now - entry.LastFailure > TimeSpan.FromSeconds(Math.Max(0, _options.FailureWindowSeconds)); + private bool RecordSuccessCore(string key, Entry entry) + { + if (entry.InFlight == 0) _entries.Remove(key); + else + { + entry.Failures = 0; + entry.LockedUntil = default; + entry.RefusalLogged = false; + } + return false; + } + + private bool RecordFailureCore(Entry entry, DateTimeOffset now) + { + Refresh(entry, now); + if (entry.LockedUntil > now) return false; // a refusal (or a late in-flight failure) never extends it + entry.Failures++; + entry.LastFailure = now; + if (entry.Failures < _options.MaxFailedAttempts) return false; + entry.LockedUntil = now + TimeSpan.FromSeconds(Math.Max(0, _options.LockoutSeconds)); + entry.RefusalLogged = false; + return entry.LockedUntil > now; + } + + /// + /// Forgets a run of failures that has gone stale: an expired lockout always starts a fresh count (else a + /// LockoutSeconds shorter than FailureWindowSeconds relocked on the first failure after it), + /// and so does a failure older than the window. + /// + private void Refresh(Entry entry, DateTimeOffset now) + { + if (entry.LockedUntil != default && entry.LockedUntil <= now) + { + entry.Failures = 0; + entry.LockedUntil = default; + entry.RefusalLogged = false; + } + else if (entry.LockedUntil == default && entry.Failures > 0 + && now - entry.LastFailure > TimeSpan.FromSeconds(Math.Max(0, _options.FailureWindowSeconds))) + { + entry.Failures = 0; + entry.RefusalLogged = false; + } + } private void Prune(DateTimeOffset now) { - foreach (var key in _entries.Where(e => IsExpired(e.Value, now)).Select(e => e.Key).ToList()) + foreach (var key in _entries.Where(e => IsIdle(e.Value, now)).Select(e => e.Key).ToList()) _entries.Remove(key); } - /// One client's failure count and lockout. - private readonly record struct Entry(int Failures, DateTimeOffset LastFailure, DateTimeOffset LockedUntil); + private bool IsIdle(Entry entry, DateTimeOffset now) + { + Refresh(entry, now); + return entry.InFlight == 0 && entry.Failures == 0 && entry.LockedUntil == default; + } + + /// One client's failure count, in-flight reservations and lockout. Mutated only under the gate. + private sealed class Entry + { + public int Failures; + public int InFlight; + public DateTimeOffset LastFailure; + public DateTimeOffset LockedUntil; + public bool RefusalLogged; + } } 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 15c620202..a7484189b 100644 --- a/tests/Server/ZB.MOM.WW.OtOpcUa.Host.IntegrationTests/OpcUaApplicationHostOptionsValidatorTests.cs +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.Host.IntegrationTests/OpcUaApplicationHostOptionsValidatorTests.cs @@ -56,4 +56,27 @@ public sealed class OpcUaApplicationHostOptionsValidatorTests result.Failed.ShouldBeTrue(); result.Failures.ShouldContain("OpcUa:EnabledSecurityProfiles must contain at least 1 item(s) (had 0)"); } + + /// #833 review: an enabled lockout with a non-positive duration fails; a disabled one is not checked. + [Theory] + [InlineData(5, 0, 60, false)] + [InlineData(5, 300, 0, false)] + [InlineData(5, -1, -1, false)] + [InlineData(5, 30, 600, true)] + [InlineData(0, 0, 0, true)] + [InlineData(-1, 0, 0, true)] + public void Credential_lockout_durations_must_be_positive_while_enabled( + int maxFailedAttempts, int lockoutSeconds, int windowSeconds, bool valid) + { + var result = Sut.Validate(null, new OpcUaApplicationHostOptions + { + CredentialLockout = new ZB.MOM.WW.OtOpcUa.OpcUaServer.Security.OpcUaCredentialLockoutOptions + { + MaxFailedAttempts = maxFailedAttempts, LockoutSeconds = lockoutSeconds, FailureWindowSeconds = windowSeconds, + }, + }); + + result.Succeeded.ShouldBe(valid); + if (!valid) result.Failures!.ShouldContain(f => f.Contains("OpcUa:CredentialLockout:LockoutSeconds")); + } } 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 723c2cf25..077d77bab 100644 --- a/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/FailedCredentialLockoutTests.cs +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/FailedCredentialLockoutTests.cs @@ -102,6 +102,94 @@ public sealed class FailedCredentialLockoutTests lockout.TrackedCount.ShouldBe(1, "expired entries are pruned to make room"); } + /// + /// #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. + /// + [Fact] + public async Task A_parallel_burst_is_held_to_the_threshold_before_any_failure_is_recorded() + { + 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 attempts = Enumerable.Range(0, burst).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(); + + allChecked.Wait(TimeSpan.FromSeconds(10), TestContext.Current.CancellationToken).ShouldBeTrue(); + 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"); + } + + [Fact] + public void In_flight_attempts_are_refused_as_TooManyInFlight_and_released_slots_free_up() + { + 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(); + + 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); + } + + /// + /// #833 review — with LockoutSeconds shorter than FailureWindowSeconds, the count survived the expired lockout + /// (the last failure was still inside the window), so the very next failure relocked the client. + /// + [Fact] + public void An_expired_lockout_starts_a_fresh_count_even_inside_the_failure_window() + { + var clock = new ManualClock(); + var lockout = new FailedCredentialLockout( + new OpcUaCredentialLockoutOptions { MaxFailedAttempts = 2, FailureWindowSeconds = 600, LockoutSeconds = 10 }, clock); + lockout.RecordFailure(Client).ShouldBeFalse(); + lockout.RecordFailure(Client).ShouldBeTrue(); + + clock.Advance(TimeSpan.FromSeconds(11)); + lockout.IsLockedOut(Client, out _).ShouldBeFalse(); + lockout.RecordFailure(Client).ShouldBeFalse("one failure after the lockout is the first of a new run"); + lockout.IsLockedOut(Client, out _).ShouldBeFalse(); + 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() {