diff --git a/archreview/plans/R2-08-ldap-historyread-async-plan.md.tasks.json b/archreview/plans/R2-08-ldap-historyread-async-plan.md.tasks.json index 08933cfd..c76c8bfc 100644 --- a/archreview/plans/R2-08-ldap-historyread-async-plan.md.tasks.json +++ b/archreview/plans/R2-08-ldap-historyread-async-plan.md.tasks.json @@ -4,7 +4,7 @@ { "id": "R2-08-T1", "subject": "S2 stall-repro test (RED): slow LDAP backend must deny within AuthTimeout — fails on current code", "status": "completed", "blockedBy": [] }, { "id": "R2-08-T2", "subject": "LdapOptions: ConnectionTimeoutMs/AuthTimeoutMs/MaxConcurrentAuthentications/OutageFailureThreshold/OutageCooldownSeconds + ToLibraryOptions projection + validator rules", "status": "completed", "blockedBy": [] }, { "id": "R2-08-T3", "subject": "LdapAuthResult.IsSystemFailure seam + OtOpcUaLdapAuthService.Adapt mapping (ServiceAccountBindFailed => system-side)", "status": "completed", "blockedBy": [] }, - { "id": "R2-08-T4", "subject": "LdapOpcUaUserAuthenticator: WaitAsync wall-clock bound + SemaphoreSlim concurrency cap with release-on-core-completion (turns T1 green)", "status": "pending", "blockedBy": ["R2-08-T1", "R2-08-T2"] }, + { "id": "R2-08-T4", "subject": "LdapOpcUaUserAuthenticator: WaitAsync wall-clock bound + SemaphoreSlim concurrency cap with release-on-core-completion (turns T1 green)", "status": "completed", "blockedBy": ["R2-08-T1", "R2-08-T2"] }, { "id": "R2-08-T5", "subject": "Directory-outage circuit: fail-fast deny after N consecutive system-side failures, half-open after cooldown", "status": "pending", "blockedBy": ["R2-08-T3", "R2-08-T4"] }, { "id": "R2-08-T6", "subject": "Outage-sim integration test (real library, unroutable host, via internal HandleImpersonation) + SDK m_eventLock contract doc + docs/security.md", "status": "pending", "blockedBy": ["R2-08-T4"] }, { "id": "R2-08-T7", "subject": "S3 sequential-serving repro test (RED): MaxObservedConcurrency >= 2 on an 8-node Raw batch — fails on current code", "status": "pending", "blockedBy": [] }, diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.Host/OpcUa/LdapOpcUaUserAuthenticator.cs b/src/Server/ZB.MOM.WW.OtOpcUa.Host/OpcUa/LdapOpcUaUserAuthenticator.cs index 4a03e34a..2d26b832 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.Host/OpcUa/LdapOpcUaUserAuthenticator.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.Host/OpcUa/LdapOpcUaUserAuthenticator.cs @@ -1,5 +1,6 @@ using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Logging; +using Microsoft.Extensions.Options; using ZB.MOM.WW.Auth.Abstractions.Roles; using ZB.MOM.WW.OtOpcUa.OpcUaServer.Security; using ZB.MOM.WW.OtOpcUa.Security.Ldap; @@ -16,22 +17,108 @@ namespace ZB.MOM.WW.OtOpcUa.Host.OpcUa; /// session identity for the downstream data-plane ACL evaluator. /// /// +/// /// This authenticator is registered as a singleton, but /// (and its DbContext-backed mapping service) is scoped. A per-call DI scope is opened to /// resolve the mapper so the singleton never captures a scoped dependency. +/// +/// +/// Resilience (arch-review 03/S2). The OPC UA SDK raises the impersonation callback that +/// reaches here synchronously, inside a SessionManager-wide event lock (verified +/// against OPCFoundation.NetStandard.Opc.Ua.Server 1.5.378 — +/// SessionManager.ActivateSessionAsync holds m_eventLock while invoking the +/// void impersonation delegate). A slow / hung LDAP bind inside that callback therefore does +/// not merely stall one SDK thread — it serializes every session activation server-wide +/// (Anonymous and X509 included). The callback contract is irreducibly synchronous (there is no +/// async impersonation callback), so the fix lives entirely at this boundary: a hard whole-flow +/// wall-clock bound () and a bounded in-flight concurrency cap +/// (), both fail-closed. All new paths deny. +/// /// -public sealed class LdapOpcUaUserAuthenticator( - ILdapAuthService ldap, - IServiceScopeFactory scopeFactory, - ILogger logger) - : IOpcUaUserAuthenticator +public sealed class LdapOpcUaUserAuthenticator : IOpcUaUserAuthenticator { + private readonly ILdapAuthService _ldap; + private readonly IServiceScopeFactory _scopeFactory; + private readonly ILogger _logger; + private readonly LdapOptions _options; + + /// Bounds in-flight authentication flows. A boundary timeout abandons its core task, which + /// keeps holding its slot until it actually completes — so an outage cannot accumulate unbounded + /// orphan binds. + private readonly SemaphoreSlim _gate; + + /// Constructs the authenticator from its dependencies and the bound LDAP options. + /// The app LDAP auth service (shared with the Admin UI login flow). + /// Opens a per-call DI scope to resolve the scoped role mapper. + /// The bound (timeout + concurrency knobs). + /// The logger. + public LdapOpcUaUserAuthenticator( + ILdapAuthService ldap, + IServiceScopeFactory scopeFactory, + IOptions options, + ILogger logger) + { + _ldap = ldap; + _scopeFactory = scopeFactory; + _options = options.Value; + _logger = logger; + _gate = new SemaphoreSlim(Math.Max(1, _options.MaxConcurrentAuthentications)); + } + /// public async Task AuthenticateUserNameAsync(string username, string password, CancellationToken ct) { + var authTimeout = TimeSpan.FromMilliseconds(Math.Max(1, _options.AuthTimeoutMs)); + + // Bounded in-flight concurrency: acquire a slot within our own wall-clock budget. All slots + // busy is LOCAL backpressure, not directory evidence — deny "busy" (fail-closed). + if (!await _gate.WaitAsync(authTimeout, ct).ConfigureAwait(false)) + { + _logger.LogWarning( + "LDAP authentication backend busy (concurrency cap {Cap}) — denying OPC UA user {User}", + _options.MaxConcurrentAuthentications, username); + return OpcUaUserAuthResult.Deny("Authentication backend busy"); + } + + // Release the slot when the CORE task completes — deliberately NOT in a finally here. A boundary + // timeout below abandons the core task (it runs to completion in the background; the underlying + // library is documented never-throw, so there are no unobserved exceptions), and it must keep + // counting against the cap until it finishes. The point of the boundary is to release the SDK + // thread (and m_eventLock), not to cancel the bind. + var core = AuthenticateCoreAsync(username, password, ct); + _ = core.ContinueWith( + static (_, state) => ((SemaphoreSlim)state!).Release(), + _gate, + CancellationToken.None, + TaskContinuationOptions.None, + TaskScheduler.Default); + try { - var result = await ldap.AuthenticateAsync(username, password, ct).ConfigureAwait(false); + return await core.WaitAsync(authTimeout, ct).ConfigureAwait(false); + } + catch (TimeoutException) + { + _logger.LogWarning( + "LDAP authentication timed out after {Timeout} for OPC UA user {User} — denying (fail-closed)", + authTimeout, username); + return OpcUaUserAuthResult.Deny("Authentication timed out"); + } + } + + /// + /// The unbounded core authenticate flow (bind + role-map) that the outer + /// wraps in a wall-clock bound + concurrency cap. Fail-closed: + /// any backend throw becomes a denial rather than escaping into the SDK. + /// + /// The username to authenticate. + /// The cleartext password. + /// Cancellation token. + private async Task AuthenticateCoreAsync(string username, string password, CancellationToken ct) + { + try + { + var result = await _ldap.AuthenticateAsync(username, password, ct).ConfigureAwait(false); if (!result.Success) { return OpcUaUserAuthResult.Deny(result.Error ?? "Invalid credentials"); @@ -42,7 +129,7 @@ public sealed class LdapOpcUaUserAuthenticator( } catch (Exception ex) when (ex is not OperationCanceledException) { - logger.LogWarning(ex, "LDAP authentication threw for OPC UA user {User}", username); + _logger.LogWarning(ex, "LDAP authentication threw for OPC UA user {User}", username); return OpcUaUserAuthResult.Deny("Authentication backend error"); } } @@ -62,7 +149,7 @@ public sealed class LdapOpcUaUserAuthenticator( { try { - await using var scope = scopeFactory.CreateAsyncScope(); + await using var scope = _scopeFactory.CreateAsyncScope(); var mapper = scope.ServiceProvider.GetRequiredService>(); var mapping = await mapper.MapAsync(groups, ct).ConfigureAwait(false); @@ -73,7 +160,7 @@ public sealed class LdapOpcUaUserAuthenticator( } catch (Exception ex) when (ex is not OperationCanceledException) { - logger.LogWarning(ex, + _logger.LogWarning(ex, "Role-map lookup failed for OPC UA user {User}; using pre-resolved baseline roles", username); return preResolved; } diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.Host.IntegrationTests/LdapAuthResilienceTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.Host.IntegrationTests/LdapAuthResilienceTests.cs index d43fafd4..1447894b 100644 --- a/tests/Server/ZB.MOM.WW.OtOpcUa.Host.IntegrationTests/LdapAuthResilienceTests.cs +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.Host.IntegrationTests/LdapAuthResilienceTests.cs @@ -1,6 +1,7 @@ using System.Diagnostics; using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Logging.Abstractions; +using Microsoft.Extensions.Options; using Shouldly; using Xunit; using ZB.MOM.WW.Auth.Abstractions.Roles; @@ -32,10 +33,7 @@ public sealed class LdapAuthResilienceTests TimeSpan.FromSeconds(5), new LdapAuthResult(true, "Alice", "alice", new[] { "configeditor" }, Array.Empty(), null)); var mapper = new FakeMapper(g => g.ToArray()); - var sut = new LdapOpcUaUserAuthenticator( - ldap, - ScopeFactoryWith(mapper), - NullLogger.Instance); + var sut = Build(ldap, mapper, new LdapOptions { AuthTimeoutMs = 200 }); var sw = Stopwatch.StartNew(); var result = await sut @@ -49,6 +47,69 @@ public sealed class LdapAuthResilienceTests sw.Elapsed.ShouldBeLessThan(TimeSpan.FromSeconds(1.5)); } + /// A fast, healthy backend is unaffected by the resilience wrapping — success + roles pass + /// through untouched (03/S2). + [Fact] + public async Task Authenticate_fast_success_unaffected() + { + var ldap = new DelayingFakeLdap( + TimeSpan.Zero, + new LdapAuthResult(true, "Alice", "alice", new[] { "configeditor" }, Array.Empty(), null)); + var mapper = new FakeMapper(g => g.Select(x => x == "configeditor" ? "Designer" : x).ToArray()); + var sut = Build(ldap, mapper, new LdapOptions()); + + var result = await sut + .AuthenticateUserNameAsync("alice", "secret", CancellationToken.None) + .WaitAsync(TimeSpan.FromSeconds(10), CancellationToken.None); + + result.Success.ShouldBeTrue(); + result.DisplayName.ShouldBe("Alice"); + result.Roles.ShouldBe(new[] { "Designer" }); + } + + /// With the concurrency cap saturated (a parked in-flight bind holding the only slot), a + /// second call is denied "backend busy" within its own budget — it never waits behind the parked + /// bind indefinitely (03/S2). + [Fact] + public async Task Authenticate_saturated_concurrency_denies_busy() + { + var ldap = new GatedFakeLdap( + new LdapAuthResult(true, "Alice", "alice", new[] { "configeditor" }, Array.Empty(), null)); + var mapper = new FakeMapper(g => g.ToArray()); + // MaxConcurrentAuthentications = 1 → the second call cannot get a slot; small AuthTimeoutMs so the + // "busy" denial is quick and the first call's boundary also trips fast (its core stays parked, + // holding the slot). + var sut = Build(ldap, mapper, new LdapOptions { MaxConcurrentAuthentications = 1, AuthTimeoutMs = 300 }); + + // First call: acquires the only slot; its core parks in the gated fake. + var first = sut.AuthenticateUserNameAsync("alice", "secret", CancellationToken.None); + + // Wait until the fake has actually been entered (slot acquired, core parked). + var spin = Stopwatch.StartNew(); + while (ldap.Entered == 0 && spin.Elapsed < TimeSpan.FromSeconds(5)) + await Task.Delay(10, TestContext.Current.CancellationToken); + ldap.Entered.ShouldBe(1); + + // Second concurrent call: the slot is held by the parked core, so acquisition times out → busy. + var second = await sut + .AuthenticateUserNameAsync("bob", "secret", CancellationToken.None) + .WaitAsync(TimeSpan.FromSeconds(10), CancellationToken.None); + + second.Success.ShouldBeFalse(); + second.Error.ShouldNotBeNull(); + second.Error!.ShouldContain("busy"); + ldap.Entered.ShouldBe(1, "the second call must be denied without ever reaching the backend"); + + // Release the parked bind so the first call's core completes cleanly (no orphaned resources). + ldap.Release(); + await first.WaitAsync(TimeSpan.FromSeconds(10), CancellationToken.None); + } + + private static LdapOpcUaUserAuthenticator Build( + ILdapAuthService ldap, IGroupRoleMapper mapper, LdapOptions options) => + new(ldap, ScopeFactoryWith(mapper), Options.Create(options), + NullLogger.Instance); + /// Builds an IServiceScopeFactory whose scopes resolve the supplied mapper. private static IServiceScopeFactory ScopeFactoryWith(IGroupRoleMapper mapper) { @@ -92,6 +153,40 @@ public sealed class LdapAuthResilienceTests } } + /// + /// A fake that parks ASYNCHRONOUSLY (awaits a + /// ) until the test calls — used to hold an + /// in-flight bind so the concurrency cap is deterministically saturated WITHOUT blocking a thread. + /// Records how many calls have entered (past the slot acquire). + /// + private sealed class GatedFakeLdap : ILdapAuthService + { + private readonly TaskCompletionSource _park = new(TaskCreationOptions.RunContinuationsAsynchronously); + private readonly LdapAuthResult _then; + private int _entered; + + /// Initializes the fake with the result to return once released. + /// The result returned once is called. + public GatedFakeLdap(LdapAuthResult then) => _then = then; + + /// Number of authenticate calls that reached the backend body. + public int Entered => Volatile.Read(ref _entered); + + /// Releases the parked bind so its awaiting call can complete. + public void Release() => _park.TrySetResult(); + + /// Marks entry, awaits the parking gate, then returns the canned result. + /// The username (ignored). + /// The password (ignored). + /// Cancellation token, observed while parked. + public async Task AuthenticateAsync(string username, string password, CancellationToken ct = default) + { + Interlocked.Increment(ref _entered); + await _park.Task.WaitAsync(ct).ConfigureAwait(false); + return _then; + } + } + /// Test group→role mapper driven by a delegate over the supplied groups. private sealed class FakeMapper(Func, IReadOnlyList> map) : IGroupRoleMapper { diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.Host.IntegrationTests/LdapOpcUaUserAuthenticatorTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.Host.IntegrationTests/LdapOpcUaUserAuthenticatorTests.cs index 8f859751..b076de93 100644 --- a/tests/Server/ZB.MOM.WW.OtOpcUa.Host.IntegrationTests/LdapOpcUaUserAuthenticatorTests.cs +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.Host.IntegrationTests/LdapOpcUaUserAuthenticatorTests.cs @@ -1,5 +1,6 @@ using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Logging.Abstractions; +using Microsoft.Extensions.Options; using Shouldly; using Xunit; using ZB.MOM.WW.Auth.Abstractions.Roles; @@ -26,7 +27,7 @@ public sealed class LdapOpcUaUserAuthenticatorTests // group "configeditor" -> "Designer" (canonical, Task 1.7). var ldap = new FakeLdap(new LdapAuthResult(true, "Alice", "alice", new[] { "configeditor" }, Array.Empty(), null)); var mapper = new FakeMapper(g => g.Select(x => x == "configeditor" ? "Designer" : x).ToArray()); - var sut = new LdapOpcUaUserAuthenticator(ldap, ScopeFactoryWith(mapper), NullLogger.Instance); + var sut = new LdapOpcUaUserAuthenticator(ldap, ScopeFactoryWith(mapper), Options.Create(new LdapOptions()), NullLogger.Instance); var result = await sut.AuthenticateUserNameAsync("alice", "secret", CancellationToken.None); @@ -44,7 +45,7 @@ public sealed class LdapOpcUaUserAuthenticatorTests // nothing, so the union is exactly {Administrator}. var ldap = new FakeLdap(new LdapAuthResult(true, "dev", "dev", new[] { "dev" }, new[] { "Administrator" }, null)); var mapper = new FakeMapper(_ => Array.Empty()); - var sut = new LdapOpcUaUserAuthenticator(ldap, ScopeFactoryWith(mapper), NullLogger.Instance); + var sut = new LdapOpcUaUserAuthenticator(ldap, ScopeFactoryWith(mapper), Options.Create(new LdapOptions()), NullLogger.Instance); var result = await sut.AuthenticateUserNameAsync("dev", "anything", CancellationToken.None); @@ -59,7 +60,7 @@ public sealed class LdapOpcUaUserAuthenticatorTests { var ldap = new FakeLdap(new LdapAuthResult(true, "dev", "dev", new[] { "dev" }, new[] { "Administrator" }, null)); var mapper = new FakeMapper(_ => throw new InvalidOperationException("DB down")); - var sut = new LdapOpcUaUserAuthenticator(ldap, ScopeFactoryWith(mapper), NullLogger.Instance); + var sut = new LdapOpcUaUserAuthenticator(ldap, ScopeFactoryWith(mapper), Options.Create(new LdapOptions()), NullLogger.Instance); var result = await sut.AuthenticateUserNameAsync("dev", "anything", CancellationToken.None); @@ -73,7 +74,7 @@ public sealed class LdapOpcUaUserAuthenticatorTests { var ldap = new FakeLdap(new LdapAuthResult(false, null, "mallory", Array.Empty(), Array.Empty(), "Invalid username or password")); var mapper = new FakeMapper(g => g.ToArray()); - var sut = new LdapOpcUaUserAuthenticator(ldap, ScopeFactoryWith(mapper), NullLogger.Instance); + var sut = new LdapOpcUaUserAuthenticator(ldap, ScopeFactoryWith(mapper), Options.Create(new LdapOptions()), NullLogger.Instance); var result = await sut.AuthenticateUserNameAsync("mallory", "wrong", CancellationToken.None); @@ -87,7 +88,7 @@ public sealed class LdapOpcUaUserAuthenticatorTests { var ldap = new FakeLdap(_ => throw new InvalidOperationException("LDAP unreachable")); var mapper = new FakeMapper(g => g.ToArray()); - var sut = new LdapOpcUaUserAuthenticator(ldap, ScopeFactoryWith(mapper), NullLogger.Instance); + var sut = new LdapOpcUaUserAuthenticator(ldap, ScopeFactoryWith(mapper), Options.Create(new LdapOptions()), NullLogger.Instance); var result = await sut.AuthenticateUserNameAsync("anyone", "x", CancellationToken.None); @@ -102,7 +103,7 @@ public sealed class LdapOpcUaUserAuthenticatorTests { var ldap = new FakeLdap(new LdapAuthResult(true, null, "alice", new[] { "ReadOnly" }, Array.Empty(), null)); var mapper = new FakeMapper(g => g.ToArray()); - var sut = new LdapOpcUaUserAuthenticator(ldap, ScopeFactoryWith(mapper), NullLogger.Instance); + var sut = new LdapOpcUaUserAuthenticator(ldap, ScopeFactoryWith(mapper), Options.Create(new LdapOptions()), NullLogger.Instance); var result = await sut.AuthenticateUserNameAsync("alice", "x", CancellationToken.None);