From 25a1e6114145cd993d7b111ae148c6b11ebc428b Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Wed, 11 Nov 2026 01:23:33 -0500 Subject: [PATCH 1/7] fix(adminui): render the equipment page confirm outside the tab branches (#832) The shared RawConfirmDialog sat inside the Alarms tab branch, so Remove reference (Tags) and Delete virtual tag (Virtual Tags) set it visible on a page that did not render it. Pinned by brace depth in DeleteConfirmationWiringTests. --- .../Components/Pages/Uns/EquipmentPage.razor | 12 +++++---- .../Uns/DeleteConfirmationWiringTests.cs | 25 +++++++++++++++++++ 2 files changed, 32 insertions(+), 5 deletions(-) diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Pages/Uns/EquipmentPage.razor b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Pages/Uns/EquipmentPage.razor index 105fa06b1..e3adbff5b 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Pages/Uns/EquipmentPage.razor +++ b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Pages/Uns/EquipmentPage.razor @@ -311,12 +311,14 @@ else - - @* One confirm for every destructive row action on this page (R04 H1) — the same RawConfirmDialog - /raw routes its deletes through. A mis-click used to delete immediately. *@ - } + + @* One confirm for every destructive row action on this page (R04 H1) — the same RawConfirmDialog + /raw routes its deletes through. A mis-click used to delete immediately. It must render OUTSIDE the + tab branches (#832): inside the Alarms branch it existed only on that tab, so the Tags tab's Remove + and the Virtual Tags tab's Delete opened a dialog that was never on the page. *@ + } diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Uns/DeleteConfirmationWiringTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Uns/DeleteConfirmationWiringTests.cs index 3505c267b..767554a60 100644 --- a/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Uns/DeleteConfirmationWiringTests.cs +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Uns/DeleteConfirmationWiringTests.cs @@ -28,6 +28,31 @@ public sealed class DeleteConfirmationWiringTests Regex.Matches(src, "btn-outline-danger").Count.ShouldBe(3); } + /// + /// #832: the shared confirm used to sit INSIDE the Alarms tab branch, so it was on the page only while that + /// tab was showing — the Tags tab's Remove and the Virtual Tags tab's Delete set the dialog visible and + /// nothing appeared. Pinned by brace depth: the dialog must be at the depth of the tab @if chain, + /// not inside any branch of it. + /// + [Fact] + public void Equipment_page_confirm_renders_outside_every_tab_branch() + { + var src = AdminUiSource.Read("Components", "Pages", "Uns", "EquipmentPage.razor"); + var markup = src[..src.IndexOf("@code", StringComparison.Ordinal)]; + + int DepthAt(int index) => + markup[..index].Count(c => c == '{') - markup[..index].Count(c => c == '}'); + + var tabChain = markup.IndexOf("@if (_activeTab == \"details\")", StringComparison.Ordinal); + var dialog = markup.IndexOf(" Date: Wed, 11 Nov 2026 01:23:33 -0500 Subject: [PATCH 2/7] fix(opcua): failed sign-ins no longer lock a client out of anonymous sessions (#833) OPC Foundation SDK 2.0.1's SessionManager lockout refused EVERY activation (Anonymous included, before the token is read) from a client with 5 failed identity validations in 60 s, for 5 minutes, keyed on cert thumbprint else peer IP. Disable it (MaxFailedAuthenticationAttempts=0 on built and loaded configs) and enforce FailedCredentialLockout in the identity-registry adapter instead: UserName activations only, keyed on thumbprint+peer together, same 5/60s/300s defaults, configurable via OpcUa:CredentialLockout. --- .claude/rules/data-plane-auth.md | 6 + docs/Configuration.md | 3 + docs/security.md | 34 ++++ .../OpcUaApplicationHost.cs | 57 +++++- .../Security/FailedCredentialLockout.cs | 159 +++++++++++++++ .../CredentialLockoutWireTests.cs | 185 ++++++++++++++++++ .../FailedCredentialLockoutTests.cs | 120 ++++++++++++ 7 files changed, 562 insertions(+), 2 deletions(-) create mode 100644 src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/Security/FailedCredentialLockout.cs create mode 100644 tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.IntegrationTests/CredentialLockoutWireTests.cs create mode 100644 tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/FailedCredentialLockoutTests.cs diff --git a/.claude/rules/data-plane-auth.md b/.claude/rules/data-plane-auth.md index feb04ccf3..3f65d65e1 100644 --- a/.claude/rules/data-plane-auth.md +++ b/.claude/rules/data-plane-auth.md @@ -206,6 +206,12 @@ Two things to know before relying on any of this: the same `RoleCarryingUserIdentity` a key produces. Set it `false` to make keys mandatory. A key may **not** use the `anonymous` principal, and a failure to read anonymous's grants **denies** rather than admitting with no roles. +- ⚠️ **The SDK's failed-auth lockout is OFF; ours is UserName-only (#833).** SDK 2.0.1 `SessionManager` refuses + EVERY activation (Anonymous included, before the token is read) from a client with 5 failures in 60 s, for 5 min, + keyed cert-thumbprint-else-peer-IP. `OpcUaApplicationHost.DisableSdkAuthenticationLockout` zeroes + `MaxFailedAuthenticationAttempts` on built AND loaded configs; `FailedCredentialLockout` (`OpcUa:CredentialLockout`, + 5 / 60 s / 300 s) counts and refuses UserName activations only, keyed thumbprint+peer together. Do not re-enable the + SDK setting. `docs/security.md` §"Failed-credential lockout". - **Keys reach a driver-only node inside the deployment artifact** (stage 4). `ConfigComposer` snapshots the `ApiKey` rows — the peppered HMAC only, never a secret, and the pepper is node config that is deliberately *not* in the artifact — and `DeploymentArtifact.ParseDataPlaneAuthority` decodes them diff --git a/docs/Configuration.md b/docs/Configuration.md index b708fd53a..6b17cef46 100644 --- a/docs/Configuration.md +++ b/docs/Configuration.md @@ -74,6 +74,9 @@ 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:FailureWindowSeconds` | int | `60` | A failure more than this long after the client's previous one starts a fresh count. | +| `CredentialLockout:LockoutSeconds` | int | `300` | How long a locked-out client's UserName activations are refused. | > **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 be8288f35..74ab377c0 100644 --- a/docs/security.md +++ b/docs/security.md @@ -313,6 +313,40 @@ charged process-wide only — never handed a fresh budget. Clients behind one NA guard applies to a driver-only node: its `ArtifactApiKeyStore` is two in-memory dictionary probes and is never wrapped. +### Failed-credential lockout (`OpcUa:CredentialLockout`) + +**A failed sign-in never locks a client out of anonymous access (#833).** OPC Foundation SDK 2.0.1's +`SessionManager` has its own lockout: after `ServerConfiguration.MaxFailedAuthenticationAttempts` (SDK +default **5**) failed identity validations — each within 60 s of the previous — it refuses **every** +`ActivateSession` from that client for **5 minutes** with `BadUserAccessDenied` ("Too many failed +authentication attempts"). Its check runs before the identity token is looked at, so it refused +**Anonymous** too (even a namespace-array read needs an activated session), a successful anonymous +activation did not clear it, and the authenticator was never called (no "anonymous admitted" line). Its key +is the client certificate thumbprint when one is presented, else the peer IP — so certificate-less clients +behind one NAT share a key, and on a `None` endpoint a presented certificate is not proven. That is what +the docker-dev live gate hit: four failed `multi-role` UserName activations against central-1, then every +anonymous session refused for five minutes while central-2 (which never saw the failures) kept admitting. + +The host now sets `MaxFailedAuthenticationAttempts = 0` on every configuration it builds or loads +(`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); a successful UserName + activation clears the count. **Anonymous activations never consult it**, from any client. +- **Keyed on certificate thumbprint *and* peer address together**, so locking a client out needs both its + certificate and its source address, and NAT neighbours with different certificates do not share a count. +- 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. + +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`. + ### LDAP bind flow (`OtOpcUaLdapAuthService`) LDAP is configured under the `Security:Ldap` section (bound to `LdapOptions`, `src/Server/ZB.MOM.WW.OtOpcUa.Security/Ldap/LdapOptions.cs`, `SectionName = "Security:Ldap"`). The app authenticator is `OtOpcUaLdapAuthService` — a thin wrapper around the shared `ZB.MOM.WW.Auth.Ldap` directory client that adds two app-only concerns the shared library deliberately does not model: the `Enabled` master switch and `DevStubMode`. Since #520 it serves the **Admin UI cookie login (`/auth/login`) only** — `LdapOpcUaUserAuthenticator` is deleted and the OPC UA UserName path authenticates API keys instead (see [API-key identity](#api-key-identity-opc-ua-data-plane)). diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/OpcUaApplicationHost.cs b/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/OpcUaApplicationHost.cs index cfaecdca3..c5b049966 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/OpcUaApplicationHost.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/OpcUaApplicationHost.cs @@ -113,6 +113,12 @@ public sealed class OpcUaApplicationHostOptions /// own transport defaults, so an absent section changes nothing. /// public OpcUaQuotaOptions Quotas { get; set; } = new(); + + /// + /// Failed-credential lockout for UserName activations (OpcUa:CredentialLockout:*, lmxopcua#833). + /// Replaces the SDK's own lockout, which also refused Anonymous; see . + /// + public OpcUaCredentialLockoutOptions CredentialLockout { get; set; } = new(); } /// @@ -175,6 +181,7 @@ public sealed class OpcUaApplicationHost : IAsyncDisposable private StandardServer? _server; private readonly ITelemetryContext _telemetry = DefaultTelemetry.Create(_ => { }); private readonly List _identityAuthenticators = []; + private readonly FailedCredentialLockout _credentialLockout; private readonly object _lifetimeLock = new(); private readonly TaskCompletionSource _disposeCompletion = new(TaskCreationOptions.RunContinuationsAsynchronously); private int _disposeStarted; @@ -191,6 +198,7 @@ public sealed class OpcUaApplicationHost : IAsyncDisposable _options = options; _logger = logger; _userAuthenticator = userAuthenticator ?? NullOpcUaUserAuthenticator.Instance; + _credentialLockout = new FailedCredentialLockout(options.CredentialLockout ?? new OpcUaCredentialLockoutOptions()); } /// Gets the OPC Foundation application instance, or null if not yet started. @@ -260,14 +268,47 @@ public sealed class OpcUaApplicationHost : IAsyncDisposable ct.ThrowIfCancellationRequested(); var args = new ImpersonateEventArgs(context.TokenHandler, context.UserTokenPolicy, context.EndpointDescription); var peer = (host._server as OtOpcUaSdkServer)?.ActivationPeerAddress; + + // 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. + var lockoutKey = tokenType == UserTokenType.UserName + ? FailedCredentialLockout.KeyFor(context.ChannelCertificate?.Thumbprint, peer) + : null; + if (lockoutKey is not null && host._credentialLockout.IsLockedOut(lockoutKey, out var remaining)) + { + var seconds = (long)Math.Ceiling(remaining.TotalSeconds); + host._logger.LogInformation( + "OpcUaApplicationHost: UserName activation refused — client {Client} is locked out for {Seconds}s more", + lockoutKey, seconds); + return AuthenticationResult.Reject(ServiceResult.Create(StatusCodes.BadUserAccessDenied, + $"Too many failed authentication attempts. Try again in {seconds} seconds.")); + } + // 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); - return ServiceResult.IsBad(args.IdentityValidationError) + var result = ServiceResult.IsBad(args.IdentityValidationError) ? AuthenticationResult.Reject(args.IdentityValidationError) : args.Identity is { } identity ? AuthenticationResult.Accept(identity) : AuthenticationResult.Reject(new ServiceResult(StatusCodes.BadIdentityTokenRejected)); + + if (lockoutKey is not null) + { + if (args.Identity is not null && !ServiceResult.IsBad(args.IdentityValidationError)) + { + host._credentialLockout.RecordSuccess(lockoutKey); + } + else if (host._credentialLockout.RecordFailure(lockoutKey)) + { + host._logger.LogWarning( + "OpcUaApplicationHost: client {Client} locked out of UserName activation for {Seconds}s after " + + "{Attempts} failed attempts (OpcUa:CredentialLockout); anonymous activation is unaffected", + lockoutKey, host._options.CredentialLockout.LockoutSeconds, host._options.CredentialLockout.MaxFailedAttempts); + } + } + return result; } } @@ -488,7 +529,9 @@ public sealed class OpcUaApplicationHost : IAsyncDisposable { if (!string.IsNullOrWhiteSpace(_options.ApplicationConfigPath)) { - return await _application!.LoadApplicationConfigurationAsync(_options.ApplicationConfigPath, true, ct); + var loaded = await _application!.LoadApplicationConfigurationAsync(_options.ApplicationConfigPath, true, ct); + if (loaded.ServerConfiguration is { } loadedServer) DisableSdkAuthenticationLockout(loadedServer); + return loaded; } var serverConfig = new ServerConfiguration @@ -496,6 +539,7 @@ public sealed class OpcUaApplicationHost : IAsyncDisposable BaseAddresses = [$"opc.tcp://{_options.PublicHostname}:{_options.OpcUaPort}/OtOpcUa"], }; ApplyRequestQuotas(serverConfig, _options.Quotas); + DisableSdkAuthenticationLockout(serverConfig); serverConfig.SecurityPolicies = BuildSecurityPolicies(_options.EnabledSecurityProfiles).ToArray(); serverConfig.UserTokenPolicies = BuildUserTokenPolicies(_options.AllowAnonymous).ToArray(); @@ -531,6 +575,15 @@ public sealed class OpcUaApplicationHost : IAsyncDisposable return config; } + /// + /// Switches off the SDK's per-client failed-authentication lockout (lmxopcua#833). It refuses EVERY + /// activation from a locked client — Anonymous included — for 5 minutes; + /// enforces the UserName-only replacement. + /// + /// The configuration to set. + internal static void DisableSdkAuthenticationLockout(ServerConfiguration serverConfig) => + serverConfig.MaxFailedAuthenticationAttempts = 0; + /// /// Applies the request-processing quotas (OpcUa:Quotas:*, review 01 P6 — they were hard-coded at /// 5 / 100 / 200) to the SDK server configuration. diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/Security/FailedCredentialLockout.cs b/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/Security/FailedCredentialLockout.cs new file mode 100644 index 000000000..cecc28ae1 --- /dev/null +++ b/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/Security/FailedCredentialLockout.cs @@ -0,0 +1,159 @@ +namespace ZB.MOM.WW.OtOpcUa.OpcUaServer.Security; + +/// +/// The OpcUa:CredentialLockout section: how many failed UserName activations one client may make +/// before its UserName activations are refused for a while (lmxopcua#833). +/// +/// +/// The defaults are the OPC Foundation SDK 2.0.1 SessionManager lockout this replaces: 5 failures, +/// each inside 60 s of the previous one, lock the client out for 5 minutes. +/// +public sealed class OpcUaCredentialLockoutOptions +{ + /// Failed UserName activations that lock a client out; 0 or less disables the lockout. + public int MaxFailedAttempts { get; set; } = 5; + + /// A failure more than this many seconds after the previous one starts a fresh count. + public int FailureWindowSeconds { get; set; } = 60; + + /// How long a locked-out client's UserName activations are refused, in seconds. + public int LockoutSeconds { get; set; } = 300; +} + +/// +/// Brute-force lockout for UserName (API-key) activations, scoped so it can never refuse an +/// anonymous activation (lmxopcua#833). +/// +/// +/// +/// Why this exists instead of the SDK's own lockout. OPC Foundation SDK 2.0.1's +/// SessionManager.ActivateSessionAsync counts every failed identity validation per client and, +/// 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. +/// +/// +/// What it does. Only UserName activations are counted and refused. The key is the presented +/// client certificate thumbprint and the observed peer address together, so locking a victim +/// out needs both its certificate and its source address. Same timing as the SDK: a failure within +/// of the previous one adds to the +/// count; at the client is refused for +/// ; a refusal during the lockout is not +/// itself counted (the lockout never extends itself); a successful UserName activation clears the +/// client's count. +/// +/// +/// Memory is bounded at : expired entries are pruned when it fills, and +/// a client that still finds it full is not tracked (it can only evade by rotating certificates or +/// addresses, which it could do against the SDK lockout too; API-key secrets are not guessable). +/// +/// +public sealed class FailedCredentialLockout +{ + /// The most clients tracked at once. + public const int MaxTrackedClients = 4096; + + private readonly OpcUaCredentialLockoutOptions _options; + private readonly TimeProvider _time; + private readonly Dictionary _entries = new(StringComparer.Ordinal); + private readonly object _gate = new(); + + /// Initializes a new instance of the class. + /// The configured thresholds. + /// The clock; when null. + public FailedCredentialLockout(OpcUaCredentialLockoutOptions options, TimeProvider? time = null) + { + _options = options ?? throw new ArgumentNullException(nameof(options)); + _time = time ?? TimeProvider.System; + } + + /// Whether the lockout is switched on. + public bool Enabled => _options.MaxFailedAttempts > 0; + + /// Builds the lockout key for a client. + /// The presented client certificate's thumbprint, if any. + /// The transport-observed peer address, if known. + /// The key; never null. + public static string KeyFor(string? certificateThumbprint, System.Net.IPAddress? peer) => + $"{(string.IsNullOrEmpty(certificateThumbprint) ? "-" : certificateThumbprint)}@{peer?.ToString() ?? "-"}"; + + /// Whether is locked out right now. + /// The client key from . + /// The time left on the lockout when this returns true. + /// when the client's UserName activations must be refused. + public bool IsLockedOut(string key, out TimeSpan remaining) + { + remaining = TimeSpan.Zero; + if (!Enabled) return false; + var now = _time.GetUtcNow(); + lock (_gate) + { + if (!_entries.TryGetValue(key, out var entry)) return false; + if (entry.LockedUntil > now) + { + remaining = entry.LockedUntil - now; + return true; + } + if (IsExpired(entry, now)) _entries.Remove(key); + return false; + } + } + + /// Records one failed UserName activation. + /// The client key. + /// when this failure locked the client out. + public bool RecordFailure(string key) + { + if (!Enabled) return false; + var now = _time.GetUtcNow(); + lock (_gate) + { + if (_entries.TryGetValue(key, out var entry)) + { + if (entry.LockedUntil > now) return false; // a refusal never extends the lockout + entry = IsExpired(entry, now) ? new Entry(1, now, default) : entry with { Failures = entry.Failures + 1, LastFailure = now }; + } + else + { + if (_entries.Count >= MaxTrackedClients) Prune(now); + if (_entries.Count >= MaxTrackedClients) return false; + entry = new Entry(1, now, default); + } + + if (entry.Failures >= _options.MaxFailedAttempts) + entry = entry with { LockedUntil = now + TimeSpan.FromSeconds(Math.Max(0, _options.LockoutSeconds)) }; + _entries[key] = entry; + return entry.LockedUntil > now; + } + } + + /// Clears a client's count after a successful UserName activation. + /// The client key. + public void RecordSuccess(string key) + { + lock (_gate) _entries.Remove(key); + } + + /// How many clients are tracked (test observation). + internal int TrackedCount + { + get { lock (_gate) return _entries.Count; } + } + + private bool IsExpired(Entry entry, DateTimeOffset now) => + entry.LockedUntil <= now && now - entry.LastFailure > TimeSpan.FromSeconds(Math.Max(0, _options.FailureWindowSeconds)); + + private void Prune(DateTimeOffset now) + { + foreach (var key in _entries.Where(e => IsExpired(e.Value, now)).Select(e => e.Key).ToList()) + _entries.Remove(key); + } + + /// One client's failure count and lockout. + private readonly record struct Entry(int Failures, DateTimeOffset LastFailure, DateTimeOffset LockedUntil); +} diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.IntegrationTests/CredentialLockoutWireTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.IntegrationTests/CredentialLockoutWireTests.cs new file mode 100644 index 000000000..9519f4447 --- /dev/null +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.IntegrationTests/CredentialLockoutWireTests.cs @@ -0,0 +1,185 @@ +using System.Net; +using System.Text; +using Microsoft.Extensions.Logging.Abstractions; +using Opc.Ua; +using Opc.Ua.Client; +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.Core.Authorization; +using ZB.MOM.WW.OtOpcUa.OpcUaServer.Security; + +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 (). +/// +public sealed class CredentialLockoutWireTests +{ + private const string GoodKey = "otua_k-good_secret"; + + [Fact] + public async Task Failed_credentials_never_lock_the_client_out_of_anonymous_activation() + { + var ct = TestContext.Current.CancellationToken; + var auth = new CountingAuthenticator(); + await using var rig = await Rig.StartAsync(auth, new OpcUaCredentialLockoutOptions(), ct); + + for (var i = 0; i < 6; i++) + { + var rejected = await Should.ThrowAsync(() => rig.ConnectAsync($"otua_k-bad{i}_x", ct)); + rejected.StatusCode.ShouldBeOneOf(StatusCodes.BadIdentityTokenRejected, StatusCodes.BadUserAccessDenied); + } + + using var anonymous = await rig.ConnectAsync(credential: null, ct); + var ns = await anonymous.ReadValueAsync(VariableIds.Server_NamespaceArray, ct); + StatusCode.IsGood(ns.StatusCode).ShouldBeTrue(); + auth.AnonymousCalls.ShouldBe(1, "the anonymous activation must reach the authenticator, not be refused before it"); + } + + [Fact] + public async Task Failed_credentials_still_lock_out_further_UserName_activations_from_that_client() + { + var ct = TestContext.Current.CancellationToken; + var auth = new CountingAuthenticator(); + await using var rig = await Rig.StartAsync( + auth, new OpcUaCredentialLockoutOptions { MaxFailedAttempts = 3 }, ct); + + for (var i = 0; i < 3; i++) + { + var rejected = await Should.ThrowAsync(() => rig.ConnectAsync($"otua_k-bad{i}_x", ct)); + rejected.StatusCode.ShouldBe(StatusCodes.BadIdentityTokenRejected); + } + + var callsBefore = auth.UserNameCalls; + var locked = await Should.ThrowAsync(() => rig.ConnectAsync(GoodKey, ct)); + locked.StatusCode.ShouldBe(StatusCodes.BadUserAccessDenied); + auth.UserNameCalls.ShouldBe(callsBefore, "a locked-out client's credential is refused without being checked"); + } + + [Fact] + public async Task A_disabled_lockout_admits_a_valid_key_after_any_number_of_failures() + { + // Also proves the SDK's own lockout (default 5) is switched off: with ours disabled nothing may refuse. + var ct = TestContext.Current.CancellationToken; + var auth = new CountingAuthenticator(); + await using var rig = await Rig.StartAsync( + auth, new OpcUaCredentialLockoutOptions { MaxFailedAttempts = 0 }, ct); + + for (var i = 0; i < 7; i++) + { + var rejected = await Should.ThrowAsync(() => rig.ConnectAsync($"otua_k-bad{i}_x", ct)); + rejected.StatusCode.ShouldBe(StatusCodes.BadIdentityTokenRejected); + } + + using var session = await rig.ConnectAsync(GoodKey, ct); + session.Connected.ShouldBeTrue(); + } + + private sealed class Rig : IAsyncDisposable + { + private readonly string _pkiRoot; + private OpcUaApplicationHost _host = null!; + private int _port; + + private Rig(string pkiRoot) => _pkiRoot = pkiRoot; + + public static async Task StartAsync( + IOpcUaUserAuthenticator authenticator, OpcUaCredentialLockoutOptions lockout, CancellationToken ct) + { + var rig = new Rig(Path.Combine(Path.GetTempPath(), $"otopcua-lockout-{Guid.NewGuid():N}")); + 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); + }, + () => new OtOpcUaSdkServer(), ct); + return rig; + } + + /// Opens a session; a null credential activates anonymously. + public async Task ConnectAsync(string? credential, CancellationToken ct) + { + var appConfig = new ApplicationConfiguration(DefaultTelemetry.Create(_ => { })) + { + ApplicationName = "OtOpcUa.LockoutClient", + ApplicationUri = "urn:OtOpcUa.LockoutClient", + ApplicationType = ApplicationType.Client, + SecurityConfiguration = TestClientSecurity.Build(TestClientSecurity.AllocatePkiRoot()), + ClientConfiguration = new ClientConfiguration { DefaultSessionTimeout = 60_000 }, + }; + await appConfig.ValidateAsync(ApplicationType.Client, ct); + + var endpoint = await CoreClientUtils.SelectEndpointAsync( + appConfig, $"opc.tcp://127.0.0.1:{_port}/OtOpcUa", false, DefaultTelemetry.Create(_ => { }), ct); + + return await new DefaultSessionFactory(DefaultTelemetry.Create(_ => { })).CreateAsync( + appConfig, + new ConfiguredEndpoint(null, endpoint ?? throw new InvalidOperationException("no endpoint"), EndpointConfiguration.Create(appConfig)), + updateBeforeConnect: false, + checkDomain: false, + sessionName: "CredentialLockoutWireTests", + sessionTimeout: 60_000, + identity: credential is null + ? new UserIdentity() + : new UserIdentity("apikey", Encoding.UTF8.GetBytes(credential)), + preferredLocales: default, + ct: ct); + } + + public async ValueTask DisposeAsync() + { + await _host.DisposeAsync(); + if (Directory.Exists(_pkiRoot)) + { + try { Directory.Delete(_pkiRoot, recursive: true); } + catch (IOException) { /* best effort */ } + } + } + } + + /// Admits and anonymous; denies everything else; counts calls. + private sealed class CountingAuthenticator : IOpcUaUserAuthenticator + { + private int _userNameCalls; + private int _anonymousCalls; + + public int UserNameCalls => Volatile.Read(ref _userNameCalls); + public int AnonymousCalls => Volatile.Read(ref _anonymousCalls); + + public Task AuthenticateUserNameAsync(string username, string password, CancellationToken ct) + { + Interlocked.Increment(ref _userNameCalls); + return Task.FromResult(password == GoodKey + ? OpcUaUserAuthResult.AllowKey("good", [], "k-good") + : OpcUaUserAuthResult.Deny("Invalid API key.")); + } + + public Task AuthenticateUserNameAsync( + string username, string password, IPEndPoint? remoteEndpoint, CancellationToken ct) => + AuthenticateUserNameAsync(username, password, ct); + + public Task AuthenticateAnonymousAsync(CancellationToken ct) + { + Interlocked.Increment(ref _anonymousCalls); + return Task.FromResult(OpcUaUserAuthResult.Allow(OpcUaPrincipals.Anonymous, [])); + } + } +} diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/FailedCredentialLockoutTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/FailedCredentialLockoutTests.cs new file mode 100644 index 000000000..723c2cf25 --- /dev/null +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/FailedCredentialLockoutTests.cs @@ -0,0 +1,120 @@ +using System.Net; +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.OpcUaServer.Security; + +namespace ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests; + +/// lmxopcua#833 — the UserName-only lockout that replaces the SDK's all-token lockout. +public sealed class FailedCredentialLockoutTests +{ + private static readonly string Client = FailedCredentialLockout.KeyFor("THUMB", IPAddress.Parse("10.0.0.5")); + + [Fact] + public void Locks_at_the_threshold_and_releases_after_the_lockout() + { + var clock = new ManualClock(); + var lockout = new FailedCredentialLockout(new OpcUaCredentialLockoutOptions(), clock); + + for (var i = 0; i < 4; i++) lockout.RecordFailure(Client).ShouldBeFalse(); + lockout.IsLockedOut(Client, out _).ShouldBeFalse(); + lockout.RecordFailure(Client).ShouldBeTrue("the fifth failure locks"); + lockout.IsLockedOut(Client, out var remaining).ShouldBeTrue(); + remaining.ShouldBe(TimeSpan.FromMinutes(5)); + + clock.Advance(TimeSpan.FromSeconds(299)); + lockout.IsLockedOut(Client, out _).ShouldBeTrue(); + clock.Advance(TimeSpan.FromSeconds(1)); + lockout.IsLockedOut(Client, out _).ShouldBeFalse(); + lockout.RecordFailure(Client).ShouldBeFalse("a lockout that ran out starts a fresh count"); + } + + [Fact] + public void A_refusal_during_the_lockout_does_not_extend_it() + { + var clock = new ManualClock(); + var lockout = new FailedCredentialLockout(new OpcUaCredentialLockoutOptions { MaxFailedAttempts = 1 }, clock); + lockout.RecordFailure(Client).ShouldBeTrue(); + clock.Advance(TimeSpan.FromSeconds(200)); + lockout.RecordFailure(Client).ShouldBeFalse(); + clock.Advance(TimeSpan.FromSeconds(100)); + lockout.IsLockedOut(Client, out _).ShouldBeFalse(); + } + + [Fact] + public void Failures_spaced_wider_than_the_window_never_accumulate() + { + var clock = new ManualClock(); + var lockout = new FailedCredentialLockout(new OpcUaCredentialLockoutOptions(), clock); + for (var i = 0; i < 10; i++) + { + lockout.RecordFailure(Client).ShouldBeFalse(); + clock.Advance(TimeSpan.FromSeconds(61)); + } + lockout.IsLockedOut(Client, out _).ShouldBeFalse(); + } + + [Fact] + public void A_success_clears_the_count() + { + var lockout = new FailedCredentialLockout(new OpcUaCredentialLockoutOptions(), new ManualClock()); + for (var i = 0; i < 4; i++) lockout.RecordFailure(Client); + lockout.RecordSuccess(Client); + for (var i = 0; i < 4; i++) lockout.RecordFailure(Client).ShouldBeFalse(); + lockout.IsLockedOut(Client, out _).ShouldBeFalse(); + } + + [Fact] + public void The_key_needs_both_certificate_and_address_so_one_client_cannot_lock_out_another() + { + var lockout = new FailedCredentialLockout(new OpcUaCredentialLockoutOptions { MaxFailedAttempts = 1 }, new ManualClock()); + var attacker = FailedCredentialLockout.KeyFor("VICTIMTHUMB", IPAddress.Parse("10.0.0.99")); + var victim = FailedCredentialLockout.KeyFor("VICTIMTHUMB", IPAddress.Parse("10.0.0.5")); + var natNeighbour = FailedCredentialLockout.KeyFor("OTHERTHUMB", IPAddress.Parse("10.0.0.99")); + + lockout.RecordFailure(attacker).ShouldBeTrue(); + lockout.IsLockedOut(victim, out _).ShouldBeFalse("same certificate, different address"); + lockout.IsLockedOut(natNeighbour, out _).ShouldBeFalse("same address, different certificate"); + } + + [Fact] + public void Zero_attempts_disables_it() + { + var lockout = new FailedCredentialLockout(new OpcUaCredentialLockoutOptions { MaxFailedAttempts = 0 }, new ManualClock()); + for (var i = 0; i < 50; i++) lockout.RecordFailure(Client).ShouldBeFalse(); + lockout.IsLockedOut(Client, out _).ShouldBeFalse(); + lockout.TrackedCount.ShouldBe(0); + } + + [Fact] + public void Tracking_is_bounded_and_prunes_expired_clients_first() + { + var clock = new ManualClock(); + var lockout = new FailedCredentialLockout(new OpcUaCredentialLockoutOptions(), clock); + for (var i = 0; i < FailedCredentialLockout.MaxTrackedClients; i++) lockout.RecordFailure($"c{i}"); + lockout.TrackedCount.ShouldBe(FailedCredentialLockout.MaxTrackedClients); + + lockout.RecordFailure("overflow"); + lockout.TrackedCount.ShouldBe(FailedCredentialLockout.MaxTrackedClients, "full and nothing expired ⇒ not tracked"); + + clock.Advance(TimeSpan.FromSeconds(61)); + lockout.RecordFailure("overflow"); + lockout.TrackedCount.ShouldBe(1, "expired entries are pruned to make room"); + } + + [Fact] + public void The_host_switches_the_sdk_lockout_off() + { + var config = new Opc.Ua.ServerConfiguration(); + config.MaxFailedAuthenticationAttempts.ShouldBe(5, "the SDK default this guards against"); + OpcUaApplicationHost.DisableSdkAuthenticationLockout(config); + config.MaxFailedAuthenticationAttempts.ShouldBe(0); + } + + private sealed class ManualClock : TimeProvider + { + private DateTimeOffset _now = new(2026, 11, 11, 0, 0, 0, TimeSpan.Zero); + public override DateTimeOffset GetUtcNow() => _now; + public void Advance(TimeSpan by) => _now += by; + } +} From f8f6781a242bf406b59387b91df8d4f6659814a7 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Wed, 11 Nov 2026 01:23:33 -0500 Subject: [PATCH 3/7] feat(mtconnect): warn at Test Connect and deploy when SampleCount >= the Agent bufferSize (#825) Parse the /probe Header bufferSize; MTConnectDriverProbe reads sampleCount (default 1000) and answers Ok with a WARNING message (amber chip in the Test Connect button); InitializeAsync logs the same Warning and exposes agentBufferSize on the diagnostics bag. Live-checked against the shared cppagent fixture (bufferSize 16384). --- docs/drivers/MTConnect.md | 5 + .../MTConnectDriver.SampleLag.cs | 27 ++++ .../MTConnectDriver.cs | 9 ++ .../MTConnectDriverProbe.cs | 21 ++- .../MTConnectDtos.cs | 7 +- .../MTConnectProbeParser.cs | 16 ++- .../Drivers/DriverTestConnectButton.razor | 21 ++- .../MTConnectAgentIntegrationTests.cs | 29 ++++ .../MTConnectBufferSizeWarningTests.cs | 128 ++++++++++++++++++ 9 files changed, 258 insertions(+), 5 deletions(-) create mode 100644 tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect.Tests/MTConnectBufferSizeWarningTests.cs diff --git a/docs/drivers/MTConnect.md b/docs/drivers/MTConnect.md index 3731bbb23..81641b708 100644 --- a/docs/drivers/MTConnect.md +++ b/docs/drivers/MTConnect.md @@ -372,6 +372,11 @@ Degraded across further re-baselines until a chunk is delivered. Values keep flo off the re-baselines and `LastSuccessfulRead` keeps advancing — each `/current` is the Agent answering — so the comms-loss watchdog does not reconnect-loop a configuration problem. Size `SampleCount` below the Agent's `bufferSize` (in the `/probe` header). +**It is caught before it happens (#825):** the `/probe` header's `bufferSize` is parsed +(`MTConnectProbeModel.BufferSize`, advisory — missing/malformed is null, never a parse failure); **Test +Connect** reads `sampleCount` (default 1000 when absent) and answers Ok **with a warning** (an amber +"OK with warning" chip showing the text) when it is at or above the buffer, and the driver's **initialize** +(every deploy) logs the same Warning and puts `agentBufferSize` on the `/hosts` diagnostics bag. **An Agent `instanceId` change means the Agent restarted**, and is checked **before** the sequence-gap check (a restart also usually trips the gap, so diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect/MTConnectDriver.SampleLag.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect/MTConnectDriver.SampleLag.cs index 210bf153e..3d6594792 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect/MTConnectDriver.SampleLag.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect/MTConnectDriver.SampleLag.cs @@ -55,6 +55,27 @@ public sealed partial class MTConnectDriver : IDriverDiagnostics /// Times a pump raised its count. Diagnostics only. private long _sampleCountRaises; + /// The Agent's /probe bufferSize as of the last initialize, or 0 when unknown. Diagnostics only. + private long _agentBufferSize; + + /// + /// lmxopcua#825 — the configuration warning for a the Agent + /// will refuse. An Agent answers /sample?count=N with OUT_OF_RANGE ("'count' must be less than + /// bufferSize") when N ≥ bufferSize, so such a driver never streams: it re-baselines from + /// /current and reports Degraded after three of them. Shared by + /// (Test Connect) and the driver's initialize (deploy), so both say the same thing. + /// + /// The configured count. + /// The Agent's buffer size, or null when its header did not say. + /// The warning text, or when the count fits (or the buffer size is unknown). + internal static string? SampleCountBufferWarning(int sampleCount, long? bufferSize) => + bufferSize is { } size && sampleCount >= size + ? $"SampleCount {sampleCount.ToString(CultureInfo.InvariantCulture)} is at or above the Agent's bufferSize " + + $"{size.ToString(CultureInfo.InvariantCulture)}: the Agent refuses every /sample stream (OUT_OF_RANGE), so " + + "values will only re-baseline from /current and the driver reports Degraded. Set SampleCount below " + + $"{size.ToString(CultureInfo.InvariantCulture)}." + : null; + /// The ceiling a pump started with may raise its count to. /// The configured . /// The ceiling — never below . @@ -77,6 +98,12 @@ public sealed partial class MTConnectDriver : IDriverDiagnostics bag["sampleLag"] = lag.ToString(CultureInfo.InvariantCulture); } + var bufferSize = Interlocked.Read(ref _agentBufferSize); + if (bufferSize > 0) + { + bag["agentBufferSize"] = bufferSize.ToString(CultureInfo.InvariantCulture); + } + bag["sampleSaturatedChunks"] = Interlocked.Read(ref _saturatedChunks).ToString(CultureInfo.InvariantCulture); bag["sampleCountRaises"] = Interlocked.Read(ref _sampleCountRaises).ToString(CultureInfo.InvariantCulture); diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect/MTConnectDriver.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect/MTConnectDriver.cs index dd6b53940..352220eb5 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect/MTConnectDriver.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect/MTConnectDriver.cs @@ -3333,6 +3333,15 @@ public sealed partial class MTConnectDriver // ---- commit point: everything below is non-throwing bookkeeping ---- Volatile.Write(ref _options, options); + // lmxopcua#825 — a SampleCount the Agent will refuse is a configuration error; say so at deploy rather than + // leaving it to surface as three progress-free re-baselines and a Degraded stream. + Interlocked.Exchange(ref _agentBufferSize, probe.BufferSize ?? 0); + if (SampleCountBufferWarning(options.SampleCount, probe.BufferSize) is { } bufferWarning) + { + _logger.LogWarning("MTConnect driver {DriverInstanceId} ({AgentUri}): {Warning}", + _driverInstanceId, AgentDisplay(options), bufferWarning); + } + // Derived from the options AND the /probe model just fetched: a raw tag whose TagConfig // blob declares no type takes the type the Agent itself declares for that DataItem, so a // browse-committed tag (whose blob carries only the address) publishes as the number it diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect/MTConnectDriverProbe.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect/MTConnectDriverProbe.cs index 0e668b404..cc402eb07 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect/MTConnectDriverProbe.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect/MTConnectDriverProbe.cs @@ -132,8 +132,18 @@ public sealed class MTConnectDriverProbe : IDriverProbe /// driver's tag/timing configuration — see the "reachable ≠ deployable" remarks on the type. /// public string? DeviceName { get; set; } + + /// + /// The /sample count (lmxopcua#825). Not needed to reach the Agent, but read so Test Connect + /// can warn when it is at or above the Agent's bufferSize — a config the Agent will refuse on every + /// stream. Absent ⇒ the driver's own default. + /// + public int? SampleCount { get; set; } } + /// The driver's default SampleCount (MTConnectDriver.ParseOptions) when the config omits it. + private const int DefaultSampleCount = 1_000; + /// public async Task ProbeAsync(string configJson, TimeSpan timeout, CancellationToken ct) { @@ -195,10 +205,17 @@ public sealed class MTConnectDriverProbe : IDriverProbe try { using var client = new MTConnectAgentClient(options, _handler); - _ = await client.ProbeAsync(cts.Token).ConfigureAwait(false); + var model = await client.ProbeAsync(cts.Token).ConfigureAwait(false); sw.Stop(); - return new(true, $"MTConnect /probe OK ({safeUri})", sw.Elapsed); + // Reachable but misconfigured stays Ok (the endpoint answered — Test Connect's question), with the + // warning in the message so the operator sees it before deploying (lmxopcua#825). + var warning = MTConnectDriver.SampleCountBufferWarning(dto.SampleCount ?? DefaultSampleCount, model.BufferSize); + return new(true, + warning is null + ? $"MTConnect /probe OK ({safeUri})" + : $"MTConnect /probe OK ({safeUri}) — WARNING: {warning}", + sw.Elapsed); } catch (OperationCanceledException) { diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect/MTConnectDtos.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect/MTConnectDtos.cs index d9992bf01..a7b6ef792 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect/MTConnectDtos.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect/MTConnectDtos.cs @@ -8,7 +8,12 @@ namespace ZB.MOM.WW.OtOpcUa.Driver.MTConnect; /// XElement. /// /// Every device the Agent's probe response declares. -public sealed record MTConnectProbeModel(IReadOnlyList Devices); +/// +/// The Agent's observation ring-buffer size (the /probe Header's bufferSize), or +/// when the header omits it or it is not a positive integer. A /sample +/// count at or above it is refused OUT_OF_RANGE by the Agent (lmxopcua#825). +/// +public sealed record MTConnectProbeModel(IReadOnlyList Devices, long? BufferSize = null); /// /// Common shape shared by and : diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect/MTConnectProbeParser.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect/MTConnectProbeParser.cs index d50d66bfa..e9561a22b 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect/MTConnectProbeParser.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect/MTConnectProbeParser.cs @@ -132,7 +132,21 @@ internal static class MTConnectProbeParser "MTConnect /probe response declares no devices; refusing to report an empty device model as a successful probe."); } - return new MTConnectProbeModel(devices); + return new MTConnectProbeModel(devices, ReadBufferSize(root)); + } + + /// + /// The Header's bufferSize (lmxopcua#825). Advisory only — a missing, malformed or non-positive + /// value is , never a parse failure, because nothing but a configuration warning reads it. + /// + private static long? ReadBufferSize(XElement root) + { + var header = MTConnectXml.ChildrenNamed(root, "Header").FirstOrDefault(); + var raw = header is null ? null : MTConnectXml.OptionalAttribute(header, "bufferSize"); + return long.TryParse(raw, System.Globalization.NumberStyles.None, System.Globalization.CultureInfo.InvariantCulture, out var size) + && size > 0 + ? size + : null; } private static MTConnectDevice ReadDevice(XElement element) => diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Shared/Drivers/DriverTestConnectButton.razor b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Shared/Drivers/DriverTestConnectButton.razor index 06ec04ec1..f133dae8f 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Shared/Drivers/DriverTestConnectButton.razor +++ b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Shared/Drivers/DriverTestConnectButton.razor @@ -12,7 +12,16 @@ @if (_result is not null) { - @if (_result.Ok) + @if (_result.Ok && WarningOf(_result.Message) is { } warning) + { + @* Reachable but misconfigured (e.g. MTConnect SampleCount >= the Agent's bufferSize, #825): a probe + that answers Ok with a "WARNING:" message is shown amber with the warning, never a plain green OK. *@ + + OK with warning · @_result.LatencyMs?.ToString("F0") ms + +
@warning
+ } + else if (_result.Ok) { OK · @_result.LatencyMs?.ToString("F0") ms @@ -28,6 +37,16 @@ @code { + /// The marker a probe puts in an Ok message to flag a warning (see MTConnectDriverProbe). + internal const string WarningMarker = "WARNING:"; + + /// The warning text after , or null when the message carries none. + internal static string? WarningOf(string? message) + { + var at = message?.IndexOf(WarningMarker, StringComparison.Ordinal) ?? -1; + return at < 0 ? null : message![(at + WarningMarker.Length)..].Trim(); + } + /// Driver type key — must match an installed IDriverProbe.DriverType. [Parameter, EditorRequired] public string DriverType { get; set; } = ""; diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect.IntegrationTests/MTConnectAgentIntegrationTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect.IntegrationTests/MTConnectAgentIntegrationTests.cs index 47a57aa6d..85639ebcd 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect.IntegrationTests/MTConnectAgentIntegrationTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect.IntegrationTests/MTConnectAgentIntegrationTests.cs @@ -947,6 +947,35 @@ public sealed class MTConnectAgentIntegrationTests(MTConnectAgentFixture agent) seen.Skip(before).ShouldContain(e => e.Snapshot.StatusCode == Good); } + /// + /// #825 — Test Connect reads the live Agent's /probe bufferSize and warns (still Ok) when the + /// configured SampleCount would be refused; one below the buffer is the falsifiability control. + /// + [Fact(Timeout = TestTimeoutMs)] + public async Task Test_Connect_warns_when_SampleCount_reaches_the_live_agents_buffer_size() + { + SkipIfDown(); + + var bufferSize = (int?)agent.ProbeDocument?.Root?.Elements().FirstOrDefault(e => e.Name.LocalName == "Header") + ?.Attribute("bufferSize"); + if (bufferSize is not > 1) + { + Assert.Skip($"The Agent at {agent.AgentUri} declares no usable bufferSize in its /probe header."); + } + + var refused = await new MTConnectDriverProbe().ProbeAsync( + JsonSerializer.Serialize(new { agentUri = agent.AgentUri, sampleCount = bufferSize.Value }), + TimeSpan.FromSeconds(15), Ct); + refused.Ok.ShouldBeTrue(refused.Message); + refused.Message!.ShouldContain($"WARNING: SampleCount {bufferSize.Value} is at or above the Agent's bufferSize {bufferSize.Value}"); + + var fits = await new MTConnectDriverProbe().ProbeAsync( + JsonSerializer.Serialize(new { agentUri = agent.AgentUri, sampleCount = bufferSize.Value - 1 }), + TimeSpan.FromSeconds(15), Ct); + fits.Ok.ShouldBeTrue(fits.Message); + fits.Message!.ShouldNotContain("WARNING"); + } + private static bool Is(string? value, string expected) => string.Equals(value?.Trim(), expected, StringComparison.OrdinalIgnoreCase); diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect.Tests/MTConnectBufferSizeWarningTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect.Tests/MTConnectBufferSizeWarningTests.cs new file mode 100644 index 000000000..1f6c72e67 --- /dev/null +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect.Tests/MTConnectBufferSizeWarningTests.cs @@ -0,0 +1,128 @@ +using System.Net; +using System.Net.Http; +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.Core.Abstractions; + +namespace ZB.MOM.WW.OtOpcUa.Driver.MTConnect.Tests; + +/// +/// lmxopcua#825 — a SampleCount at or above the Agent's bufferSize is refused OUT_OF_RANGE on +/// every /sample stream. The /probe header carries bufferSize, so Test Connect and the +/// driver's initialize (deploy) both warn instead of leaving it to surface as a Degraded stream. +/// +[Trait("Category", "Unit")] +public sealed class MTConnectBufferSizeWarningTests +{ + private static string ProbeXml(string? bufferSizeAttribute) => + $""" + + +
+ + + + + + + """; + + [Theory] + [InlineData("16384", 16384L)] + [InlineData(null, null)] + [InlineData("0", null)] + [InlineData("-5", null)] + [InlineData("lots", null)] + public void The_probe_parser_reads_the_header_buffer_size_and_never_fails_on_it(string? attribute, long? expected) => + MTConnectProbeParser.Parse(ProbeXml(attribute)).BufferSize.ShouldBe(expected); + + [Theory] + [InlineData(16383, 16384L, false)] + [InlineData(16384, 16384L, true)] + [InlineData(20000, 16384L, true)] + [InlineData(1_000_000, null, false)] + public void The_warning_fires_exactly_at_and_above_the_buffer_size(int sampleCount, long? bufferSize, bool warns) + { + var warning = MTConnectDriver.SampleCountBufferWarning(sampleCount, bufferSize); + (warning is not null).ShouldBe(warns); + if (warns) warning!.ShouldContain($"below {bufferSize}"); + } + + [Fact] + public async Task Test_Connect_stays_ok_but_carries_the_warning_when_SampleCount_does_not_fit() + { + var probe = new MTConnectDriverProbe(new StubHandler(ProbeXml("16384"))); + var r = await probe.ProbeAsync( + "{\"agentUri\":\"http://stub-agent:5000\",\"sampleCount\":16384}", TimeSpan.FromSeconds(3), + TestContext.Current.CancellationToken); + + r.Ok.ShouldBeTrue("the Agent answered — reachability is not the problem"); + r.Message!.ShouldContain("WARNING:"); + r.Message!.ShouldContain("bufferSize 16384"); + } + + [Fact] + public async Task Test_Connect_checks_the_driver_default_when_SampleCount_is_absent() + { + // The default SampleCount is 1000 — an Agent with a tiny buffer refuses it too. + var probe = new MTConnectDriverProbe(new StubHandler(ProbeXml("512"))); + var r = await probe.ProbeAsync( + "{\"agentUri\":\"http://stub-agent:5000\"}", TimeSpan.FromSeconds(3), TestContext.Current.CancellationToken); + + r.Ok.ShouldBeTrue(); + r.Message!.ShouldContain("SampleCount 1000 is at or above the Agent's bufferSize 512"); + } + + [Fact] + public async Task Test_Connect_message_is_unchanged_when_SampleCount_fits() + { + var probe = new MTConnectDriverProbe(new StubHandler(ProbeXml("16384"))); + var r = await probe.ProbeAsync( + "{\"agentUri\":\"http://stub-agent:5000\",\"sampleCount\":1000}", TimeSpan.FromSeconds(3), + TestContext.Current.CancellationToken); + + r.Ok.ShouldBeTrue(); + r.Message!.ShouldNotContain("WARNING"); + } + + [Theory] + [InlineData(131072, true)] + [InlineData(1000, false)] + public async Task Initialize_warns_and_surfaces_the_buffer_size_on_the_diagnostics_bag(int sampleCount, bool warns) + { + var client = CannedAgentClient.FromFixtures(); // Fixtures/probe.xml: bufferSize="131072" + client.Probe.BufferSize.ShouldBe(131072L); + var logger = new RecordingDriverLogger(); + var driver = new MTConnectDriver( + new MTConnectDriverOptions + { + AgentUri = "http://fixture-agent:5000", + RequestTimeoutMs = 5000, + SampleCount = sampleCount, + Tags = [new MTConnectTagDefinition("dev1_pos", DriverDataType.Float64)], + }, + "mt-825", + _ => client, + logger); + try + { + await driver.InitializeAsync("{}", TestContext.Current.CancellationToken); + + logger.Warnings.Any(w => w.Contains("bufferSize", StringComparison.Ordinal)).ShouldBe(warns); + driver.GetDiagnostics()["agentBufferSize"].ShouldBe("131072"); + } + finally + { + await driver.ShutdownAsync(TestContext.Current.CancellationToken); + } + } + + private sealed class StubHandler(string probeXml) : HttpMessageHandler + { + protected override Task SendAsync(HttpRequestMessage request, CancellationToken ct) => + Task.FromResult(new HttpResponseMessage(HttpStatusCode.OK) + { + Content = new StringContent(probeXml, System.Text.Encoding.UTF8, "application/xml"), + }); + } +} From 8a6f0e3c84eb5a62ca036c616646e158930e13d2 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Wed, 11 Nov 2026 01:38:48 -0500 Subject: [PATCH 4/7] fix(mtconnect): the Agent refuses count ABOVE bufferSize, not at it (#825) Live gate against the cppagent 2.7 fixture: count=16384 with bufferSize 16384 streams; 16385 is OUT_OF_RANGE ('count' must be less than 16385). Warn only when SampleCount > bufferSize, pin both sides of the edge against the live Agent, and correct the #804 wording that said 'at or above'. --- .claude/rules/drivers.md | 2 +- docs/drivers/MTConnect.md | 7 ++++--- .../MTConnectDriver.SampleLag.cs | 9 +++++---- .../MTConnectDriver.cs | 4 ++-- .../MTConnectDriverProbe.cs | 2 +- .../MTConnectDtos.cs | 2 +- .../MTConnectXml.cs | 2 +- .../Drivers/DriverTestConnectButton.razor | 2 +- .../MTConnectAgentIntegrationTests.cs | 19 ++++++++++++++----- .../MTConnectBufferSizeWarningTests.cs | 19 ++++++++++--------- .../MTConnectSubscribeTests.cs | 2 +- 11 files changed, 41 insertions(+), 29 deletions(-) diff --git a/.claude/rules/drivers.md b/.claude/rules/drivers.md index 1c2dfd3e1..8047bfd87 100644 --- a/.claude/rules/drivers.md +++ b/.claude/rules/drivers.md @@ -296,7 +296,7 @@ host logger factory (wired in `DriverFactoryBootstrap`; the FOCAS and TwinCAT fa `InvalidDataException` — before #804 the 400 was a transport fault and the `OUT_OF_RANGE` re-baseline never ran live. The restart (`instanceId`) test also runs on every re-baseline snapshot (on 2.x that is where a restart is first seen). 3 re-baselines with no `/sample` chunk ⇒ Degraded with `LastSuccessfulRead` still advancing (no - watchdog reconnect loop over a `SampleCount` ≥ `bufferSize`). Every parse prohibits DTDs; the streams walk is + watchdog reconnect loop over a `SampleCount` > `bufferSize` — live: `count == bufferSize` is accepted; Test Connect + init warn, #825). Every parse prohibits DTDs; the streams walk is depth-bounded (64 — an unbounded recursion is a process-killing `StackOverflowException`). Non-finite floats are never Good; integer-valued decimals serve integral tags losslessly. Matrix: `docs/drivers/MTConnect.md`. - **MTConnect CONDITION alarms (#626):** a CONDITION raw tag is a native Part 9 condition only when its `TagConfig` diff --git a/docs/drivers/MTConnect.md b/docs/drivers/MTConnect.md index 81641b708..90b4f4cbd 100644 --- a/docs/drivers/MTConnect.md +++ b/docs/drivers/MTConnect.md @@ -362,7 +362,7 @@ what the driver's cursor expects) is detected **two ways**, both triggering a **Re-baselines without progress are bounded *and visible* (#804).** The reconnect backoff already paced a stream that only ever re-baselines (`A_rebaseline_that_delivers_nothing…`), but every `/current` also marked the stream delivered, so the driver read **Healthy** -for ever while no `/sample` chunk arrived — the live case is a `SampleCount` at or +for ever while no `/sample` chunk arrived — the live case is a `SampleCount` above the Agent's `bufferSize`, refused `OUT_OF_RANGE` (`'count' must be less than N`) on every open. After **3** consecutive re-baselines with no chunk in between (`MaxRebaselinesWithoutProgress`, the Java driver's `MAX_REBASELINES_WITHOUT_PROGRESS`) @@ -371,11 +371,12 @@ observations in between"` and one Warning naming the `bufferSize` cause; it stay Degraded across further re-baselines until a chunk is delivered. Values keep flowing off the re-baselines and `LastSuccessfulRead` keeps advancing — each `/current` is the Agent answering — so the comms-loss watchdog does not reconnect-loop a configuration -problem. Size `SampleCount` below the Agent's `bufferSize` (in the `/probe` header). +problem. Size `SampleCount` at most the Agent's `bufferSize` (in the `/probe` header). **It is caught before it happens (#825):** the `/probe` header's `bufferSize` is parsed (`MTConnectProbeModel.BufferSize`, advisory — missing/malformed is null, never a parse failure); **Test Connect** reads `sampleCount` (default 1000 when absent) and answers Ok **with a warning** (an amber -"OK with warning" chip showing the text) when it is at or above the buffer, and the driver's **initialize** +"OK with warning" chip showing the text) when it is **above** the buffer (live, cppagent 2.7: `count == bufferSize` is +accepted, `bufferSize + 1` refused `'count' must be less than {bufferSize+1}`), and the driver's **initialize** (every deploy) logs the same Warning and puts `agentBufferSize` on the `/hosts` diagnostics bag. **An Agent `instanceId` change means the Agent restarted**, and is checked diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect/MTConnectDriver.SampleLag.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect/MTConnectDriver.SampleLag.cs index 3d6594792..1d130fbd1 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect/MTConnectDriver.SampleLag.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect/MTConnectDriver.SampleLag.cs @@ -61,7 +61,8 @@ public sealed partial class MTConnectDriver : IDriverDiagnostics /// /// lmxopcua#825 — the configuration warning for a the Agent /// will refuse. An Agent answers /sample?count=N with OUT_OF_RANGE ("'count' must be less than - /// bufferSize") when N ≥ bufferSize, so such a driver never streams: it re-baselines from + /// bufferSize + 1") when N > bufferSize — live-measured on cppagent 2.7 (bufferSize 16384: 16384 is + /// accepted, 16385 refused) — so such a driver never streams: it re-baselines from /// /current and reports Degraded after three of them. Shared by /// (Test Connect) and the driver's initialize (deploy), so both say the same thing. /// @@ -69,10 +70,10 @@ public sealed partial class MTConnectDriver : IDriverDiagnostics /// The Agent's buffer size, or null when its header did not say. /// The warning text, or when the count fits (or the buffer size is unknown). internal static string? SampleCountBufferWarning(int sampleCount, long? bufferSize) => - bufferSize is { } size && sampleCount >= size - ? $"SampleCount {sampleCount.ToString(CultureInfo.InvariantCulture)} is at or above the Agent's bufferSize " + bufferSize is { } size && sampleCount > size + ? $"SampleCount {sampleCount.ToString(CultureInfo.InvariantCulture)} is above the Agent's bufferSize " + $"{size.ToString(CultureInfo.InvariantCulture)}: the Agent refuses every /sample stream (OUT_OF_RANGE), so " - + "values will only re-baseline from /current and the driver reports Degraded. Set SampleCount below " + + "values will only re-baseline from /current and the driver reports Degraded. Set SampleCount to at most " + $"{size.ToString(CultureInfo.InvariantCulture)}." : null; diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect/MTConnectDriver.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect/MTConnectDriver.cs index 352220eb5..8cc6566a1 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect/MTConnectDriver.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect/MTConnectDriver.cs @@ -2350,7 +2350,7 @@ public sealed partial class MTConnectDriver // #804 — the backoff above PACES a stream that only ever re-baselines, but each /current // also marked the stream delivered, so it read Healthy for ever while no /sample chunk - // arrived (a SampleCount at or above the Agent's bufferSize is refused OUT_OF_RANGE on + // arrived (a SampleCount above the Agent's bufferSize is refused OUT_OF_RANGE on // EVERY open). After MaxRebaselinesWithoutProgress in a row it is reported Degraded, and // stays so (holdStalled) until a chunk is delivered. if (!outcome.DeliveredAny && @@ -2363,7 +2363,7 @@ public sealed partial class MTConnectDriver _logger.LogWarning( outcome.Error, - "MTConnect driver {DriverInstanceId}: {Count} consecutive /current re-baselines of {AgentUri} without a single /sample chunk in between; reporting the stream Degraded until one is delivered. Values are re-baselined, not streamed. An Agent answering every /sample with OUT_OF_RANGE usually means SampleCount is at or above its bufferSize.", + "MTConnect driver {DriverInstanceId}: {Count} consecutive /current re-baselines of {AgentUri} without a single /sample chunk in between; reporting the stream Degraded until one is delivered. Values are re-baselined, not streamed. An Agent answering every /sample with OUT_OF_RANGE usually means SampleCount is above its bufferSize.", _driverInstanceId, rebaselinesWithoutProgress, AgentDisplay(options)); } diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect/MTConnectDriverProbe.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect/MTConnectDriverProbe.cs index cc402eb07..62cd96dc0 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect/MTConnectDriverProbe.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect/MTConnectDriverProbe.cs @@ -135,7 +135,7 @@ public sealed class MTConnectDriverProbe : IDriverProbe /// /// The /sample count (lmxopcua#825). Not needed to reach the Agent, but read so Test Connect - /// can warn when it is at or above the Agent's bufferSize — a config the Agent will refuse on every + /// can warn when it is above the Agent's bufferSize — a config the Agent will refuse on every /// stream. Absent ⇒ the driver's own default. /// public int? SampleCount { get; set; } diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect/MTConnectDtos.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect/MTConnectDtos.cs index a7b6ef792..3a1b48c4c 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect/MTConnectDtos.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect/MTConnectDtos.cs @@ -11,7 +11,7 @@ namespace ZB.MOM.WW.OtOpcUa.Driver.MTConnect; /// /// The Agent's observation ring-buffer size (the /probe Header's bufferSize), or /// when the header omits it or it is not a positive integer. A /sample -/// count at or above it is refused OUT_OF_RANGE by the Agent (lmxopcua#825). +/// count above it is refused OUT_OF_RANGE by the Agent (lmxopcua#825; live: count == bufferSize is accepted). /// public sealed record MTConnectProbeModel(IReadOnlyList Devices, long? BufferSize = null); diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect/MTConnectXml.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect/MTConnectXml.cs index c9237a7fd..8285b25dd 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect/MTConnectXml.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect/MTConnectXml.cs @@ -76,7 +76,7 @@ internal static class MTConnectXml /// /// Why this exists (#804). A modern Agent (cppagent 2.x, the fixture's /// mtconnect/agent:2.7) does not serve its error documents under HTTP 200 the way older - /// ones did: it answers an out-of-range /sample?from= — and a count at or above its + /// ones did: it answers an out-of-range /sample?from= — and a count above its /// buffer — with HTTP 400, and an unknown device with 404, each carrying a /// well-formed MTConnectError body. A status check alone turns those into an /// , which the pump treats as a transient transport loss and diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Shared/Drivers/DriverTestConnectButton.razor b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Shared/Drivers/DriverTestConnectButton.razor index f133dae8f..356b8ef7f 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Shared/Drivers/DriverTestConnectButton.razor +++ b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Shared/Drivers/DriverTestConnectButton.razor @@ -14,7 +14,7 @@ { @if (_result.Ok && WarningOf(_result.Message) is { } warning) { - @* Reachable but misconfigured (e.g. MTConnect SampleCount >= the Agent's bufferSize, #825): a probe + @* Reachable but misconfigured (e.g. MTConnect SampleCount > the Agent's bufferSize, #825): a probe that answers Ok with a "WARNING:" message is shown amber with the warning, never a plain green OK. *@ OK with warning · @_result.LatencyMs?.ToString("F0") ms diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect.IntegrationTests/MTConnectAgentIntegrationTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect.IntegrationTests/MTConnectAgentIntegrationTests.cs index 85639ebcd..b77ade7cd 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect.IntegrationTests/MTConnectAgentIntegrationTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect.IntegrationTests/MTConnectAgentIntegrationTests.cs @@ -868,7 +868,7 @@ public sealed class MTConnectAgentIntegrationTests(MTConnectAgentFixture agent) } /// - /// #804 — a SampleCount at or above the Agent's bufferSize is refused OUT_OF_RANGE on + /// #804 — a SampleCount above the Agent's bufferSize is refused OUT_OF_RANGE on /// EVERY /sample open (live: 'count' must be less than N). The driver keeps serving values off /// /current re-baselines, and — after three in a row with no /sample chunk — reports the stream /// Degraded with LastSuccessfulRead still advancing, rather than Healthy for ever (before #804 the @@ -949,7 +949,8 @@ public sealed class MTConnectAgentIntegrationTests(MTConnectAgentFixture agent) /// /// #825 — Test Connect reads the live Agent's /probe bufferSize and warns (still Ok) when the - /// configured SampleCount would be refused; one below the buffer is the falsifiability control. + /// configured SampleCount would be refused. The live Agent accepts count == bufferSize and refuses + /// bufferSize + 1, so both sides of that edge are checked against the Agent itself as well as the probe. /// [Fact(Timeout = TestTimeoutMs)] public async Task Test_Connect_warns_when_SampleCount_reaches_the_live_agents_buffer_size() @@ -963,14 +964,22 @@ public sealed class MTConnectAgentIntegrationTests(MTConnectAgentFixture agent) Assert.Skip($"The Agent at {agent.AgentUri} declares no usable bufferSize in its /probe header."); } + // The Agent's own edge, so the warning threshold is pinned to what the Agent actually does. + using (var http = new HttpClient { Timeout = TimeSpan.FromSeconds(15) }) + { + (await http.GetStringAsync($"{agent.AgentUri}/sample?count={bufferSize.Value}", Ct)).ShouldNotContain("OUT_OF_RANGE"); + var over = await http.GetAsync($"{agent.AgentUri}/sample?count={bufferSize.Value + 1}", Ct); + (await over.Content.ReadAsStringAsync(Ct)).ShouldContain("OUT_OF_RANGE"); + } + var refused = await new MTConnectDriverProbe().ProbeAsync( - JsonSerializer.Serialize(new { agentUri = agent.AgentUri, sampleCount = bufferSize.Value }), + JsonSerializer.Serialize(new { agentUri = agent.AgentUri, sampleCount = bufferSize.Value + 1 }), TimeSpan.FromSeconds(15), Ct); refused.Ok.ShouldBeTrue(refused.Message); - refused.Message!.ShouldContain($"WARNING: SampleCount {bufferSize.Value} is at or above the Agent's bufferSize {bufferSize.Value}"); + refused.Message!.ShouldContain($"WARNING: SampleCount {bufferSize.Value + 1} is above the Agent's bufferSize {bufferSize.Value}"); var fits = await new MTConnectDriverProbe().ProbeAsync( - JsonSerializer.Serialize(new { agentUri = agent.AgentUri, sampleCount = bufferSize.Value - 1 }), + JsonSerializer.Serialize(new { agentUri = agent.AgentUri, sampleCount = bufferSize.Value }), TimeSpan.FromSeconds(15), Ct); fits.Ok.ShouldBeTrue(fits.Message); fits.Message!.ShouldNotContain("WARNING"); diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect.Tests/MTConnectBufferSizeWarningTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect.Tests/MTConnectBufferSizeWarningTests.cs index 1f6c72e67..dbe00e969 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect.Tests/MTConnectBufferSizeWarningTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect.Tests/MTConnectBufferSizeWarningTests.cs @@ -7,7 +7,7 @@ using ZB.MOM.WW.OtOpcUa.Core.Abstractions; namespace ZB.MOM.WW.OtOpcUa.Driver.MTConnect.Tests; /// -/// lmxopcua#825 — a SampleCount at or above the Agent's bufferSize is refused OUT_OF_RANGE on +/// lmxopcua#825 — a SampleCount above the Agent's bufferSize is refused OUT_OF_RANGE on /// every /sample stream. The /probe header carries bufferSize, so Test Connect and the /// driver's initialize (deploy) both warn instead of leaving it to surface as a Degraded stream. /// @@ -37,15 +37,15 @@ public sealed class MTConnectBufferSizeWarningTests MTConnectProbeParser.Parse(ProbeXml(attribute)).BufferSize.ShouldBe(expected); [Theory] - [InlineData(16383, 16384L, false)] - [InlineData(16384, 16384L, true)] + [InlineData(16384, 16384L, false)] // live, cppagent 2.7: count == bufferSize is accepted + [InlineData(16385, 16384L, true)] // ... and bufferSize + 1 refused OUT_OF_RANGE [InlineData(20000, 16384L, true)] [InlineData(1_000_000, null, false)] - public void The_warning_fires_exactly_at_and_above_the_buffer_size(int sampleCount, long? bufferSize, bool warns) + public void The_warning_fires_only_above_the_buffer_size(int sampleCount, long? bufferSize, bool warns) { var warning = MTConnectDriver.SampleCountBufferWarning(sampleCount, bufferSize); (warning is not null).ShouldBe(warns); - if (warns) warning!.ShouldContain($"below {bufferSize}"); + if (warns) warning!.ShouldContain($"at most {bufferSize}"); } [Fact] @@ -53,7 +53,7 @@ public sealed class MTConnectBufferSizeWarningTests { var probe = new MTConnectDriverProbe(new StubHandler(ProbeXml("16384"))); var r = await probe.ProbeAsync( - "{\"agentUri\":\"http://stub-agent:5000\",\"sampleCount\":16384}", TimeSpan.FromSeconds(3), + "{\"agentUri\":\"http://stub-agent:5000\",\"sampleCount\":16385}", TimeSpan.FromSeconds(3), TestContext.Current.CancellationToken); r.Ok.ShouldBeTrue("the Agent answered — reachability is not the problem"); @@ -70,7 +70,7 @@ public sealed class MTConnectBufferSizeWarningTests "{\"agentUri\":\"http://stub-agent:5000\"}", TimeSpan.FromSeconds(3), TestContext.Current.CancellationToken); r.Ok.ShouldBeTrue(); - r.Message!.ShouldContain("SampleCount 1000 is at or above the Agent's bufferSize 512"); + r.Message!.ShouldContain("SampleCount 1000 is above the Agent's bufferSize 512"); } [Fact] @@ -78,7 +78,7 @@ public sealed class MTConnectBufferSizeWarningTests { var probe = new MTConnectDriverProbe(new StubHandler(ProbeXml("16384"))); var r = await probe.ProbeAsync( - "{\"agentUri\":\"http://stub-agent:5000\",\"sampleCount\":1000}", TimeSpan.FromSeconds(3), + "{\"agentUri\":\"http://stub-agent:5000\",\"sampleCount\":16384}", TimeSpan.FromSeconds(3), TestContext.Current.CancellationToken); r.Ok.ShouldBeTrue(); @@ -86,7 +86,8 @@ public sealed class MTConnectBufferSizeWarningTests } [Theory] - [InlineData(131072, true)] + [InlineData(131073, true)] + [InlineData(131072, false)] [InlineData(1000, false)] public async Task Initialize_warns_and_surfaces_the_buffer_size_on_the_diagnostics_bag(int sampleCount, bool warns) { diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect.Tests/MTConnectSubscribeTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect.Tests/MTConnectSubscribeTests.cs index ccaccf26a..b60d1683e 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect.Tests/MTConnectSubscribeTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect.Tests/MTConnectSubscribeTests.cs @@ -1168,7 +1168,7 @@ public sealed class MTConnectSubscribeTests /// /// #804 (Java-driver parity: MAX_REBASELINES_WITHOUT_PROGRESS). An Agent that answers EVERY - /// /sample open with OUT_OF_RANGE (live: a count at or above its bufferSize) is + /// /sample open with OUT_OF_RANGE (live: a count above its bufferSize) is /// re-baselined from /current each time, and the backoff paces that — but every re-baseline also marked /// the stream delivered, so the driver read Healthy for ever while not one /sample chunk arrived. /// After in a row it is Degraded and stays so From 0856261ef85699c3a172eedadfec03cd3149feec Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Wed, 11 Nov 2026 01:57:21 -0500 Subject: [PATCH 5/7] docs(mtconnect): record the #823 end-to-end gate (adapter stop, Agent restart/stop/pause, genuine overrun) --- docs/drivers/MTConnect.md | 17 +++++++++++++++++ .../MTConnectAgentIntegrationTests.cs | 2 +- 2 files changed, 18 insertions(+), 1 deletion(-) diff --git a/docs/drivers/MTConnect.md b/docs/drivers/MTConnect.md index 90b4f4cbd..222216b11 100644 --- a/docs/drivers/MTConnect.md +++ b/docs/drivers/MTConnect.md @@ -722,6 +722,23 @@ deliberately different, kept. - **Deferred live checks:** these legs ran through the driver, **not** through a deployed OtOpcUa server and an OPC UA client (the docker-dev rig was not used); the no-progress cap was observed via a `SampleCount` above the Agent's `bufferSize`, not a genuinely lapped ring buffer; no real machine tool or non-cppagent Agent was exercised. + **Closed by the #823 gate below**, except the real-machine / non-cppagent caveat. + +### End-to-end gate through a deployed server (#823, 2026-11-11, docker-dev) + +An MTConnect driver (`SampleCount` 1000, `HeartbeatMs` 10000, `RequestTimeoutMs` 5000) against the shared fixture, +two raw tags (`fixture_x_pos` Double, `fixture_execution` String) referenced into a UNS equipment, deployed to the +central pair, and two Client.CLI subscriptions on central-1 (`ns=3` equipment folder, `ns=2` raw device). Raw and UNS +NodeIds moved together in every leg. Fixture containers were restored healthy afterwards. + +| Leg | What happened on the wire | +|---|---| +| Adapter stop | Both NodeIds `BadNoCommunication` within 1 s of the adapter dying (Agent marks items UNAVAILABLE); driver stayed Healthy. Adapter start ⇒ Good 1–2 s later. | +| Agent restart (`docker restart`, ~12 s) | Stream lost and reconnected ~1 s; the stale cursor was refused `OUT_OF_RANGE`, the re-baseline saw the new `instanceId`, **rediscovery raised** — `/hosts` shows the **RE-BROWSE** chip. Values stayed Good (outage shorter than the comms-loss window). | +| Agent stopped ~2 min | Values held Good-but-stale; **Bad at 118 s** (comms-loss watchdog, connection refused). Agent start ⇒ re-init on the 10 s retry, Good ~8 s after the Agent was back. | +| Agent paused (`docker pause`) | Stream idle watchdog at 24 s (2 × 10000 + 5000); **Bad at 94 s** (watchdog); unpause ⇒ Good within ~3 s. | +| Genuine buffer overrun | A stalling TCP proxy froze the driver's transport while 32 000 observations were injected (`AllowPut`) — the Agent's `firstSequence` passed the driver's cursor (75 856 → 77 083). On release the resume was refused `OUT_OF_RANGE`, the driver re-baselined from `/current` within 1 s and resumed; values Good, no restart misdiagnosed. A raised-count stream (count 16, interval 5 s) was **not** lapped by a 266 obs/s burst: chunks are sent back-to-back while data is pending, so `interval` does not cap throughput. | +| `SampleCount` vs `bufferSize` (#825) | Test Connect rendered the amber "OK with warning" chip and a deploy logged the Warning (`/hosts` shows `agentBufferSize`). The same gate measured the Agent's real edge — `count = 16384` streams, `16385` is `OUT_OF_RANGE` — so the first build's "at or above" threshold was corrected to "above" (pinned live by `Test_Connect_warns_when_SampleCount_exceeds_the_live_agents_buffer_size`). | ## Known limitations diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect.IntegrationTests/MTConnectAgentIntegrationTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect.IntegrationTests/MTConnectAgentIntegrationTests.cs index b77ade7cd..7df610d57 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect.IntegrationTests/MTConnectAgentIntegrationTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect.IntegrationTests/MTConnectAgentIntegrationTests.cs @@ -953,7 +953,7 @@ public sealed class MTConnectAgentIntegrationTests(MTConnectAgentFixture agent) /// bufferSize + 1, so both sides of that edge are checked against the Agent itself as well as the probe. /// [Fact(Timeout = TestTimeoutMs)] - public async Task Test_Connect_warns_when_SampleCount_reaches_the_live_agents_buffer_size() + public async Task Test_Connect_warns_when_SampleCount_exceeds_the_live_agents_buffer_size() { SkipIfDown(); From 988927c8acd0594ccba14404cb3d6c3a107efe7a Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Wed, 11 Nov 2026 02:32:40 -0500 Subject: [PATCH 6/7] fix(opcua): credential lockout review fixes (#833) - Reserve an in-flight slot per client before authenticating (TryBegin / Complete / Abandon under one lock); refuse once recorded failures + in-flight attempts reach MaxFailedAttempts, so a parallel burst cannot pass the check before its first failure is recorded. - An expired lockout resets the count (LockoutSeconds < FailureWindowSeconds relocked on every single failure after the first lockout). - Validate OpcUa:CredentialLockout at host start: disabled, or both durations > 0. - Log the first refusal of a lockout episode at Information, the rest at Debug. - Document that on None endpoints the key is effectively IP-only (NAT neighbours share a UserName count) and a null peer shares one key. --- .claude/rules/data-plane-auth.md | 5 +- docs/Configuration.md | 4 +- docs/security.md | 21 +- .../OpcUaApplicationHostOptionsValidator.cs | 9 + .../OpcUaApplicationHost.cs | 74 ++++-- .../Security/FailedCredentialLockout.cs | 232 +++++++++++++++--- ...cUaApplicationHostOptionsValidatorTests.cs | 23 ++ .../FailedCredentialLockoutTests.cs | 88 +++++++ 8 files changed, 386 insertions(+), 70 deletions(-) diff --git a/.claude/rules/data-plane-auth.md b/.claude/rules/data-plane-auth.md index 3f65d65e1..63579321b 100644 --- a/.claude/rules/data-plane-auth.md +++ b/.claude/rules/data-plane-auth.md @@ -211,7 +211,10 @@ Two things to know before relying on any of this: keyed cert-thumbprint-else-peer-IP. `OpcUaApplicationHost.DisableSdkAuthenticationLockout` zeroes `MaxFailedAuthenticationAttempts` on built AND loaded configs; `FailedCredentialLockout` (`OpcUa:CredentialLockout`, 5 / 60 s / 300 s) counts and refuses UserName activations only, keyed thumbprint+peer together. Do not re-enable the - SDK setting. `docs/security.md` §"Failed-credential lockout". + SDK setting. The check RESERVES an in-flight slot (`TryBegin`/`Complete`/`Abandon`) — a check and a separate + record let a parallel burst through; recorded + in-flight ≥ max ⇒ refused. An expired lockout resets the count. On + `None` endpoints the key is effectively IP-only (NAT neighbours share it, UserName only). `docs/security.md` + §"Failed-credential lockout". - **Keys reach a driver-only node inside the deployment artifact** (stage 4). `ConfigComposer` snapshots the `ApiKey` rows — the peppered HMAC only, never a secret, and the pepper is node config that is deliberately *not* in the artifact — and `DeploymentArtifact.ParseDataPlaneAuthority` decodes them diff --git a/docs/Configuration.md b/docs/Configuration.md index 6b17cef46..b2e887300 100644 --- a/docs/Configuration.md +++ b/docs/Configuration.md @@ -75,8 +75,8 @@ Until #678.6 the overlay never loaded on a node carrying a `cluster-*` role (the | `AutoAcceptUntrustedClientCertificates` | bool | `false` | Auto-trust unknown client certs on first connect (dev convenience). Not validated. See [`security.md`](security.md). | | `PeerApplicationUris` | list of string | `[]` (empty) | Partner node `ApplicationUri`s published in `Server.ServerArray` for redundancy discovery. See [`Redundancy.md`](Redundancy.md). | | `CredentialLockout:MaxFailedAttempts` | int | `5` | Failed UserName (API-key) activations from one client before its UserName activations are refused `BadUserAccessDenied` (#833). `≤ 0` disables. **Never refuses Anonymous.** See [`security.md`](security.md#failed-credential-lockout-opcuacredentiallockout). | -| `CredentialLockout:FailureWindowSeconds` | int | `60` | A failure more than this long after the client's previous one starts a fresh count. | -| `CredentialLockout:LockoutSeconds` | int | `300` | How long a locked-out client's UserName activations are refused. | +| `CredentialLockout:FailureWindowSeconds` | int | `60` | A failure more than this long after the client's previous one starts a fresh count. Must be `> 0` while the lockout is enabled (validated). | +| `CredentialLockout:LockoutSeconds` | int | `300` | How long a locked-out client's UserName activations are refused; an expired lockout starts a fresh count. Must be `> 0` while the lockout is enabled (validated). | > **Transport security profiles** (the values in `EnabledSecurityProfiles` — `None`, `Basic256Sha256Sign`, `Basic256Sha256SignAndEncrypt`) and the PKI trust flow are documented in full in [`security.md`](security.md). This page does not duplicate them. diff --git a/docs/security.md b/docs/security.md index 74ab377c0..50d7ab563 100644 --- a/docs/security.md +++ b/docs/security.md @@ -334,10 +334,23 @@ The host now sets `MaxFailedAuthenticationAttempts = 0` on every configuration i - **UserName activations only.** A failed one counts; at `MaxFailedAttempts` (5) within `FailureWindowSeconds` (60) of each other, that client's **UserName** activations are refused `BadUserAccessDenied` for `LockoutSeconds` (300) without the key store being consulted, and a Warning is - logged once. A refusal during the lockout is not counted (it never extends itself); a successful UserName - activation clears the count. **Anonymous activations never consult it**, from any client. -- **Keyed on certificate thumbprint *and* peer address together**, so locking a client out needs both its - certificate and its source address, and NAT neighbours with different certificates do not share a count. + logged once. A refusal during the lockout is not counted (it never extends itself); an **expired lockout + always starts a fresh count**, whatever `FailureWindowSeconds` is; a successful UserName activation clears the + count. **Anonymous activations never consult it**, from any client. The first refusal of each lockout episode + is logged at Information, the rest at Debug. +- **Parallel activations are held to the threshold.** The check reserves an in-flight slot under the same lock, + and a client whose recorded failures plus in-progress UserName activations already reach + `MaxFailedAttempts` is refused (`BadUserAccessDenied`, "too many authentication attempts in progress"), so a + burst cannot slip past the check before its first failure is recorded. ⚠️ The same rule caps a **legitimate** + client at `MaxFailedAttempts` concurrent UserName activations (the check is milliseconds, so this only bites a + burst of simultaneous session opens from one key). +- **Keyed on certificate thumbprint *and* peer address together.** That separates clients only when they present + a certificate. ⚠️ On a `None` endpoint clients commonly present none, so the key is **effectively the IP alone**: + NAT neighbours there **do** share one count (for UserName activations only — never for anonymous ones). When the + transport reports no peer address at all, every certificate-less client shares the single key `-@-`. (Observed on + docker-dev: every host client arrives as `-@192.168.97.1`.) +- **Validated at host start:** with `MaxFailedAttempts > 0`, both `LockoutSeconds` and `FailureWindowSeconds` must be + `> 0` (`OpcUaApplicationHostOptionsValidator`); `MaxFailedAttempts <= 0` disables the lockout. - Every denial counts, including a key-store timeout or miss-budget refusal (as the SDK's did) — a ConfigDb outage can therefore lock a keyed client out for up to `LockoutSeconds` after recovery. - Bounded at 4096 tracked clients (expired entries are pruned; a client arriving at a full table is not diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.Host/Configuration/OpcUaApplicationHostOptionsValidator.cs b/src/Server/ZB.MOM.WW.OtOpcUa.Host/Configuration/OpcUaApplicationHostOptionsValidator.cs index 789afca9b..c33afe3a3 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.Host/Configuration/OpcUaApplicationHostOptionsValidator.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.Host/Configuration/OpcUaApplicationHostOptionsValidator.cs @@ -29,5 +29,14 @@ public sealed class OpcUaApplicationHostOptionsValidator : OptionsValidatorBase< // IReadOnlyCollection, so it can't bind to MinCount's IReadOnlyCollection parameter // directly. ToList() bridges to the shared primitive while preserving the count (and message). builder.MinCount(o.EnabledSecurityProfiles?.ToList(), 1, "OpcUa:EnabledSecurityProfiles"); + + // #833 review: an enabled credential lockout needs positive durations — a zero LockoutSeconds would lock + // nothing, a zero FailureWindowSeconds would never accumulate. MaxFailedAttempts <= 0 disables it. + var lockout = o.CredentialLockout; + builder.RequireThat(lockout is null || lockout.MaxFailedAttempts <= 0 + || (lockout.LockoutSeconds > 0 && lockout.FailureWindowSeconds > 0), + "OpcUa:CredentialLockout:LockoutSeconds and OpcUa:CredentialLockout:FailureWindowSeconds must both be " + + "greater than 0 while OpcUa:CredentialLockout:MaxFailedAttempts is greater than 0 (set " + + "MaxFailedAttempts to 0 to disable the lockout)"); } } diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/OpcUaApplicationHost.cs b/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/OpcUaApplicationHost.cs index c5b049966..62402e731 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/OpcUaApplicationHost.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/OpcUaApplicationHost.cs @@ -275,40 +275,62 @@ public sealed class OpcUaApplicationHost : IAsyncDisposable var lockoutKey = tokenType == UserTokenType.UserName ? FailedCredentialLockout.KeyFor(context.ChannelCertificate?.Thumbprint, peer) : null; - if (lockoutKey is not null && host._credentialLockout.IsLockedOut(lockoutKey, out var remaining)) + if (lockoutKey is not null + && !host._credentialLockout.TryBegin(lockoutKey, out var refusal, out var remaining, out var firstRefusal)) { - var seconds = (long)Math.Ceiling(remaining.TotalSeconds); - host._logger.LogInformation( - "OpcUaApplicationHost: UserName activation refused — client {Client} is locked out for {Seconds}s more", - lockoutKey, seconds); + // Logged once per lockout episode at Information; every further refusal at Debug, so a client hammering + // a lockout cannot flood the log (#833 review). + var level = firstRefusal ? LogLevel.Information : LogLevel.Debug; + if (refusal == CredentialLockoutRefusal.LockedOut) + { + var seconds = (long)Math.Ceiling(remaining.TotalSeconds); + host._logger.Log(level, + "OpcUaApplicationHost: UserName activation refused — client {Client} is locked out for {Seconds}s more", + lockoutKey, seconds); + return AuthenticationResult.Reject(ServiceResult.Create(StatusCodes.BadUserAccessDenied, + $"Too many failed authentication attempts. Try again in {seconds} seconds.")); + } + + host._logger.Log(level, + "OpcUaApplicationHost: UserName activation refused — client {Client} already has as many failed plus " + + "in-progress attempts as OpcUa:CredentialLockout allows", + lockoutKey); return AuthenticationResult.Reject(ServiceResult.Create(StatusCodes.BadUserAccessDenied, - $"Too many failed authentication attempts. Try again in {seconds} seconds.")); + "Too many authentication attempts in progress. Try again shortly.")); } - // Port zero is only an adapter for the existing IP-budget seam: the SDK exposes no peer port. - await HandleImpersonationAsync(host._userAuthenticator, args, host._logger, host._options.AllowAnonymous, - peer is null ? null : new System.Net.IPEndPoint(peer, 0), ct).ConfigureAwait(false); - var result = ServiceResult.IsBad(args.IdentityValidationError) - ? AuthenticationResult.Reject(args.IdentityValidationError) - : args.Identity is { } identity - ? AuthenticationResult.Accept(identity) - : AuthenticationResult.Reject(new ServiceResult(StatusCodes.BadIdentityTokenRejected)); - - if (lockoutKey is not null) + // The slot TryBegin reserved is released on every path — completed with the outcome, or abandoned + // (uncounted) when the activation was cancelled or threw. + var completed = false; + try { - if (args.Identity is not null && !ServiceResult.IsBad(args.IdentityValidationError)) + // Port zero is only an adapter for the existing IP-budget seam: the SDK exposes no peer port. + await HandleImpersonationAsync(host._userAuthenticator, args, host._logger, host._options.AllowAnonymous, + peer is null ? null : new System.Net.IPEndPoint(peer, 0), ct).ConfigureAwait(false); + var result = ServiceResult.IsBad(args.IdentityValidationError) + ? AuthenticationResult.Reject(args.IdentityValidationError) + : args.Identity is { } identity + ? AuthenticationResult.Accept(identity) + : AuthenticationResult.Reject(new ServiceResult(StatusCodes.BadIdentityTokenRejected)); + + if (lockoutKey is not null) { - host._credentialLockout.RecordSuccess(lockoutKey); - } - else if (host._credentialLockout.RecordFailure(lockoutKey)) - { - host._logger.LogWarning( - "OpcUaApplicationHost: client {Client} locked out of UserName activation for {Seconds}s after " - + "{Attempts} failed attempts (OpcUa:CredentialLockout); anonymous activation is unaffected", - lockoutKey, host._options.CredentialLockout.LockoutSeconds, host._options.CredentialLockout.MaxFailedAttempts); + var admitted = args.Identity is not null && !ServiceResult.IsBad(args.IdentityValidationError); + completed = true; + if (host._credentialLockout.Complete(lockoutKey, admitted)) + { + host._logger.LogWarning( + "OpcUaApplicationHost: client {Client} locked out of UserName activation for {Seconds}s after " + + "{Attempts} failed attempts (OpcUa:CredentialLockout); anonymous activation is unaffected", + lockoutKey, host._options.CredentialLockout.LockoutSeconds, host._options.CredentialLockout.MaxFailedAttempts); + } } + return result; + } + finally + { + if (lockoutKey is not null && !completed) host._credentialLockout.Abandon(lockoutKey); } - return result; } } diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/Security/FailedCredentialLockout.cs b/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/Security/FailedCredentialLockout.cs index cecc28ae1..5a3d27304 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/Security/FailedCredentialLockout.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/Security/FailedCredentialLockout.cs @@ -6,7 +6,9 @@ namespace ZB.MOM.WW.OtOpcUa.OpcUaServer.Security; /// /// /// The defaults are the OPC Foundation SDK 2.0.1 SessionManager lockout this replaces: 5 failures, -/// each inside 60 s of the previous one, lock the client out for 5 minutes. +/// each inside 60 s of the previous one, lock the client out for 5 minutes. When enabled +/// ( > 0) both durations must be positive — enforced at host start by +/// OpcUaApplicationHostOptionsValidator. /// public sealed class OpcUaCredentialLockoutOptions { @@ -20,6 +22,22 @@ public sealed class OpcUaCredentialLockoutOptions public int LockoutSeconds { get; set; } = 300; } +/// Why refused an activation. +public enum CredentialLockoutRefusal +{ + /// Not refused. + None, + + /// The client is locked out. + LockedOut, + + /// + /// The client's recorded failures plus its activations still being checked already reach the threshold, so + /// one more concurrent attempt could exceed it before any of them has been recorded. + /// + TooManyInFlight, +} + /// /// Brute-force lockout for UserName (API-key) activations, scoped so it can never refuse an /// anonymous activation (lmxopcua#833). @@ -38,18 +56,28 @@ public sealed class OpcUaCredentialLockoutOptions /// MaxFailedAuthenticationAttempts = 0 and enforces this narrower lockout instead. /// /// -/// What it does. Only UserName activations are counted and refused. The key is the presented -/// client certificate thumbprint and the observed peer address together, so locking a victim -/// out needs both its certificate and its source address. Same timing as the SDK: a failure within +/// What it does. Only UserName activations are counted and refused; Anonymous never consults it. +/// The key is the presented client certificate thumbprint and the observed peer address together. +/// ⚠️ That only separates clients that present a certificate: on a None endpoint clients commonly +/// present none, so the key is effectively the IP alone and NAT neighbours there share one count +/// (for UserName activations only — never for anonymous ones); and when the transport reports no peer +/// address, every certificate-less client shares the single key -@-. Timing: a failure within /// of the previous one adds to the /// count; at the client is refused for /// ; a refusal during the lockout is not -/// itself counted (the lockout never extends itself); a successful UserName activation clears the -/// client's count. +/// counted (the lockout never extends itself); an expired lockout starts a fresh count (whatever the +/// window); a successful UserName activation clears the client's count. /// /// -/// Memory is bounded at : expired entries are pruned when it fills, and -/// a client that still finds it full is not tracked (it can only evade by rotating certificates or +/// Concurrency. The authenticate call is awaited between the check and the outcome, so a check and a +/// separate record would let a burst of parallel activations all pass before the first failure lands. +/// therefore reserves an in-flight slot under the same lock as the check, and +/// refuses while recorded failures + in-flight attempts already reach the threshold; +/// releases the slot and records the outcome. +/// +/// +/// Memory is bounded at : expired, idle entries are pruned when it fills, +/// and a client that still finds it full is not tracked (it can only evade by rotating certificates or /// addresses, which it could do against the SDK lockout too; API-key secrets are not guessable). /// /// @@ -82,10 +110,93 @@ public sealed class FailedCredentialLockout public static string KeyFor(string? certificateThumbprint, System.Net.IPAddress? peer) => $"{(string.IsNullOrEmpty(certificateThumbprint) ? "-" : certificateThumbprint)}@{peer?.ToString() ?? "-"}"; - /// Whether is locked out right now. + /// + /// Checks and, when it may proceed, reserves an in-flight slot that the caller MUST + /// release with (only when this returns ). + /// + /// The client key from . + /// Why it was refused, when this returns false. + /// The time left on the lockout when refused . + /// + /// True for the first refusal of this lockout episode (so the caller logs it once at Information and the + /// rest at Debug). + /// + /// when the activation may be authenticated. + public bool TryBegin(string key, out CredentialLockoutRefusal refusal, out TimeSpan remaining, out bool firstRefusal) + { + refusal = CredentialLockoutRefusal.None; + remaining = TimeSpan.Zero; + firstRefusal = false; + if (!Enabled) return true; + var now = _time.GetUtcNow(); + lock (_gate) + { + if (!_entries.TryGetValue(key, out var entry)) + { + if (_entries.Count >= MaxTrackedClients) Prune(now); + if (_entries.Count >= MaxTrackedClients) return true; // untracked: nothing to reserve + entry = new Entry(); + _entries[key] = entry; + } + + Refresh(entry, now); + if (entry.LockedUntil > now) + { + refusal = CredentialLockoutRefusal.LockedOut; + remaining = entry.LockedUntil - now; + firstRefusal = !entry.RefusalLogged; + entry.RefusalLogged = true; + return false; + } + + if (entry.Failures + entry.InFlight >= _options.MaxFailedAttempts) + { + refusal = CredentialLockoutRefusal.TooManyInFlight; + firstRefusal = !entry.RefusalLogged; + entry.RefusalLogged = true; + return false; + } + + entry.InFlight++; + return true; + } + } + + /// Releases the slot reserved and records the activation's outcome. + /// The client key. + /// Whether the activation was admitted. + /// when this failure locked the client out. + public bool Complete(string key, bool succeeded) + { + if (!Enabled) return false; + var now = _time.GetUtcNow(); + lock (_gate) + { + if (!_entries.TryGetValue(key, out var entry)) return false; // was untracked at TryBegin + if (entry.InFlight > 0) entry.InFlight--; + return succeeded ? RecordSuccessCore(key, entry) : RecordFailureCore(entry, now); + } + } + + /// Releases a reserved slot without recording an outcome (the activation was cancelled or threw). + /// The client key. + public void Abandon(string key) + { + if (!Enabled) return; + lock (_gate) + { + if (_entries.TryGetValue(key, out var entry) && entry.InFlight > 0) + { + entry.InFlight--; + if (IsIdle(entry, _time.GetUtcNow())) _entries.Remove(key); + } + } + } + + /// Whether is locked out right now (observation; reserves nothing). /// The client key from . /// The time left on the lockout when this returns true. - /// when the client's UserName activations must be refused. + /// when the client's UserName activations are refused. public bool IsLockedOut(string key, out TimeSpan remaining) { remaining = TimeSpan.Zero; @@ -94,17 +205,14 @@ public sealed class FailedCredentialLockout lock (_gate) { if (!_entries.TryGetValue(key, out var entry)) return false; - if (entry.LockedUntil > now) - { - remaining = entry.LockedUntil - now; - return true; - } - if (IsExpired(entry, now)) _entries.Remove(key); - return false; + Refresh(entry, now); + if (entry.LockedUntil <= now) return false; + remaining = entry.LockedUntil - now; + return true; } } - /// Records one failed UserName activation. + /// Records one failed UserName activation with no reservation (test seam / direct callers). /// The client key. /// when this failure locked the client out. public bool RecordFailure(string key) @@ -113,30 +221,25 @@ public sealed class FailedCredentialLockout var now = _time.GetUtcNow(); lock (_gate) { - if (_entries.TryGetValue(key, out var entry)) - { - if (entry.LockedUntil > now) return false; // a refusal never extends the lockout - entry = IsExpired(entry, now) ? new Entry(1, now, default) : entry with { Failures = entry.Failures + 1, LastFailure = now }; - } - else + if (!_entries.TryGetValue(key, out var entry)) { if (_entries.Count >= MaxTrackedClients) Prune(now); if (_entries.Count >= MaxTrackedClients) return false; - entry = new Entry(1, now, default); + entry = new Entry(); + _entries[key] = entry; } - - if (entry.Failures >= _options.MaxFailedAttempts) - entry = entry with { LockedUntil = now + TimeSpan.FromSeconds(Math.Max(0, _options.LockoutSeconds)) }; - _entries[key] = entry; - return entry.LockedUntil > now; + return RecordFailureCore(entry, now); } } - /// Clears a client's count after a successful UserName activation. + /// Clears a client's count after a successful UserName activation (no reservation). /// The client key. public void RecordSuccess(string key) { - lock (_gate) _entries.Remove(key); + lock (_gate) + { + if (_entries.TryGetValue(key, out var entry)) RecordSuccessCore(key, entry); + } } /// How many clients are tracked (test observation). @@ -145,15 +248,70 @@ public sealed class FailedCredentialLockout get { lock (_gate) return _entries.Count; } } - private bool IsExpired(Entry entry, DateTimeOffset now) => - entry.LockedUntil <= now && now - entry.LastFailure > TimeSpan.FromSeconds(Math.Max(0, _options.FailureWindowSeconds)); + private bool RecordSuccessCore(string key, Entry entry) + { + if (entry.InFlight == 0) _entries.Remove(key); + else + { + entry.Failures = 0; + entry.LockedUntil = default; + entry.RefusalLogged = false; + } + return false; + } + + private bool RecordFailureCore(Entry entry, DateTimeOffset now) + { + Refresh(entry, now); + if (entry.LockedUntil > now) return false; // a refusal (or a late in-flight failure) never extends it + entry.Failures++; + entry.LastFailure = now; + if (entry.Failures < _options.MaxFailedAttempts) return false; + entry.LockedUntil = now + TimeSpan.FromSeconds(Math.Max(0, _options.LockoutSeconds)); + entry.RefusalLogged = false; + return entry.LockedUntil > now; + } + + /// + /// Forgets a run of failures that has gone stale: an expired lockout always starts a fresh count (else a + /// LockoutSeconds shorter than FailureWindowSeconds relocked on the first failure after it), + /// and so does a failure older than the window. + /// + private void Refresh(Entry entry, DateTimeOffset now) + { + if (entry.LockedUntil != default && entry.LockedUntil <= now) + { + entry.Failures = 0; + entry.LockedUntil = default; + entry.RefusalLogged = false; + } + else if (entry.LockedUntil == default && entry.Failures > 0 + && now - entry.LastFailure > TimeSpan.FromSeconds(Math.Max(0, _options.FailureWindowSeconds))) + { + entry.Failures = 0; + entry.RefusalLogged = false; + } + } private void Prune(DateTimeOffset now) { - foreach (var key in _entries.Where(e => IsExpired(e.Value, now)).Select(e => e.Key).ToList()) + foreach (var key in _entries.Where(e => IsIdle(e.Value, now)).Select(e => e.Key).ToList()) _entries.Remove(key); } - /// One client's failure count and lockout. - private readonly record struct Entry(int Failures, DateTimeOffset LastFailure, DateTimeOffset LockedUntil); + private bool IsIdle(Entry entry, DateTimeOffset now) + { + Refresh(entry, now); + return entry.InFlight == 0 && entry.Failures == 0 && entry.LockedUntil == default; + } + + /// One client's failure count, in-flight reservations and lockout. Mutated only under the gate. + private sealed class Entry + { + public int Failures; + public int InFlight; + public DateTimeOffset LastFailure; + public DateTimeOffset LockedUntil; + public bool RefusalLogged; + } } diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.Host.IntegrationTests/OpcUaApplicationHostOptionsValidatorTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.Host.IntegrationTests/OpcUaApplicationHostOptionsValidatorTests.cs index 15c620202..a7484189b 100644 --- a/tests/Server/ZB.MOM.WW.OtOpcUa.Host.IntegrationTests/OpcUaApplicationHostOptionsValidatorTests.cs +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.Host.IntegrationTests/OpcUaApplicationHostOptionsValidatorTests.cs @@ -56,4 +56,27 @@ public sealed class OpcUaApplicationHostOptionsValidatorTests result.Failed.ShouldBeTrue(); result.Failures.ShouldContain("OpcUa:EnabledSecurityProfiles must contain at least 1 item(s) (had 0)"); } + + /// #833 review: an enabled lockout with a non-positive duration fails; a disabled one is not checked. + [Theory] + [InlineData(5, 0, 60, false)] + [InlineData(5, 300, 0, false)] + [InlineData(5, -1, -1, false)] + [InlineData(5, 30, 600, true)] + [InlineData(0, 0, 0, true)] + [InlineData(-1, 0, 0, true)] + public void Credential_lockout_durations_must_be_positive_while_enabled( + int maxFailedAttempts, int lockoutSeconds, int windowSeconds, bool valid) + { + var result = Sut.Validate(null, new OpcUaApplicationHostOptions + { + CredentialLockout = new ZB.MOM.WW.OtOpcUa.OpcUaServer.Security.OpcUaCredentialLockoutOptions + { + MaxFailedAttempts = maxFailedAttempts, LockoutSeconds = lockoutSeconds, FailureWindowSeconds = windowSeconds, + }, + }); + + result.Succeeded.ShouldBe(valid); + if (!valid) result.Failures!.ShouldContain(f => f.Contains("OpcUa:CredentialLockout:LockoutSeconds")); + } } diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/FailedCredentialLockoutTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/FailedCredentialLockoutTests.cs index 723c2cf25..077d77bab 100644 --- a/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/FailedCredentialLockoutTests.cs +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/FailedCredentialLockoutTests.cs @@ -102,6 +102,94 @@ public sealed class FailedCredentialLockoutTests lockout.TrackedCount.ShouldBe(1, "expired entries are pruned to make room"); } + /// + /// #833 review — the authenticate call is awaited between the check and the outcome. A burst of parallel + /// activations that all check before any failure is recorded must still be held to the threshold: the check + /// reserves an in-flight slot, so only MaxFailedAttempts may be in progress at once. + /// + [Fact] + public async Task A_parallel_burst_is_held_to_the_threshold_before_any_failure_is_recorded() + { + var lockout = new FailedCredentialLockout(new OpcUaCredentialLockoutOptions { MaxFailedAttempts = 3 }, new ManualClock()); + const int burst = 20; + using var allChecked = new CountdownEvent(burst); + var release = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + var admitted = 0; + + var attempts = Enumerable.Range(0, burst).Select(attempt => Task.Run(async () => + { + var ok = lockout.TryBegin(Client, out _, out _, out _); + if (ok) Interlocked.Increment(ref admitted); + allChecked.Signal(); + await release.Task; // every attempt checks before any outcome lands — the race window + if (ok) lockout.Complete(Client, succeeded: false); + }, TestContext.Current.CancellationToken)).ToArray(); + + allChecked.Wait(TimeSpan.FromSeconds(10), TestContext.Current.CancellationToken).ShouldBeTrue(); + release.SetResult(); + await Task.WhenAll(attempts); + + admitted.ShouldBe(3, "only MaxFailedAttempts activations may be checked at once"); + lockout.IsLockedOut(Client, out _).ShouldBeTrue("the three failures lock the client"); + } + + [Fact] + public void In_flight_attempts_are_refused_as_TooManyInFlight_and_released_slots_free_up() + { + var lockout = new FailedCredentialLockout(new OpcUaCredentialLockoutOptions { MaxFailedAttempts = 2 }, new ManualClock()); + lockout.TryBegin(Client, out _, out _, out _).ShouldBeTrue(); + lockout.TryBegin(Client, out _, out _, out _).ShouldBeTrue(); + lockout.TryBegin(Client, out var refusal, out _, out var first).ShouldBeFalse(); + refusal.ShouldBe(CredentialLockoutRefusal.TooManyInFlight); + first.ShouldBeTrue(); + + lockout.Abandon(Client); // a cancelled activation frees its slot, uncounted + lockout.Complete(Client, succeeded: true); // a success clears the count + lockout.TryBegin(Client, out _, out _, out _).ShouldBeTrue(); + lockout.Complete(Client, succeeded: true); + lockout.TrackedCount.ShouldBe(0); + } + + /// + /// #833 review — with LockoutSeconds shorter than FailureWindowSeconds, the count survived the expired lockout + /// (the last failure was still inside the window), so the very next failure relocked the client. + /// + [Fact] + public void An_expired_lockout_starts_a_fresh_count_even_inside_the_failure_window() + { + var clock = new ManualClock(); + var lockout = new FailedCredentialLockout( + new OpcUaCredentialLockoutOptions { MaxFailedAttempts = 2, FailureWindowSeconds = 600, LockoutSeconds = 10 }, clock); + lockout.RecordFailure(Client).ShouldBeFalse(); + lockout.RecordFailure(Client).ShouldBeTrue(); + + clock.Advance(TimeSpan.FromSeconds(11)); + lockout.IsLockedOut(Client, out _).ShouldBeFalse(); + lockout.RecordFailure(Client).ShouldBeFalse("one failure after the lockout is the first of a new run"); + lockout.IsLockedOut(Client, out _).ShouldBeFalse(); + lockout.RecordFailure(Client).ShouldBeTrue(); + } + + [Fact] + public void Only_the_first_refusal_of_a_lockout_episode_is_flagged_for_Information_logging() + { + var clock = new ManualClock(); + var lockout = new FailedCredentialLockout(new OpcUaCredentialLockoutOptions { MaxFailedAttempts = 1, LockoutSeconds = 10 }, clock); + lockout.RecordFailure(Client).ShouldBeTrue(); + + lockout.TryBegin(Client, out var r1, out _, out var first1).ShouldBeFalse(); + r1.ShouldBe(CredentialLockoutRefusal.LockedOut); + first1.ShouldBeTrue(); + lockout.TryBegin(Client, out _, out _, out var first2).ShouldBeFalse(); + first2.ShouldBeFalse(); + + clock.Advance(TimeSpan.FromSeconds(11)); + lockout.TryBegin(Client, out _, out _, out _).ShouldBeTrue(); + lockout.Complete(Client, succeeded: false).ShouldBeTrue("a new episode"); + lockout.TryBegin(Client, out _, out _, out var first3).ShouldBeFalse(); + first3.ShouldBeTrue("each lockout episode logs its first refusal once"); + } + [Fact] public void The_host_switches_the_sdk_lockout_off() { From 2314a538426ec10a1e293d22306ce18a73afd51a Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Wed, 11 Nov 2026 02:43:38 -0500 Subject: [PATCH 7/7] fix(opcua): parallel UserName activations wait for a slot instead of being refused (#833) A legitimate consumer opening several keyed sessions at once from one key (e.g. a ScadaBridge site node behind one NAT IP on a None endpoint) must never see BadUserAccessDenied for parallelism. BeginAsync now hands out at most MaxFailedAttempts - recordedFailures verification slots per key; further activations queue FIFO and are woken as slots free. A waiter is refused only when the key locks out (never reaching the authenticator) or, uncounted, after OpcUa:CredentialLockout:SlotWaitSeconds (default 10, BadTimeout). Cancelled waiters are dequeued; pruning accounts for waiters. The security property is unchanged: no more than MaxFailedAttempts unverified guesses in progress-or-failed per window. --- .claude/rules/data-plane-auth.md | 6 +- docs/Configuration.md | 1 + docs/security.md | 19 +- .../OpcUaApplicationHostOptionsValidator.cs | 5 +- .../OpcUaApplicationHost.cs | 34 ++- .../Security/FailedCredentialLockout.cs | 259 ++++++++++++------ ...cUaApplicationHostOptionsValidatorTests.cs | 13 + .../CredentialLockoutWireTests.cs | 23 ++ .../FailedCredentialLockoutTests.cs | 196 +++++++++---- 9 files changed, 397 insertions(+), 159 deletions(-) diff --git a/.claude/rules/data-plane-auth.md b/.claude/rules/data-plane-auth.md index ce69f08be..d59a8ef0a 100644 --- a/.claude/rules/data-plane-auth.md +++ b/.claude/rules/data-plane-auth.md @@ -240,8 +240,10 @@ Two things to know before relying on any of this: keyed cert-thumbprint-else-peer-IP. `OpcUaApplicationHost.DisableSdkAuthenticationLockout` zeroes `MaxFailedAuthenticationAttempts` on built AND loaded configs; `FailedCredentialLockout` (`OpcUa:CredentialLockout`, 5 / 60 s / 300 s) counts and refuses UserName activations only, keyed thumbprint+peer together. Do not re-enable the - SDK setting. The check RESERVES an in-flight slot (`TryBegin`/`Complete`/`Abandon`) — a check and a separate - record let a parallel burst through; recorded + in-flight ≥ max ⇒ refused. An expired lockout resets the count. On + SDK setting. The check takes a verification SLOT (`BeginAsync`/`Complete`/`Abandon`) — a check and a + separate record let a parallel burst through. At most `max − failures` verify at once; the rest WAIT (never refused + for parallelism — ScadaBridge opens many keyed sessions from one IP), refused only on lockout or, uncounted, after + `SlotWaitSeconds`. An expired lockout resets the count. On `None` endpoints the key is effectively IP-only (NAT neighbours share it, UserName only). `docs/security.md` §"Failed-credential lockout". - **Keys reach a driver-only node inside the deployment artifact** (stage 4). `ConfigComposer` snapshots diff --git a/docs/Configuration.md b/docs/Configuration.md index f100852c7..23f2e0e55 100644 --- a/docs/Configuration.md +++ b/docs/Configuration.md @@ -76,6 +76,7 @@ Until #678.6 the overlay never loaded on a node carrying a `cluster-*` role (the | `PeerApplicationUris` | list of string | `[]` (empty) | Partner node `ApplicationUri`s published in `Server.ServerArray` for redundancy discovery. See [`Redundancy.md`](Redundancy.md). | | `CredentialLockout:MaxFailedAttempts` | int | `5` | Failed UserName (API-key) activations from one client before its UserName activations are refused `BadUserAccessDenied` (#833). `≤ 0` disables. **Never refuses Anonymous.** See [`security.md`](security.md#failed-credential-lockout-opcuacredentiallockout). | | `CredentialLockout:FailureWindowSeconds` | int | `60` | A failure more than this long after the client's previous one starts a fresh count. Must be `> 0` while the lockout is enabled (validated). | +| `CredentialLockout:SlotWaitSeconds` | int | `10` | How long a UserName activation waits for one of its client's verification slots (at most `MaxFailedAttempts − failures` verify at once; the rest queue) before that one attempt fails `BadTimeout`, uncounted. Must be `> 0` while enabled (validated). | | `CredentialLockout:LockoutSeconds` | int | `300` | How long a locked-out client's UserName activations are refused; an expired lockout starts a fresh count. Must be `> 0` while the lockout is enabled (validated). | > **Transport security profiles** (the values in `EnabledSecurityProfiles` — `None`, `Basic256Sha256Sign`, `Basic256Sha256SignAndEncrypt`) and the PKI trust flow are documented in full in [`security.md`](security.md). This page does not duplicate them. diff --git a/docs/security.md b/docs/security.md index 247a824f2..b2fa3e8e4 100644 --- a/docs/security.md +++ b/docs/security.md @@ -338,19 +338,22 @@ The host now sets `MaxFailedAuthenticationAttempts = 0` on every configuration i always starts a fresh count**, whatever `FailureWindowSeconds` is; a successful UserName activation clears the count. **Anonymous activations never consult it**, from any client. The first refusal of each lockout episode is logged at Information, the rest at Debug. -- **Parallel activations are held to the threshold.** The check reserves an in-flight slot under the same lock, - and a client whose recorded failures plus in-progress UserName activations already reach - `MaxFailedAttempts` is refused (`BadUserAccessDenied`, "too many authentication attempts in progress"), so a - burst cannot slip past the check before its first failure is recorded. ⚠️ The same rule caps a **legitimate** - client at `MaxFailedAttempts` concurrent UserName activations (the check is milliseconds, so this only bites a - burst of simultaneous session opens from one key). +- **Parallel activations wait — they are never refused for parallelism.** A client gets at most + `MaxFailedAttempts − recordedFailures` verification **slots**; a UserName activation beyond that **queues** (FIFO) + until one frees, so no more than `MaxFailedAttempts` unverified guesses are ever in progress-or-failed per window, + while a legitimate consumer opening many keyed sessions at once (e.g. a ScadaBridge site node behind one NAT IP on a + `None` endpoint) just waits milliseconds and is admitted. A waiter is refused only when the key actually **locks + out** (`BadUserAccessDenied`, without reaching the authenticator), or after `SlotWaitSeconds` (default 10) + with `BadTimeout` — **not** counted as a credential failure. A cancelled activation leaves the queue and frees + any slot it held. - **Keyed on certificate thumbprint *and* peer address together.** That separates clients only when they present a certificate. ⚠️ On a `None` endpoint clients commonly present none, so the key is **effectively the IP alone**: NAT neighbours there **do** share one count (for UserName activations only — never for anonymous ones). When the transport reports no peer address at all, every certificate-less client shares the single key `-@-`. (Observed on docker-dev: every host client arrives as `-@192.168.97.1`.) -- **Validated at host start:** with `MaxFailedAttempts > 0`, both `LockoutSeconds` and `FailureWindowSeconds` must be - `> 0` (`OpcUaApplicationHostOptionsValidator`); `MaxFailedAttempts <= 0` disables the lockout. +- **Validated at host start:** with `MaxFailedAttempts > 0`, `LockoutSeconds`, `FailureWindowSeconds` and + `SlotWaitSeconds` must all be `> 0` (`OpcUaApplicationHostOptionsValidator`); `MaxFailedAttempts <= 0` disables + the lockout. - Every denial counts, including a key-store timeout or miss-budget refusal (as the SDK's did) — a ConfigDb outage can therefore lock a keyed client out for up to `LockoutSeconds` after recovery. - Bounded at 4096 tracked clients (expired entries are pruned; a client arriving at a full table is not diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.Host/Configuration/OpcUaApplicationHostOptionsValidator.cs b/src/Server/ZB.MOM.WW.OtOpcUa.Host/Configuration/OpcUaApplicationHostOptionsValidator.cs index c33afe3a3..c51ed0d40 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.Host/Configuration/OpcUaApplicationHostOptionsValidator.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.Host/Configuration/OpcUaApplicationHostOptionsValidator.cs @@ -34,8 +34,9 @@ public sealed class OpcUaApplicationHostOptionsValidator : OptionsValidatorBase< // nothing, a zero FailureWindowSeconds would never accumulate. MaxFailedAttempts <= 0 disables it. var lockout = o.CredentialLockout; builder.RequireThat(lockout is null || lockout.MaxFailedAttempts <= 0 - || (lockout.LockoutSeconds > 0 && lockout.FailureWindowSeconds > 0), - "OpcUa:CredentialLockout:LockoutSeconds and OpcUa:CredentialLockout:FailureWindowSeconds must both be " + || (lockout.LockoutSeconds > 0 && lockout.FailureWindowSeconds > 0 && lockout.SlotWaitSeconds > 0), + "OpcUa:CredentialLockout:LockoutSeconds, OpcUa:CredentialLockout:FailureWindowSeconds and " + + "OpcUa:CredentialLockout:SlotWaitSeconds must all be " + "greater than 0 while OpcUa:CredentialLockout:MaxFailedAttempts is greater than 0 (set " + "MaxFailedAttempts to 0 to disable the lockout)"); } diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/OpcUaApplicationHost.cs b/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/OpcUaApplicationHost.cs index 62402e731..61f8db9ab 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/OpcUaApplicationHost.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/OpcUaApplicationHost.cs @@ -275,31 +275,35 @@ public sealed class OpcUaApplicationHost : IAsyncDisposable var lockoutKey = tokenType == UserTokenType.UserName ? FailedCredentialLockout.KeyFor(context.ChannelCertificate?.Thumbprint, peer) : null; - if (lockoutKey is not null - && !host._credentialLockout.TryBegin(lockoutKey, out var refusal, out var remaining, out var firstRefusal)) + if (lockoutKey is not null) { - // Logged once per lockout episode at Information; every further refusal at Debug, so a client hammering - // a lockout cannot flood the log (#833 review). - var level = firstRefusal ? LogLevel.Information : LogLevel.Debug; - if (refusal == CredentialLockoutRefusal.LockedOut) + // Waits (bounded) for one of this client's verification slots — parallel activations queue, they are + // never refused for parallelism. Refused only when locked out, or (uncounted) when no slot freed in time. + var decision = await host._credentialLockout.BeginAsync(lockoutKey, ct).ConfigureAwait(false); + if (decision.Verdict == CredentialLockoutVerdict.LockedOut) { - var seconds = (long)Math.Ceiling(remaining.TotalSeconds); - host._logger.Log(level, + // Logged once per lockout episode at Information; every further refusal at Debug, so a client + // hammering a lockout cannot flood the log (#833 review). + var seconds = (long)Math.Ceiling(decision.Remaining.TotalSeconds); + host._logger.Log(decision.FirstRefusal ? LogLevel.Information : LogLevel.Debug, "OpcUaApplicationHost: UserName activation refused — client {Client} is locked out for {Seconds}s more", lockoutKey, seconds); return AuthenticationResult.Reject(ServiceResult.Create(StatusCodes.BadUserAccessDenied, $"Too many failed authentication attempts. Try again in {seconds} seconds.")); } - host._logger.Log(level, - "OpcUaApplicationHost: UserName activation refused — client {Client} already has as many failed plus " - + "in-progress attempts as OpcUa:CredentialLockout allows", - lockoutKey); - return AuthenticationResult.Reject(ServiceResult.Create(StatusCodes.BadUserAccessDenied, - "Too many authentication attempts in progress. Try again shortly.")); + if (decision.Verdict == CredentialLockoutVerdict.SlotWaitTimedOut) + { + host._logger.LogWarning( + "OpcUaApplicationHost: UserName activation for client {Client} timed out waiting for a " + + "verification slot (OpcUa:CredentialLockout:SlotWaitSeconds); not counted as a failure", + lockoutKey); + return AuthenticationResult.Reject(ServiceResult.Create(StatusCodes.BadTimeout, + "Timed out waiting to verify the credential. Try again.")); + } } - // The slot TryBegin reserved is released on every path — completed with the outcome, or abandoned + // The slot BeginAsync granted is released on every path — completed with the outcome, or abandoned // (uncounted) when the activation was cancelled or threw. var completed = false; try diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/Security/FailedCredentialLockout.cs b/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/Security/FailedCredentialLockout.cs index 5a3d27304..24fffa4dc 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/Security/FailedCredentialLockout.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/Security/FailedCredentialLockout.cs @@ -7,7 +7,7 @@ namespace ZB.MOM.WW.OtOpcUa.OpcUaServer.Security; /// /// The defaults are the OPC Foundation SDK 2.0.1 SessionManager lockout this replaces: 5 failures, /// each inside 60 s of the previous one, lock the client out for 5 minutes. When enabled -/// ( > 0) both durations must be positive — enforced at host start by +/// ( > 0) every duration must be positive — enforced at host start by /// OpcUaApplicationHostOptionsValidator. /// public sealed class OpcUaCredentialLockoutOptions @@ -20,24 +20,37 @@ public sealed class OpcUaCredentialLockoutOptions /// How long a locked-out client's UserName activations are refused, in seconds. public int LockoutSeconds { get; set; } = 300; -} - -/// Why refused an activation. -public enum CredentialLockoutRefusal -{ - /// Not refused. - None, - - /// The client is locked out. - LockedOut, /// - /// The client's recorded failures plus its activations still being checked already reach the threshold, so - /// one more concurrent attempt could exceed it before any of them has been recorded. + /// How long a UserName activation waits for one of its client's verification slots before that one attempt + /// is failed (BadTimeout, NOT counted as a credential failure). Only reached when a client has more + /// verifications in progress than minus its recorded failures; a verification + /// is milliseconds, so in practice this bounds a pathological store stall. Kept well under the SDK's + /// operation timeout so the client gets this answer rather than a transport timeout. /// - TooManyInFlight, + public int SlotWaitSeconds { get; set; } = 10; } +/// The outcome of . +public enum CredentialLockoutVerdict +{ + /// A verification slot is held — authenticate, then call . + Proceed, + + /// The client is locked out; refuse without authenticating. + LockedOut, + + /// No slot freed within the wait; fail this one attempt, uncounted. + SlotWaitTimedOut, +} + +/// What decided. +/// Proceed, locked out, or timed out waiting. +/// Time left on the lockout when . +/// True for the first refusal of a lockout episode (log it at Information, the rest at Debug). +public readonly record struct CredentialLockoutDecision( + CredentialLockoutVerdict Verdict, TimeSpan Remaining, bool FirstRefusal); + /// /// Brute-force lockout for UserName (API-key) activations, scoped so it can never refuse an /// anonymous activation (lmxopcua#833). @@ -69,16 +82,22 @@ public enum CredentialLockoutRefusal /// window); a successful UserName activation clears the client's count. /// /// -/// Concurrency. The authenticate call is awaited between the check and the outcome, so a check and a -/// separate record would let a burst of parallel activations all pass before the first failure lands. -/// therefore reserves an in-flight slot under the same lock as the check, and -/// refuses while recorded failures + in-flight attempts already reach the threshold; -/// releases the slot and records the outcome. +/// Concurrency — parallelism waits, it is never refused. The authenticate call is awaited between the +/// check and the outcome, so a check and a separate record would let a parallel burst of guesses all pass +/// before the first failure lands. therefore hands out at most +/// MaxFailedAttempts − recordedFailures verification slots per key; an activation beyond that +/// waits (FIFO) for a slot, and / release it and wake the +/// next waiter. So no more than MaxFailedAttempts unverified guesses are ever in progress-or-failed +/// per window, while a legitimate client opening many keyed sessions at once (a ScadaBridge site node behind +/// one NAT IP) just queues for milliseconds. Waiters are refused only when the key actually locks out (they +/// never reach the authenticator) or after +/// (uncounted); a cancelled waiter is removed from the queue. /// /// -/// Memory is bounded at : expired, idle entries are pruned when it fills, -/// and a client that still finds it full is not tracked (it can only evade by rotating certificates or -/// addresses, which it could do against the SDK lockout too; API-key secrets are not guessable). +/// Memory is bounded at : entries with no failures, slots or waiters are +/// pruned when it fills, and a client that still finds it full is not tracked (it can only evade by +/// rotating certificates or addresses, which it could do against the SDK lockout too; API-key secrets are +/// not guessable). /// /// public sealed class FailedCredentialLockout @@ -111,85 +130,95 @@ public sealed class FailedCredentialLockout $"{(string.IsNullOrEmpty(certificateThumbprint) ? "-" : certificateThumbprint)}@{peer?.ToString() ?? "-"}"; /// - /// Checks and, when it may proceed, reserves an in-flight slot that the caller MUST - /// release with (only when this returns ). + /// Acquires a verification slot for , waiting for one when the client already has as + /// many in progress as it may. On the caller MUST release the + /// slot with or . /// /// The client key from . - /// Why it was refused, when this returns false. - /// The time left on the lockout when refused . - /// - /// True for the first refusal of this lockout episode (so the caller logs it once at Information and the - /// rest at Debug). - /// - /// when the activation may be authenticated. - public bool TryBegin(string key, out CredentialLockoutRefusal refusal, out TimeSpan remaining, out bool firstRefusal) + /// Cancels the wait; a cancelled waiter is dequeued and the exception propagates. + /// The decision. + public async ValueTask BeginAsync(string key, CancellationToken ct = default) { - refusal = CredentialLockoutRefusal.None; - remaining = TimeSpan.Zero; - firstRefusal = false; - if (!Enabled) return true; - var now = _time.GetUtcNow(); + if (!Enabled) return new(CredentialLockoutVerdict.Proceed, TimeSpan.Zero, false); + + Waiter waiter; + Entry entry; lock (_gate) { - if (!_entries.TryGetValue(key, out var entry)) + var now = _time.GetUtcNow(); + if (!_entries.TryGetValue(key, out var existing)) { if (_entries.Count >= MaxTrackedClients) Prune(now); - if (_entries.Count >= MaxTrackedClients) return true; // untracked: nothing to reserve - entry = new Entry(); - _entries[key] = entry; + // Untracked when still full: it gets no slot accounting, as before (see remarks). + if (_entries.Count >= MaxTrackedClients) return new(CredentialLockoutVerdict.Proceed, TimeSpan.Zero, false); + existing = new Entry(); + _entries[key] = existing; } + entry = existing; Refresh(entry, now); - if (entry.LockedUntil > now) + if (TryLockedOut(entry, now, out var refused)) return refused; + if (entry.Waiters.Count == 0 && HasFreeSlot(entry)) { - refusal = CredentialLockoutRefusal.LockedOut; - remaining = entry.LockedUntil - now; - firstRefusal = !entry.RefusalLogged; - entry.RefusalLogged = true; - return false; + entry.InFlight++; + return new(CredentialLockoutVerdict.Proceed, TimeSpan.Zero, false); } - if (entry.Failures + entry.InFlight >= _options.MaxFailedAttempts) + waiter = new Waiter(); + entry.Waiters.AddLast(waiter.Node = new LinkedListNode(waiter)); + } + + var wait = TimeSpan.FromSeconds(Math.Max(1, _options.SlotWaitSeconds)); + try + { + return await waiter.Completion.Task.WaitAsync(wait, _time, ct).ConfigureAwait(false); + } + catch (Exception ex) when (ex is TimeoutException or OperationCanceledException) + { + lock (_gate) { - refusal = CredentialLockoutRefusal.TooManyInFlight; - firstRefusal = !entry.RefusalLogged; - entry.RefusalLogged = true; - return false; + if (waiter.Completion.Task.IsCompletedSuccessfully) + { + // Decided in the same instant the wait gave up. A timeout takes the decision as made; a + // cancellation hands a granted slot back (waking the next waiter) so nothing leaks. + var decided = waiter.Completion.Task.Result; + if (ex is not OperationCanceledException) return decided; + if (decided.Verdict == CredentialLockoutVerdict.Proceed) ReleaseCore(key, entry, outcome: null); + } + else + { + entry.Waiters.Remove(waiter.Node!); + waiter.Completion.TrySetCanceled(); + RemoveIfIdle(key, entry); + } } - entry.InFlight++; - return true; + if (ex is OperationCanceledException) throw; + return new(CredentialLockoutVerdict.SlotWaitTimedOut, TimeSpan.Zero, false); } } - /// Releases the slot reserved and records the activation's outcome. + /// Releases a slot from and records the activation's outcome. /// The client key. /// Whether the activation was admitted. /// when this failure locked the client out. public bool Complete(string key, bool succeeded) { if (!Enabled) return false; - var now = _time.GetUtcNow(); lock (_gate) { - if (!_entries.TryGetValue(key, out var entry)) return false; // was untracked at TryBegin - if (entry.InFlight > 0) entry.InFlight--; - return succeeded ? RecordSuccessCore(key, entry) : RecordFailureCore(entry, now); + return _entries.TryGetValue(key, out var entry) && ReleaseCore(key, entry, succeeded); } } - /// Releases a reserved slot without recording an outcome (the activation was cancelled or threw). + /// Releases a slot without recording an outcome (the activation was cancelled or threw). /// The client key. public void Abandon(string key) { if (!Enabled) return; lock (_gate) { - if (_entries.TryGetValue(key, out var entry) && entry.InFlight > 0) - { - entry.InFlight--; - if (IsIdle(entry, _time.GetUtcNow())) _entries.Remove(key); - } + if (_entries.TryGetValue(key, out var entry)) ReleaseCore(key, entry, outcome: null); } } @@ -212,7 +241,7 @@ public sealed class FailedCredentialLockout } } - /// Records one failed UserName activation with no reservation (test seam / direct callers). + /// Records one failed UserName activation with no slot (test seam / direct callers). /// The client key. /// when this failure locked the client out. public bool RecordFailure(string key) @@ -228,17 +257,22 @@ public sealed class FailedCredentialLockout entry = new Entry(); _entries[key] = entry; } - return RecordFailureCore(entry, now); + var locked = RecordFailureCore(entry, now); + Pump(entry, now); + return locked; } } - /// Clears a client's count after a successful UserName activation (no reservation). + /// Clears a client's count after a successful UserName activation (no slot). /// The client key. public void RecordSuccess(string key) { lock (_gate) { - if (_entries.TryGetValue(key, out var entry)) RecordSuccessCore(key, entry); + if (!_entries.TryGetValue(key, out var entry)) return; + ClearCount(entry); + Pump(entry, _time.GetUtcNow()); + RemoveIfIdle(key, entry); } } @@ -248,16 +282,65 @@ public sealed class FailedCredentialLockout get { lock (_gate) return _entries.Count; } } - private bool RecordSuccessCore(string key, Entry entry) + /// How many activations are waiting for a slot on (test observation). + internal int WaitingCount(string key) { - if (entry.InFlight == 0) _entries.Remove(key); - else + lock (_gate) return _entries.TryGetValue(key, out var e) ? e.Waiters.Count : 0; + } + + private bool HasFreeSlot(Entry entry) => entry.Failures + entry.InFlight < _options.MaxFailedAttempts; + + private bool TryLockedOut(Entry entry, DateTimeOffset now, out CredentialLockoutDecision refused) + { + refused = default; + if (entry.LockedUntil <= now) return false; + refused = new(CredentialLockoutVerdict.LockedOut, entry.LockedUntil - now, !entry.RefusalLogged); + entry.RefusalLogged = true; + return true; + } + + /// Releases one slot, records (null = uncounted) and wakes waiters. + private bool ReleaseCore(string key, Entry entry, bool? outcome) + { + var now = _time.GetUtcNow(); + if (entry.InFlight > 0) entry.InFlight--; + var lockedNow = false; + if (outcome == true) ClearCount(entry); + else if (outcome == false) lockedNow = RecordFailureCore(entry, now); + Pump(entry, now); + RemoveIfIdle(key, entry); + return lockedNow; + } + + /// + /// Hands freed slots to waiters in order — or, when the key is locked out, refuses every waiter (they never + /// reach the authenticator). Called under the gate after anything that can free a slot or lock the key. + /// + private void Pump(Entry entry, DateTimeOffset now) + { + Refresh(entry, now); + while (entry.Waiters.First is { } first) { - entry.Failures = 0; - entry.LockedUntil = default; - entry.RefusalLogged = false; + if (TryLockedOut(entry, now, out var refused)) + { + entry.Waiters.RemoveFirst(); + first.Value.Completion.TrySetResult(refused); + continue; + } + + if (!HasFreeSlot(entry)) return; + entry.Waiters.RemoveFirst(); + entry.InFlight++; + if (!first.Value.Completion.TrySetResult(new(CredentialLockoutVerdict.Proceed, TimeSpan.Zero, false))) + entry.InFlight--; // a waiter that already gave up never takes the slot } - return false; + } + + private static void ClearCount(Entry entry) + { + entry.Failures = 0; + entry.LockedUntil = default; + entry.RefusalLogged = false; } private bool RecordFailureCore(Entry entry, DateTimeOffset now) @@ -281,9 +364,7 @@ public sealed class FailedCredentialLockout { if (entry.LockedUntil != default && entry.LockedUntil <= now) { - entry.Failures = 0; - entry.LockedUntil = default; - entry.RefusalLogged = false; + ClearCount(entry); } else if (entry.LockedUntil == default && entry.Failures > 0 && now - entry.LastFailure > TimeSpan.FromSeconds(Math.Max(0, _options.FailureWindowSeconds))) @@ -293,19 +374,34 @@ public sealed class FailedCredentialLockout } } + private void RemoveIfIdle(string key, Entry entry) + { + if (IsIdle(entry, _time.GetUtcNow())) _entries.Remove(key); + } + private void Prune(DateTimeOffset now) { foreach (var key in _entries.Where(e => IsIdle(e.Value, now)).Select(e => e.Key).ToList()) _entries.Remove(key); } + /// Idle = nothing to remember: no failures, no lockout, no slot held and nobody waiting. private bool IsIdle(Entry entry, DateTimeOffset now) { Refresh(entry, now); - return entry.InFlight == 0 && entry.Failures == 0 && entry.LockedUntil == default; + return entry.InFlight == 0 && entry.Waiters.Count == 0 && entry.Failures == 0 && entry.LockedUntil == default; } - /// One client's failure count, in-flight reservations and lockout. Mutated only under the gate. + /// One activation waiting for a slot. + private sealed class Waiter + { + public readonly TaskCompletionSource Completion = + new(TaskCreationOptions.RunContinuationsAsynchronously); + + public LinkedListNode? Node; + } + + /// One client's failure count, slots, waiters and lockout. Mutated only under the gate. private sealed class Entry { public int Failures; @@ -313,5 +409,6 @@ public sealed class FailedCredentialLockout public DateTimeOffset LastFailure; public DateTimeOffset LockedUntil; public bool RefusalLogged; + public readonly LinkedList Waiters = new(); } } diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.Host.IntegrationTests/OpcUaApplicationHostOptionsValidatorTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.Host.IntegrationTests/OpcUaApplicationHostOptionsValidatorTests.cs index a7484189b..ead7d38cb 100644 --- a/tests/Server/ZB.MOM.WW.OtOpcUa.Host.IntegrationTests/OpcUaApplicationHostOptionsValidatorTests.cs +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.Host.IntegrationTests/OpcUaApplicationHostOptionsValidatorTests.cs @@ -79,4 +79,17 @@ public sealed class OpcUaApplicationHostOptionsValidatorTests result.Succeeded.ShouldBe(valid); if (!valid) result.Failures!.ShouldContain(f => f.Contains("OpcUa:CredentialLockout:LockoutSeconds")); } + + /// #833 review: the slot wait must be positive while the lockout is enabled. + [Fact] + public void Credential_lockout_slot_wait_must_be_positive_while_enabled() + { + var result = Sut.Validate(null, new OpcUaApplicationHostOptions + { + CredentialLockout = new ZB.MOM.WW.OtOpcUa.OpcUaServer.Security.OpcUaCredentialLockoutOptions { SlotWaitSeconds = 0 }, + }); + + result.Failed.ShouldBeTrue(); + result.Failures!.ShouldContain(f => f.Contains("OpcUa:CredentialLockout:SlotWaitSeconds")); + } } diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.IntegrationTests/CredentialLockoutWireTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.IntegrationTests/CredentialLockoutWireTests.cs index 9519f4447..342c67c33 100644 --- a/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.IntegrationTests/CredentialLockoutWireTests.cs +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.IntegrationTests/CredentialLockoutWireTests.cs @@ -59,6 +59,29 @@ public sealed class CredentialLockoutWireTests auth.UserNameCalls.ShouldBe(callsBefore, "a locked-out client's credential is refused without being checked"); } + /// + /// #833 review — one client opening more keyed sessions at once than MaxFailedAttempts (a ScadaBridge site + /// node behind one NAT IP) is never refused for parallelism: the extra activations wait for a slot. + /// + [Fact] + public async Task Parallel_valid_activations_beyond_the_threshold_all_succeed() + { + var ct = TestContext.Current.CancellationToken; + var auth = new CountingAuthenticator(); + await using var rig = await Rig.StartAsync(auth, new OpcUaCredentialLockoutOptions { MaxFailedAttempts = 2 }, ct); + + var sessions = await Task.WhenAll(Enumerable.Range(0, 8).Select(_ => rig.ConnectAsync(GoodKey, ct))); + try + { + sessions.ShouldAllBe(s => s.Connected); + auth.UserNameCalls.ShouldBe(8); + } + finally + { + foreach (var s in sessions) s.Dispose(); + } + } + [Fact] public async Task A_disabled_lockout_admits_a_valid_key_after_any_number_of_failures() { diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/FailedCredentialLockoutTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/FailedCredentialLockoutTests.cs index 077d77bab..1bd619e8f 100644 --- a/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/FailedCredentialLockoutTests.cs +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/FailedCredentialLockoutTests.cs @@ -102,52 +102,166 @@ public sealed class FailedCredentialLockoutTests lockout.TrackedCount.ShouldBe(1, "expired entries are pruned to make room"); } + private static CancellationToken Ct => TestContext.Current.CancellationToken; + /// - /// #833 review — the authenticate call is awaited between the check and the outcome. A burst of parallel - /// activations that all check before any failure is recorded must still be held to the threshold: the check - /// reserves an in-flight slot, so only MaxFailedAttempts may be in progress at once. + /// #833 review — a legitimate client opening many keyed sessions at once (a ScadaBridge site node behind one + /// NAT IP) must never be refused for parallelism: at most MaxFailedAttempts verify concurrently, the rest + /// WAIT for a slot, and all 20 succeed. /// [Fact] - public async Task A_parallel_burst_is_held_to_the_threshold_before_any_failure_is_recorded() + public async Task An_all_valid_parallel_burst_queues_for_slots_and_every_activation_succeeds() { - var lockout = new FailedCredentialLockout(new OpcUaCredentialLockoutOptions { MaxFailedAttempts = 3 }, new ManualClock()); - const int burst = 20; - using var allChecked = new CountdownEvent(burst); - var release = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); - var admitted = 0; + var lockout = new FailedCredentialLockout(new OpcUaCredentialLockoutOptions { MaxFailedAttempts = 5 }, new ManualClock()); + var concurrent = 0; + var maxConcurrent = 0; + var proceeded = 0; - var attempts = Enumerable.Range(0, burst).Select(attempt => Task.Run(async () => + await Task.WhenAll(Enumerable.Range(0, 20).Select(attempt => Task.Run(async () => { - var ok = lockout.TryBegin(Client, out _, out _, out _); - if (ok) Interlocked.Increment(ref admitted); - allChecked.Signal(); - await release.Task; // every attempt checks before any outcome lands — the race window - if (ok) lockout.Complete(Client, succeeded: false); - }, TestContext.Current.CancellationToken)).ToArray(); + var decision = await lockout.BeginAsync(Client, Ct); + decision.Verdict.ShouldBe(CredentialLockoutVerdict.Proceed); + Interlocked.Increment(ref proceeded); + var now = Interlocked.Increment(ref concurrent); + InterlockedMax(ref maxConcurrent, now); + await Task.Delay(20, Ct); // the authenticate call + Interlocked.Decrement(ref concurrent); + lockout.Complete(Client, succeeded: true); + }, Ct))); - allChecked.Wait(TimeSpan.FromSeconds(10), TestContext.Current.CancellationToken).ShouldBeTrue(); + proceeded.ShouldBe(20); + maxConcurrent.ShouldBe(5, "exactly MaxFailedAttempts verify at once — no more, and the cap is actually reached"); + lockout.TrackedCount.ShouldBe(0, "nothing left behind once the burst drains"); + } + + /// + /// #833 review — the security half: a parallel burst of guesses that all arrive before any failure is + /// recorded still reaches the authenticator only MaxFailedAttempts times; once those fail, every waiter is + /// refused as locked out without being authenticated. + /// + [Fact] + public async Task An_all_bad_parallel_burst_authenticates_only_max_attempts_and_refuses_the_waiters_as_locked_out() + { + var lockout = new FailedCredentialLockout(new OpcUaCredentialLockoutOptions { MaxFailedAttempts = 5 }, new ManualClock()); + var release = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + var authenticated = 0; + var lockedOut = 0; + + var attempts = Enumerable.Range(0, 20).Select(attempt => Task.Run(async () => + { + var decision = await lockout.BeginAsync(Client, Ct); + if (decision.Verdict == CredentialLockoutVerdict.LockedOut) + { + Interlocked.Increment(ref lockedOut); + return; + } + + decision.Verdict.ShouldBe(CredentialLockoutVerdict.Proceed); + Interlocked.Increment(ref authenticated); + await release.Task; // hold the slots until all 15 others are queued behind them + lockout.Complete(Client, succeeded: false); + }, Ct)).ToArray(); + + await WaitUntilAsync(() => lockout.WaitingCount(Client) == 15); + authenticated.ShouldBe(5); release.SetResult(); await Task.WhenAll(attempts); - admitted.ShouldBe(3, "only MaxFailedAttempts activations may be checked at once"); - lockout.IsLockedOut(Client, out _).ShouldBeTrue("the three failures lock the client"); + authenticated.ShouldBe(5, "no waiter may reach the authenticator once the key locks out"); + lockedOut.ShouldBe(15); + lockout.IsLockedOut(Client, out _).ShouldBeTrue(); + lockout.WaitingCount(Client).ShouldBe(0); + } + + /// After a recorded failure the client has one slot fewer: capacity is MaxFailedAttempts − failures. + [Fact] + public async Task Recorded_failures_shrink_the_number_of_concurrent_verifications() + { + var lockout = new FailedCredentialLockout(new OpcUaCredentialLockoutOptions { MaxFailedAttempts = 3 }, new ManualClock()); + lockout.RecordFailure(Client); + lockout.RecordFailure(Client); + + (await lockout.BeginAsync(Client, Ct)).Verdict.ShouldBe(CredentialLockoutVerdict.Proceed); + var second = lockout.BeginAsync(Client, Ct).AsTask(); + await WaitUntilAsync(() => lockout.WaitingCount(Client) == 1); + second.IsCompleted.ShouldBeFalse("one failure short of the threshold leaves exactly one slot"); + + lockout.Complete(Client, succeeded: true); // a success clears the count and hands the slot on + (await second).Verdict.ShouldBe(CredentialLockoutVerdict.Proceed); + lockout.Complete(Client, succeeded: true); + lockout.TrackedCount.ShouldBe(0); + } + + /// A cancelled waiter is dequeued and leaves nothing behind (no leaked slot, entry prunable). + [Fact] + public async Task A_cancelled_waiter_is_removed_and_leaks_nothing() + { + var lockout = new FailedCredentialLockout(new OpcUaCredentialLockoutOptions { MaxFailedAttempts = 1 }, new ManualClock()); + (await lockout.BeginAsync(Client, Ct)).Verdict.ShouldBe(CredentialLockoutVerdict.Proceed); + + using var cts = CancellationTokenSource.CreateLinkedTokenSource(Ct); + var waiting = lockout.BeginAsync(Client, cts.Token).AsTask(); + await WaitUntilAsync(() => lockout.WaitingCount(Client) == 1); + cts.Cancel(); + await Should.ThrowAsync(waiting); + lockout.WaitingCount(Client).ShouldBe(0); + + lockout.Complete(Client, succeeded: true); + lockout.TrackedCount.ShouldBe(0, "the held slot and the cancelled waiter are both gone"); + (await lockout.BeginAsync(Client, Ct)).Verdict.ShouldBe(CredentialLockoutVerdict.Proceed); + lockout.Abandon(Client); + lockout.TrackedCount.ShouldBe(0); + } + + /// A waiter that never gets a slot times out, uncounted — the client is not one failure closer. + [Fact] + public async Task A_slot_wait_timeout_fails_that_attempt_without_counting_it() + { + var lockout = new FailedCredentialLockout( + new OpcUaCredentialLockoutOptions { MaxFailedAttempts = 1, SlotWaitSeconds = 1 }, new ManualClock()); + (await lockout.BeginAsync(Client, Ct)).Verdict.ShouldBe(CredentialLockoutVerdict.Proceed); + + (await lockout.BeginAsync(Client, Ct)).Verdict.ShouldBe(CredentialLockoutVerdict.SlotWaitTimedOut); + lockout.WaitingCount(Client).ShouldBe(0); + + lockout.Complete(Client, succeeded: true); + lockout.TrackedCount.ShouldBe(0, "the timed-out attempt recorded no failure"); } [Fact] - public void In_flight_attempts_are_refused_as_TooManyInFlight_and_released_slots_free_up() + public async Task Only_the_first_refusal_of_a_lockout_episode_is_flagged_for_Information_logging() { - var lockout = new FailedCredentialLockout(new OpcUaCredentialLockoutOptions { MaxFailedAttempts = 2 }, new ManualClock()); - lockout.TryBegin(Client, out _, out _, out _).ShouldBeTrue(); - lockout.TryBegin(Client, out _, out _, out _).ShouldBeTrue(); - lockout.TryBegin(Client, out var refusal, out _, out var first).ShouldBeFalse(); - refusal.ShouldBe(CredentialLockoutRefusal.TooManyInFlight); - first.ShouldBeTrue(); + var clock = new ManualClock(); + var lockout = new FailedCredentialLockout(new OpcUaCredentialLockoutOptions { MaxFailedAttempts = 1, LockoutSeconds = 10 }, clock); + lockout.RecordFailure(Client).ShouldBeTrue(); - lockout.Abandon(Client); // a cancelled activation frees its slot, uncounted - lockout.Complete(Client, succeeded: true); // a success clears the count - lockout.TryBegin(Client, out _, out _, out _).ShouldBeTrue(); - lockout.Complete(Client, succeeded: true); - lockout.TrackedCount.ShouldBe(0); + var r1 = await lockout.BeginAsync(Client, Ct); + r1.Verdict.ShouldBe(CredentialLockoutVerdict.LockedOut); + r1.FirstRefusal.ShouldBeTrue(); + (await lockout.BeginAsync(Client, Ct)).FirstRefusal.ShouldBeFalse(); + + clock.Advance(TimeSpan.FromSeconds(11)); + (await lockout.BeginAsync(Client, Ct)).Verdict.ShouldBe(CredentialLockoutVerdict.Proceed); + lockout.Complete(Client, succeeded: false).ShouldBeTrue("a new episode"); + (await lockout.BeginAsync(Client, Ct)).FirstRefusal.ShouldBeTrue("each lockout episode logs its first refusal once"); + } + + private static void InterlockedMax(ref int target, int value) + { + int seen; + while ((seen = Volatile.Read(ref target)) < value && Interlocked.CompareExchange(ref target, value, seen) != seen) + { + } + } + + private static async Task WaitUntilAsync(Func condition) + { + var deadline = DateTime.UtcNow + TimeSpan.FromSeconds(10); + while (!condition()) + { + DateTime.UtcNow.ShouldBeLessThan(deadline, "condition not reached in time"); + await Task.Delay(5, Ct); + } } /// @@ -170,26 +284,6 @@ public sealed class FailedCredentialLockoutTests lockout.RecordFailure(Client).ShouldBeTrue(); } - [Fact] - public void Only_the_first_refusal_of_a_lockout_episode_is_flagged_for_Information_logging() - { - var clock = new ManualClock(); - var lockout = new FailedCredentialLockout(new OpcUaCredentialLockoutOptions { MaxFailedAttempts = 1, LockoutSeconds = 10 }, clock); - lockout.RecordFailure(Client).ShouldBeTrue(); - - lockout.TryBegin(Client, out var r1, out _, out var first1).ShouldBeFalse(); - r1.ShouldBe(CredentialLockoutRefusal.LockedOut); - first1.ShouldBeTrue(); - lockout.TryBegin(Client, out _, out _, out var first2).ShouldBeFalse(); - first2.ShouldBeFalse(); - - clock.Advance(TimeSpan.FromSeconds(11)); - lockout.TryBegin(Client, out _, out _, out _).ShouldBeTrue(); - lockout.Complete(Client, succeeded: false).ShouldBeTrue("a new episode"); - lockout.TryBegin(Client, out _, out _, out var first3).ShouldBeFalse(); - first3.ShouldBeTrue("each lockout episode logs its first refusal once"); - } - [Fact] public void The_host_switches_the_sdk_lockout_off() {