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 {