From ffba75bb93e27ef02a93ff7af8529f380311e97d Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Wed, 11 Nov 2026 03:47:10 -0500 Subject: [PATCH 1/2] fix(opcua): credential lockout counts per credential identity, ignores transient denials (#844, #845) #844: a success cleared the whole per-client count, so a client holding a valid key could make four guesses at another key id, sign in with its own, and repeat forever. Failures are now charged to the (client, credential identity) pair -- IOpcUaUserAuthenticator.DescribeCredential, key: for API keys -- and to the client as a whole. A success clears only its own identity's count; the per-identity threshold locks that identity; a new per-client cap (OpcUa:CredentialLockout:MaxFailedAttemptsPerClient, default 2 x MaxFailedAttempts) that a success never resets bounds spraying across key ids and locks every credential. Slots are bounded on both counts; a waiter whose identity has room is not held behind one whose identity has none. #845: (1) store timeouts, miss-budget refusals, store/authenticator exceptions, an unset pepper and cancellation are reported Transient (OpcUaUserAuthResult.Unavailable) and released uncounted, so a ConfigDb stall can no longer lock every keyed client out; the client still sees the same opaque denial. (2) a full 4096-entry table defers its next prune until the earliest entry could age out instead of rescanning under the lock per failure. (3) ApplicationUri deliberately NOT added to the key: on a None channel it is a free string, so it would let an attacker reset the count per guess -- documented. (4) wire tests: distinct client certificates on a Sign endpoint are locked out separately; the ApplicationConfigPath branch keeps the SDK lockout off (falsified by removing the loaded-branch disable). --- .claude/rules/data-plane-auth.md | 8 +- docs/Configuration.md | 5 +- docs/security.md | 91 ++- .../OpcUaApplicationHostOptionsValidator.cs | 7 + .../OpcUa/ApiKeyOpcUaUserAuthenticator.cs | 14 +- .../OpcUaApplicationHost.cs | 83 ++- .../Security/FailedCredentialLockout.cs | 539 ++++++++++++++---- .../Security/IOpcUaUserAuthenticator.cs | 29 + ...cUaApplicationHostOptionsValidatorTests.cs | 20 + .../OpcUa/ApiKeyAuthenticationTests.cs | 40 +- .../CredentialLockoutWireTests.cs | 254 +++++++-- .../FailedCredentialLockoutTests.cs | 224 +++++++- .../OpcUaApplicationHostImpersonationTests.cs | 44 +- 13 files changed, 1150 insertions(+), 208 deletions(-) diff --git a/.claude/rules/data-plane-auth.md b/.claude/rules/data-plane-auth.md index d59a8ef0a..4b80ca976 100644 --- a/.claude/rules/data-plane-auth.md +++ b/.claude/rules/data-plane-auth.md @@ -244,7 +244,13 @@ Two things to know before relying on any of this: 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` + `None` endpoints the key is effectively IP-only (NAT neighbours share it, UserName only). ⚠️ **Two counts (#844):** + per (client, credential identity = `IOpcUaUserAuthenticator.DescribeCredential`, `key:` / `user:`), + cleared by a success with THAT identity, locks that identity; per client (`MaxFailedAttemptsPerClient`, default + 2 × max) — **never reset by a success** (else a valid key launders guesses at another key id) — locks every + credential. ⚠️ **Only definite bad credentials count (#845):** store timeout / miss-budget / store throw / unset + pepper / authenticator throw / cancel ⇒ `OpcUaUserAuthResult.Unavailable` (`Transient`) ⇒ `Abandon`, uncounted. + ApplicationUri is deliberately NOT in the key (free string on `None` ⇒ trivial evasion). `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 diff --git a/docs/Configuration.md b/docs/Configuration.md index 23f2e0e55..f14c22f94 100644 --- a/docs/Configuration.md +++ b/docs/Configuration.md @@ -74,9 +74,10 @@ Until #678.6 the overlay never loaded on a node carrying a `cluster-*` role (the | `EnabledSecurityProfiles` | list of `OpcUaSecurityProfile` | `[None, Basic256Sha256Sign, Basic256Sha256SignAndEncrypt]` | Transport-security profiles, one endpoint per entry. Must contain ≥1. Profile detail in [`security.md`](security.md). | | `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:MaxFailedAttempts` | int | `5` | Failed UserName (API-key) activations for ONE credential identity (key id, else username) from one client before that client is refused that identity `BadUserAccessDenied` (#833, per-identity since #844 — a success clears only its own identity's count). Only definite bad credentials count; store timeouts / miss-budget refusals / authenticator faults do not (#845). `≤ 0` disables the whole lockout. **Never refuses Anonymous.** See [`security.md`](security.md#failed-credential-lockout-opcuacredentiallockout). | +| `CredentialLockout:MaxFailedAttemptsPerClient` | int | `0` (⇒ `2 × MaxFailedAttempts`) | Failed UserName activations from one client across EVERY identity before all its UserName activations are refused (#844). **A success never resets it**; it ages out through `FailureWindowSeconds`. Must be `0` or `>= MaxFailedAttempts` (validated). | | `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:SlotWaitSeconds` | int | `10` | How long a UserName activation waits for one of its client's verification slots (at most `MaxFailedAttempts − failures` per identity, and `MaxFailedAttemptsPerClient − failures` per client, 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 b2fa3e8e4..9db3f91ea 100644 --- a/docs/security.md +++ b/docs/security.md @@ -331,37 +331,74 @@ The host now sets `MaxFailedAuthenticationAttempts = 0` on every configuration i (`OpcUaApplicationHost.DisableSdkAuthenticationLockout`) and enforces a narrower replacement, `FailedCredentialLockout`, in its identity-registry adapter: -- **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); 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 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. +- **UserName activations only.** **Anonymous activations never consult it**, from any client. +- **Two counts per client (#844).** Each *definite* failure is charged to the (client, **credential identity**) + pair and to the client as a whole. The credential identity is `IOpcUaUserAuthenticator.DescribeCredential` — + for API keys `key:` (the public lookup index, never the secret), for anything that is not a well-formed key + `user:`. + - **Per credential:** at `MaxFailedAttempts` (5) failures of one identity, each within `FailureWindowSeconds` (60) of + the previous, that client is refused **that identity** `BadUserAccessDenied` for `LockoutSeconds` (300), without + the key store being consulted. A success with **that same identity** clears its count. + - **Per client:** at `MaxFailedAttemptsPerClient` failures across **every** identity (default `0` ⇒ + `2 × MaxFailedAttempts` = 10), the client is refused **every** UserName activation for `LockoutSeconds`. + ⚠️ **A success never resets this count** — it only ages out through `FailureWindowSeconds` or an expired lockout. + - Why both: before #844 one count per client was cleared by *any* success, so a client holding a valid key could make + four guesses at another key id, sign in with its own key, and repeat forever. The per-credential count alone would + fix that but would let a client spray four guesses at each of unboundedly many key ids; the client-wide cap that a + success cannot reset bounds the spray (≤ 10 unverified guesses per window chain, then 5 minutes locked), while a + legitimate client with one stale/revoked key and one good key loses only the stale one. + - Timing per count: a refusal during a lockout is not counted (it never extends itself); an **expired lockout always + starts a fresh count**, whatever `FailureWindowSeconds` is (an expired client-wide lockout clears every identity's + count too). A Warning is logged when a count locks (naming the scope and identity); the first refusal of each + lockout episode is logged at Information, the rest at Debug. Identities per client are bounded at 64; further ones + share one overflow count (conservative — never laxer). +- **Only a definite bad credential counts (#845).** A wrong secret, an unknown/disabled/expired key id, a malformed + credential or the wrong username counts. A denial that says nothing about the credential does **not**: a key-store + timeout (`Security:ApiKeys:StoreTimeoutMs`), a miss-budget refusal, any other store exception, an unset pepper, an + authenticator that throws, or a cancelled activation. The authenticator reports those with + `OpcUaUserAuthResult.Unavailable(...)` (`Transient = true`) and the host releases the slot uncounted — so a ConfigDb + stall can no longer lock every keyed client out for 5 minutes. The client sees the same opaque + `BadIdentityTokenRejected` either way (no oracle). +- **Parallel activations wait — they are never refused for parallelism.** An activation gets a verification + **slot** only while `identityFailures + identityInFlight < MaxFailedAttempts` **and** + `clientFailures + clientInFlight < MaxFailedAttemptsPerClient`; beyond that it **queues** until one frees (a queued + activation whose identity has room is not held behind one whose identity has none). So no more unverified guesses + than either threshold 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 what it needs actually **locks out** (`BadUserAccessDenied`, without reaching + the authenticator), or after `SlotWaitSeconds` (default 10) with `BadTimeout` — **not** counted as a 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`.) + a certificate — on a `Sign`/`SignAndEncrypt` endpoint the certificate is proven, and two clients behind one address + with different certificates are locked out separately (pinned over the wire). ⚠️ On a `None` endpoint clients + commonly present none, so the key is **effectively the IP alone**: NAT neighbours there **do** share the client-wide + count (for UserName activations only — never for anonymous ones; the per-credential count is shared only by + neighbours presenting the same key id). 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`.) +- **The client `ApplicationUri` is deliberately NOT part of the key (#845, decided).** It would separate NAT + neighbours on a `None` endpoint, but there it is a free, unauthenticated string in `CreateSession` (and the + presented certificate is unproven), so an attacker would reset both counts simply by sending a new `ApplicationUri` + with every guess — turning the lockout off for exactly the client it exists to stop. Keying on it is safe only + where the certificate is proven, and there the thumbprint already separates clients. Operators who need NAT + neighbours isolated should enable a signed endpoint (each client then presents its own certificate), not widen the key. - **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 - tracked). API-key secrets are not guessable, so this is defence in depth, not the brute-force control. + `SlotWaitSeconds` must all be `> 0`, and `MaxFailedAttemptsPerClient` must be `0` or `>= MaxFailedAttempts` + (`OpcUaApplicationHostOptionsValidator`); `MaxFailedAttempts <= 0` disables the lockout. +- **Bounded at 4096 tracked clients.** Entries with nothing left to remember are pruned when the table fills; a client + arriving at a still-full table is not tracked. When a prune frees nothing, the next one is deferred until the earliest + entry could have aged out (#845) — a flood of new client keys at a full table used to rescan all 4096 entries under the + global lock on every failure. A release that frees an entry re-arms pruning immediately. API-key secrets are not + guessable, so this is defence in depth, not the brute-force control. +- **The SDK lockout is off on both configuration paths** — the configuration the host builds and one loaded from + `OpcUa:ApplicationConfigPath` (whose own `MaxFailedAuthenticationAttempts` is overwritten with `0`; pinned over the + wire with a file carrying `2`). Pinned over the wire by `CredentialLockoutWireTests` (OpcUaServer.IntegrationTests — anonymous admitted after -six failures; a valid key refused while locked out; `MaxFailedAttempts = 0` admits after seven failures, -proving the SDK lockout is off) and unit-tested by `FailedCredentialLockoutTests`. +twelve failures; a valid key refused while client-wide locked out; own-key successes do not launder guesses at another +key id; transient denials never lock; distinct client certificates on a `Sign` endpoint are locked out separately; the +`ApplicationConfigPath` branch keeps the SDK lockout off; `MaxFailedAttempts = 0` admits after seven failures, +proving the SDK lockout is off) and unit-tested by `FailedCredentialLockoutTests`, `OpcUaApplicationHostImpersonationTests` +and `ApiKeyAuthenticationTests` (Host.Tests). ### LDAP bind flow (`OtOpcUaLdapAuthService`) 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 c51ed0d40..50f16c71f 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.Host/Configuration/OpcUaApplicationHostOptionsValidator.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.Host/Configuration/OpcUaApplicationHostOptionsValidator.cs @@ -39,5 +39,12 @@ public sealed class OpcUaApplicationHostOptionsValidator : OptionsValidatorBase< + "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)"); + + // #844: the per-client cap (which a success never resets) below the per-credential threshold would make every + // per-credential lockout a client-wide one; 0 means 2 x MaxFailedAttempts. + builder.RequireThat(lockout is null || lockout.MaxFailedAttempts <= 0 || lockout.MaxFailedAttemptsPerClient <= 0 + || lockout.MaxFailedAttemptsPerClient >= lockout.MaxFailedAttempts, + "OpcUa:CredentialLockout:MaxFailedAttemptsPerClient must be 0 (= 2 x MaxFailedAttempts) or at least " + + "OpcUa:CredentialLockout:MaxFailedAttempts"); } } diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.Host/OpcUa/ApiKeyOpcUaUserAuthenticator.cs b/src/Server/ZB.MOM.WW.OtOpcUa.Host/OpcUa/ApiKeyOpcUaUserAuthenticator.cs index 78747f046..d264e0968 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.Host/OpcUa/ApiKeyOpcUaUserAuthenticator.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.Host/OpcUa/ApiKeyOpcUaUserAuthenticator.cs @@ -107,8 +107,11 @@ public sealed class ApiKeyOpcUaUserAuthenticator : IOpcUaUserAuthenticator { // The contract forbids throwing: an exception here escapes into the SDK's session-activation // path and surfaces as BadInternalError. A store outage denies, it does not crash. + // lmxopcua#845: a store timeout, miss-budget refusal or outage says nothing about the credential, so it + // is reported Transient and the failed-credential lockout does not count it (a ConfigDb stall must not + // lock every keyed client out). The client still sees the same opaque denial. _log.LogError(ex, "ApiKeyAuth: key store threw for key id {ApiKeyId}", apiKeyId); - return OpcUaUserAuthResult.Deny(DenyMessage); + return OpcUaUserAuthResult.Unavailable(DenyMessage); } if (lookup is null) @@ -129,7 +132,7 @@ public sealed class ApiKeyOpcUaUserAuthenticator : IOpcUaUserAuthenticator _log.LogError(ex, "ApiKeyAuth: {Section}:Pepper is not configured — every key will be rejected until it is", ApiKeyOptions.SectionName); - return OpcUaUserAuthResult.Deny(DenyMessage); + return OpcUaUserAuthResult.Unavailable(DenyMessage); // a deployment error, not a bad credential (#845) } if (!verified) @@ -161,6 +164,13 @@ public sealed class ApiKeyOpcUaUserAuthenticator : IOpcUaUserAuthenticator return OpcUaUserAuthResult.AllowKey(lookup.Key.Principal, roles, apiKeyId); } + /// + // lmxopcua#844 — the key id is public (it is the lookup index, never the secret), so guesses at different keys + // are counted apart and a client's own successful key cannot clear the count of a key it is guessing at. + // A credential that is not a well-formed key falls back to the username (all such guesses share one count). + public string DescribeCredential(string username, string password) => + ApiKeySecret.TryParse(password, out var apiKeyId, out _) ? "key:" + apiKeyId : "user:" + username; + /// // Resolves the anonymous principal's grants through the same mapper a key's principal // goes through, so anonymous is authorized by ordinary ACL rows rather than by a special case. diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/OpcUaApplicationHost.cs b/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/OpcUaApplicationHost.cs index 61f8db9ab..5d55c433b 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/OpcUaApplicationHost.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/OpcUaApplicationHost.cs @@ -272,22 +272,30 @@ public sealed class OpcUaApplicationHost : IAsyncDisposable // lmxopcua#833 — only credential-bearing (UserName) activations are counted and refused. Anonymous // never consults the lockout, so failed sign-ins cannot lock a client (or its NAT neighbours) out of // anonymous access the way the SDK's own lockout did. + // The ApplicationUri is deliberately NOT part of the key (lmxopcua#845): on a None channel it is a free, + // unauthenticated string, so an attacker could reset its count by varying it. var lockoutKey = tokenType == UserTokenType.UserName ? FailedCredentialLockout.KeyFor(context.ChannelCertificate?.Thumbprint, peer) : null; + // lmxopcua#844 — failures are counted per (client, credential identity) as well as per client, so a + // success clears only its own identity's count. + var credentialIdentity = lockoutKey is null + ? FailedCredentialLockout.NoIdentity + : DescribeCredential(host._userAuthenticator, args.UserIdentityTokenHandler); if (lockoutKey is not null) { // 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); + var decision = await host._credentialLockout.BeginAsync(lockoutKey, credentialIdentity, ct).ConfigureAwait(false); if (decision.Verdict == CredentialLockoutVerdict.LockedOut) { // 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); + "OpcUaApplicationHost: UserName activation refused — client {Client} is locked out ({Scope}: {Identity}) for {Seconds}s more", + lockoutKey, decision.Scope, + decision.Scope == CredentialLockoutScope.Client ? "every credential" : credentialIdentity, seconds); return AuthenticationResult.Reject(ServiceResult.Create(StatusCodes.BadUserAccessDenied, $"Too many failed authentication attempts. Try again in {seconds} seconds.")); } @@ -309,8 +317,9 @@ public sealed class OpcUaApplicationHost : IAsyncDisposable try { // 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 transient = 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 @@ -321,19 +330,29 @@ public sealed class OpcUaApplicationHost : IAsyncDisposable { var admitted = args.Identity is not null && !ServiceResult.IsBad(args.IdentityValidationError); completed = true; - if (host._credentialLockout.Complete(lockoutKey, admitted)) + if (!admitted && transient) + { + // lmxopcua#845 — a store timeout / miss-budget refusal / authenticator fault is not a bad + // credential: release the slot uncounted. + host._credentialLockout.Abandon(lockoutKey, credentialIdentity); + } + else if (host._credentialLockout.Complete(lockoutKey, credentialIdentity, admitted) is var scope + && scope != CredentialLockoutScope.None) { 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); + "OpcUaApplicationHost: client {Client} locked out of UserName activation ({Scope}: {Identity}) " + + "for {Seconds}s after repeated failed attempts (OpcUa:CredentialLockout: {PerCredential} per " + + "credential, {PerClient} per client); anonymous activation is unaffected", + lockoutKey, scope, scope == CredentialLockoutScope.Client ? "every credential" : credentialIdentity, + host._options.CredentialLockout.LockoutSeconds, host._options.CredentialLockout.MaxFailedAttempts, + host._credentialLockout.ClientThreshold); } } return result; } finally { - if (lockoutKey is not null && !completed) host._credentialLockout.Abandon(lockoutKey); + if (lockoutKey is not null && !completed) host._credentialLockout.Abandon(lockoutKey, credentialIdentity); } } } @@ -407,7 +426,10 @@ public sealed class OpcUaApplicationHost : IAsyncDisposable /// Whether anonymous sessions may connect. /// The activating client's IP endpoint when known (lmxopcua#751), handed to /// the authenticator for per-client accounting; null when it could not be resolved. - internal static void HandleImpersonation( + /// when a UserName denial was TRANSIENT — not a verdict on the credential + /// (the authenticator threw or reported ), so the failed-credential + /// lockout must not count it (lmxopcua#845). + internal static bool HandleImpersonation( IOpcUaUserAuthenticator authenticator, ImpersonateEventArgs args, ILogger logger, @@ -416,7 +438,7 @@ public sealed class OpcUaApplicationHost : IAsyncDisposable => HandleImpersonationAsync(authenticator, args, logger, allowAnonymous, remoteEndpoint) .AsTask().GetAwaiter().GetResult(); - private static async ValueTask HandleImpersonationAsync( + private static async ValueTask HandleImpersonationAsync( IOpcUaUserAuthenticator authenticator, ImpersonateEventArgs args, ILogger logger, bool allowAnonymous = true, System.Net.IPEndPoint? remoteEndpoint = null, CancellationToken cancellationToken = default) @@ -424,13 +446,13 @@ public sealed class OpcUaApplicationHost : IAsyncDisposable if (args.UserIdentityTokenHandler.Token is AnonymousIdentityToken anonymousToken) { await HandleAnonymousImpersonationAsync(authenticator, args, logger, allowAnonymous, anonymousToken, cancellationToken).ConfigureAwait(false); - return; + return false; } if (args.UserIdentityTokenHandler.Token is not UserNameIdentityToken token) { // X509 tokens — let the SDK's default validation stand. - return; + return false; } string password; @@ -444,7 +466,7 @@ public sealed class OpcUaApplicationHost : IAsyncDisposable { logger.LogWarning(ex, "OpcUaApplicationHost: failed to decrypt UserName token"); args.IdentityValidationError = ServiceResult.Create(StatusCodes.BadIdentityTokenRejected, "UserName token decryption failed"); - return; + return false; } OpcUaUserAuthResult result; @@ -456,22 +478,45 @@ public sealed class OpcUaApplicationHost : IAsyncDisposable } catch (Exception ex) { + // Includes cancellation: neither says anything about the credential (lmxopcua#845). logger.LogError(ex, "OpcUaApplicationHost: UserName authenticator threw for {User}", token.UserName); args.IdentityValidationError = ServiceResult.Create(StatusCodes.BadIdentityTokenRejected, "Authentication failed"); - return; + return true; } if (!result.Success) { - logger.LogInformation("OpcUaApplicationHost: UserName auth denied for {User}: {Error}", - token.UserName, result.Error); + logger.LogInformation("OpcUaApplicationHost: UserName auth denied for {User}{Transient}: {Error}", + token.UserName, result.Transient ? " (transient, not counted toward the lockout)" : string.Empty, result.Error); args.IdentityValidationError = ServiceResult.Create(StatusCodes.BadIdentityTokenRejected, result.Error ?? "Invalid credentials"); - return; + return result.Transient; } args.Identity = new RoleCarryingUserIdentity(token, result.Roles, result.Principal, result.ApiKeyId); logger.LogInformation("OpcUaApplicationHost: UserName auth granted for {User} as {Principal} ({Roles})", token.UserName, result.Principal, string.Join(",", result.Roles)); + return false; + } + + /// + /// The credential identity a UserName activation presents, for the lockout's per-identity count + /// (lmxopcua#844): over the decrypted token. Never + /// throws: an unreadable token or a faulting implementation falls back to the username. + /// + internal static string DescribeCredential(IOpcUaUserAuthenticator authenticator, IUserIdentityTokenHandler handler) + { + if (handler.Token is not UserNameIdentityToken token) return FailedCredentialLockout.NoIdentity; + var username = token.UserName ?? string.Empty; + try + { + var decrypted = (handler as UserNameIdentityTokenHandler)?.DecryptedPassword; + var password = decrypted is null ? string.Empty : Encoding.UTF8.GetString(decrypted); + return authenticator.DescribeCredential(username, password) ?? "user:" + username; + } + catch (Exception) + { + return "user:" + username; + } } /// 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 24fffa4dc..79bcf0fc6 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/Security/FailedCredentialLockout.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/Security/FailedCredentialLockout.cs @@ -12,9 +12,23 @@ namespace ZB.MOM.WW.OtOpcUa.OpcUaServer.Security; /// public sealed class OpcUaCredentialLockoutOptions { - /// Failed UserName activations that lock a client out; 0 or less disables the lockout. + /// + /// Failed UserName activations for ONE credential identity (an API key id, else the username) from one client + /// that lock that client out of using that identity; 0 or less disables the whole lockout. A success + /// with the same identity clears its count (lmxopcua#844). + /// public int MaxFailedAttempts { get; set; } = 5; + /// + /// Failed UserName activations from one client, across EVERY identity, that lock the client out of all + /// UserName activations (lmxopcua#844). A success does NOT reset this count — it only ages out through + /// or an expired lockout — so a client cannot make + /// − 1 guesses at someone else's key, sign in with its own, and repeat; nor + /// spray a few guesses at each of many key ids. 0 (the default) means 2 × ; + /// an explicit value must be at least . + /// + public int MaxFailedAttemptsPerClient { get; set; } + /// A failure more than this many seconds after the previous one starts a fresh count. public int FailureWindowSeconds { get; set; } = 60; @@ -31,29 +45,45 @@ public sealed class OpcUaCredentialLockoutOptions public int SlotWaitSeconds { get; set; } = 10; } -/// The outcome of . +/// The outcome of . public enum CredentialLockoutVerdict { - /// A verification slot is held — authenticate, then call . + /// A verification slot is held — authenticate, then call . Proceed, - /// The client is locked out; refuse without authenticating. + /// The client, or the credential identity it presented, is locked out; refuse without authenticating. LockedOut, /// No slot freed within the wait; fail this one attempt, uncounted. SlotWaitTimedOut, } -/// What decided. +/// 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). +/// What is locked out when . public readonly record struct CredentialLockoutDecision( - CredentialLockoutVerdict Verdict, TimeSpan Remaining, bool FirstRefusal); + CredentialLockoutVerdict Verdict, TimeSpan Remaining, bool FirstRefusal, + CredentialLockoutScope Scope = CredentialLockoutScope.None); + +/// What a lockout covers (lmxopcua#844). +public enum CredentialLockoutScope +{ + /// Nothing is locked. + None, + + /// One credential identity (key id / username) from one client. + Credential, + + /// Every UserName activation from one client. + Client, +} /// /// Brute-force lockout for UserName (API-key) activations, scoped so it can never refuse an -/// anonymous activation (lmxopcua#833). +/// anonymous activation (lmxopcua#833), and counted per credential identity as well as per client +/// (lmxopcua#844). /// /// /// @@ -62,42 +92,56 @@ public readonly record struct CredentialLockoutDecision( /// at ServerConfiguration.MaxFailedAuthenticationAttempts (default 5), refuses every /// activation from that client for 5 minutes with BadUserAccessDenied — the lockout check runs /// before the token is even looked at, so it refuses Anonymous too, and a successful anonymous -/// activation does not clear it. Its key is the client certificate thumbprint when one is presented, -/// else the peer IP: on a None endpoint the presented certificate is not proven, so a client -/// could lock out another client's sessions by presenting its certificate, and behind a NAT every -/// certificate-less client shares one key. The host therefore sets -/// MaxFailedAuthenticationAttempts = 0 and enforces this narrower lockout instead. +/// activation does not clear it. The host therefore sets MaxFailedAuthenticationAttempts = 0 and +/// enforces this narrower lockout instead. /// /// -/// 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 -/// 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. +/// Client key. Only UserName activations are counted and refused; Anonymous never consults it. The +/// client key is the presented client certificate thumbprint and the observed peer address together. +/// ⚠️ On a None endpoint clients commonly present no certificate, so the key is effectively the IP +/// alone and NAT neighbours there share the client-wide count (UserName only). The client's +/// ApplicationUri is deliberately NOT part of the key (lmxopcua#845): it is a free, unauthenticated +/// string on a None channel, so adding it would let one attacker reset its count by varying it. /// /// -/// 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. +/// Two counts (lmxopcua#844). Each failure is charged to the (client, credential identity) +/// pair — the API key id, or the username when the credential carries no key id — AND to the client as a +/// whole. A success clears only its own identity's count. At +/// failures of one identity, that client is +/// refused that identity for ; at the +/// per-client cap (, default 2× that), which a success never resets, the client +/// is refused every UserName activation. So a client holding a valid key can no longer launder guesses +/// at another key id by signing in with its own between them (the success does not touch the victim +/// identity's count), and spraying a few guesses at each of many key ids still hits the client-wide cap. A +/// legitimate client with one stale key and one good key loses only the stale one. Timing per count: a +/// failure more than after the previous one +/// of that count starts it afresh; a refusal during a lockout is not counted; an expired lockout starts a +/// fresh count (an expired client-wide lockout clears every identity's too). /// /// -/// 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). +/// Only a definite bad credential counts (lmxopcua#845). The caller completes a transient denial — a +/// key-store timeout, a miss-budget refusal, an authenticator exception, cancellation — with +/// , which releases the slot uncounted; a ConfigDb stall therefore +/// cannot lock every keyed client out. +/// +/// +/// Concurrency — parallelism waits, it is never refused. +/// hands out a verification slot only +/// while both identityFailures + identityInFlight < MaxFailedAttempts and +/// clientFailures + clientInFlight < ClientThreshold; an activation beyond that waits for a +/// slot (a waiter whose identity has room is not held behind one whose identity has none). No more +/// unverified guesses than either threshold are ever in progress-or-failed, 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 what they need locks out, or (uncounted) after +/// ; a cancelled waiter is dequeued. +/// +/// +/// Memory is bounded at clients and +/// identities per client (identities beyond that share one +/// overflow count — conservative, never laxer). Entries with nothing to remember are pruned when the table +/// fills; a client that still finds it full is not tracked. When a prune frees nothing, the next one waits +/// until the earliest entry could have expired (lmxopcua#845), so a flood of new client keys at a full table +/// does not rescan it under the lock on every failure. /// /// public sealed class FailedCredentialLockout @@ -105,10 +149,21 @@ public sealed class FailedCredentialLockout /// The most clients tracked at once. public const int MaxTrackedClients = 4096; + /// The most credential identities tracked per client; further ones share one overflow count. + public const int MaxTrackedIdentitiesPerClient = 64; + + /// The identity used when the caller supplies none (a direct caller / test seam). + public const string NoIdentity = ""; + + private const string OverflowIdentity = "\0overflow"; + private const int MaxIdentityLength = 256; + private readonly OpcUaCredentialLockoutOptions _options; private readonly TimeProvider _time; private readonly Dictionary _entries = new(StringComparer.Ordinal); private readonly object _gate = new(); + private DateTimeOffset _nextPruneAt = DateTimeOffset.MinValue; + private int _pruneScans; /// Initializes a new instance of the class. /// The configured thresholds. @@ -122,6 +177,13 @@ public sealed class FailedCredentialLockout /// Whether the lockout is switched on. public bool Enabled => _options.MaxFailedAttempts > 0; + /// The effective per-client cap: the configured value, or 2 × MaxFailedAttempts when unset. + public int ClientThreshold => _options.MaxFailedAttemptsPerClient > 0 + ? _options.MaxFailedAttemptsPerClient + : 2 * Math.Max(0, _options.MaxFailedAttempts); + + private int IdentityThreshold => _options.MaxFailedAttempts; + /// Builds the lockout key for a client. /// The presented client certificate's thumbprint, if any. /// The transport-observed peer address, if known. @@ -129,17 +191,28 @@ public sealed class FailedCredentialLockout public static string KeyFor(string? certificateThumbprint, System.Net.IPAddress? peer) => $"{(string.IsNullOrEmpty(certificateThumbprint) ? "-" : certificateThumbprint)}@{peer?.ToString() ?? "-"}"; + /// with . + /// The client key from . + /// Cancels the wait. + /// The decision. + public ValueTask BeginAsync(string key, CancellationToken ct = default) => + BeginAsync(key, NoIdentity, ct); + /// - /// 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 . + /// Acquires a verification slot for from , waiting for one + /// when the client or the identity already has as many in progress as it may. On + /// the caller MUST release the slot with + /// or , passing the same + /// identity. /// /// The client key from . + /// The credential identity (key id, else username) the attempt presents. /// Cancels the wait; a cancelled waiter is dequeued and the exception propagates. /// The decision. - public async ValueTask BeginAsync(string key, CancellationToken ct = default) + public async ValueTask BeginAsync(string key, string identity, CancellationToken ct = default) { if (!Enabled) return new(CredentialLockoutVerdict.Proceed, TimeSpan.Zero, false); + identity = Normalize(identity); Waiter waiter; Entry entry; @@ -148,24 +221,25 @@ public sealed class FailedCredentialLockout var now = _time.GetUtcNow(); if (!_entries.TryGetValue(key, out var existing)) { - if (_entries.Count >= MaxTrackedClients) Prune(now); - // 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; + existing = TryAddEntry(key, now); + // Untracked when still full: it gets no slot accounting (see remarks). + if (existing is null) return new(CredentialLockoutVerdict.Proceed, TimeSpan.Zero, false); } entry = existing; Refresh(entry, now); - if (TryLockedOut(entry, now, out var refused)) return refused; - if (entry.Waiters.Count == 0 && HasFreeSlot(entry)) + var slotIdentity = SlotIdentity(entry, identity); + if (TryLockedOut(entry, slotIdentity, now, out var refused)) return refused; + if (entry.Waiters.Count == 0 && HasFreeSlot(entry, slotIdentity)) { - entry.InFlight++; + TakeSlot(entry, slotIdentity); return new(CredentialLockoutVerdict.Proceed, TimeSpan.Zero, false); } - waiter = new Waiter(); + waiter = new Waiter(identity); entry.Waiters.AddLast(waiter.Node = new LinkedListNode(waiter)); + // A waiter whose identity has room is not held behind one whose identity has none. + Pump(entry, now); } var wait = TimeSpan.FromSeconds(Math.Max(1, _options.SlotWaitSeconds)); @@ -183,12 +257,13 @@ public sealed class FailedCredentialLockout // 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); + if (decided.Verdict == CredentialLockoutVerdict.Proceed) ReleaseCore(key, entry, identity, outcome: null); } else { entry.Waiters.Remove(waiter.Node!); waiter.Completion.TrySetCanceled(); + Pump(entry, _time.GetUtcNow()); RemoveIfIdle(key, entry); } } @@ -198,79 +273,123 @@ public sealed class FailedCredentialLockout } } - /// Releases a slot from and records the activation's outcome. + /// with . /// The client key. /// Whether the activation was admitted. - /// when this failure locked the client out. - public bool Complete(string key, bool succeeded) + /// What this failure locked out, if anything. + public CredentialLockoutScope Complete(string key, bool succeeded) => Complete(key, NoIdentity, succeeded); + + /// + /// Releases a slot from and records a DEFINITE + /// outcome: admitted, or refused for a bad credential. A transient denial goes to + /// instead (#845). + /// + /// The client key. + /// The identity passed to BeginAsync. + /// Whether the activation was admitted. + /// What this failure locked out, if anything. + public CredentialLockoutScope Complete(string key, string identity, bool succeeded) { - if (!Enabled) return false; + if (!Enabled) return CredentialLockoutScope.None; + identity = Normalize(identity); lock (_gate) { - return _entries.TryGetValue(key, out var entry) && ReleaseCore(key, entry, succeeded); + return _entries.TryGetValue(key, out var entry) + ? ReleaseCore(key, entry, identity, succeeded) + : CredentialLockoutScope.None; } } - /// Releases a slot without recording an outcome (the activation was cancelled or threw). + /// with . /// The client key. - public void Abandon(string key) + public void Abandon(string key) => Abandon(key, NoIdentity); + + /// + /// Releases a slot without recording an outcome: the activation was cancelled or threw, or was denied for a + /// transient reason (store timeout, miss budget, authenticator fault) rather than a bad credential (#845). + /// + /// The client key. + /// The identity passed to BeginAsync. + public void Abandon(string key, string identity) { if (!Enabled) return; + identity = Normalize(identity); lock (_gate) { - if (_entries.TryGetValue(key, out var entry)) ReleaseCore(key, entry, outcome: null); + if (_entries.TryGetValue(key, out var entry)) ReleaseCore(key, entry, identity, outcome: null); } } - /// Whether is locked out right now (observation; reserves nothing). + /// Whether is locked out client-wide or for . /// The client key from . /// The time left on the lockout when this returns true. - /// when the client's UserName activations are refused. - public bool IsLockedOut(string key, out TimeSpan remaining) + /// when such an activation would be refused. + public bool IsLockedOut(string key, out TimeSpan remaining) => IsLockedOut(key, NoIdentity, out remaining); + + /// + /// Whether an activation presenting from would be refused + /// right now — client-wide or for that identity (observation; reserves nothing). + /// + /// The client key from . + /// The credential identity. + /// The time left on the lockout when this returns true. + /// when the activation would be refused. + public bool IsLockedOut(string key, string identity, out TimeSpan remaining) { remaining = TimeSpan.Zero; if (!Enabled) return false; + identity = Normalize(identity); var now = _time.GetUtcNow(); lock (_gate) { if (!_entries.TryGetValue(key, out var entry)) return false; Refresh(entry, now); - if (entry.LockedUntil <= now) return false; - remaining = entry.LockedUntil - now; + if (entry.LockedUntil > now) + { + remaining = entry.LockedUntil - now; + return true; + } + + if (!entry.Identities.TryGetValue(SlotIdentity(entry, identity), out var id) || id.LockedUntil <= now) + return false; + remaining = id.LockedUntil - now; return true; } } /// 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) + /// The credential identity. + /// when this failure locked the identity or the client out. + public bool RecordFailure(string key, string identity = NoIdentity) { if (!Enabled) return false; + identity = Normalize(identity); var now = _time.GetUtcNow(); lock (_gate) { if (!_entries.TryGetValue(key, out var entry)) { - if (_entries.Count >= MaxTrackedClients) Prune(now); - if (_entries.Count >= MaxTrackedClients) return false; - entry = new Entry(); - _entries[key] = entry; + entry = TryAddEntry(key, now); + if (entry is null) return false; } - var locked = RecordFailureCore(entry, now); + + var locked = RecordFailureCore(entry, SlotIdentity(entry, identity), now); Pump(entry, now); - return locked; + return locked != CredentialLockoutScope.None; } } - /// Clears a client's count after a successful UserName activation (no slot). + /// Clears one identity's count after a successful UserName activation (no slot). /// The client key. - public void RecordSuccess(string key) + /// The credential identity that succeeded. + public void RecordSuccess(string key, string identity = NoIdentity) { + identity = Normalize(identity); lock (_gate) { if (!_entries.TryGetValue(key, out var entry)) return; - ClearCount(entry); + RecordSuccessCore(entry, SlotIdentity(entry, identity)); Pump(entry, _time.GetUtcNow()); RemoveIfIdle(key, entry); } @@ -282,126 +401,305 @@ public sealed class FailedCredentialLockout get { lock (_gate) return _entries.Count; } } + /// How many full-table prune scans have run (test observation, #845). + internal int PruneScanCount + { + get { lock (_gate) return _pruneScans; } + } + /// How many activations are waiting for a slot on (test observation). internal int WaitingCount(string key) { lock (_gate) return _entries.TryGetValue(key, out var e) ? e.Waiters.Count : 0; } - private bool HasFreeSlot(Entry entry) => entry.Failures + entry.InFlight < _options.MaxFailedAttempts; + /// Bounds an attacker-chosen identity string; distinct long values stay distinct. + private static string Normalize(string? identity) + { + identity ??= NoIdentity; + if (identity.Length <= MaxIdentityLength) return identity; + var hash = System.Security.Cryptography.SHA256.HashData(System.Text.Encoding.UTF8.GetBytes(identity)); + return "\0sha256:" + Convert.ToHexString(hash); + } - private bool TryLockedOut(Entry entry, DateTimeOffset now, out CredentialLockoutDecision refused) + private Entry? TryAddEntry(string key, DateTimeOffset now) + { + if (_entries.Count >= MaxTrackedClients && now >= _nextPruneAt) Prune(now); + if (_entries.Count >= MaxTrackedClients) return null; + var entry = new Entry(); + _entries[key] = entry; + return entry; + } + + /// + /// The identity a slot / failure is charged to: the identity itself while it is tracked or there is room to + /// track it, else the one shared overflow identity (a slot is always reserved for it). + /// + private static string SlotIdentity(Entry entry, string identity) => + entry.Identities.ContainsKey(identity) || entry.Identities.Count < MaxTrackedIdentitiesPerClient - 1 + ? identity + : OverflowIdentity; + + private static IdentityState GetOrAddIdentity(Entry entry, string slotIdentity) + { + if (!entry.Identities.TryGetValue(slotIdentity, out var id)) + { + id = new IdentityState(); + entry.Identities[slotIdentity] = id; + } + return id; + } + + private bool ClientHasRoom(Entry entry) => entry.Failures + entry.InFlight < ClientThreshold; + + private bool HasFreeSlot(Entry entry, string slotIdentity) => + ClientHasRoom(entry) + && (!entry.Identities.TryGetValue(slotIdentity, out var id) || id.Failures + id.InFlight < IdentityThreshold); + + private static void TakeSlot(Entry entry, string slotIdentity) + { + entry.InFlight++; + GetOrAddIdentity(entry, slotIdentity).InFlight++; + } + + private static bool TryLockedOut(Entry entry, string slotIdentity, 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; + if (entry.LockedUntil > now) + { + refused = new(CredentialLockoutVerdict.LockedOut, entry.LockedUntil - now, !entry.RefusalLogged, + CredentialLockoutScope.Client); + entry.RefusalLogged = true; + return true; + } + + if (!entry.Identities.TryGetValue(slotIdentity, out var id) || id.LockedUntil <= now) return false; + refused = new(CredentialLockoutVerdict.LockedOut, id.LockedUntil - now, !id.RefusalLogged, + CredentialLockoutScope.Credential); + id.RefusalLogged = true; return true; } /// Releases one slot, records (null = uncounted) and wakes waiters. - private bool ReleaseCore(string key, Entry entry, bool? outcome) + private CredentialLockoutScope ReleaseCore(string key, Entry entry, string identity, 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); + + // The slot was charged to the identity itself or, when the table was full, to the overflow identity. + var slotIdentity = entry.Identities.TryGetValue(identity, out var own) && own.InFlight > 0 + ? identity + : entry.Identities.TryGetValue(OverflowIdentity, out var overflow) && overflow.InFlight > 0 + ? OverflowIdentity + : SlotIdentity(entry, identity); + if (entry.Identities.TryGetValue(slotIdentity, out var held) && held.InFlight > 0) held.InFlight--; + + var locked = CredentialLockoutScope.None; + if (outcome == true) RecordSuccessCore(entry, slotIdentity); + else if (outcome == false) locked = RecordFailureCore(entry, slotIdentity, now); Pump(entry, now); RemoveIfIdle(key, entry); - return lockedNow; + return locked; } /// - /// 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. + /// Hands freed slots to waiters in order — passing over one whose identity is full — and refuses every + /// waiter whose identity, or whose client, is locked out (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) + var node = entry.Waiters.First; + while (node is not null) { - if (TryLockedOut(entry, now, out var refused)) + var next = node.Next; + var waiter = node.Value; + var slotIdentity = SlotIdentity(entry, waiter.Identity); + if (TryLockedOut(entry, slotIdentity, now, out var refused)) { - entry.Waiters.RemoveFirst(); - first.Value.Completion.TrySetResult(refused); - continue; + entry.Waiters.Remove(node); + waiter.Completion.TrySetResult(refused); + } + else if (ClientHasRoom(entry) && HasFreeSlot(entry, slotIdentity)) + { + entry.Waiters.Remove(node); + if (waiter.Completion.TrySetResult(new(CredentialLockoutVerdict.Proceed, TimeSpan.Zero, false))) + TakeSlot(entry, slotIdentity); // a waiter that already gave up never takes the slot } - 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 + node = next; } } - private static void ClearCount(Entry entry) + private static void RecordSuccessCore(Entry entry, string slotIdentity) { - entry.Failures = 0; - entry.LockedUntil = default; - entry.RefusalLogged = false; + // Only this identity's count — never the client-wide one (#844). + if (!entry.Identities.TryGetValue(slotIdentity, out var id)) return; + id.Failures = 0; + id.LockedUntil = default; + id.RefusalLogged = false; + if (id.InFlight == 0) entry.Identities.Remove(slotIdentity); } - private bool RecordFailureCore(Entry entry, DateTimeOffset now) + private CredentialLockoutScope RecordFailureCore(Entry entry, string slotIdentity, DateTimeOffset now) { Refresh(entry, now); - if (entry.LockedUntil > now) return false; // a refusal (or a late in-flight failure) never extends it + if (entry.LockedUntil > now) return CredentialLockoutScope.None; // a late in-flight failure never extends it + + var lockFor = TimeSpan.FromSeconds(Math.Max(0, _options.LockoutSeconds)); + var result = CredentialLockoutScope.None; + var id = GetOrAddIdentity(entry, slotIdentity); + if (id.LockedUntil <= now) + { + id.Failures++; + id.LastFailure = now; + if (id.Failures >= IdentityThreshold && lockFor > TimeSpan.Zero) + { + id.LockedUntil = now + lockFor; + id.RefusalLogged = false; + result = CredentialLockoutScope.Credential; + } + } + 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; + if (entry.Failures >= ClientThreshold && lockFor > TimeSpan.Zero) + { + entry.LockedUntil = now + lockFor; + entry.RefusalLogged = false; + result = CredentialLockoutScope.Client; + } + + return result; } /// - /// Forgets a run of failures that has gone stale: an expired lockout always starts a fresh count (else a + /// Forgets runs of failures that have 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. + /// and so does a failure older than the window. Applies to the client-wide count and each identity's. /// private void Refresh(Entry entry, DateTimeOffset now) { + var window = TimeSpan.FromSeconds(Math.Max(0, _options.FailureWindowSeconds)); if (entry.LockedUntil != default && entry.LockedUntil <= now) { - ClearCount(entry); + // An expired client-wide lockout clears every identity's count as well. + ClearCount(ref entry.Failures, ref entry.LockedUntil, ref entry.RefusalLogged); + foreach (var id in entry.Identities.Values) + ClearCount(ref id.Failures, ref id.LockedUntil, ref id.RefusalLogged); } - else if (entry.LockedUntil == default && entry.Failures > 0 - && now - entry.LastFailure > TimeSpan.FromSeconds(Math.Max(0, _options.FailureWindowSeconds))) + else if (entry.LockedUntil == default && entry.Failures > 0 && now - entry.LastFailure > window) { entry.Failures = 0; entry.RefusalLogged = false; } + + if (entry.Identities.Count == 0) return; + List? idle = null; + foreach (var (name, id) in entry.Identities) + { + if (id.LockedUntil != default && id.LockedUntil <= now) + { + ClearCount(ref id.Failures, ref id.LockedUntil, ref id.RefusalLogged); + } + else if (id.LockedUntil == default && id.Failures > 0 && now - id.LastFailure > window) + { + id.Failures = 0; + id.RefusalLogged = false; + } + + if (id.Failures == 0 && id.InFlight == 0 && id.LockedUntil == default) (idle ??= []).Add(name); + } + + if (idle is null) return; + foreach (var name in idle) entry.Identities.Remove(name); + } + + private static void ClearCount(ref int failures, ref DateTimeOffset lockedUntil, ref bool refusalLogged) + { + failures = 0; + lockedUntil = default; + refusalLogged = false; } private void RemoveIfIdle(string key, Entry entry) { - if (IsIdle(entry, _time.GetUtcNow())) _entries.Remove(key); + if (!IsIdle(entry, _time.GetUtcNow())) return; + _entries.Remove(key); + _nextPruneAt = DateTimeOffset.MinValue; // room was made; a full table may prune again } private void Prune(DateTimeOffset now) { - foreach (var key in _entries.Where(e => IsIdle(e.Value, now)).Select(e => e.Key).ToList()) - _entries.Remove(key); + _pruneScans++; + var earliest = DateTimeOffset.MaxValue; + List? idle = null; + foreach (var (key, entry) in _entries) + { + if (IsIdle(entry, now)) (idle ??= []).Add(key); + else if (EarliestIdle(entry) is var at && at < earliest) earliest = at; + } + + if (idle is not null) + { + foreach (var key in idle) _entries.Remove(key); + } + + // #845 — when nothing could be freed, do not rescan the full table on every new client's failure: wait + // until the earliest entry could have gone idle by the clock. An entry holding a slot or a waiter goes + // idle only through a release, and RemoveIfIdle resets this when that frees room. + _nextPruneAt = _entries.Count >= MaxTrackedClients ? earliest : DateTimeOffset.MinValue; } - /// Idle = nothing to remember: no failures, no lockout, no slot held and nobody waiting. + /// The earliest instant could be idle by the clock alone. + private DateTimeOffset EarliestIdle(Entry entry) + { + if (entry.InFlight > 0 || entry.Waiters.Count > 0) return DateTimeOffset.MaxValue; + var window = TimeSpan.FromSeconds(Math.Max(0, _options.FailureWindowSeconds)); + var at = Expiry(entry.LockedUntil, entry.Failures, entry.LastFailure, window); + foreach (var id in entry.Identities.Values) + { + var idAt = Expiry(id.LockedUntil, id.Failures, id.LastFailure, window); + if (idAt > at) at = idAt; + } + return at; + + static DateTimeOffset Expiry(DateTimeOffset lockedUntil, int failures, DateTimeOffset lastFailure, TimeSpan window) => + lockedUntil != default ? lockedUntil + : failures > 0 ? lastFailure + window + TimeSpan.FromTicks(1) + : DateTimeOffset.MinValue; + } + + /// Idle = nothing to remember: no failures, lockout, slot, waiter or identity. private bool IsIdle(Entry entry, DateTimeOffset now) { Refresh(entry, now); - return entry.InFlight == 0 && entry.Waiters.Count == 0 && entry.Failures == 0 && entry.LockedUntil == default; + return entry.InFlight == 0 && entry.Waiters.Count == 0 && entry.Failures == 0 + && entry.LockedUntil == default && entry.Identities.Count == 0; } /// One activation waiting for a slot. - private sealed class Waiter + private sealed class Waiter(string identity) { public readonly TaskCompletionSource Completion = new(TaskCreationOptions.RunContinuationsAsynchronously); + public readonly string Identity = identity; public LinkedListNode? Node; } - /// One client's failure count, slots, waiters and lockout. Mutated only under the gate. + /// One credential identity's count, slots and lockout within one client. Mutated only under the gate. + private sealed class IdentityState + { + public int Failures; + public int InFlight; + public DateTimeOffset LastFailure; + public DateTimeOffset LockedUntil; + public bool RefusalLogged; + } + + /// One client's aggregate count, slots, waiters, lockout and identities. Mutated only under the gate. private sealed class Entry { public int Failures; @@ -410,5 +708,8 @@ public sealed class FailedCredentialLockout public DateTimeOffset LockedUntil; public bool RefusalLogged; public readonly LinkedList Waiters = new(); + public readonly Dictionary Identities = new(StringComparer.Ordinal); } } + + diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/Security/IOpcUaUserAuthenticator.cs b/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/Security/IOpcUaUserAuthenticator.cs index 2fa14de03..633b500ab 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/Security/IOpcUaUserAuthenticator.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/Security/IOpcUaUserAuthenticator.cs @@ -45,6 +45,18 @@ public interface IOpcUaUserAuthenticator string username, string password, System.Net.IPEndPoint? remoteEndpoint, CancellationToken ct) => AuthenticateUserNameAsync(username, password, ct); + /// + /// Names the credential identity a UserName attempt presents, for the failed-credential lockout's + /// per-identity count (lmxopcua#844): a success clears only its own identity's count, so a client holding one + /// valid credential cannot use it to launder guesses at another. Called before authentication; must be cheap, + /// do no I/O and not throw. The default is the username; an implementation whose secret carries a public + /// identifier (an API key id) should return that, so guesses at different keys are told apart. + /// + /// The presented username. + /// The presented cleartext password. + /// A stable, non-secret identity string. It is logged — never include secret material. + string DescribeCredential(string username, string password) => "user:" + username; + /// /// Resolves what an anonymous session is allowed to do. Anonymous is a normal /// principal — — not a special case, so it can be granted @@ -101,6 +113,14 @@ public sealed record OpcUaUserAuthResult( /// public string? ApiKeyId { get; init; } + /// + /// True when the denial is NOT a verdict on the credential — the backing store timed out or refused the + /// lookup, the authenticator is misconfigured, the call was cancelled (lmxopcua#845). The failed-credential + /// lockout does not count it, so an infrastructure stall cannot lock every keyed client out. The client still + /// sees the same opaque denial. + /// + public bool Transient { get; init; } + /// Creates a successful authentication result with the specified display name and roles. /// The display name for the authenticated user — also taken as the principal. /// The roles to assign to the authenticated user. @@ -121,6 +141,15 @@ public sealed record OpcUaUserAuthResult( /// A failed OpcUaUserAuthResult. public static OpcUaUserAuthResult Deny(string error) => new(false, null, Array.Empty(), error); + + /// + /// Creates a denial that is not a verdict on the credential (store outage, misconfiguration, cancellation) — + /// see . + /// + /// The error message returned to the client (keep it as opaque as a normal denial). + /// A failed, transient OpcUaUserAuthResult. + public static OpcUaUserAuthResult Unavailable(string error) => + new(false, null, Array.Empty(), error) { Transient = true }; } /// 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 ead7d38cb..0d8e5b684 100644 --- a/tests/Server/ZB.MOM.WW.OtOpcUa.Host.IntegrationTests/OpcUaApplicationHostOptionsValidatorTests.cs +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.Host.IntegrationTests/OpcUaApplicationHostOptionsValidatorTests.cs @@ -92,4 +92,24 @@ public sealed class OpcUaApplicationHostOptionsValidatorTests result.Failed.ShouldBeTrue(); result.Failures!.ShouldContain(f => f.Contains("OpcUa:CredentialLockout:SlotWaitSeconds")); } + + /// #844: the per-client cap may be unset (0) or at least the per-credential threshold. + [Theory] + [InlineData(0, true)] + [InlineData(5, true)] + [InlineData(20, true)] + [InlineData(4, false)] + public void Credential_lockout_per_client_cap_is_unset_or_at_least_the_per_credential_threshold(int perClient, bool valid) + { + var result = Sut.Validate(null, new OpcUaApplicationHostOptions + { + CredentialLockout = new ZB.MOM.WW.OtOpcUa.OpcUaServer.Security.OpcUaCredentialLockoutOptions + { + MaxFailedAttempts = 5, MaxFailedAttemptsPerClient = perClient, + }, + }); + + result.Succeeded.ShouldBe(valid); + if (!valid) result.Failures!.ShouldContain(f => f.Contains("OpcUa:CredentialLockout:MaxFailedAttemptsPerClient")); + } } diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.Host.Tests/OpcUa/ApiKeyAuthenticationTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.Host.Tests/OpcUa/ApiKeyAuthenticationTests.cs index b5905c34c..556ca6c25 100644 --- a/tests/Server/ZB.MOM.WW.OtOpcUa.Host.Tests/OpcUa/ApiKeyAuthenticationTests.cs +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.Host.Tests/OpcUa/ApiKeyAuthenticationTests.cs @@ -154,6 +154,42 @@ public sealed class ApiKeyAuthenticationTests result.Success.ShouldBeFalse(); result.Error.ShouldBe(ApiKeyOpcUaUserAuthenticator.DenyMessage); + result.Transient.ShouldBeTrue("#845: a store outage / timeout / miss-budget refusal is not a bad credential"); + } + + /// + /// #845 — only a definite verdict on the credential is non-transient: a wrong secret, an unknown key id, a + /// malformed credential and the wrong username all count toward the failed-credential lockout. + /// + [Fact] + public async Task Definite_credential_failures_are_not_transient() + { + var (presented, store) = SetUp(); + var auth = Auth(store); + var ct = TestContext.Current.CancellationToken; + OpcUaUserAuthResult[] results = + [ + await auth.AuthenticateUserNameAsync("apikey", $"otua_{store.KeyId}_wrong", ct), + await auth.AuthenticateUserNameAsync("apikey", "otua_ffffffff_whatever", ct), + await auth.AuthenticateUserNameAsync("apikey", "not-a-key", ct), + await auth.AuthenticateUserNameAsync("alice", presented, ct), + ]; + results.ShouldAllBe(r => !r.Success && !r.Transient); + } + + /// + /// #844 — the lockout counts per credential identity: the key id (public — the lookup index, never the + /// secret), else the username. The secret must never appear in it (it is logged). + /// + [Fact] + public void The_credential_identity_is_the_key_id_and_never_the_secret() + { + var (presented, store) = SetUp(); + var auth = Auth(store); + var identity = auth.DescribeCredential("apikey", presented); + identity.ShouldBe($"key:{store.KeyId}"); + identity.ShouldNotContain(presented.Split('_')[^1]); + auth.DescribeCredential("apikey", "not-a-key").ShouldBe("user:apikey"); } [Fact] @@ -165,7 +201,9 @@ public sealed class ApiKeyAuthenticationTests Options.Create(new ApiKeyOptions { Username = "apikey", Pepper = "" }), NullLogger.Instance); - (await auth.AuthenticateUserNameAsync("apikey", presented, TestContext.Current.CancellationToken)).Success.ShouldBeFalse(); + var result = await auth.AuthenticateUserNameAsync("apikey", presented, TestContext.Current.CancellationToken); + result.Success.ShouldBeFalse(); + result.Transient.ShouldBeTrue("#845: a deployment error, not a bad credential"); } // ---------- anonymous as a normal principal (D3) ---------- 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 342c67c33..f135da93c 100644 --- a/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.IntegrationTests/CredentialLockoutWireTests.cs +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.IntegrationTests/CredentialLockoutWireTests.cs @@ -1,8 +1,10 @@ +using System.Collections.Concurrent; using System.Net; using System.Text; using Microsoft.Extensions.Logging.Abstractions; using Opc.Ua; using Opc.Ua.Client; +using Opc.Ua.Configuration; using Shouldly; using Xunit; using ZB.MOM.WW.OtOpcUa.Core.Authorization; @@ -14,7 +16,8 @@ namespace ZB.MOM.WW.OtOpcUa.OpcUaServer.IntegrationTests; /// lmxopcua#833 over the wire. A few failed UserName activations used to trip the OPC Foundation SDK's /// per-client lockout, which then refused EVERY activation from that client — Anonymous included — with /// BadUserAccessDenied for 5 minutes. The host now disables the SDK lockout and enforces a -/// UserName-only replacement (). +/// UserName-only replacement (), counted per credential identity as well as +/// per client (#844) and blind to transient denials (#845). /// public sealed class CredentialLockoutWireTests { @@ -27,7 +30,7 @@ public sealed class CredentialLockoutWireTests var auth = new CountingAuthenticator(); await using var rig = await Rig.StartAsync(auth, new OpcUaCredentialLockoutOptions(), ct); - for (var i = 0; i < 6; i++) + for (var i = 0; i < 12; i++) { var rejected = await Should.ThrowAsync(() => rig.ConnectAsync($"otua_k-bad{i}_x", ct)); rejected.StatusCode.ShouldBeOneOf(StatusCodes.BadIdentityTokenRejected, StatusCodes.BadUserAccessDenied); @@ -45,7 +48,7 @@ public sealed class CredentialLockoutWireTests var ct = TestContext.Current.CancellationToken; var auth = new CountingAuthenticator(); await using var rig = await Rig.StartAsync( - auth, new OpcUaCredentialLockoutOptions { MaxFailedAttempts = 3 }, ct); + auth, new OpcUaCredentialLockoutOptions { MaxFailedAttempts = 3, MaxFailedAttemptsPerClient = 3 }, ct); for (var i = 0; i < 3; i++) { @@ -59,6 +62,58 @@ public sealed class CredentialLockoutWireTests auth.UserNameCalls.ShouldBe(callsBefore, "a locked-out client's credential is refused without being checked"); } + /// + /// #844 over the wire — guesses at another key id interleaved with sign-ins on the client's own valid key. The + /// own-key success used to clear the whole count, so the cycle never locked. Now the victim key id locks for + /// that client after MaxFailedAttempts guesses (refused unauthenticated), while the client's own key still works. + /// + [Fact] + public async Task Own_key_successes_do_not_launder_guesses_at_another_key_id() + { + var ct = TestContext.Current.CancellationToken; + var auth = new CountingAuthenticator(); + await using var rig = await Rig.StartAsync(auth, new OpcUaCredentialLockoutOptions { MaxFailedAttempts = 3 }, ct); + + for (var round = 0; round < 3; round++) + { + var rejected = await Should.ThrowAsync( + () => rig.ConnectAsync($"otua_k-victim_guess{round}", ct)); + rejected.StatusCode.ShouldBe(StatusCodes.BadIdentityTokenRejected); + using var own = await rig.ConnectAsync(GoodKey, ct); + own.Connected.ShouldBeTrue(); + } + + var callsBefore = auth.UserNameCalls; + var locked = await Should.ThrowAsync(() => rig.ConnectAsync("otua_k-victim_guess3", ct)); + locked.StatusCode.ShouldBe(StatusCodes.BadUserAccessDenied); + auth.UserNameCalls.ShouldBe(callsBefore, "the locked key id is refused without being checked"); + + using var stillOwn = await rig.ConnectAsync(GoodKey, ct); + stillOwn.Connected.ShouldBeTrue("a per-credential lockout leaves the client's own key usable"); + } + + /// + /// #845 over the wire — a transient denial (the authenticator reports the key store unavailable) is not a bad + /// credential: any number of them leave the client able to sign in once the store recovers. + /// + [Fact] + public async Task Transient_denials_never_lock_the_client_out() + { + var ct = TestContext.Current.CancellationToken; + var auth = new CountingAuthenticator(); + await using var rig = await Rig.StartAsync(auth, new OpcUaCredentialLockoutOptions { MaxFailedAttempts = 2 }, ct); + + for (var i = 0; i < 6; i++) + { + var rejected = await Should.ThrowAsync(() => rig.ConnectAsync(StallingKey(i % 2), ct)); + rejected.StatusCode.ShouldBe(StatusCodes.BadIdentityTokenRejected, "the client sees the ordinary opaque denial"); + } + + using var session = await rig.ConnectAsync(GoodKey, ct); + session.Connected.ShouldBeTrue(); + auth.UserNameCalls.ShouldBe(7, "every transient attempt reached the authenticator: none were refused as locked out"); + } + /// /// #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. @@ -101,59 +156,181 @@ public sealed class CredentialLockoutWireTests session.Connected.ShouldBeTrue(); } + /// + /// #845 — on a Sign endpoint the client certificate is proven, so it separates clients that share an address: + /// a client locked out client-wide does not lock out a second client (its own certificate) from the same IP. + /// (On a None endpoint clients commonly present no certificate and the key is effectively the IP — see + /// docs/security.md.) + /// + [Fact] + public async Task On_a_Sign_endpoint_distinct_client_certificates_are_locked_out_separately() + { + var ct = TestContext.Current.CancellationToken; + var auth = new CountingAuthenticator(); + await using var rig = await Rig.StartAsync( + auth, new OpcUaCredentialLockoutOptions { MaxFailedAttempts = 2, MaxFailedAttemptsPerClient = 2 }, ct, + OpcUaSecurityProfile.Basic256Sha256Sign); + + for (var i = 0; i < 2; i++) + { + var rejected = await Should.ThrowAsync( + () => rig.ConnectAsync($"otua_k-bad{i}_x", ct, clientCertificate: "ClientA")); + rejected.StatusCode.ShouldBe(StatusCodes.BadIdentityTokenRejected); + } + + var locked = await Should.ThrowAsync( + () => rig.ConnectAsync(GoodKey, ct, clientCertificate: "ClientA")); + locked.StatusCode.ShouldBe(StatusCodes.BadUserAccessDenied, "client A is locked out client-wide"); + + using var other = await rig.ConnectAsync(GoodKey, ct, clientCertificate: "ClientB"); + other.Connected.ShouldBeTrue("same address, different (proven) certificate: a different client"); + other.Endpoint.SecurityMode.ShouldBe(MessageSecurityMode.Sign); + } + + /// + /// #845 — the OpcUa:ApplicationConfigPath branch loads an SDK XML configuration instead of building one. + /// That file may carry the SDK's own MaxFailedAuthenticationAttempts; the host must still switch it off, + /// or failed sign-ins would again lock the client out of ANONYMOUS activation. + /// + [Fact] + public async Task A_loaded_application_configuration_still_has_the_sdk_lockout_switched_off() + { + var ct = TestContext.Current.CancellationToken; + var auth = new CountingAuthenticator(); + await using var rig = await Rig.StartFromConfigFileAsync( + auth, new OpcUaCredentialLockoutOptions { MaxFailedAttempts = 0 }, sdkMaxFailedAttempts: 2, ct); + + rig.LoadedSdkMaxFailedAttempts.ShouldBe(0, "the host zeroed the value the file carried"); + for (var i = 0; i < 4; i++) + { + var rejected = await Should.ThrowAsync(() => rig.ConnectAsync($"otua_k-bad{i}_x", ct)); + rejected.StatusCode.ShouldBe(StatusCodes.BadIdentityTokenRejected, "the SDK lockout (2) would refuse these"); + } + + using var anonymous = await rig.ConnectAsync(credential: null, ct); + anonymous.Connected.ShouldBeTrue(); + using var keyed = await rig.ConnectAsync(GoodKey, ct); + keyed.Connected.ShouldBeTrue(); + } + + private static string StallingKey(int n) => $"otua_k-stall{n}_secret"; + private sealed class Rig : IAsyncDisposable { private readonly string _pkiRoot; + private readonly ConcurrentBag _clientApps = []; + private readonly ConcurrentDictionary _clientPkiRoots = new(StringComparer.Ordinal); private OpcUaApplicationHost _host = null!; private int _port; + private bool _secure; private Rig(string pkiRoot) => _pkiRoot = pkiRoot; + /// The SDK lockout value of the configuration the host actually runs with. + public int LoadedSdkMaxFailedAttempts => + _host.ApplicationInstance!.ApplicationConfiguration!.ServerConfiguration!.MaxFailedAuthenticationAttempts; + public static async Task StartAsync( - IOpcUaUserAuthenticator authenticator, OpcUaCredentialLockoutOptions lockout, CancellationToken ct) + IOpcUaUserAuthenticator authenticator, OpcUaCredentialLockoutOptions lockout, CancellationToken ct, + OpcUaSecurityProfile profile = OpcUaSecurityProfile.None) { - var rig = new Rig(Path.Combine(Path.GetTempPath(), $"otopcua-lockout-{Guid.NewGuid():N}")); + var rig = new Rig(Path.Combine(Path.GetTempPath(), $"otopcua-lockout-{Guid.NewGuid():N}")) + { + _secure = profile != OpcUaSecurityProfile.None, + }; var applicationUri = $"urn:OtOpcUa.Lockout.{Guid.NewGuid():N}"; (rig._host, _) = await OpcUaHostStart.StartAsync( port => { rig._port = port; - return new OpcUaApplicationHost( - new OpcUaApplicationHostOptions - { - ApplicationName = "OtOpcUa.Lockout", - ApplicationUri = applicationUri, - OpcUaPort = port, - PublicHostname = "127.0.0.1", - PkiStoreRoot = rig._pkiRoot, - EnabledSecurityProfiles = [OpcUaSecurityProfile.None], - AutoAcceptUntrustedClientCertificates = true, - CredentialLockout = lockout, - }, - NullLogger.Instance, - authenticator); + return new OpcUaApplicationHost(rig.Options(port, applicationUri, lockout, profile), + NullLogger.Instance, authenticator); }, () => new OtOpcUaSdkServer(), ct); return rig; } - /// Opens a session; a null credential activates anonymously. - public async Task ConnectAsync(string? credential, CancellationToken ct) + /// + /// Boots once to let the host build (and the SDK validate) a configuration, saves it as an SDK XML file + /// carrying , then boots the host from that file through + /// OpcUa:ApplicationConfigPath. + /// + public static async Task StartFromConfigFileAsync( + IOpcUaUserAuthenticator authenticator, OpcUaCredentialLockoutOptions lockout, int sdkMaxFailedAttempts, + CancellationToken ct) { - var appConfig = new ApplicationConfiguration(DefaultTelemetry.Create(_ => { })) + var seed = await StartAsync(authenticator, lockout, ct); + var configFile = Path.Combine(seed._pkiRoot, "OtOpcUa.Lockout.Config.xml"); + var applicationUri = seed._host.ApplicationInstance!.ApplicationConfiguration!.ApplicationUri; + var saved = seed._host.ApplicationInstance.ApplicationConfiguration; + await seed._host.DisposeAsync(); + + (seed._host, _) = await OpcUaHostStart.StartAsync( + port => + { + seed._port = port; + saved.ServerConfiguration!.BaseAddresses = [$"opc.tcp://127.0.0.1:{port}/OtOpcUa"]; + saved.ServerConfiguration.MaxFailedAuthenticationAttempts = sdkMaxFailedAttempts; + saved.SaveToFile(configFile); + var options = seed.Options(port, applicationUri!, lockout, OpcUaSecurityProfile.None); + options.ApplicationConfigPath = configFile; + return new OpcUaApplicationHost(options, NullLogger.Instance, authenticator); + }, + () => new OtOpcUaSdkServer(), ct); + File.ReadAllText(configFile).ShouldContain($"{sdkMaxFailedAttempts}<"); + return seed; + } + + private OpcUaApplicationHostOptions Options( + int port, string applicationUri, OpcUaCredentialLockoutOptions lockout, OpcUaSecurityProfile profile) => new() + { + ApplicationName = "OtOpcUa.Lockout", + ApplicationUri = applicationUri, + OpcUaPort = port, + PublicHostname = "127.0.0.1", + PkiStoreRoot = _pkiRoot, + EnabledSecurityProfiles = [profile], + AutoAcceptUntrustedClientCertificates = true, + CredentialLockout = lockout, + }; + + /// + /// Opens a session; a null credential activates anonymously. With the + /// client presents its own application certificate (one per name, created once) over a secure endpoint. + /// + public async Task ConnectAsync(string? credential, CancellationToken ct, string? clientCertificate = null) + { + var telemetry = DefaultTelemetry.Create(_ => { }); + var name = clientCertificate ?? "LockoutClient"; + var pki = clientCertificate is null + ? TestClientSecurity.AllocatePkiRoot() + : _clientPkiRoots.GetOrAdd(clientCertificate, _ => TestClientSecurity.AllocatePkiRoot()); + var security = TestClientSecurity.Build(pki); + security.ApplicationCertificate!.SubjectName = $"CN=OtOpcUa.{name}"; + var appConfig = new ApplicationConfiguration(telemetry) { - ApplicationName = "OtOpcUa.LockoutClient", - ApplicationUri = "urn:OtOpcUa.LockoutClient", + ApplicationName = $"OtOpcUa.{name}", + ApplicationUri = $"urn:localhost:OtOpcUa.{name}", ApplicationType = ApplicationType.Client, - SecurityConfiguration = TestClientSecurity.Build(TestClientSecurity.AllocatePkiRoot()), + SecurityConfiguration = security, ClientConfiguration = new ClientConfiguration { DefaultSessionTimeout = 60_000 }, }; await appConfig.ValidateAsync(ApplicationType.Client, ct); + if (clientCertificate is not null) + { + var app = new ApplicationInstance(telemetry) + { + ApplicationName = appConfig.ApplicationName, ApplicationType = ApplicationType.Client, + ApplicationConfiguration = appConfig, + }; + _clientApps.Add(app); + (await app.CheckApplicationInstanceCertificatesAsync(true, lifeTimeInMonths: 12, ct)).ShouldBeTrue(); + } var endpoint = await CoreClientUtils.SelectEndpointAsync( - appConfig, $"opc.tcp://127.0.0.1:{_port}/OtOpcUa", false, DefaultTelemetry.Create(_ => { }), ct); + appConfig, $"opc.tcp://127.0.0.1:{_port}/OtOpcUa", _secure, telemetry, ct); - return await new DefaultSessionFactory(DefaultTelemetry.Create(_ => { })).CreateAsync( + return await new DefaultSessionFactory(telemetry).CreateAsync( appConfig, new ConfiguredEndpoint(null, endpoint ?? throw new InvalidOperationException("no endpoint"), EndpointConfiguration.Create(appConfig)), updateBeforeConnect: false, @@ -170,15 +347,21 @@ public sealed class CredentialLockoutWireTests public async ValueTask DisposeAsync() { await _host.DisposeAsync(); - if (Directory.Exists(_pkiRoot)) + foreach (var app in _clientApps) await app.DisposeAsync(); + foreach (var dir in _clientPkiRoots.Values.Append(_pkiRoot)) { - try { Directory.Delete(_pkiRoot, recursive: true); } + if (!Directory.Exists(dir)) continue; + try { Directory.Delete(dir, recursive: true); } catch (IOException) { /* best effort */ } } } } - /// Admits and anonymous; denies everything else; counts calls. + /// + /// Admits and anonymous; reports the key store unavailable (Transient) for + /// s; denies everything else; counts calls. Names the credential by key id, as the + /// production API-key authenticator does. + /// private sealed class CountingAuthenticator : IOpcUaUserAuthenticator { private int _userNameCalls; @@ -190,8 +373,9 @@ public sealed class CredentialLockoutWireTests public Task AuthenticateUserNameAsync(string username, string password, CancellationToken ct) { Interlocked.Increment(ref _userNameCalls); - return Task.FromResult(password == GoodKey - ? OpcUaUserAuthResult.AllowKey("good", [], "k-good") + return Task.FromResult( + password == GoodKey ? OpcUaUserAuthResult.AllowKey("good", [], "k-good") + : password.StartsWith("otua_k-stall", StringComparison.Ordinal) ? OpcUaUserAuthResult.Unavailable("Invalid API key.") : OpcUaUserAuthResult.Deny("Invalid API key.")); } @@ -199,6 +383,12 @@ public sealed class CredentialLockoutWireTests string username, string password, IPEndPoint? remoteEndpoint, CancellationToken ct) => AuthenticateUserNameAsync(username, password, ct); + public string DescribeCredential(string username, string password) + { + var parts = password.Split('_'); + return parts.Length == 3 && parts[0] == "otua" ? "key:" + parts[1] : "user:" + username; + } + public Task AuthenticateAnonymousAsync(CancellationToken ct) { Interlocked.Increment(ref _anonymousCalls); 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 1bd619e8f..0c0d5cfdf 100644 --- a/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/FailedCredentialLockoutTests.cs +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/FailedCredentialLockoutTests.cs @@ -55,7 +55,7 @@ public sealed class FailedCredentialLockoutTests } [Fact] - public void A_success_clears_the_count() + public void A_success_clears_that_identitys_count() { var lockout = new FailedCredentialLockout(new OpcUaCredentialLockoutOptions(), new ManualClock()); for (var i = 0; i < 4; i++) lockout.RecordFailure(Client); @@ -186,10 +186,10 @@ public sealed class FailedCredentialLockoutTests 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 + lockout.Complete(Client, succeeded: true); // a success clears the identity's count and hands the slot on (await second).Verdict.ShouldBe(CredentialLockoutVerdict.Proceed); lockout.Complete(Client, succeeded: true); - lockout.TrackedCount.ShouldBe(0); + lockout.TrackedCount.ShouldBe(1, "#844: the client-wide count survives a success until it ages out"); } /// A cancelled waiter is dequeued and leaves nothing behind (no leaked slot, entry prunable). @@ -242,10 +242,226 @@ public sealed class FailedCredentialLockoutTests clock.Advance(TimeSpan.FromSeconds(11)); (await lockout.BeginAsync(Client, Ct)).Verdict.ShouldBe(CredentialLockoutVerdict.Proceed); - lockout.Complete(Client, succeeded: false).ShouldBeTrue("a new episode"); + lockout.Complete(Client, succeeded: false).ShouldNotBe(CredentialLockoutScope.None, "a new episode"); (await lockout.BeginAsync(Client, Ct)).FirstRefusal.ShouldBeTrue("each lockout episode logs its first refusal once"); } + // ---- lmxopcua#844: per-identity counts + a client-wide cap a success never resets ---- + + /// + /// #844 — the laundering attack: four guesses at another key id, then a sign-in with the client's own valid + /// key, repeated. The success used to clear the whole per-client count, so this ran forever. Now the success + /// clears only the client's own identity: the fifth guess at the victim locks that identity. + /// + [Fact] + public void A_success_with_one_key_does_not_launder_guesses_at_another() + { + var lockout = new FailedCredentialLockout(new OpcUaCredentialLockoutOptions(), new ManualClock()); + for (var i = 0; i < 4; i++) lockout.RecordFailure(Client, "key:victim").ShouldBeFalse(); + lockout.RecordSuccess(Client, "key:own"); + lockout.RecordFailure(Client, "key:victim").ShouldBeTrue("the own-key success did not touch the victim's count"); + + lockout.IsLockedOut(Client, "key:victim", out var remaining).ShouldBeTrue(); + remaining.ShouldBe(TimeSpan.FromMinutes(5)); + lockout.IsLockedOut(Client, "key:own", out _).ShouldBeFalse( + "a per-credential lockout leaves the client's other credentials usable"); + } + + /// + /// #844 — spraying a few guesses at each of many key ids, with own-key successes in between, still hits the + /// client-wide cap (default 2 × MaxFailedAttempts), which a success never resets; then every credential — + /// the client's own valid key included — is refused. + /// + [Fact] + public async Task Spraying_many_identities_with_successes_in_between_hits_the_client_wide_cap() + { + var lockout = new FailedCredentialLockout(new OpcUaCredentialLockoutOptions(), new ManualClock()); + lockout.ClientThreshold.ShouldBe(10); + var locked = false; + var failures = 0; + for (var target = 0; !locked && target < 10; target++) + { + for (var guess = 0; guess < 4 && !locked; guess++) + { + locked = lockout.RecordFailure(Client, $"key:victim{target}"); + failures++; + } + + lockout.RecordSuccess(Client, "key:own"); + } + + locked.ShouldBeTrue(); + failures.ShouldBe(10, "the tenth failure across identities locks the client"); + lockout.IsLockedOut(Client, "key:own", out _).ShouldBeTrue("a client-wide lockout covers the client's own key too"); + var refused = await lockout.BeginAsync(Client, "key:own", Ct); + refused.Verdict.ShouldBe(CredentialLockoutVerdict.LockedOut); + refused.Scope.ShouldBe(CredentialLockoutScope.Client); + } + + [Fact] + public void An_explicit_per_client_cap_is_honoured() + { + var lockout = new FailedCredentialLockout( + new OpcUaCredentialLockoutOptions { MaxFailedAttempts = 5, MaxFailedAttemptsPerClient = 6 }, new ManualClock()); + for (var i = 0; i < 5; i++) lockout.RecordFailure(Client, $"key:k{i}").ShouldBeFalse(); + lockout.RecordFailure(Client, "key:k5").ShouldBeTrue(); + lockout.IsLockedOut(Client, "key:anything", out _).ShouldBeTrue(); + } + + /// The client-wide count also ages out through the failure window — it is not a lifetime count. + [Fact] + public void The_client_wide_count_ages_out_through_the_window() + { + var clock = new ManualClock(); + var lockout = new FailedCredentialLockout(new OpcUaCredentialLockoutOptions(), clock); + for (var i = 0; i < 9; i++) lockout.RecordFailure(Client, $"key:k{i}"); + clock.Advance(TimeSpan.FromSeconds(61)); + for (var i = 0; i < 9; i++) lockout.RecordFailure(Client, $"key:j{i}").ShouldBeFalse(); + lockout.IsLockedOut(Client, "key:own", out _).ShouldBeFalse(); + } + + /// + /// #844 — a client with a stale (revoked) key and a good one: the stale key locks only itself; the good key's + /// activations keep getting slots and are not held behind the stale identity's waiter. + /// + [Fact] + public async Task A_locked_identity_does_not_block_another_identitys_slots() + { + var lockout = new FailedCredentialLockout(new OpcUaCredentialLockoutOptions { MaxFailedAttempts = 2 }, new ManualClock()); + lockout.RecordFailure(Client, "key:stale"); + + (await lockout.BeginAsync(Client, "key:stale", Ct)).Verdict.ShouldBe(CredentialLockoutVerdict.Proceed); + var staleWaiter = lockout.BeginAsync(Client, "key:stale", Ct).AsTask(); // no stale slot left: waits + await WaitUntilAsync(() => lockout.WaitingCount(Client) == 1); + + // The good key is not queued behind the stale waiter. + (await lockout.BeginAsync(Client, "key:good", Ct)).Verdict.ShouldBe(CredentialLockoutVerdict.Proceed); + + lockout.Complete(Client, "key:stale", succeeded: false).ShouldBe(CredentialLockoutScope.Credential); + var refused = await staleWaiter; + refused.Verdict.ShouldBe(CredentialLockoutVerdict.LockedOut, "the waiter's identity locked: refused unauthenticated"); + refused.Scope.ShouldBe(CredentialLockoutScope.Credential); + + lockout.Complete(Client, "key:good", succeeded: true).ShouldBe(CredentialLockoutScope.None); + (await lockout.BeginAsync(Client, "key:good", Ct)).Verdict.ShouldBe(CredentialLockoutVerdict.Proceed); + lockout.Complete(Client, "key:good", succeeded: true); + } + + /// A parallel burst of guesses at one identity still reaches the authenticator only MaxFailedAttempts times. + [Fact] + public async Task A_parallel_burst_at_one_identity_is_capped_at_the_per_credential_threshold() + { + var lockout = new FailedCredentialLockout(new OpcUaCredentialLockoutOptions { MaxFailedAttempts = 3 }, new ManualClock()); + var release = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + var authenticated = 0; + var attempts = Enumerable.Range(0, 10).Select(_ => Task.Run(async () => + { + var decision = await lockout.BeginAsync(Client, "key:victim", Ct); + if (decision.Verdict != CredentialLockoutVerdict.Proceed) return; + Interlocked.Increment(ref authenticated); + await release.Task; + lockout.Complete(Client, "key:victim", succeeded: false); + }, Ct)).ToArray(); + + await WaitUntilAsync(() => lockout.WaitingCount(Client) == 7); + authenticated.ShouldBe(3); + release.SetResult(); + await Task.WhenAll(attempts); + authenticated.ShouldBe(3); + lockout.IsLockedOut(Client, "key:victim", out _).ShouldBeTrue(); + } + + /// Identities beyond the per-client bound share one overflow count — conservative, never laxer. + [Fact] + public void Identities_beyond_the_per_client_bound_share_one_overflow_count() + { + var lockout = new FailedCredentialLockout( + new OpcUaCredentialLockoutOptions { MaxFailedAttempts = 2, MaxFailedAttemptsPerClient = 1000 }, new ManualClock()); + for (var i = 0; i < FailedCredentialLockout.MaxTrackedIdentitiesPerClient - 1; i++) + lockout.RecordFailure(Client, $"key:k{i}").ShouldBeFalse(); + + lockout.RecordFailure(Client, "key:overflow-a").ShouldBeFalse(); + lockout.RecordFailure(Client, "key:overflow-b").ShouldBeTrue("both untracked identities share the overflow count"); + lockout.IsLockedOut(Client, "key:overflow-c", out _).ShouldBeTrue(); + lockout.IsLockedOut(Client, "key:k0", out _).ShouldBeFalse("tracked identities keep their own counts"); + } + + [Fact] + public void Oversized_identities_are_bounded_but_stay_distinct() + { + var lockout = new FailedCredentialLockout(new OpcUaCredentialLockoutOptions { MaxFailedAttempts = 1 }, new ManualClock()); + var a = "key:" + new string('a', 1000) + "1"; + var b = "key:" + new string('a', 1000) + "2"; + lockout.RecordFailure(Client, a).ShouldBeTrue(); + lockout.IsLockedOut(Client, a, out _).ShouldBeTrue(); + lockout.IsLockedOut(Client, b, out _).ShouldBeFalse(); + } + + // ---- lmxopcua#845: transient outcomes are uncounted; a full table backs off its prune ---- + + /// + /// #845 — a transient denial (store timeout, miss budget, authenticator fault) is released with Abandon and + /// never counts: a ConfigDb stall cannot lock the client out. + /// + [Fact] + public async Task Abandoned_attempts_never_count_toward_either_threshold() + { + var lockout = new FailedCredentialLockout(new OpcUaCredentialLockoutOptions { MaxFailedAttempts = 2 }, new ManualClock()); + for (var i = 0; i < 20; i++) + { + (await lockout.BeginAsync(Client, "key:k", Ct)).Verdict.ShouldBe(CredentialLockoutVerdict.Proceed); + lockout.Abandon(Client, "key:k"); + } + + lockout.IsLockedOut(Client, "key:k", out _).ShouldBeFalse(); + lockout.TrackedCount.ShouldBe(0); + } + + /// + /// #845 — at a full table with nothing expired, a new client key's failure used to rescan all 4096 entries + /// under the global lock every time. The prune now waits until the earliest entry could have expired. + /// + [Fact] + public void A_full_table_does_not_rescan_until_the_earliest_entry_could_expire() + { + var clock = new ManualClock(); + var lockout = new FailedCredentialLockout(new OpcUaCredentialLockoutOptions(), clock); + for (var i = 0; i < FailedCredentialLockout.MaxTrackedClients; i++) + { + lockout.RecordFailure($"c{i}"); + if (i == 0) clock.Advance(TimeSpan.FromSeconds(10)); // c0 is the earliest to age out + } + + for (var i = 0; i < 100; i++) lockout.RecordFailure($"new{i}"); + lockout.PruneScanCount.ShouldBe(1, "one scan found nothing to free; the next 99 did not rescan"); + lockout.TrackedCount.ShouldBe(FailedCredentialLockout.MaxTrackedClients); + + clock.Advance(TimeSpan.FromSeconds(51)); // c0's 60 s window has now passed; nobody else's has + lockout.RecordFailure("late").ShouldBeFalse(); + lockout.PruneScanCount.ShouldBe(2); + lockout.TrackedCount.ShouldBe(FailedCredentialLockout.MaxTrackedClients, "c0 pruned, 'late' took its place"); + lockout.RecordFailure("later"); + lockout.PruneScanCount.ShouldBe(3, "the table was full again: one scan…"); + lockout.RecordFailure("latest"); + lockout.PruneScanCount.ShouldBe(3, "…and then it backs off again"); + } + + /// A release that frees an entry lets a full table prune again immediately. + [Fact] + public async Task Freeing_an_entry_resets_the_prune_backoff() + { + var lockout = new FailedCredentialLockout(new OpcUaCredentialLockoutOptions(), new ManualClock()); + (await lockout.BeginAsync("holder", Ct)).Verdict.ShouldBe(CredentialLockoutVerdict.Proceed); + for (var i = 1; i < FailedCredentialLockout.MaxTrackedClients; i++) lockout.RecordFailure($"c{i}"); + lockout.RecordFailure("blocked"); + lockout.PruneScanCount.ShouldBe(1); + + lockout.Complete("holder", succeeded: true); // removes the holder's entry + for (var i = 0; i < 4; i++) lockout.RecordFailure("admitted").ShouldBeFalse(); + lockout.RecordFailure("admitted").ShouldBeTrue("tracked now: its count reached the threshold"); + } + + private static void InterlockedMax(ref int target, int value) { int seen; diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/OpcUaApplicationHostImpersonationTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/OpcUaApplicationHostImpersonationTests.cs index 4a76a34a7..4b31cdd1f 100644 --- a/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/OpcUaApplicationHostImpersonationTests.cs +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/OpcUaApplicationHostImpersonationTests.cs @@ -116,10 +116,43 @@ public sealed class OpcUaApplicationHostImpersonationTests var args = new ImpersonateEventArgs(token, UserNamePolicy, new EndpointDescription()); var authenticator = new ThrowingAuthenticator(new InvalidOperationException("LDAP unreachable")); - OpcUaApplicationHost.HandleImpersonation(authenticator, args, NullLogger.Instance); + var transient = OpcUaApplicationHost.HandleImpersonation(authenticator, args, NullLogger.Instance); args.Identity.ShouldBeNull(); args.IdentityValidationError.Code.ShouldBe(StatusCodes.BadIdentityTokenRejected.Code); + transient.ShouldBeTrue("#845: an authenticator fault is not a verdict on the credential"); + } + + /// + /// #845 — a denial the authenticator marks Transient (store timeout, miss-budget refusal) is reported as such, + /// so the lockout does not count it, while the client sees the same opaque rejection as a bad credential. + /// + [Fact] + public void HandleImpersonation_reports_a_transient_denial_and_a_definite_one_apart() + { + OpcUaUserAuthResult[] outcomes = [OpcUaUserAuthResult.Unavailable("Invalid API key."), OpcUaUserAuthResult.Deny("Invalid API key.")]; + var verdicts = outcomes.Select(outcome => + { + var token = new UserNameIdentityTokenHandler(new UserNameIdentityToken { UserName = "apikey" }) { DecryptedPassword = Encoding.UTF8.GetBytes("otua_k1_x") }; + var args = new ImpersonateEventArgs(token, UserNamePolicy, new EndpointDescription()); + var transient = OpcUaApplicationHost.HandleImpersonation(new RecordingAuthenticator(outcome), args, NullLogger.Instance); + args.IdentityValidationError.Code.ShouldBe(StatusCodes.BadIdentityTokenRejected.Code); + args.IdentityValidationError.LocalizedText.Text!.ShouldContain("Invalid API key."); + return transient; + }).ToArray(); + + verdicts.ShouldBe([true, false]); + } + + /// #844 — the default credential identity is the username; a faulting implementation falls back to it. + [Fact] + public void DescribeCredential_defaults_to_the_username_and_never_throws() + { + var handler = new UserNameIdentityTokenHandler(new UserNameIdentityToken { UserName = "alice" }) { DecryptedPassword = Encoding.UTF8.GetBytes("pw") }; + OpcUaApplicationHost.DescribeCredential(new RecordingAuthenticator(OpcUaUserAuthResult.Deny("x")), handler) + .ShouldBe("user:alice"); + OpcUaApplicationHost.DescribeCredential(new DescribingAuthenticator(), handler).ShouldBe("custom:alice:pw".Length.ToString()); + OpcUaApplicationHost.DescribeCredential(new DescribingAuthenticator(fail: true), handler).ShouldBe("user:alice"); } /// Verifies null username is normalized to empty string before authenticator call. @@ -167,6 +200,15 @@ public sealed class OpcUaApplicationHostImpersonationTests } } + private sealed class DescribingAuthenticator(bool fail = false) : IOpcUaUserAuthenticator + { + public Task AuthenticateUserNameAsync(string username, string password, CancellationToken ct) => + Task.FromResult(OpcUaUserAuthResult.Deny("x")); + + public string DescribeCredential(string username, string password) => + fail ? throw new InvalidOperationException("boom") : $"custom:{username}:{password}".Length.ToString(); + } + /// Test authenticator that throws an exception during authentication. private sealed class ThrowingAuthenticator(Exception ex) : IOpcUaUserAuthenticator { From 91e28e87bef4351929dad0b0156c19ff667b4bb6 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Wed, 11 Nov 2026 04:06:28 -0500 Subject: [PATCH 2/2] fix(opcua): condition events never block under the node-manager lock; folder-event Part 3 note (#838) #838(a): each notifier's SDK MonitoredNode2 drains a bounded channel (4096, FullMode=Wait) and the synchronous NodeState.ReportEvent blocks when it is full. Condition events are reported under Lock, and the delivery task takes Lock on a verdict-cache miss (the SDK's own GetNodeMetadata, flag off too, and #826's IsNotifierEventVisibleTo). A burst of >4096 events under one Lock hold therefore deadlocked the node manager -- FolderEventFloodWireTests stalled at exactly 4097 events with enforcement on AND off (so it predates #826). ReportConditionEvent now goes through ReportEventWithoutBlocking: ReportEventAsync completes inline while channels have room (unchanged normal path); on overflow the report continues asynchronously holding no thread or lock, and later reports queue behind it so events keep report order. The deferred work holds only frozen snapshots and drains once Lock is released; client delivery stays bounded by each item's queue. #838(b): documented in docs/security.md that Part 3 ReceiveEvents applies to the event's EventType and SourceNode (the SDK check, kept first), not to the notifier; our decision is layered on top and non-condition events on a folder item fail closed under enforcement. The reflection item stays open. --- .claude/rules/data-plane-auth.md | 7 + docs/security.md | 35 +++++ .../OtOpcUaNodeManager.cs | 91 +++++++++++- .../FolderEventFloodWireTests.cs | 136 ++++++++++++++++++ 4 files changed, 268 insertions(+), 1 deletion(-) create mode 100644 tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.IntegrationTests/FolderEventFloodWireTests.cs diff --git a/.claude/rules/data-plane-auth.md b/.claude/rules/data-plane-auth.md index 4b80ca976..5c645a617 100644 --- a/.claude/rules/data-plane-auth.md +++ b/.claude/rules/data-plane-auth.md @@ -204,6 +204,13 @@ Two things to know before relying on any of this: enforcement (`ServerEventRig.TagReferences()`); null ⇒ none (fail closed). Tests: `UnsFolderScopeWireTests.A_moved_alarm_stops_reaching_its_old_folders_subscribers…`, `NativeAlarmFolderEventAclWireTests`. + ⚠️ **Never report an event with the synchronous `NodeState.ReportEvent` while holding `Lock` (#838).** Each + notifier's `MonitoredNode2` drains a BOUNDED channel (4096, `FullMode = Wait`) and its delivery task takes `Lock` on a + verdict-cache miss (SDK `GetNodeMetadata` — flag off too — and #826's check); sync `ReportEvent` blocks on a full + channel ⇒ a >4096 burst under one `Lock` hold deadlocked the node manager (stalled at 4097). Condition events go + through `ReportEventWithoutBlocking` (async on overflow, ordered chain). Folder events follow Part 3's shape — + `ReceiveEvents` on EventType + SourceNode (SDK check, kept first) AND our decision; non-condition events on a + folder item fail closed under enforcement. Tests: `FolderEventFloodWireTests`. - **ns=0 standard nodes stay ungated by design, even with `Security:Acl:Enabled`** (#617) — `ServerStatus`, `NamespaceArray`, `ServiceLevel` etc. are served by the SDK's core node manager outside our authorizer and clients read them to connect and to judge redundancy health; they carry no plant data. Every raw/UNS node, diff --git a/docs/security.md b/docs/security.md index 9db3f91ea..6a5fc4d48 100644 --- a/docs/security.md +++ b/docs/security.md @@ -825,6 +825,41 @@ Behaviours worth knowing before enabling it: enforcement only while the installed generation carries that reference — a hand-built authority (tests) must supply `tagReferencingEquipment`. Inert with the flag off. + **Which permissions an event on a folder follows — checked against Part 3 (Gitea #838).** OPC UA Part 3's + `RolePermissions` model gives events their own permission, `ReceiveEvents` (PermissionType bit 11), and the pinned + SDK (2.0.1, `CustomNodeManager2.ValidateEventReceivePermissions`, documented there against Part 3 §8.55) applies it to + **both the event's EventType and its SourceNode** — not to the notifier the item happens to sit on. The notifier's own + permission only decides whether the item may be created/attached. So the spec's intent is *the folder AND the + source*, never "an event reported through a folder inherits the folder's permissions". This server keeps that shape: + the re-implemented `ValidateEventRolePermissions` runs the SDK's type + SourceNode check **first** and our decision + only on top (both must pass). For the event kinds that reach folder items: + - **Condition events** — SourceNode is the condition. Our decision is the condition rule above (the condition, or a + folder it legitimately notifies through for the session), which in this ACL model is the deliberate + generalisation of "the source": a `Tag` grant covers a native alarm's raw node and every UNS projection of it, and + an `Equipment` grant covers the alarms it owns or references. + - **A condition's Part 9 method audit events** (Acknowledge, Comment, Shelve…) — SourceNode is the condition, so they + are resolved to it and follow exactly the condition rule (plus the SDK's audit/SignAndEncrypt guard on the + Server-object path). + - **Any other event on a folder item** (none is produced today; `GeneralModelChangeEvent` and the rejected-write + `AuditWriteUpdateEvent` go to the Server object, #588) names no live condition and is **withheld** under + enforcement — fail closed, which is stricter than the folder's permission and never laxer than Part 3. + With the flag off only the SDK's own type + SourceNode `ReceiveEvents` check applies, as before. + + **Event bursts never block under the node-manager lock (Gitea #838).** Each notifier with event items has an SDK + `MonitoredNode2` whose delivery task drains a **bounded** channel (4096, `FullMode = Wait`), and the synchronous + `NodeState.ReportEvent` **blocks** while that channel is full. Condition events are reported under the node + manager's `Lock`, and the delivery task needs that same `Lock` on every per-event verdict-cache miss — in the SDK's + own `GetNodeMetadata` (so **with the flag off too**, i.e. this predates #826) and in the #826 check. A burst of more + than 4096 events under one hold of `Lock`, starting at a miss (a new item, or right after a deploy invalidated the + verdicts), therefore deadlocked the node manager: `FolderEventFloodWireTests` stalled at exactly 4097 events, flag on + and off. Condition events now go through `ReportEventWithoutBlocking`: `ReportEventAsync` completes inline while the + channels have room (the normal case, unchanged); when one is full that report continues asynchronously, holding no + thread and no lock, and later reports queue behind it so events still leave **in report order**. The deferred work + holds only frozen snapshots — bounded by the burst — and drains once `Lock` is released; what a client receives is + still bounded by its item's queue (oldest discarded). Pinned by `FolderEventFloodWireTests` (6000 events under one + `Lock` hold with publishing disabled: completes, deferred reports drain to zero, the item delivers at most its + revised `QueueSize` in increasing order ending with the last event, and the next event is delivered). + **Model-change and audit events on the Server object are gated too** (Gitea #588). The node manager's own server-wide events — `GeneralModelChangeEvent` (NodeAdded / NodeDeleted / DataTypeChanged) and the `AuditWriteUpdateEvent` of a rejected device write — used to go straight to `Server.ReportEvent`, where diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/OtOpcUaNodeManager.cs b/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/OtOpcUaNodeManager.cs index 62489d97c..84ee718bb 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/OtOpcUaNodeManager.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/OtOpcUaNodeManager.cs @@ -1362,7 +1362,7 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2, INodeManager3 // subscriptions, per item, through the ACL (see OnConditionReportEvent). var snapshot = new InstanceStateSnapshot(); snapshot.Initialize(SystemContext, alarm); - alarm.ReportEvent(SystemContext, snapshot); + ReportEventWithoutBlocking(alarm, snapshot); } catch (Exception ex) { @@ -1375,6 +1375,95 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2, INodeManager3 } } + /// Serializes . + private readonly object _eventReportGate = new(); + + /// The tail of the deferred condition-event reports (Gitea #838); completed when none is pending. + private Task _eventReportChain = Task.CompletedTask; + + private int _deferredEventReports; + + /// Condition-event reports currently deferred because a notifier's delivery channel was full (test observation). + internal int DeferredEventReports => Volatile.Read(ref _deferredEventReports); + + /// + /// Reports a condition event through the condition's notifier chain WITHOUT blocking the caller, in report + /// order (Gitea #838). + /// + /// The condition. + /// The frozen snapshot of the event. + /// + /// + /// Why not NodeState.ReportEvent. Each notifier with event items has an SDK + /// MonitoredNode2 whose delivery task drains a BOUNDED channel (4096, FullMode = Wait), and + /// the synchronous ReportEvent BLOCKS when that channel is full. Condition events are reported + /// under Lock, and the delivery task takes Lock on every per-event verdict-cache miss (the + /// SDK's own GetNodeMetadata, and #826's ). So a burst of more + /// than 4096 events under one hold of Lock, starting at a miss, parked the consumer on Lock + /// while the producer waited on the consumer: a deadlock that froze the whole node manager (pinned by + /// FolderEventFloodWireTests, which stalled at exactly 4097 events before this). + /// + /// + /// What this does. The common case is unchanged and synchronous: ReportEventAsync completes + /// inline while every channel has room. Only when a channel is full does that report continue + /// asynchronously (awaiting channel space, holding no thread and no lock), and every later report queues + /// behind it, so events still leave in the order they were reported. The deferred work holds only frozen + /// snapshots, so it is bounded by the burst that caused it and drains once Lock is released. + /// Delivery to clients stays bounded by each monitored item's own queue (oldest discarded). A deferred + /// report runs the condition's synchronous OnReportEvent hook () + /// off Lock; that hook takes Lock itself where it reads guarded state. + /// + /// + private void ReportEventWithoutBlocking(NodeState source, IFilterTarget e) + { + lock (_eventReportGate) + { + if (_eventReportChain.IsCompleted) + { + var report = source.ReportEventAsync(SystemContext, e); + if (report.IsCompletedSuccessfully) return; + Interlocked.Increment(ref _deferredEventReports); + _eventReportChain = ObserveEventReportAsync(report.AsTask(), source); + return; + } + + Interlocked.Increment(ref _deferredEventReports); + _eventReportChain = ReportAfterAsync(_eventReportChain, source, e); + } + } + + private async Task ReportAfterAsync(Task previous, NodeState source, IFilterTarget e) + { + await previous.ConfigureAwait(false); // never faults: every link observes its own failure + Task report; + try + { + report = source.ReportEventAsync(SystemContext, e).AsTask(); + } + catch (Exception ex) + { + report = Task.FromException(ex); + } + + await ObserveEventReportAsync(report, source).ConfigureAwait(false); + } + + private async Task ObserveEventReportAsync(Task report, NodeState source) + { + try + { + await report.ConfigureAwait(false); + } + catch (Exception ex) + { + _logger.LogError(ex, "OtOpcUaNodeManager: failed to report Part 9 condition event for {NodeId}", source.NodeId); + } + finally + { + Interlocked.Decrement(ref _deferredEventReports); + } + } + /// /// The gate-relevant slice of a Part 9 condition's state — exactly the fields that drive a /// condition event AND that an can change. As a record, two diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.IntegrationTests/FolderEventFloodWireTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.IntegrationTests/FolderEventFloodWireTests.cs new file mode 100644 index 000000000..60bc81b50 --- /dev/null +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.IntegrationTests/FolderEventFloodWireTests.cs @@ -0,0 +1,136 @@ +using Opc.Ua; +using Opc.Ua.Client; +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.Commons.OpcUa; +using ZB.MOM.WW.OtOpcUa.Configuration.Entities; +using ZB.MOM.WW.OtOpcUa.Configuration.Enums; +using ZB.MOM.WW.OtOpcUa.Core.Abstractions; +using ZB.MOM.WW.OtOpcUa.Core.Authorization; + +namespace ZB.MOM.WW.OtOpcUa.OpcUaServer.IntegrationTests; + +/// +/// Gitea #838 (follow-up to #826): the per-folder event ACL runs on the SDK's MonitoredNode2 delivery task, +/// and needs the node manager's Lock on a verdict-cache miss. That delivery task drains a BOUNDED channel +/// (4096, FullMode = Wait), and the synchronous NodeState.ReportEvent the node manager calls under +/// Lock BLOCKS when the channel is full. So a burst of more than 4096 events reported under one hold of +/// Lock, starting at a verdict-cache miss, parks the consumer on Lock while the producer waits on the +/// consumer — a deadlock that also freezes every other operation needing Lock. Publishing is switched off +/// on the client so nothing downstream drains either (the slow / absent consumer case). +/// +public sealed class FolderEventFloodWireTests +{ + private const string EqA = ServerEventRig.EqA; + private const int FloodSize = 6000; // > the SDK's 4096-slot per-node channel + + /// + /// Per-folder ACL on or off. The hazard does NOT need #826: with enforcement off the SDK's own + /// ValidateEventRolePermissions still reaches GetNodeMetadata, which takes Lock, on the + /// first event to a new item. + /// + [Theory] + [InlineData(true)] + [InlineData(false)] + public async Task A_flood_beyond_the_channel_under_the_node_manager_lock_neither_deadlocks_nor_grows_unbounded(bool enforced) + { + var ct = TestContext.Current.CancellationToken; + var rig = await ServerEventRig.StartAsync("Folder838Flood", encrypted: false, ct); + var deadlocked = false; + ISession? keyed = null; + var reported = 0; + try + { + rig.KeyRoles = []; + rig.MaterialiseNative(); + NodeAcl[] grants = [KeyEquipmentGrant(EqA)]; + rig.Enforce(enforced, [ServerEventRig.Key()], grants); + + keyed = await rig.ConnectAsync(anonymous: false, ct); + var events = new RecordedEvents(); + var subscription = await RecordedEvents.SubscribeAsync(keyed, events, ct); + var onA = RecordedEvents.AddEventItem(subscription, ServerEventRig.UnsNodeId(keyed, EqA)); + await subscription.ApplyChangesAsync(ct); + onA.Status.Error.ShouldBeNull(); + + // No publishing: the server-side item queue (QueueSize 1000) is the only sink, as with a stalled client. + await subscription.SetPublishingModeAsync(false, ct); + + // The whole burst under ONE hold of the node manager's Lock — the shape of any multi-event apply/projection + // pass — starting from an uncached verdict (the item is new), so the delivery task needs Lock for event 1. + var flood = Task.Factory.StartNew(() => + { + lock (rig.Nm.Lock) + { + for (var i = 0; i < FloodSize; i++) + { + rig.Project(native: true, State($"f{i}")); + Volatile.Write(ref reported, i + 1); + } + } + }, ct, TaskCreationOptions.LongRunning, TaskScheduler.Default); + + try + { + await flood.WaitAsync(TimeSpan.FromSeconds(60), ct); + } + catch (TimeoutException) + { + deadlocked = true; + throw new ShouldAssertException( + $"reporting {FloodSize} folder events under the node manager's Lock stalled after " + + $"{Volatile.Read(ref reported)} — the SDK's per-node event channel filled while its consumer waited " + + "on Lock (deadlock)"); + } + + // The overflow was deferred rather than blocking, and drains once Lock is free. + await RecordedEvents.WaitUntilAsync(() => rig.Nm.DeferredEventReports == 0); + // …and the SDK's channel (up to 4096 accepted events) finishes draining into the item queue, still with + // publishing off, so the count below measures the item queue and not events in transit. + await Task.Delay(TimeSpan.FromSeconds(2), ct); + + // Delivery is bounded by the item's queue (QueueSize 1000, oldest discarded), not by the flood, and the + // events that survive arrive in report order, ending with the last one reported. + await subscription.SetPublishingModeAsync(true, ct); + var last = $"f{FloodSize - 1}"; + await RecordedEvents.WaitUntilAsync(() => events.Has(onA, last)); + await Task.Delay(TimeSpan.FromMilliseconds(500), ct); + var flooded = events.Of(onA) + .Select(RecordedEvents.Message) + .Where(m => m?.StartsWith('f') == true) + .Select(m => int.Parse(m![1..], System.Globalization.CultureInfo.InvariantCulture)) + .ToList(); + flooded.Count.ShouldBeGreaterThan(0); + flooded.Count.ShouldBeLessThanOrEqualTo((int)onA.Status.QueueSize, "delivery is bounded by the item's queue, not the flood"); + flooded.ShouldBe(flooded.Order().ToList(), "events leave in the order they were reported"); + flooded[^1].ShouldBe(FloodSize - 1); + + // And the node manager is live: a later event is decided by the per-folder ACL and delivered. + rig.Project(native: true, State("after")); + await RecordedEvents.WaitUntilAsync(() => events.Has(onA, "after")); + } + finally + { + // A deadlocked rig cannot be disposed (its Lock is held by a blocked thread) and neither can a session on it + // (CloseSession needs the server); leak both rather than hang the run. + if (!deadlocked) + { + keyed?.Dispose(); + await rig.DisposeAsync(); + } + } + } + + private static AlarmConditionSnapshot State(string message) => + new(true, false, false, true, AlarmShelvingKind.Unshelved, 700, message); + + private static NodeAcl KeyEquipmentGrant(string equipment) => new() + { + NodeAclId = $"acl-key-{equipment}", + ClusterId = ServerEventRig.Cluster, + Principal = ServerEventRig.KeyPrincipal, + ScopeKind = NodeAclScopeKind.Equipment, + ScopeId = $"eq-{equipment}", + PermissionFlags = NodePermissions.ReadOnly, + }; +}