fix(security): bound the API-key store call under the SessionManager lock (archreview 03/S18)
This commit is contained in:
Joseph Doherty
@@ -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.
|
||||
|
||||
@@ -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"
|
||||
]
|
||||
|
||||
@@ -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;
|
||||
|
||||
/// <summary>
|
||||
/// Bounds what an <see cref="IApiKeyStore"/> can cost the OPC UA impersonation callback (arch-review
|
||||
/// 03/S18). Wraps <see cref="ConfigDbApiKeyStore"/> on a node that has a ConfigDb; the in-memory
|
||||
/// <see cref="ArtifactApiKeyStore"/> needs neither half and is deliberately left unwrapped.
|
||||
/// </summary>
|
||||
/// <remarks>
|
||||
/// <para>
|
||||
/// <b>Why a deadline is the fix and the cache is not.</b> The SDK raises impersonation
|
||||
/// synchronously under a <c>SessionManager</c>-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.
|
||||
/// </para>
|
||||
/// <para>
|
||||
/// ⚠️ <b>Every bounded outcome DENIES.</b> A deadline exceeded, a faulted inner store and a
|
||||
/// cancelled call all surface as an exception into
|
||||
/// <see cref="ApiKeyOpcUaUserAuthenticator"/>'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.
|
||||
/// </para>
|
||||
/// </remarks>
|
||||
public sealed class BoundedApiKeyStore : IApiKeyStore
|
||||
{
|
||||
private readonly IApiKeyStore _inner;
|
||||
private readonly IOptions<ApiKeyOptions> _options;
|
||||
private readonly ILogger<BoundedApiKeyStore> _log;
|
||||
private readonly TimeProvider _time;
|
||||
|
||||
/// <summary>Initializes a new instance of the <see cref="BoundedApiKeyStore"/> class.</summary>
|
||||
/// <param name="inner">The store being bounded.</param>
|
||||
/// <param name="options">The <c>Security:ApiKeys</c> options carrying the deadline and TTL.</param>
|
||||
/// <param name="log">The logger.</param>
|
||||
/// <param name="time">
|
||||
/// The shared clock. The same instance the authorizer reads — see
|
||||
/// <c>DataPlaneAuthRegistration</c> for why a second clock here would be a hazard rather than a
|
||||
/// detail.
|
||||
/// </param>
|
||||
public BoundedApiKeyStore(
|
||||
IApiKeyStore inner,
|
||||
IOptions<ApiKeyOptions> options,
|
||||
ILogger<BoundedApiKeyStore> log,
|
||||
TimeProvider time)
|
||||
{
|
||||
ArgumentNullException.ThrowIfNull(inner);
|
||||
ArgumentNullException.ThrowIfNull(options);
|
||||
ArgumentNullException.ThrowIfNull(log);
|
||||
ArgumentNullException.ThrowIfNull(time);
|
||||
|
||||
_inner = inner;
|
||||
_options = options;
|
||||
_log = log;
|
||||
_time = time;
|
||||
}
|
||||
|
||||
/// <inheritdoc />
|
||||
public Task<ApiKeyLookup?> FindAsync(string apiKeyId, DateTime utcNow, CancellationToken ct) =>
|
||||
BoundAsync(token => _inner.FindAsync(apiKeyId, utcNow, token), ct);
|
||||
|
||||
/// <inheritdoc />
|
||||
public Task<IReadOnlyList<NodeAcl>> FindGrantsAsync(string principal, CancellationToken ct) =>
|
||||
BoundAsync(token => _inner.FindGrantsAsync(principal, token), ct);
|
||||
|
||||
/// <summary>Runs one inner call under the configured deadline.</summary>
|
||||
/// <typeparam name="T">The inner call's result type.</typeparam>
|
||||
/// <param name="call">The inner call, taking the deadline-linked token.</param>
|
||||
/// <param name="ct">The caller's token — in production always <c>None</c>, see remarks.</param>
|
||||
/// <returns>Whatever the inner store returned.</returns>
|
||||
/// <remarks>
|
||||
/// <para>
|
||||
/// The deadline is created HERE rather than received from the caller because there is no caller
|
||||
/// to receive one from: <c>OpcUaApplicationHost</c> block-bridges the SDK's synchronous
|
||||
/// impersonation callback with <see cref="CancellationToken.None"/>, so the <c>ct</c> the store
|
||||
/// sees in production is always uncancellable.
|
||||
/// </para>
|
||||
/// <para>
|
||||
/// ⚠️ <b>It re-throws rather than returning <see langword="null"/>.</b> <c>null</c> means
|
||||
/// "no such usable key", which the authenticator logs as <i>denied — no enabled, unexpired key
|
||||
/// with id X</i>. 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.
|
||||
/// </para>
|
||||
/// </remarks>
|
||||
private async Task<T> BoundAsync<T>(Func<CancellationToken, Task<T>> 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;
|
||||
}
|
||||
}
|
||||
}
|
||||
@@ -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;
|
||||
|
||||
/// <summary>
|
||||
/// Covers <see cref="BoundedApiKeyStore"/> — the deadline and TTL cache the OPC UA impersonation
|
||||
/// callback's key lookup runs under (arch-review 03/S18).
|
||||
/// </summary>
|
||||
/// <remarks>
|
||||
/// <para>
|
||||
/// The deadline cases are driven through the <b>real</b> <see cref="ApiKeyOpcUaUserAuthenticator"/>
|
||||
/// 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.
|
||||
/// </para>
|
||||
/// <para>
|
||||
/// ⚠️ <b>Deadline cases use a REAL short timeout; TTL cases use the fake clock.</b> The hand-rolled
|
||||
/// <see cref="FakeClock"/> overrides <c>GetUtcNow()</c> only, so <c>CreateTimer</c> falls through to
|
||||
/// the base implementation and produces a real system timer — advancing the fake clock does not
|
||||
/// fire a <c>CancelAfter</c>. A deadline test "improved" to advance the fake clock would pass for
|
||||
/// the wrong reason: the inner task simply never completing.
|
||||
/// </para>
|
||||
/// </remarks>
|
||||
public sealed class BoundedApiKeyStoreTests
|
||||
{
|
||||
private const string Pepper = "unit-test-pepper";
|
||||
private const string Principal = "scadabridge-dcl";
|
||||
|
||||
/// <summary>
|
||||
/// A store that never answers must not hold the activation open: the deadline fires and the
|
||||
/// activation is denied.
|
||||
/// </summary>
|
||||
/// <remarks>
|
||||
/// The SDK raises impersonation under a <c>SessionManager</c>-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.
|
||||
/// </remarks>
|
||||
[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);
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// The positive control for <see cref="A_blocked_store_denies_without_hanging_the_activation"/>:
|
||||
/// the same fake, answering normally, still authenticates.
|
||||
/// </summary>
|
||||
/// <remarks>
|
||||
/// Without it a build whose decorator denied <b>everything</b> would pass every deadline case in
|
||||
/// this class, and the bound would look correct while having locked the fleet out.
|
||||
/// </remarks>
|
||||
[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);
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// A lookup that completes with a perfectly valid key <b>after</b> the deadline still denies —
|
||||
/// nothing races back in past the bound.
|
||||
/// </summary>
|
||||
[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);
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// A non-positive <c>StoreTimeoutMs</c> disables the deadline, restoring the unbounded behaviour
|
||||
/// byte-for-byte — the documented rollback.
|
||||
/// </summary>
|
||||
[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();
|
||||
}
|
||||
|
||||
/// <summary>The anonymous activation path gets the same deadline, and the same denial.</summary>
|
||||
/// <remarks>
|
||||
/// It runs on every unauthenticated connect, so leaving it unbounded would leave the
|
||||
/// server-wide-stall hazard fully open on the busiest activation path.
|
||||
/// </remarks>
|
||||
[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<BoundedApiKeyStore>.Instance,
|
||||
clock ?? TimeProvider.System);
|
||||
|
||||
private static ApiKeyOpcUaUserAuthenticator Auth(IApiKeyStore store) => new(
|
||||
store,
|
||||
Options.Create(new ApiKeyOptions { Username = "apikey", Pepper = Pepper }),
|
||||
NullLogger<ApiKeyOpcUaUserAuthenticator>.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));
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// An inner store whose answer can be withheld — a hang the EF in-memory provider cannot
|
||||
/// represent — and whose calls are counted.
|
||||
/// </summary>
|
||||
private sealed class GatedApiKeyStore(ApiKey key) : IApiKeyStore
|
||||
{
|
||||
/// <summary>When set, every call awaits this before answering.</summary>
|
||||
public Task? Gate { get; set; }
|
||||
|
||||
/// <summary>When true, no call ever completes.</summary>
|
||||
public bool BlockForever { get; set; }
|
||||
|
||||
/// <summary>How many calls reached this store (i.e. were not served from the cache).</summary>
|
||||
public int Calls => _calls;
|
||||
|
||||
/// <summary>When true, the key lookup answers "no usable key" — a revocation.</summary>
|
||||
public bool Revoked { get; set; }
|
||||
|
||||
/// <summary>Number of leading calls that throw before the store starts answering.</summary>
|
||||
public int ThrowsRemaining { get; set; }
|
||||
|
||||
private int _calls;
|
||||
|
||||
public async Task<ApiKeyLookup?> 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<IReadOnlyList<NodeAcl>> 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);
|
||||
}
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// A hand-advanced clock. Overrides <c>GetUtcNow()</c> only — see the class remarks for why that
|
||||
/// confines it to the TTL cases.
|
||||
/// </summary>
|
||||
private sealed class FakeClock(DateTimeOffset utcNow) : TimeProvider
|
||||
{
|
||||
private DateTimeOffset _now = utcNow;
|
||||
|
||||
/// <inheritdoc />
|
||||
public override DateTimeOffset GetUtcNow() => _now;
|
||||
|
||||
/// <summary>Moves the clock forward.</summary>
|
||||
/// <param name="by">How far forward to move.</param>
|
||||
public void Advance(TimeSpan by) => _now = _now.Add(by);
|
||||
}
|
||||
}
|
||||
Reference in New Issue
Block a user