From 3b56e5bd01a25f5822e4d94da6e5f133c3abc08c Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Thu, 20 Aug 2026 09:38:41 -0400 Subject: [PATCH] fix(security): bound the API-key store call under the SessionManager lock (archreview 03/S18) --- .../plans/R3-05-acl-hot-path-expiry-plan.md | 15 ++ ...-05-acl-hot-path-expiry-plan.md.tasks.json | 2 +- .../OpcUa/BoundedApiKeyStore.cs | 115 +++++++++ .../OpcUa/BoundedApiKeyStoreTests.cs | 244 ++++++++++++++++++ 4 files changed, 375 insertions(+), 1 deletion(-) create mode 100644 src/Server/ZB.MOM.WW.OtOpcUa.Host/OpcUa/BoundedApiKeyStore.cs create mode 100644 tests/Server/ZB.MOM.WW.OtOpcUa.Host.Tests/OpcUa/BoundedApiKeyStoreTests.cs diff --git a/archreview/plans/R3-05-acl-hot-path-expiry-plan.md b/archreview/plans/R3-05-acl-hot-path-expiry-plan.md index 0ecd9c86..48b81e82 100644 --- a/archreview/plans/R3-05-acl-hot-path-expiry-plan.md +++ b/archreview/plans/R3-05-acl-hot-path-expiry-plan.md @@ -603,3 +603,18 @@ for the orchestrator. - **Collateral:** `MeshRegistrationRoleScopingTests.HasAdminMarkerScan_MatchesTheKnownShapeOfProgramCs` pins the `if (hasAdmin)` marker count in `Program.cs`; the move took one braced marker with it, so the pin went 6 → 5 (the test's own message asks for exactly that when a marker moves). + +### T2 — `BoundedApiKeyStore` deadline + +- **RED observed:** `error CS0246: The type or namespace name 'BoundedApiKeyStore' could not be found` — + type absent, as predicted. +- **DEVIATION (API):** the plan's snippet calls `cts.CancelAfter(TimeSpan, TimeProvider)`. **No such + overload exists** (`error CS1501: No overload for method 'CancelAfter' takes 2 arguments`). The + clock-aware equivalent is the `CancellationTokenSource(TimeSpan, TimeProvider)` **constructor**, linked + with the caller's token — same behaviour, same injected clock, and the linked source still lets the + `when (!ct.IsCancellationRequested)` filter tell a deadline from a caller cancellation. +- Added beyond the plan's list: `A_zero_timeout_disables_the_deadline` (the documented rollback, and the + in-class control proving the *bound* is what denies rather than the fake) and + `Anonymous_grant_resolution_is_bounded_too` (moved forward from the plan's T3 list — the deadline half + reaches `FindGrantsAsync` in the same change, so it is asserted in the same change). +- 5 tests, green. diff --git a/archreview/plans/R3-05-acl-hot-path-expiry-plan.md.tasks.json b/archreview/plans/R3-05-acl-hot-path-expiry-plan.md.tasks.json index 46fc793e..f7855020 100644 --- a/archreview/plans/R3-05-acl-hot-path-expiry-plan.md.tasks.json +++ b/archreview/plans/R3-05-acl-hot-path-expiry-plan.md.tasks.json @@ -11,7 +11,7 @@ { "id": "T2", "subject": "BoundedApiKeyStore: hard per-call deadline on the key store under the SessionManager lock, fail-closed (03/S18)", - "status": "pending", + "status": "done", "blockedBy": [ "T1" ] diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.Host/OpcUa/BoundedApiKeyStore.cs b/src/Server/ZB.MOM.WW.OtOpcUa.Host/OpcUa/BoundedApiKeyStore.cs new file mode 100644 index 00000000..cd085541 --- /dev/null +++ b/src/Server/ZB.MOM.WW.OtOpcUa.Host/OpcUa/BoundedApiKeyStore.cs @@ -0,0 +1,115 @@ +using Microsoft.Extensions.Logging; +using Microsoft.Extensions.Options; +using ZB.MOM.WW.OtOpcUa.Configuration.Entities; +using ZB.MOM.WW.OtOpcUa.Core.Authorization; + +namespace ZB.MOM.WW.OtOpcUa.Host.OpcUa; + +/// +/// Bounds what an can cost the OPC UA impersonation callback (arch-review +/// 03/S18). Wraps on a node that has a ConfigDb; the in-memory +/// needs neither half and is deliberately left unwrapped. +/// +/// +/// +/// Why a deadline is the fix and the cache is not. The SDK raises impersonation +/// synchronously under a SessionManager-wide lock, so a store that does not answer does not +/// stall one activation — it stalls every activation in the server. A cache lowers how OFTEN the +/// store is asked; only a deadline bounds how LONG one ask may hold that lock. +/// +/// +/// ⚠️ Every bounded outcome DENIES. A deadline exceeded, a faulted inner store and a +/// cancelled call all surface as an exception into +/// 's catch-all, which returns the same opaque denial as +/// a bad credential. Nothing here can admit a session the inner store did not authorize — the +/// decorator can only return what the inner store returned, or throw. +/// +/// +public sealed class BoundedApiKeyStore : IApiKeyStore +{ + private readonly IApiKeyStore _inner; + private readonly IOptions _options; + private readonly ILogger _log; + private readonly TimeProvider _time; + + /// Initializes a new instance of the class. + /// The store being bounded. + /// The Security:ApiKeys options carrying the deadline and TTL. + /// The logger. + /// + /// The shared clock. The same instance the authorizer reads — see + /// DataPlaneAuthRegistration for why a second clock here would be a hazard rather than a + /// detail. + /// + public BoundedApiKeyStore( + IApiKeyStore inner, + IOptions options, + ILogger log, + TimeProvider time) + { + ArgumentNullException.ThrowIfNull(inner); + ArgumentNullException.ThrowIfNull(options); + ArgumentNullException.ThrowIfNull(log); + ArgumentNullException.ThrowIfNull(time); + + _inner = inner; + _options = options; + _log = log; + _time = time; + } + + /// + public Task FindAsync(string apiKeyId, DateTime utcNow, CancellationToken ct) => + BoundAsync(token => _inner.FindAsync(apiKeyId, utcNow, token), ct); + + /// + public Task> FindGrantsAsync(string principal, CancellationToken ct) => + BoundAsync(token => _inner.FindGrantsAsync(principal, token), ct); + + /// Runs one inner call under the configured deadline. + /// The inner call's result type. + /// The inner call, taking the deadline-linked token. + /// The caller's token — in production always None, see remarks. + /// Whatever the inner store returned. + /// + /// + /// The deadline is created HERE rather than received from the caller because there is no caller + /// to receive one from: OpcUaApplicationHost block-bridges the SDK's synchronous + /// impersonation callback with , so the ct the store + /// sees in production is always uncancellable. + /// + /// + /// ⚠️ It re-throws rather than returning . null means + /// "no such usable key", which the authenticator logs as denied — no enabled, unexpired key + /// with id X. A timeout is not that, and an operator reading that line during a ConfigDb + /// outage would go hunting for a key problem that does not exist. The distinct Warning below is + /// the honest signal; the denial itself is unchanged either way. + /// + /// + private async Task BoundAsync(Func> call, CancellationToken ct) + { + var timeoutMs = _options.Value.StoreTimeoutMs; + if (timeoutMs <= 0) return await call(ct).ConfigureAwait(false); + + // The deadline runs on the INJECTED clock (the TimeProvider ctor overload), not on a plain + // CancelAfter, so it is the same TimeProvider the authorizer and the revalidation timer read. + // Linked with the caller's token so a genuine caller cancellation is still honoured — and so the + // catch below can tell the two apart. + using var deadline = new CancellationTokenSource(TimeSpan.FromMilliseconds(timeoutMs), _time); + using var cts = CancellationTokenSource.CreateLinkedTokenSource(ct, deadline.Token); + + try + { + return await call(cts.Token).ConfigureAwait(false); + } + catch (OperationCanceledException) when (!ct.IsCancellationRequested) + { + _log.LogWarning( + "ApiKeyStore: the key store did not answer within {TimeoutMs} ms — denying this activation. " + + "Every OPC UA session activation on this server is serialized behind this call; " + + "check ConfigDb health.", + timeoutMs); + throw; + } + } +} diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.Host.Tests/OpcUa/BoundedApiKeyStoreTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.Host.Tests/OpcUa/BoundedApiKeyStoreTests.cs new file mode 100644 index 00000000..f448aa4c --- /dev/null +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.Host.Tests/OpcUa/BoundedApiKeyStoreTests.cs @@ -0,0 +1,244 @@ +using Microsoft.Extensions.Logging.Abstractions; +using Microsoft.Extensions.Options; +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.Configuration.Entities; +using ZB.MOM.WW.OtOpcUa.Core.Authorization; +using ZB.MOM.WW.OtOpcUa.Host.OpcUa; + +namespace ZB.MOM.WW.OtOpcUa.Host.Tests.OpcUa; + +/// +/// Covers — the deadline and TTL cache the OPC UA impersonation +/// callback's key lookup runs under (arch-review 03/S18). +/// +/// +/// +/// The deadline cases are driven through the real +/// rather than the store directly, because the fail-closed conversion — a throw becoming the +/// standard opaque denial — is part of what is being asserted, and it lives in the authenticator's +/// catch-all rather than in the decorator. +/// +/// +/// ⚠️ Deadline cases use a REAL short timeout; TTL cases use the fake clock. The hand-rolled +/// overrides GetUtcNow() only, so CreateTimer falls through to +/// the base implementation and produces a real system timer — advancing the fake clock does not +/// fire a CancelAfter. A deadline test "improved" to advance the fake clock would pass for +/// the wrong reason: the inner task simply never completing. +/// +/// +public sealed class BoundedApiKeyStoreTests +{ + private const string Pepper = "unit-test-pepper"; + private const string Principal = "scadabridge-dcl"; + + /// + /// A store that never answers must not hold the activation open: the deadline fires and the + /// activation is denied. + /// + /// + /// The SDK raises impersonation under a SessionManager-wide lock, so a lookup that does not + /// return stalls every session activation in the server — not just this one. That is the whole of + /// 03/S18, and it is why the bound is a deadline rather than a cache. + /// + [Fact] + public async Task A_blocked_store_denies_without_hanging_the_activation() + { + var (presented, inner) = SetUp(); + inner.BlockForever = true; + var auth = Auth(Bounded(inner, timeoutMs: 150, cacheSeconds: 0)); + + var result = await auth.AuthenticateUserNameAsync("apikey", presented, CancellationToken.None) + .WaitAsync(TimeSpan.FromSeconds(5)); + + result.Success.ShouldBeFalse(); + result.Error.ShouldBe(ApiKeyOpcUaUserAuthenticator.DenyMessage); + } + + /// + /// The positive control for : + /// the same fake, answering normally, still authenticates. + /// + /// + /// Without it a build whose decorator denied everything would pass every deadline case in + /// this class, and the bound would look correct while having locked the fleet out. + /// + [Fact] + public async Task A_reachable_store_still_authenticates() + { + var (presented, inner) = SetUp(); + var auth = Auth(Bounded(inner, timeoutMs: 150, cacheSeconds: 0)); + + var result = await auth.AuthenticateUserNameAsync("apikey", presented, CancellationToken.None); + + result.Success.ShouldBeTrue(); + result.Principal.ShouldBe(Principal); + } + + /// + /// A lookup that completes with a perfectly valid key after the deadline still denies — + /// nothing races back in past the bound. + /// + [Fact] + public async Task A_deadline_never_admits() + { + var (presented, inner) = SetUp(); + var gate = new TaskCompletionSource(); + inner.Gate = gate.Task; + var auth = Auth(Bounded(inner, timeoutMs: 100, cacheSeconds: 0)); + + var pending = auth.AuthenticateUserNameAsync("apikey", presented, CancellationToken.None); + + // Let the deadline pass, THEN hand the inner store its (valid) answer. + await Task.Delay(400); + gate.SetResult(); + + var result = await pending.WaitAsync(TimeSpan.FromSeconds(5)); + result.Success.ShouldBeFalse(); + result.Error.ShouldBe(ApiKeyOpcUaUserAuthenticator.DenyMessage); + } + + /// + /// A non-positive StoreTimeoutMs disables the deadline, restoring the unbounded behaviour + /// byte-for-byte — the documented rollback. + /// + [Fact] + public async Task A_zero_timeout_disables_the_deadline() + { + var (presented, inner) = SetUp(); + var gate = new TaskCompletionSource(); + inner.Gate = gate.Task; + var auth = Auth(Bounded(inner, timeoutMs: 0, cacheSeconds: 0)); + + var pending = auth.AuthenticateUserNameAsync("apikey", presented, CancellationToken.None); + + // Far longer than any deadline this class configures; an active bound would have denied by now. + await Task.Delay(300); + pending.IsCompleted.ShouldBeFalse("a disabled deadline must not cancel a slow lookup"); + + gate.SetResult(); + (await pending.WaitAsync(TimeSpan.FromSeconds(5))).Success.ShouldBeTrue(); + } + + /// The anonymous activation path gets the same deadline, and the same denial. + /// + /// It runs on every unauthenticated connect, so leaving it unbounded would leave the + /// server-wide-stall hazard fully open on the busiest activation path. + /// + [Fact] + public async Task Anonymous_grant_resolution_is_bounded_too() + { + var (_, inner) = SetUp(); + inner.BlockForever = true; + var auth = Auth(Bounded(inner, timeoutMs: 150, cacheSeconds: 0)); + + var result = await auth.AuthenticateAnonymousAsync(CancellationToken.None) + .WaitAsync(TimeSpan.FromSeconds(5)); + + result.Success.ShouldBeFalse(); + } + + // ---------- helpers ---------- + + private static BoundedApiKeyStore Bounded( + IApiKeyStore inner, int timeoutMs, int cacheSeconds, TimeProvider? clock = null) => new( + inner, + Options.Create(new ApiKeyOptions + { + Username = "apikey", + Pepper = Pepper, + StoreTimeoutMs = timeoutMs, + StoreCacheSeconds = cacheSeconds, + }), + NullLogger.Instance, + clock ?? TimeProvider.System); + + private static ApiKeyOpcUaUserAuthenticator Auth(IApiKeyStore store) => new( + store, + Options.Create(new ApiKeyOptions { Username = "apikey", Pepper = Pepper }), + NullLogger.Instance); + + private static (string Presented, GatedApiKeyStore Store) SetUp() + { + var (presented, apiKeyId, hash) = ApiKeySecret.Generate(Pepper); + var key = new ApiKey + { + ApiKeyId = apiKeyId, + Name = "test key", + SecretHash = hash, + Principal = Principal, + Enabled = true, + CreatedUtc = DateTime.UtcNow, + }; + return (presented, new GatedApiKeyStore(key)); + } + + /// + /// An inner store whose answer can be withheld — a hang the EF in-memory provider cannot + /// represent — and whose calls are counted. + /// + private sealed class GatedApiKeyStore(ApiKey key) : IApiKeyStore + { + /// When set, every call awaits this before answering. + public Task? Gate { get; set; } + + /// When true, no call ever completes. + public bool BlockForever { get; set; } + + /// How many calls reached this store (i.e. were not served from the cache). + public int Calls => _calls; + + /// When true, the key lookup answers "no usable key" — a revocation. + public bool Revoked { get; set; } + + /// Number of leading calls that throw before the store starts answering. + public int ThrowsRemaining { get; set; } + + private int _calls; + + public async Task FindAsync(string apiKeyId, DateTime utcNow, CancellationToken ct) + { + await WaitAsync(ct).ConfigureAwait(false); + if (Revoked) return null; + return string.Equals(apiKeyId, key.ApiKeyId, StringComparison.Ordinal) + ? new ApiKeyLookup(key, []) + : null; + } + + public async Task> FindGrantsAsync(string principal, CancellationToken ct) + { + await WaitAsync(ct).ConfigureAwait(false); + return []; + } + + private async Task WaitAsync(CancellationToken ct) + { + Interlocked.Increment(ref _calls); + + if (ThrowsRemaining > 0) + { + ThrowsRemaining--; + throw new InvalidOperationException("store is down"); + } + + if (BlockForever) await Task.Delay(Timeout.Infinite, ct).ConfigureAwait(false); + if (Gate is { } gate) await gate.WaitAsync(ct).ConfigureAwait(false); + } + } + + /// + /// A hand-advanced clock. Overrides GetUtcNow() only — see the class remarks for why that + /// confines it to the TTL cases. + /// + private sealed class FakeClock(DateTimeOffset utcNow) : TimeProvider + { + private DateTimeOffset _now = utcNow; + + /// + public override DateTimeOffset GetUtcNow() => _now; + + /// Moves the clock forward. + /// How far forward to move. + public void Advance(TimeSpan by) => _now = _now.Add(by); + } +}