Files
lmxopcua/archreview/plans/R3-05-acl-hot-path-expiry-plan.md
T
Joseph Doherty cde5cdfd9f fix(docker-dev): wire OTOPCUA_ACL_ENABLED to the centrals; record the S18 follow-up
The R3-05 gate's own recipe correction, actioned. Security__Acl__Enabled:
"${OTOPCUA_ACL_ENABLED:-false}" was wired to site-a-1/site-a-2 only, so
`OTOPCUA_ACL_ENABLED=true docker compose up -d central-1 central-2` was a silent
no-op ("Running", no recreate, no env) — on the one node kind that carries
BoundedApiKeyStore at all, since only a fused admin+driver node wraps
ConfigDbApiKeyStore. The gate had to proceed via a scratch compose override.

The key is now on central-1 and central-2 too, same env var and same default
false, so today's rig behaviour is byte-identical and the plan's step 1 does what
it says. site-b stays hardcoded OFF as the A/B control.

Plan gains a "## S18 hard-deadline follow-up (2026-08-20)" section: the
cooperative-vs-hard root cause, the red-before-green observation (3/18 failing
with TimeoutException, and why the 15 existing deadline tests could not see it),
the fix, and the still-owed live re-gate table (expected vs. what the gate saw).
2026-08-20 13:29:30 -04:00

95 KiB
Raw Blame History

Design + Implementation Plan — R3-05 ACL Hot Path + Clock-Driven Key Expiry

  • Source report: archreview/03-server-runtime.md §S18 (~line 82) · §S19 (~line 86) — round-3 re-review, 2026-08-19
  • Round-2 design consumed: plans/R2-08-ldap-historyread-async-plan.md §S2 — the SDK SessionManager-lock analysis this finding inherits (that plan's timeout/concurrency/circuit machinery was deleted, not relocated, by #520; S18 is the residual it left behind)
  • Plan verified against tree at: master 7c77a8c7 (clean; code identical to the review commit 12b0035a — the intervening commit is the review's own docs merge) — every anchor re-opened and re-checked 2026-08-20
  • Scope: OVERALL prioritized action item #5 — "ACL hot path on admin/fused nodes: per-call timeout or small TTL cache in ConfigDbApiKeyStore (revocation stays immediate via the authorizer's per-op check); low-frequency RevalidateMonitoredItems timer for clock-driven key expiry" (03 S18, 03 S19). Both findings are NEW this round; no branch has ever targeted either.
  • Cross-cutting requirement (OVERALL theme 6, "doc drift is now a regression vector"): this plan's verification found two documented claims that the code does not support — the store table in docs/security.md:180-183 and the CLAUDE.md "asymmetric revocation" sentence. Both are corrected here in the same change as the code, not filed as a doc sweep.
  • Fail-closed invariant (non-negotiable, both findings): every new failure path — store deadline exceeded, store faulted, cache miss during an outage, timer tick throwing — must deny, never admit. The authenticator's existing catch-all (ApiKeyOpcUaUserAuthenticator.cs:91-97, :163-170) already converts a throw into Deny(DenyMessage); the new code is designed to ride that rather than to invent a second denial path.

Verification summary

Both findings CONFIRMED at 7c77a8c7. Neither is stale, neither is partially fixed, and — unusually — every anchor line in both findings is EXACT, with zero drift. Anchor status:

Finding Report anchor Status at 7c77a8c7
03/S18 ApiKeyOpcUaUserAuthenticator.cs:22 ("no network I/O on this path at all") Exact. The claim is on line 22 of the class remarks. It is true for ArtifactApiKeyStore and false for ConfigDbApiKeyStore; the doc does not qualify by node role.
03/S18 ConfigDbApiKeyStore.cs:90-115 (per-activation EF query) Exact. FindAsync :90-108 — CreateDbContextAsync + SingleOrDefaultAsync on ApiKeys + LoadGrantsAsync (:129-155, a second ToListAsync on NodeAcls). So it is two round trips per UserName activation, not one. FindGrantsAsync :111-115 is a third for the anonymous path. No CommandTimeout, no deadline, no cache, no concurrency bound anywhere in the file.
03/S18 OpcUaApplicationHost.cs:317-319 (block-bridge, CancellationToken.None) Exact. AuthenticateUserNameAsync(…, CancellationToken.None).GetAwaiter().GetResult(). Anonymous path is the same shape at :375-376. The ct the store receives is therefore always None in production — a timeout cannot arrive from the caller and must be created inside.
03/S18 OpcUaApplicationHost.cs:269-275 (records the #520 deletion) Exact. "…so the bound/concurrency/circuit machinery that used to live there is gone rather than relocated." The surrounding remarks :262-276 still carry the R2-08 SDK finding verbatim: the handler is raised inside SessionManager.m_eventLock, so a blocked handler serializes every session activation server-wide, Anonymous and X509 included.
03/S18 recommendation: "revocation stays immediate … because DataPlaneAuthorizer.Evaluate re-checks the key per operation — DataPlaneAuthorizer.cs:238-248" Anchor exact, INFERENCE WRONG — corrected below. The per-op re-check is snapshot.FindUsableKey(keyId, now) at :247, where snapshot = _authority.Current (:230). That is the in-memory artifact snapshot (DataPlaneAuthority.cs:105-112), not IApiKeyStore and not the ConfigDb. See "What the per-operation re-check actually reads" below.
03/S19 OtOpcUaNodeManager.cs:320-328 ("this is the only trigger") Exact. OnAuthorityChanged at :328 is the sole caller of RevalidateMonitoredItems; :322-325 already anticipates a periodic trigger and states the constraint it must honour ("must give each tick a fresh memo … must not be tempted to reuse one across ticks").
03/S19 SessionAclDecisionCache.cs:20-28 (key check deliberately excluded from the cache) Exact, and the reasoning is the same one this plan uses: "a key's ExpiresUtc passes with the clock, not with a deployment".

Supporting facts verified (each one load-bearing for a design decision below):

  • RevalidateMonitoredItems is public (OtOpcUaNodeManager.cs:3235) — a timer can call it with no new seam. It early-returns unless AclEnforced and _liveItems is non-empty (:3237).
  • The pass's verdict flows IsSubscriptionStillPermitted (:3319-3341) → DataPlaneAuthorizer.IsAllowedCached (:170-175) → Evaluate → _time.GetUtcNow() (:246). The expiry verdict is therefore driven by the authorizer's injected TimeProvider, which is exactly what makes a FakeTimeProvider expiry test possible without touching the node manager.
  • The memo at :3246 is a local Dictionary constructed per call — the constraint :322-325 names is already satisfied by construction, so a periodic trigger needs no change to the pass itself.
  • The stand-down mechanics are already correct and reusable: queue BadUserAccessDenied then detach from MonitoredNode2 (:3270-3279), never MonitoringMode.Disabled (:3203-3208), and the restore branch (:3258-3265) re-attaches when a verdict flips back. A clock-driven pass inherits all of it, including reversibility.
  • DataPlaneAuthorizer takes TimeProvider? time = null (:87-103) but Program.cs:408-412 constructs it without one, so production runs TimeProvider.System. There is no TimeProvider registration in DI anywhere in Program.cs.
  • Options: ApiKeyOptions and AclOptions are bound with plain Configure<> (Program.cs:153-156), not AddValidatedOptions — so neither section has a fail-fast validator today.
  • Store wiring: Program.cs:414-434 registers ConfigDbApiKeyStore on hasAdmin (fused included); :435-442 registers ArtifactApiKeyStore on driver-only; :443-449 registers the deny-all NullOpcUaUserAuthenticator when neither role.
  • OtOpcUaServerHostedService (:81-110 ctor, :117 StartAsync) already holds the node manager (_server.NodeManager), the authorizer (_dataPlaneAuthorizer, :39) and an ILoggerFactory; it wires both onto the server at :237 / :242 and tears them down at :280-297. It is the natural and only sensible home for a revalidation timer.
  • Test infrastructure: FakeTimeProvider is hand-rolled in three places (DataPlaneAuthorizerTests.cs:618, NodeManagerStage6EnforcementTests.cs:796, TriePermissionEvaluatorTests.cs:19) — the Microsoft.Extensions.TimeProvider.Testing package is not in Directory.Packages.props. Each override supplies GetUtcNow() only, so CreateTimer falls through to the base implementation and produces a real system timer. This shapes the test design (see Finding 2 → Tests).
  • The over-the-wire rig exists: tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.IntegrationTests/AclSubscriptionEnforcementTests.cs boots a real OtOpcUaSdkServer, connects a real Opc.Ua.Client session, and already proves deploy-driven revocation reaches a live subscription (Revoking_a_grant_stops_a_live_subscription :52, restore at :134). Its Rig builds the authorizer at :336 — a TimeProvider can be threaded in there. ⚠️ It connects anonymously (:29-32 remarks), so the expiry test needs a key-carrying session it does not have yet.
  • ConfigDb store tests use the EF in-memory provider (ConfigDbApiKeyStoreTests.cs:29-34) and never touch central SQL — so the new store tests stay in the fast unit leg.
  • docker-dev already exposes the flag: Security__Acl__Enabled: "${OTOPCUA_ACL_ENABLED:-false}" on both central services (docker-compose.yml:585, :702), and a committed dev pepper at :164. The /apikeys page (AdminUI/Components/Pages/ApiKeys.razor:1) mints keys with a date-only expiry input (:151-155 — midnight UTC), and offers Revoke / Re-enable / Delete but no expiry editor for an existing key. That constrains the live-gate recipe (see Finding 2 → Live gate).

What the per-operation re-check actually reads — the question the scope asked, answered

No. The per-operation re-check does NOT hit ConfigDbApiKeyStore, and it issues no SQL of any kind.

The full call graph, with the I/O and threading facts attached to each edge:

# Edge Thread Lock held I/O
1 SDK SessionManager.ActivateSessionAsync → OpcUaApplicationHost.HandleImpersonation (:280) SDK request thread SessionManager.m_eventLock, server-wide —
2 HandleImpersonation → ApiKeyOpcUaUserAuthenticator.AuthenticateUserNameAsync via GetAwaiter().GetResult() (:317-319), ct = CancellationToken.None same (blocked) same —
3 authenticator → IApiKeyStore.FindAsync (ApiKeyOpcUaUserAuthenticator.cs:89) same same depends on the store
3a …→ ConfigDbApiKeyStore.FindAsync (admin / fused) same same ⚠️ TWO SQL round trips (ApiKeys :96-103, NodeAcls :132-136), unbounded, ct = None
3b …→ ArtifactApiKeyStore.FindAsync (driver-only) same same none — two dictionary probes on the in-memory snapshot (:64-73)
4 anonymous activation → FindGrantsAsync (:161) same same one SQL round trip (3a) / none (3b)
5 per-operation Read/Write/Browse/Subscribe/HistoryRead → DataPlaneAuthorizer.Evaluate (:212) SDK request thread node-manager Lock on some seams, never the SessionManager lock —
6 Evaluate → _authority.Current (:230) → snapshot.FindUsableKey(keyId, now) (:247) same same none — one dictionary probe + two field comparisons on the artifact snapshot (DataPlaneAuthority.cs:105-112)
7 Evaluate → Decisions.TryGet (:261-269) — grant-walk memo only, consulted after step 6 same same none
8 deploy apply → DataPlaneAuthorityState.Apply (:289-337) → Notify (:335, outside the state lock) → OtOpcUaNodeManager.OnAuthorityChanged (:328) → RevalidateMonitoredItems (:3235) deploy thread takes the node-manager Lock per item (:3262, :3277) none

So the answer is the good one for performance — a Read or Browse costs zero SQL — and it carries a consequence the report's recommendation did not anticipate:

⚠️ On a fused admin+driver node the authenticator and the per-operation re-check read DIFFERENT key sources. Activation reads the ConfigDb (live); every subsequent operation on that session re-checks the artifact snapshot, because OpcUaPublishActor installs the artifact's keys on IDataPlaneAuthoritySource on every apply (OpcUaPublishActor.cs:755-761) and that is what _authority.Current returns on every driver-role node, fused or not.

Two documented claims fall out of that, and both are wrong today:

  1. docs/security.md:180-183 — "admin (including fused admin,driver) | ConfigDbApiKeyStore … | Revocation takes effect On the next request". True only for a new session activation. For a session that is already open, clearing Enabled in the ConfigDb reaches nothing until the next deployment re-installs the snapshot — the same latency the driver-only node has.
  2. CLAUDE.md — "⚠️ The consequence is asymmetric revocation: immediate on a ConfigDb node, next-deployment on a driver-only one." Same defect: the asymmetry is real but it is activation-scoped, not session-scoped.

What is immediate on both node kinds is expiry, because FindUsableKey compares ExpiresUtc against the authorizer's clock on every call (DataPlaneAuthority.cs:110) — which is precisely why S19's subscription hole is the one remaining gap in the expiry story rather than a curiosity.

This correction also rules out one of the designs the scope offered. A TTL cache inside the ConfigDb store cannot be justified by "revocation stays immediate via the authorizer's per-op check", because that check consults a source the cache does not sit in front of. The TTL's honest cost is stated in Finding 1 instead.

Design choices resolved in this plan

  1. S18 primary mechanism: a deadline, not a cache. The named hazard is unbounded blocking under a server-wide lock; a cache reduces the frequency of the round trip but does nothing about the first one, and a hung DB blocks every cold key for as long as SqlClient allows. The TTL cache is kept as a second, subordinate layer because it turns a burst of activations into one round trip.
  2. S18 placement: a decorator (BoundedApiKeyStore : IApiKeyStore), not edits inside ConfigDbApiKeyStore. Three reasons: it is testable against a fake inner store with a TaskCompletionSource (a hang is unrepresentable with the EF in-memory provider); it keeps the cluster-confinement logic — the store's actual subject, and the thing a past bug turned on — untouched; and it is trivially not applied to ArtifactApiKeyStore, which needs neither half.
  3. S18 does NOT unify the two stores. Option (c) from the scope — a background refresher feeding an ArtifactApiKeyStore-shaped in-memory store on both node kinds — is rejected below with reasons; the short version is that it would delete the one operator-visible property a ConfigDb node has (activation-time revocation) and would introduce a boot window in which a fused central denies every key.
  4. S19 expires monitored items only; it does NOT close the session. Reasons in Finding 2. The deciding one is consistency: clock-driven denial must look exactly like deploy-driven denial to a client, and the deploy path deliberately does not close sessions.
  5. S19's timer is inert unless ACL enforcement is on, because RevalidateMonitoredItems early-returns on !AclEnforced (:3237). That is stated rather than worked around: with Security:Acl:Enabled=false there is no per-operation key check anywhere, so an expired key retains its session's roles until it disconnects. Named as a residual, not fixed here.
  6. Event monitored items are out of scope, and this is a discovery, not an omission. _liveItems is populated only from OnMonitoredItemCreated (:3138-3143), whose parameter is ISampledDataChangeMonitoredItem. Event subscriptions are gated at creation (SubscribeToEvents :3092-3113) and then tracked by nothing, so neither the deploy trigger nor the new timer revalidates them — an alarm feed authorized under a key survives both revocation and expiry. Named as a residual with a recommended follow-up.
  7. TimeProvider gets a DI registration. Both new components need one, and the authorizer already documents a two-clock hazard (DataPlaneAuthorizer.cs:321-332: "any divergence between the two clocks denies every request"). One registered singleton threaded into the authorizer, the store decorator and the timer makes divergence unrepresentable rather than merely unlikely.

Finding 1 — 03/S18: API-key lookup is a per-activation SQL round trip on admin/fused nodes, under the SessionManager lock, with no timeout (Medium)

Restatement

ApiKeyOpcUaUserAuthenticator's central claim — "no directory call, no retry, and no network I/O on this path at all" (:22) — holds only where IApiKeyStore is ArtifactApiKeyStore. On an admin or fused node the store is ConfigDbApiKeyStore, and a UserName activation costs two EF queries (the key row, then the principal's NodeAcl rows), issued with CancellationToken.None, with no command timeout and no cache — from inside a synchronous callback that the SDK raises while holding a SessionManager-wide lock. A ConfigDb that is slow or hung rather than refusing therefore stalls every session activation on the server, Anonymous and X509 included, one SqlClient timeout at a time. #520 deleted the LDAP-era wall-clock bound, concurrency cap and outage circuit as unnecessary — correctly for the in-memory store, and the ConfigDb store then walked network I/O back into the same callback with none of them.

Verification

Confirmed exact (see summary table). Additional facts established while verifying:

  • It is two round trips per activation, not one: SingleOrDefaultAsync on ApiKeys (:96-103) then ToListAsync on NodeAcls (:132-136). Anonymous activation costs one (FindGrantsAsync :111-115). The report's "an EF query per activation" undercounts.
  • The ct parameter is decorative on this path: the only production caller passes CancellationToken.None (OpcUaApplicationHost.cs:318, :375). A deadline must be created inside the store; there is no caller to receive one from.
  • Bounded concurrency is genuinely unnecessary here, unlike in the LDAP era. The SessionManager lock means at most one impersonation callback is in flight process-wide, so the store sees no concurrency from this path at all. IApiKeyStore has no other production consumer (grep: the interface is referenced only by the authenticator, the two stores and their tests). A semaphore would be a no-op guarding a lock. This is a real simplification against R2-08's shape and is worth stating so nobody re-adds it.
  • The failure posture already routes correctly: the authenticator catches every exception and returns Deny(DenyMessage) (:91-97, and :163-170 for anonymous). An OperationCanceledException from a deadline therefore denies by construction, with no new denial path to get wrong.

Root cause

A seam introduced to serve two node roles was documented and reasoned about as if it had one implementation. The class remark at :22 describes ArtifactApiKeyStore's cost model and was never qualified when ConfigDbApiKeyStore — which the same PR wrote — was bound behind the same interface on the busier node. The deletion of the LDAP guards was justified against that unqualified claim.

Proposed design

A single decorator, BoundedApiKeyStore, wrapping the ConfigDb store on admin/fused nodes. Two layers, in priority order: a hard deadline (the fix) and a short TTL read-through cache (the amplifier). Both configurable, both fail-closed, neither applied to the artifact store.

// src/Server/ZB.MOM.WW.OtOpcUa.Host/OpcUa/BoundedApiKeyStore.cs  (new)
/// <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
///         <c>ApiKeyOpcUaUserAuthenticator</c>'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.
///     </para>
///     <para>
///         ⚠️ <b>Faults and timeouts are NEVER cached.</b> Caching a deny would be fail-closed and is still
///         wrong: it would extend a ConfigDb outage past the outage, denying valid keys for a further TTL
///         after the database came back. Only a COMPLETED lookup — a hit or an authoritative miss — is
///         stored.
///     </para>
/// </remarks>
public sealed class BoundedApiKeyStore : IApiKeyStore
{
    public BoundedApiKeyStore(
        IApiKeyStore inner,
        IOptions<ApiKeyOptions> options,
        ILogger<BoundedApiKeyStore> log,
        TimeProvider time);
    ...
}

Deadline. Each call runs the inner task under a linked CTS created from StoreTimeoutMs:

using var cts = CancellationTokenSource.CreateLinkedTokenSource(ct);
cts.CancelAfter(TimeSpan.FromMilliseconds(_timeoutMs), _time);
try
{
    return await _inner.FindAsync(apiKeyId, utcNow, cts.Token).ConfigureAwait(false);
}
catch (OperationCanceledException) when (!ct.IsCancellationRequested)
{
    OtOpcUaTelemetry.ApiKeyStoreBounded.Add(1, new KeyValuePair<string, object?>("outcome", "timeout"));
    _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;   // the authenticator's catch-all turns this into the standard opaque denial
}

CancelAfter(TimeSpan, TimeProvider) rather than the untimed overload: it keeps the deadline on the injected clock, which is the same instance the authorizer and the S19 timer use.

Re-throwing rather than returning null is deliberate: 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 above plus the counter is the honest signal.

TTL read-through cache. Keyed separately per method — FindAsync by (apiKeyId) and FindGrantsAsync by (principal) — each entry stamped with _time.GetUtcNow(), served while age < StoreCacheSeconds, re-read after. StoreCacheSeconds = 0 disables it entirely and restores today's per-request read exactly.

Bounding: a single ConcurrentDictionary per method with a wholesale clear on overflow past a fixed 1024 entries — the same reasoning SessionAclDecisionCache documents at :29-33 and :44-56 (per-entry eviction needs knowledge the cache does not have; a repopulate is cheap and cannot be wrong), and it makes a key-id guessing flood a bounded memory event rather than an unbounded one. Negative results are cached (a flood of unknown ids must not become a SQL flood); faults and timeouts are not.

The honest cost, stated rather than hand-waved: with the cache on, activation-time revocation moves from "the next activation" to "the next activation after the TTL expires". At the default 5 s that is not a security-relevant window for a machine-to-machine data plane — and it is strictly smaller than the window that already exists on both node kinds for already-open sessions, which is the next deployment. docs/security.md gets a table row that says exactly this instead of the sentence it says today.

New options (ApiKeyOptions, Security:ApiKeys):

Key Default Meaning
StoreTimeoutMs 2000 Hard per-call deadline on the key store. Exceeded ⇒ the activation is denied and a Warning + counter fire. ≤ 0 disables the deadline (today's unbounded behaviour — do not do this on an admin node).
StoreCacheSeconds 5 TTL of the read-through cache in front of the store. 0 disables it (a read per request, today's behaviour). Bounds how long a revocation can take to reach a new activation.

Both are clamped by the consumer (≤ 0 = off) rather than validated at boot. Security:ApiKeys is bound with plain Configure<> (Program.cs:153) and has no validator; converting the section to AddValidatedOptions would add a fail-fast path to a section every existing deployment already sets, for two knobs whose bad values have a safe interpretation. Noted as the alternative considered.

New instrument (OtOpcUaTelemetry):

/// <summary>Key-store calls the S18 bound intervened on (outcome=timeout|cache_hit).</summary>
public static readonly Counter<long> ApiKeyStoreBounded =
    Meter.CreateCounter<long>("otopcua.opcua.apikey_store.bounded", unit: "{lookup}",
        description: "API-key store lookups the bound intervened on (outcome=timeout|cache_hit).");

One counter with an outcome tag rather than two: timeout is the alarm signal, cache_hit is the ratio that tells an operator whether the TTL is doing anything. A missing cache_hit stream on a busy fused central is how you notice StoreCacheSeconds was set to 0 by accident.

Wiring (Program.cs:425-427), and the guard that keeps it wired:

builder.Services.AddSingleton<IApiKeyStore>(sp => new BoundedApiKeyStore(
    new ConfigDbApiKeyStore(
        sp.GetRequiredService<IDbContextFactory<OtOpcUaConfigDbContext>>(),
        ConfigDbApiKeyStore.ResolveClusterId(sp.GetRequiredService<IOptions<AkkaClusterOptions>>().Value)),
    sp.GetRequiredService<IOptions<ApiKeyOptions>>(),
    sp.GetRequiredService<ILoggerFactory>().CreateLogger<BoundedApiKeyStore>(),
    sp.GetRequiredService<TimeProvider>()));

plus builder.Services.AddSingleton(TimeProvider.System); registered once near the options block, and threaded into the DataPlaneAuthorizer factory at :408-412.

A decorator that is registered in one branch of a role conditional is exactly the shape OVERALL theme 3 calls out ("built-but-never-wired … re-grew in the new subsystems"), so it lands with a registration guard test: build the service collection for an admin node and assert IApiKeyStore resolves to a BoundedApiKeyStore wrapping a ConfigDbApiKeyStore; build it for a driver-only node and assert a bare ArtifactApiKeyStore.

Alternatives considered:

  • (a) alone — deadline with no cache. Rejected as insufficient rather than wrong: it bounds the lock hold, but under a degraded (not hung) ConfigDb every activation still pays two round trips serially behind the lock. The cache is what makes a burst cheap.
  • (b) alone — TTL cache with no deadline (the report's second suggestion). Rejected. It does not address the finding: a cold key against a hung DB still blocks the server-wide lock indefinitely, and a hung DB is precisely the scenario S18 describes. A cache is an optimisation; the finding is about a bound.
  • (c) background refresher into a shared in-memory store, one code path for both node kinds. Architecturally the most attractive and rejected on three concrete counts. (i) It deletes the only property that distinguishes a ConfigDb node — activation-time revocation — replacing "next request" with "next refresh", a larger window than the TTL cache it would replace. (ii) It creates a boot window: a fused central that has not completed its first refresh authenticates nothing, so a restart under a slow ConfigDb denies every key rather than serving a slightly stale one; mitigating that needs a synchronous initial load, which is the blocking call again, at start-up. (iii) It duplicates DataPlaneAuthorityState, which already holds exactly this data on every driver-role node — two in-memory key sets on one fused node, refreshed on different schedules, is a divergence waiting to be found by a support ticket. Kept on record because if a future change removes the ConfigDb store's NodeAcl read, (c) becomes cheap.
  • (d) drop ConfigDbApiKeyStore on fused nodes and use ArtifactApiKeyStore everywhere. The cleanest possible answer to "no I/O in the callback", and rejected for the same reason as (c)(i) plus one more: it would make a fused central unable to authenticate any key until its first deployment, which is a regression for the docker-dev rig and for any single-node install.
  • CommandTimeout on the DbContext instead of a CTS. Rejected: it bounds the SQL command, not CreateDbContextAsync, connection acquisition from a saturated pool, or DNS — the parts most likely to hang. A CTS bounds the whole call.

Implementation steps

  1. src/Core/ZB.MOM.WW.OtOpcUa.Core/Authorization/ApiKeyOptions.cs — StoreTimeoutMs (2000) + StoreCacheSeconds (5), each with the XML doc stating what 0 means.
  2. src/Server/ZB.MOM.WW.OtOpcUa.Host/OpcUa/BoundedApiKeyStore.cs (new) — deadline, TTL cache, bounded-with-wholesale-clear storage, Warning + counter.
  3. src/Core/ZB.MOM.WW.OtOpcUa.Commons/Observability/OtOpcUaTelemetry.cs — ApiKeyStoreBounded.
  4. src/Server/ZB.MOM.WW.OtOpcUa.Host/Program.cs — register TimeProvider.System; thread it into the DataPlaneAuthorizer factory (:408-412); wrap the ConfigDb store (:425-427).
  5. src/Server/ZB.MOM.WW.OtOpcUa.Host/OpcUa/ApiKeyOpcUaUserAuthenticator.cs — correct the :16-24 remark: qualify "no network I/O on this path at all" by node role and point at BoundedApiKeyStore for what bounds the other one. (The stale claim is the doc-drift half of this finding.)
  6. src/Server/ZB.MOM.WW.OtOpcUa.Host/OpcUa/ConfigDbApiKeyStore.cs — one remark paragraph noting it is wrapped in production and that a call may be cancelled by the wrapper's deadline.

Tests

New tests/Server/ZB.MOM.WW.OtOpcUa.Host.Tests/OpcUa/BoundedApiKeyStoreTests.cs, against a hand-written FakeApiKeyStore (a TaskCompletionSource gate + a call counter — the EF in-memory provider cannot represent a hang):

  • A_blocked_store_denies_without_hanging_the_activation — inner store returns a never-completing task; StoreTimeoutMs = 150; drive the real ApiKeyOpcUaUserAuthenticator (not the store directly, so the fail-closed conversion is part of the assertion) and assert Success == false with Error == ApiKeyOpcUaUserAuthenticator.DenyMessage, within a generous 5 s test bound. Positive control in the same class: the same fake completing normally returns Success == true — without it, a build where the decorator denies everything passes.
  • A_deadline_never_admits — the inner store completes after the deadline with a valid key; assert the result is still a denial (nothing races back in).
  • A_faulted_store_is_not_cached — inner throws once then succeeds; two calls; assert the second call reaches the inner store and succeeds (an outage must not outlive itself).
  • A_second_activation_within_the_TTL_does_not_reach_the_store — fake clock, two FindAsync calls; inner call count == 1; otopcua.opcua.apikey_store.bounded{outcome=cache_hit} incremented (MeterListener, filtered by a unique tag per the metrics-leak memory).
  • FALSIFICATION CONTROL — A_revoked_key_is_denied_once_the_TTL_has_passed — seed a usable key, call once (cached allow), flip the fake inner store to return null, advance the fake clock past StoreCacheSeconds, call again ⇒ denial. This is the test that makes the documented bound a fact rather than a claim; a cache with no expiry passes every other test in this class.
  • TTL_zero_reads_through_every_time — inner call count equals the number of calls.
  • Anonymous_grants_are_bounded_and_cached_too — FindGrantsAsync gets the same deadline and TTL, keyed by principal (the anonymous activation path is the one that runs on every unauthenticated connect).
  • Cache_overflow_clears_wholesale_and_keeps_serving — 1025 distinct ids, then assert the store still answers correctly and memory bookkeeping reset (guards the bound itself).

New tests/Server/ZB.MOM.WW.OtOpcUa.Host.Tests/OpcUa/ApiKeyStoreRegistrationTests.cs:

  • admin node (OTOPCUA_ROLES=admin,driver) ⇒ IApiKeyStore is BoundedApiKeyStore wrapping ConfigDbApiKeyStore;
  • driver-only ⇒ bare ArtifactApiKeyStore;
  • both ⇒ TimeProvider resolves, and the DataPlaneAuthorizer and the store share the same instance (the two-clock guard, by reference).

Existing suites stay green unchanged: ConfigDbApiKeyStoreTests (7), ArtifactApiKeyStoreTests, ApiKeyAuthenticationTests (16) — none of them constructs the decorator, and the inner store's behaviour is untouched.

⚠️ Test-design note carried into the tasks: the hand-rolled FakeTimeProvider in this repo overrides GetUtcNow() only, so CancelAfter(TimeSpan, TimeProvider) and Task.Delay on it fall through to real timers. The TTL tests are therefore deterministic (pure timestamp arithmetic on the fake clock) while the deadline tests use a real short deadline with a wide assertion margin. Do not "improve" the deadline tests by advancing the fake clock — it will not fire the CTS, and the test will pass for the wrong reason (the inner task simply never completing).

Effort + Risk

Effort: S-M. Risk: medium-high — this is the authentication path, and every failure mode of the new code must land on deny. Mitigations: the decorator adds no admit path (it can only return what the inner store returned, or throw); the authenticator's pre-existing catch-all is the conversion point, so no new denial code exists; StoreTimeoutMs = 0 and StoreCacheSeconds = 0 restore today's behaviour byte-for-byte for a rollback; and the registration guard prevents the decorator being silently unwired. The genuine behaviour change is that a ConfigDb slower than 2 s now denies activations it would previously have served late — surfaced by a distinct Warning and a counter, and configurable.


Finding 2 — 03/S19: Key expiry never reaches live monitored items between deploys (Medium)

Restatement

RevalidateMonitoredItems is the only mechanism that re-authorizes a live subscription, and its only trigger is a deployment (OtOpcUaNodeManager.cs:328, and the code says so at :320). Every request-shaped operation re-checks the calling key's usability on each call and therefore honours ExpiresUtc the moment it passes (DataPlaneAuthorizer.cs:246-248); the decision cache deliberately excludes that check for exactly this reason (SessionAclDecisionCache.cs:20-28). A subscription authorized under a then-valid key keeps streaming past its expiry until a deployment happens to advance the generation. On a quiet fleet — a site that has been deployed once and is running — that is indefinite. Revocation-by-deploy is covered; revocation-by-clock is not, on the one path machine consumers actually use.

Verification

Confirmed exact (see summary table). The mechanism needed to fix it is already present and correct:

  • The pass is public (:3235) and takes no arguments — a timer is a call, not a new seam.
  • The verdict already comes from the authorizer's TimeProvider (Evaluate :246), so a periodic pass evaluates expiry against the clock with no change to the decision logic.
  • The memo is pass-local (:3246), which is the exact precondition :322-325 says a periodic trigger must satisfy.
  • Stand-down and restore are both implemented and reversible (:3258-3279), so a key renewed by re-activation is picked up by the next tick with no client action.
  • RevalidateMonitoredItems early-returns unless AclEnforced (:3237), so the timer is inert on the default configuration and cannot regress a deployment that has not opted into ACL enforcement.
  • ⚠️ _liveItems is fed only by OnMonitoredItemCreated(… ISampledDataChangeMonitoredItem …) (:3138-3155). Event monitored items are tracked nowhere, so alarm subscriptions are revalidated by neither trigger. Pre-existing; see residuals.

Root cause

Two revocation clocks, one trigger. The authorization model has a deployment-driven axis (grants, principals, the trie — all fixed for a generation) and a wall-clock axis (ApiKey.ExpiresUtc). Every seam that re-asks per request straddles both for free. The one seam that cannot re-ask — a subscription — was given a push on the deployment axis only, because that was the axis the feature (stage 6 revocation) was about.

Proposed design

A low-frequency timer that runs the existing pass. Deliberately minimal: no new decision code, no new state, no change to what a stand-down does.

New option (AclOptions, Security:Acl):

Key Default Meaning
RevalidationIntervalSeconds 60 How often live monitored items are re-authorized against the clock, in addition to on every deployment. 0 disables the timer (deploy-triggered only — the pre-R3-05 behaviour). Inert unless Security:Acl:Enabled is true.

60 s means a key's expiry reaches a live subscription within a minute of passing, at a cost of one pass over _liveItems per minute per node when enforcement is on and at most one dictionary probe plus a trie walk per distinct (session, realm, node) — the same work a single deploy already does, once a minute instead of once a deployment.

New component (AclRevalidationTimer) — pure, testable, no SDK types:

// src/Server/ZB.MOM.WW.OtOpcUa.Host/OpcUa/AclRevalidationTimer.cs  (new)
/// <summary>
///     Runs the node manager's monitored-item revalidation pass on a low-frequency tick, so a key that
///     expires by the CLOCK reaches a live subscription without waiting for a deployment (arch-review
///     03/S19).
/// </summary>
/// <remarks>
///     <para>
///         ⚠️ <b>The tick must never throw.</b> An unhandled exception on a timer callback thread takes
///         the process down. Every tick is wrapped; a faulting pass is logged once per fault and the
///         timer keeps running, because the next tick is the recovery.
///     </para>
///     <para>
///         ⚠️ <b>Each tick gets a fresh memo, and that is the pass's own constraint, not this class's.</b>
///         <c>RevalidateMonitoredItems</c> allocates its verdict dictionary per call — see the warning on
///         <c>OnAuthorityChanged</c>. Nothing here may hoist it.
///     </para>
/// </remarks>
internal sealed class AclRevalidationTimer : IDisposable
{
    internal AclRevalidationTimer(Action revalidate, TimeSpan interval, TimeProvider time, ILogger log);
    internal void Tick();          // the callback body, exposed for deterministic tests
    public void Dispose();
}

Construction creates the timer via time.CreateTimer(_ => Tick(), null, interval, interval); a non-positive interval creates no timer at all (and logs one Information line naming the key, so a disabled guard is visible rather than assumed).

Host wiring (OtOpcUaServerHostedService):

  • ctor gains TimeProvider time and IOptions<AclOptions> aclOptions (both required parameters against explicit registrations — the built-in container does not fall back to defaults for unregistered services, the trap Program.cs:390-395 and :443-449 both carry comments about).
  • At the end of StartAsync, after _server.NodeManager is known non-null (guarded at :139-143) and after SetDataPlaneAuthorizer (:242):
// The pass itself no-ops unless the authorizer is wired AND enabled, so this is created
// unconditionally and stays inert on the default configuration.
_revalidationTimer = new AclRevalidationTimer(
    () => _server?.NodeManager?.RevalidateMonitoredItems(),
    TimeSpan.FromSeconds(_aclOptions.RevalidationIntervalSeconds),
    _time,
    _loggerFactory.CreateLogger<AclRevalidationTimer>());
  • StopAsync disposes it first, before SetDataPlaneAuthority(null) / SetDataPlaneAuthorizer(null) (:293-297) — a tick landing on a half-torn-down node manager is the shape the teardown block at :280-297 already exists to prevent.

Session lifetime: expire the items, do NOT close the session. Four reasons, in the order they decided it:

  1. Consistency with the path that already exists. Deploy-driven revocation queues BadUserAccessDenied and detaches (:3270-3279) precisely so the client learns why its feed stopped and so a restored grant resumes it (:3209-3214). A clock-driven denial that instead killed the session would make identical authorization outcomes look completely different to a client depending on which clock moved.
  2. OPC UA gives the server no honest "expire this session" verb. The nearest thing is deleting the session server-side, which tears down every subscription with no per-item status — the client sees a transport-level failure, not an authorization one. That is strictly less information than BadUserAccessDenied on the affected items.
  3. The session is already inert for everything else. Read, Write, Browse, HistoryRead, alarm commands and new monitored items all re-check the key per call (Evaluate :247) and already deny. Closing the session removes nothing that is still working except the items this pass is about to detach anyway.
  4. Reversibility. OPC UA lets a client re-activate an existing session with a new user identity token, which re-runs the impersonation callback. Because the pass re-reads EffectiveIdentity per tick (:3256) and restores permitted items (:3258-3265), a client that renews its key recovers automatically on the next tick. Closing the session would force a full reconnect + resubscribe on every machine consumer in the fleet at once when a shared key expires — a self-inflicted thundering herd.

Documented as a consequence: an expired-key session is not disconnected; it is silenced with a reason and recovers on re-activation.

One deliberate non-change: RevalidateMonitoredItems stamps the queued DataValue from DateTime.UtcNow (:3239), not from a TimeProvider. Left alone — the node manager has no clock and giving it one for a notification timestamp would be the only reason it had one. Under a fake clock in tests the queued timestamps are wall-clock; no assertion depends on them.

Alternatives considered:

  • Wake on the nearest ExpiresUtc instead of a fixed interval. Rejected: it needs the snapshot's key set to be scanned for the next expiry and re-armed on every deploy and every activation, and it fails silently if any of those re-arms is missed — a fixed tick cannot be wrong about when it runs. The saving is a pass per minute over an in-memory dictionary.
  • Check expiry inside the value fan-out, per published value. Rejected: it puts an authorization check on the hottest path in the server (every value of every tag), to replace a check that a once-a-minute pass does for free.
  • Have DataPlaneAuthorityState raise Changed on a timer. Rejected as dishonest: Changed means "a new snapshot was published" and its handlers are documented as running on the deploy thread (DataPlaneAuthority.cs:196-198). Firing it with no new snapshot would make the event's contract a lie and would reach any future subscriber that reasonably assumed a generation change.
  • Do it in OpcUaPublishActor with an Akka timer. Rejected: it would couple the OPC UA server's authorization lifetime to the deploy actor's lifecycle, and the node manager is reachable from the hosted service directly. The hosted service already owns start/stop ordering for every other server-scoped seam.

Implementation steps

  1. src/Core/ZB.MOM.WW.OtOpcUa.Core/Authorization/AclOptions.cs — RevalidationIntervalSeconds (60) with an XML doc stating that 0 disables it and that it is inert while Enabled is false.
  2. src/Server/ZB.MOM.WW.OtOpcUa.Host/OpcUa/AclRevalidationTimer.cs (new).
  3. src/Server/ZB.MOM.WW.OtOpcUa.Host/OpcUa/OtOpcUaServerHostedService.cs — ctor params, field, creation at the end of StartAsync, dispose first in StopAsync and in DisposeAsync.
  4. src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/OtOpcUaNodeManager.cs — update the xmldoc at :316-328: it currently asserts "this is the only trigger". It becomes: deployment and the host's low-frequency timer, with the pass-local-memo constraint restated as the reason the second trigger is safe. (Theme 6: the stale summary lands in the same commit as the behaviour.)

Tests

Unit — tests/Server/ZB.MOM.WW.OtOpcUa.Host.Tests/OpcUa/AclRevalidationTimerTests.cs (new):

  • A_positive_interval_creates_a_timer_with_that_period — a RecordingTimeProvider overriding CreateTimer captures dueTime/period.
  • A_zero_interval_creates_no_timer — and logs the Information line naming the key.
  • A_throwing_pass_does_not_escape_the_tick — Tick() with a callback that throws returns normally and logs Error (a timer callback that throws kills the process).
  • Dispose_stops_further_ticks.

Unit — extend tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/NodeManagerStage6EnforcementTests.cs (the Revalidation_* family at :507-620 is the template; its FakeTimeProvider :796 and fake monitored item :845 are reused):

  • An_expired_key_stands_down_a_live_item_with_no_deploy — install a snapshot with a key expiring at T+10 min, track an item, RevalidateMonitoredItems() ⇒ still attached; advance the authorizer's FakeTimeProvider past ExpiresUtc; RevalidateMonitoredItems() again ⇒ item queued BadUserAccessDenied and removed from its MonitoredNode2 fan-out, with DataPlaneAuthorityState.Apply never called a second time. The first half is the falsification control: without it, a pass that detaches everything unconditionally passes.
  • A_renewed_key_re_attaches_the_item_on_the_next_pass — after the stand-down, install a snapshot whose key carries a later ExpiresUtc and re-run the pass ⇒ re-attached. Proves reversibility survives the clock axis (and pins the design decision not to close the session).
  • An_unexpired_key_is_untouched_by_a_clock_only_pass — advance the clock short of expiry ⇒ nothing stands down.

Wiring — extend the hosted-service tests:

  • the service, started with RevalidationIntervalSeconds = 30 and a recording TimeProvider, creates exactly one timer at 30 s;
  • with 0, none;
  • StopAsync disposes the timer before the authority/authorizer teardown (assert ordering via the recording provider + a probe on SetDataPlaneAuthorizer).

Over-the-wire — tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.IntegrationTests/AclKeyExpiryEnforcementTests.cs (new, on the AclSubscriptionEnforcementTests Rig shape): this is the one that cannot be reasoned into existence, for the same reason the existing file gives at :20-32 — whether a monitored item can recover the identity of the session that created it is an SDK assumption, and here it must recover an identity carrying a key id, which the anonymous rig has never exercised.

  • Extend the rig with a key-carrying connect: an ApiKeyOpcUaUserAuthenticator over a fake IApiKeyStore holding one key with a known ExpiresUtc, and a client UserIdentity of (apikey, otua_<id>_<secret>).
  • Subscribe to a tag the key's principal is granted; confirm values arrive.
  • Advance the rig's FakeTimeProvider (threaded into the DataPlaneAuthorizer at the rig's :336) past ExpiresUtc; invoke the revalidation tick.
  • Assert the client receives a BadUserAccessDenied data change and then nothing further, with no state.Apply call anywhere in the test — the "without a deploy" claim asserted at the wire, not at the seam.
  • Negative control in the same class: identical run without advancing the clock ⇒ values keep arriving.

Live gate — docker-dev (merge-blocking for the S19 slice)

⚠️ Recipe constraint discovered during verification, and it changes the obvious procedure. Setting ExpiresUtc in the ConfigDb on a running fleet does not affect an open session, because the per-operation re-check reads the artifact snapshot (see "What the per-operation re-check actually reads"). The expiry must therefore be in the artifact before the subscription is created. The /apikeys page only offers a date-only expiry at mint time, so the short expiry is set by SQL and then deployed.

  1. Bring the rig up with enforcement on — the flag is already parameterised (docker-compose.yml:585, :702):
    OTOPCUA_ACL_ENABLED=true docker compose -f docker-dev/docker-compose.yml up -d --build central-1 central-2
    
    (:9200 round-robins the two centrals — rebuild both, and assert image parity per container before trusting anything you see.)
  2. AdminUI http://localhost:9200/apikeys — mint a key (principal e.g. expiry-probe, any expiry). Copy the otua_… secret; it is shown once.
  3. AdminUI /clusters/{id}/acls — grant that principal Browse | Read | Subscribe on a device subtree carrying a live tag (use the scope picker; a Cluster-scoped row must have a NULL ScopeId or the deploy gate rejects it).
  4. Set the short expiry, then deploy so it reaches the artifact:
    sqlcmd -S 10.100.0.35,14330 -d OtOpcUaConfig -Q \
      "UPDATE ApiKey SET ExpiresUtc = DATEADD(minute, 4, GETUTCDATE()) WHERE Name = '<key name>'"
    
    Deploy from the AdminUI (or POST /api/deployments with X-Api-Key). Confirm the node logged the apply and the new generation.
  5. Subscribe with the key, and watch the wall clock:
    dotnet run --project src/Client/ZB.MOM.WW.OtOpcUa.Client.CLI -- subscribe \
      -u opc.tcp://localhost:4840 -n "ns=2;s=Plant/Modbus/dev1/Speed" -i 500 \
      -U apikey -P 'otua_<id>_<secret>'
    
  6. Expected, and this is the whole gate: values stream normally; then within RevalidationIntervalSeconds (≤ 60 s) of the ExpiresUtc passing, the CLI reports BadUserAccessDenied for the item and nothing further arrives — with no deployment having occurred between step 5 and step 6. The central's log carries the AclGate: RevalidateSubscriptions withheld N of M node(s) line (DataPlaneAuthorizer.LogFiltered, :155-162).
  7. Negative control (do not skip): repeat steps 2-5 with an expiry ~1 hour out and leave the subscription up for 5 minutes ⇒ values keep arriving and no withheld line appears. Without this, step 6 also passes on a build where the timer detaches everything it sees.
  8. S18 leg, same session: with the rig up, confirm otopcua.opcua.apikey_store.bounded{outcome=cache_hit} is non-zero after a handful of connects (proves the decorator is wired and the TTL is on), and that repeated connect calls do not produce one ApiKeys query each — check by pausing the SQL container briefly and confirming a connect denies within ~2 s rather than hanging the server (a second, already-open subscription on the same node must keep streaming throughout — the lock is what S18 is about).
  9. Record outcomes in archreview/plans/STATUS.md.

Effort + Risk

Effort: S code + M verification (the over-the-wire key-carrying rig extension and the live gate are the long poles). Risk: low-medium. The timer adds no decision logic and runs a pass that is already exercised on every deploy; it is inert unless Security:Acl:Enabled is on, and RevalidationIntervalSeconds = 0 restores today's behaviour exactly. The two real hazards are (a) a throwing tick killing the process — closed by the wrapped Tick() and its test — and (b) teardown ordering, closed by disposing the timer first and asserting the order.

Residuals (named, not fixed here)

  • R1 — event monitored items are revalidated by nothing. _liveItems tracks only ISampledDataChangeMonitoredItem (:3152-3155); an alarm/event subscription is gated at creation (:3092-3113) and never again, so it survives both revocation and expiry. Pre-existing and not a regression introduced here (the deploy trigger has the same hole), but it is the same class of gap S19 describes and deserves its own issue: it needs a second tracking map keyed on IEventMonitoredItem plus the SDK's own detach mechanics for event fan-out, which is materially more than a timer.
  • R2 — with Security:Acl:Enabled = false an expired key keeps its session's roles until it disconnects. There is no per-operation key check in that mode at all (AclEnforced gates every seam), so the roles resolved at activation persist. Closing it means gating on key usability independently of ACL enforcement — a behaviour change on the default configuration, which is a separate decision.
  • R3 — activation-time revocation latency is now bounded by StoreCacheSeconds rather than "next request". Deliberate and documented; recorded here so a future reader does not read the new docs/security.md row as a defect.

Task breakdown

Branch: one branch fix/archreview-r3-05-acl-hot-path-expiry off master. The two findings touch disjoint production files (S18: ApiKeyOptions, BoundedApiKeyStore, Program.cs, the two store docs; S19: AclOptions, AclRevalidationTimer, OtOpcUaServerHostedService, OtOpcUaNodeManager xmldoc) with exactly two shared touch points — Program.cs (the TimeProvider registration) and OtOpcUaTelemetry.cs — so they can run in parallel worktrees if T1 lands first. Ordering: T1 → (T2 → T3 → T4 → T5) ∥ (T6, T7 → T8 → T9) → T10 → T11 → T12.

Verification bar: every task has a deterministic unit guard; the two behaviour-changing tasks (T2/T3 on the auth path, T8 on server start/stop) additionally carry a falsification control, and the S19 slice carries the over-the-wire test (T10) and the live gate (T12). Follow TDD: write the failing test, see it FAIL, implement, see it PASS.


T1 — TimeProvider registration + new option knobs

Classification: standard (composition-root + two options types) Estimated implement time: 5 min Parallelizable with: nothing — every other task depends on it Files: src/Server/ZB.MOM.WW.OtOpcUa.Host/Program.cs, src/Core/ZB.MOM.WW.OtOpcUa.Core/Authorization/ApiKeyOptions.cs, src/Core/ZB.MOM.WW.OtOpcUa.Core/Authorization/AclOptions.cs

  1. Test first (ApiKeyStoreRegistrationTests.The_authorizer_and_the_key_store_share_one_TimeProvider): build the admin-node service collection, resolve TimeProvider and the DataPlaneAuthorizer, assert the authorizer was constructed with the registered instance (by reference, via a probe registration). Run → FAIL (TimeProvider unregistered).
  2. Implement: builder.Services.AddSingleton(TimeProvider.System); thread it into the DataPlaneAuthorizer factory (Program.cs:408-412); add ApiKeyOptions.StoreTimeoutMs (2000) / StoreCacheSeconds (5) and AclOptions.RevalidationIntervalSeconds (60), each with the "0 means…" XML doc.
  3. Re-run → PASS. Commit: feat(security): register TimeProvider + S18/S19 option knobs (archreview 03/S18,S19).

T2 — BoundedApiKeyStore deadline (fail-closed)

Classification: HIGH-RISK (authentication path; a wrong branch admits or locks out every client) Estimated implement time: 5 min Parallelizable with: T6–T9; after T1 Files: src/Server/ZB.MOM.WW.OtOpcUa.Host/OpcUa/BoundedApiKeyStore.cs (new), tests/Server/ZB.MOM.WW.OtOpcUa.Host.Tests/OpcUa/BoundedApiKeyStoreTests.cs (new)

  1. Tests: A_blocked_store_denies_without_hanging_the_activation (fake inner store gated on a TaskCompletionSource, StoreTimeoutMs=150, driven through the real ApiKeyOpcUaUserAuthenticator, assert DenyMessage within 5 s) + the positive control A_reachable_store_still_authenticates + A_deadline_never_admits. Run dotnet test tests/Server/ZB.MOM.WW.OtOpcUa.Host.Tests --filter "FullyQualifiedName~BoundedApiKeyStoreTests" → FAIL (type absent).
  2. Implement the deadline half only: linked CTS + CancelAfter(TimeSpan, TimeProvider), distinct Warning, re-throw.
  3. Filter → PASS. Commit: fix(security): bound the API-key store call under the SessionManager lock (archreview 03/S18).

T3 — TTL read-through cache + the falsification control

Classification: high-risk (changes activation-time revocation latency — the documented property) Estimated implement time: 5 min Parallelizable with: T6–T9; after T2 Files: same as T2

  1. Tests: A_second_activation_within_the_TTL_does_not_reach_the_store; A_revoked_key_is_denied_once_the_TTL_has_passed (fake clock advanced past StoreCacheSeconds ⇒ inner re-read ⇒ deny — the test that makes the documented bound a fact); TTL_zero_reads_through_every_time; A_faulted_store_is_not_cached; Anonymous_grants_are_bounded_and_cached_too; Cache_overflow_clears_wholesale_and_keeps_serving. Run filter → FAIL.
  2. Implement: per-method bounded dictionaries, timestamp from the injected clock, wholesale clear past 1024, never cache faults/timeouts.
  3. Filter → PASS. Commit: feat(security): short TTL read-through cache in front of the ConfigDb key store.

T4 — otopcua.opcua.apikey_store.bounded counter

Classification: standard Estimated implement time: 5 min Parallelizable with: T6–T9; after T3 (OtOpcUaTelemetry.cs is also touched by nothing else in this plan) Files: src/Core/ZB.MOM.WW.OtOpcUa.Commons/Observability/OtOpcUaTelemetry.cs, tests/.../BoundedApiKeyStoreTests.cs

  1. Tests (MeterListener filtered by a tag unique to the test, per the static-Meter-leak memory): a timeout increments outcome=timeout; a cache hit increments outcome=cache_hit; a plain read-through increments neither. Run filter → FAIL.
  2. Implement the counter + the two Add calls.
  3. Filter → PASS. Commit: feat(observability): meter bounded API-key store lookups.

T5 — Wire the decorator + registration guard

Classification: medium-risk (role-conditional wiring — the built-but-never-wired shape) Estimated implement time: 5 min Parallelizable with: T6–T9; after T4 Files: src/Server/ZB.MOM.WW.OtOpcUa.Host/Program.cs, src/Server/ZB.MOM.WW.OtOpcUa.Host/OpcUa/ApiKeyOpcUaUserAuthenticator.cs (doc), src/Server/ZB.MOM.WW.OtOpcUa.Host/OpcUa/ConfigDbApiKeyStore.cs (doc), tests/Server/ZB.MOM.WW.OtOpcUa.Host.Tests/OpcUa/ApiKeyStoreRegistrationTests.cs

  1. Tests: admin node ⇒ BoundedApiKeyStore wrapping ConfigDbApiKeyStore; driver-only ⇒ bare ArtifactApiKeyStore. Run → FAIL.
  2. Implement the wrap at Program.cs:425-427; correct the ApiKeyOpcUaUserAuthenticator "no network I/O at all" remark (:16-24) to qualify by node role; add the wrapped-in-production note to ConfigDbApiKeyStore.
  3. Filter → PASS; then the whole project: dotnet test tests/Server/ZB.MOM.WW.OtOpcUa.Host.Tests → PASS. Commit: fix(security): wrap the ConfigDb key store on admin nodes + registration guard.

T6 — AclRevalidationTimer (pure) + tests

Classification: standard (no SDK types, no decision logic) Estimated implement time: 5 min Parallelizable with: T2–T5; after T1 Files: src/Server/ZB.MOM.WW.OtOpcUa.Host/OpcUa/AclRevalidationTimer.cs (new), tests/Server/ZB.MOM.WW.OtOpcUa.Host.Tests/OpcUa/AclRevalidationTimerTests.cs (new)

  1. Tests: period captured by a RecordingTimeProvider.CreateTimer; zero interval ⇒ no timer + one Information line; A_throwing_pass_does_not_escape_the_tick; Dispose_stops_further_ticks. Run filter → FAIL.
  2. Implement.
  3. Filter → PASS. Commit: feat(security): AclRevalidationTimer — clock-driven monitored-item revalidation (archreview 03/S19).

T7 — Node-manager expiry tests + xmldoc correction

Classification: high-risk verification (proves the pass honours the clock; guards T8) Estimated implement time: 5 min per case Parallelizable with: T2–T5; after T1 Files: tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/NodeManagerStage6EnforcementTests.cs, src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/OtOpcUaNodeManager.cs (doc at :316-328 only)

  1. Tests (extend the Revalidation_* family): An_expired_key_stands_down_a_live_item_with_no_deploy (with its attached-before-expiry control), A_renewed_key_re_attaches_the_item_on_the_next_pass, An_unexpired_key_is_untouched_by_a_clock_only_pass. Run dotnet test tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests --filter "FullyQualifiedName~NodeManagerStage6EnforcementTests" → the expiry case may already PASS (the pass reads the clock today) — that is the point: it pins the property the timer will rely on before the timer exists. Any case that fails here is a real defect, not scaffolding.
  2. Update the :316-328 remark: deployment and the host timer; restate why the pass-local memo makes the second trigger safe.
  3. Full filter → PASS. Commit: test(opcua): pin clock-driven expiry in the revalidation pass; correct the only-trigger remark.

T8 — Wire the timer into OtOpcUaServerHostedService

Classification: medium-risk (server start/stop ordering; a tick on a torn-down node manager) Estimated implement time: 5 min Parallelizable with: T2–T5; after T6/T7 Files: src/Server/ZB.MOM.WW.OtOpcUa.Host/OpcUa/OtOpcUaServerHostedService.cs, its test suite

  1. Tests: interval 30 ⇒ one timer at 30 s; interval 0 ⇒ none; StopAsync disposes the timer before SetDataPlaneAuthorizer(null). Run → FAIL.
  2. Implement: ctor params (TimeProvider, IOptions<AclOptions>), creation at the end of StartAsync after the NodeManager null-guard (:139-143) and SetDataPlaneAuthorizer (:242), dispose-first in StopAsync (:280-297) and DisposeAsync.
  3. Filter → PASS; then the whole project. Commit: fix(host): run monitored-item revalidation on a low-frequency timer (archreview 03/S19).

T9 — Over-the-wire expiry test (key-carrying session)

Classification: high-risk verification — the deliberately-larger task Estimated implement time: ~15 min (rig extension: a key-carrying ConnectAsync + an authenticator over a fake store + a TimeProvider threaded into the rig's authorizer) Parallelizable with: nothing in the S19 slice; after T8 Files: tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.IntegrationTests/AclKeyExpiryEnforcementTests.cs (new, on the AclSubscriptionEnforcementTests rig shape)

  1. Build the rig extension, subscribe with an apikey UserName token, confirm values arrive, advance the rig's fake clock past ExpiresUtc, tick the revalidation, assert the client receives BadUserAccessDenied and then nothing — with no state.Apply in the test. Negative control in the same class: no clock advance ⇒ values keep arriving.
  2. dotnet test tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.IntegrationTests --filter "FullyQualifiedName~AclKeyExpiryEnforcementTests" → PASS, and FAIL under a control that reverts T8's timer (the deploy path alone cannot produce the denial).
  3. Commit: test(integration): key expiry stops a live subscription over the wire, with no deploy.

T10 — Docs: docs/security.md + CLAUDE.md revocation-latency correction

Classification: standard (docs) — but it carries the theme-6 correction, so it is not optional Estimated implement time: 5 min Parallelizable with: all Files: docs/security.md, CLAUDE.md

  1. docs/security.md:180-183 store table — rewrite the "Revocation takes effect" column: admin/fused = next session activation, bounded by Security:ApiKeys:StoreCacheSeconds; driver-only = next deployment; and a new row/note for already-open sessions: next deployment on BOTH node kinds for an Enabled revocation, immediate for ExpiresUtc.
  2. docs/security.md:405-425 bullets — amend "Revocation also reaches an open session" to distinguish the deployment axis from the clock axis, and extend "Revocation reaches a live subscription too" with the timer, Security:Acl:RevalidationIntervalSeconds, and the session is silenced, not closed semantics + re-activation recovery.
  3. New subsection documenting StoreTimeoutMs / StoreCacheSeconds (what a timeout looks like in the log, the counter to watch, and why the deadline denies).
  4. CLAUDE.md — correct the "⚠️ The consequence is asymmetric revocation: immediate on a ConfigDb node, next-deployment on a driver-only one" sentence to name the activation-vs-open-session distinction, and add one sentence to the ACL section that expiry now reaches live subscriptions on a timer.
  5. Commit: docs(security): correct revocation latency (activation vs open session) + S18/S19 knobs.

T11 — LIVE GATE: docker-dev expiry + bounded-store verification

Classification: HIGH-RISK verification gate (merge-blocking for the S19 slice) Estimated implement time: ~40 min rig session (step 4's expiry window is wall-clock) Parallelizable with: nothing (needs the built branch) Files: none (rig session; outcomes recorded in archreview/plans/STATUS.md)

Run the nine-step recipe in Finding 2 → Live gate: rig up with OTOPCUA_ACL_ENABLED=true (rebuild both centrals, assert image parity per container), mint + grant + SQL-set a 4-minute expiry, deploy, subscribe via Client.CLI with the otua_ credential, observe BadUserAccessDenied within one revalidation interval of expiry with no deployment in between, then the negative control (long expiry ⇒ stream continues), then the S18 leg (cache-hit counter non-zero; a paused SQL container denies a new connect within ~2 s while an existing subscription keeps streaming). Record every outcome, including anything that did not reproduce.

T12 — Residual issues filed

Classification: standard (tracker hygiene) Estimated implement time: 5 min Parallelizable with: all Files: none (Gitea)

File R1 (event monitored items revalidated by nothing) and R2 (expired key retains roles while Security:Acl:Enabled=false) as issues, each linking this plan and the anchor lines. R3 needs no issue — it is documented behaviour after T10.


Sequencing summary

T1                          (shared: TimeProvider + option knobs — everything blocks on it)
T2 → T3 → T4 → T5           (03/S18, sequential — same two files)
T6, T7 → T8 → T9            (03/S19; T6 and T7 are independent of each other)
T10 anytime after T5/T8; T11 last (after T5 + T9 merge-ready); T12 anytime

Overall effort: ~1 dev-day code+unit, plus the rig session. The two HIGH-RISK slices are T2/T3 (authentication path — every new branch must land on deny) and T11 (the gate that proves the clock axis actually reaches a real client). T9 is the largest single task and is the only one that can falsify T8 end to end.


Execution notes

Branch fix/r3-05-acl-hot-path-expiry, executed 2026-08-20 off master dd7f4954. TDD per task: failing test first, red observed and recorded, then green. T11 (live gate) deliberately left pending for the orchestrator.

T1 — TimeProvider registration + option knobs

  • RED observed: error CS1061: 'ServiceCollection' does not contain a definition for 'AddOtOpcUaDataPlaneAuth' and 'DataPlaneAuthorizer' does not contain a definition for 'Clock' — the seam and the assertion surface both absent, as predicted.
  • DEVIATION (structural, required by the plan's own test): the plan says the guard test "builds the service collection for an admin node". Program.cs is top-level statements in an executable, so no test can drive its wiring. The DataPlaneAuthorizer registration and the three-branch key-store / authenticator conditional were therefore moved verbatim into a new Host/Configuration/DataPlaneAuthRegistration.AddOtOpcUaDataPlaneAuth(hasAdmin, hasDriver) — the same shape LocalDbRegistration already uses — and Program.cs now calls it. No branch changed.
  • DEVIATION (small): DataPlaneAuthorizer gained a public Clock property. The two-clock hazard the class already documents can only be pinned by reference, and _time was private.
  • 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.

T3 — TTL read-through cache + the falsification control

  • RED observed: error CS0117: 'BoundedApiKeyStore' does not contain a definition for 'MaxCacheEntries' — the cache and its ceiling absent.
  • DEVIATION (fail-closed refinement, 2 lines): the plan keys FindAsync's cache on apiKeyId alone. Taken literally that lets the cache resurrect a key that expired by the clock for up to a TTL, which is exactly the axis 03/S19 exists to close. A cached hit is therefore re-checked against the caller's utcNow before it is served (IsExpired) — so the TTL bounds how stale an Enabled revocation may be, and nothing else. utcNow is still not part of the key (it changes every call and would defeat the cache entirely).
  • The counter's cache_hit / timeout Add calls are deliberately NOT here — they are T4, and writing them early would have made T4's red vacuous.
  • 11 tests in the class, green.

T4 — otopcua.opcua.apikey_store.bounded counter

  • RED observed (behavioural, not compile): A_timeout_meters_outcome_timeout and A_cache_hit_meters_outcome_cache_hit both [FAIL] with should be — the instrument existed nowhere, so the listener saw nothing. A_plain_read_through_meters_neither passed vacuously at red, by design: its job starts once the counter exists.
  • NOTE on the metrics-leak memory: the recorder filters on instrument name only. That is normally the leak (a parallel test's measurements land in it), and it is safe here for a checkable reason recorded on the helper: BoundedApiKeyStore is the only writer of this instrument, only this class constructs one, and xUnit never runs two tests of one class in parallel. Adding a second class that exercises the decorator means adding a discriminating tag.
  • 14 tests in the class, green.

T5 — wire the decorator + registration guard

  • RED observed: error CS1061: 'BoundedApiKeyStore' does not contain a definition for 'Inner' / … for 'Clock' — the guard could not see inside the wrapper because nothing was wrapping yet.
  • DEVIATION (small): BoundedApiKeyStore gained public Inner and Clock properties, for the same reason DataPlaneAuthorizer.Clock did in T1 — a guard that cannot see what is inside the wrapper cannot tell a correctly-wired node from one wrapping the wrong store.
  • Doc-drift half of the finding landed here: the authenticator's "no network I/O on this path at all" remark is now qualified by node role and points at the bound, and ConfigDbApiKeyStore records both that it is wrapped in production and that "revocation on the next request" is activation-scoped.
  • Host.Tests: 123 green (was 106 at base).

T6 — AclRevalidationTimer

  • RED observed: error CS0246: The type or namespace name 'AclRevalidationTimer' could not be found.
  • DEVIATION (visibility): the plan declares the class internal sealed. ZB.MOM.WW.OtOpcUa.Host has no InternalsVisibleTo, so the tests could not reach it. Made public sealed, matching BoundedApiKeyStore and every other type in Host/OpcUa/.
  • Added beyond the plan's list: A_tick_runs_the_pass — the control for the two "nothing happened" cases (a Tick() that silently did nothing would satisfy the throwing-pass and dispose cases on its own).
  • 5 tests, green. Also corrected the T4 MeterRecorder remark: the assembly declares CollectionBehavior(DisableTestParallelization = true), which is a stronger and simpler reason the name-only filter is safe than the one first written.

T7 — node-manager expiry tests + xmldoc correction

  • RED: the three new cases PASSED on first run — as the plan predicted ("the expiry case may already PASS … that is the point: it pins the property the timer will rely on before the timer exists"). The pass already reads the authorizer's TimeProvider, so no production change was needed.
  • Falsification run (recorded, since "it passed immediately" proves nothing on its own): shortening the clock advance in An_expired_key_stands_down_a_live_item_with_no_deploy from 20 min to 5 min (i.e. short of ExpiresUtc) turned it [FAIL], then restored. So the assertion tracks the clock rather than a blanket deny.
  • A_renewed_key_re_attaches_the_item_on_the_next_pass observes the restore indirectly and falsifiably: the pass skips an already-suppressed item, so standing the item down a second time can only succeed if it was genuinely re-attached in between. (NodeHandle.MonitoredNode is null on a hand-tracked stub item, so attach/detach is not directly observable at this seam — it is proven over the wire in T9.)
  • Two stale remarks corrected, not one: OnAuthorityChanged's "this is the only trigger" and IsSubscriptionStillPermitted's "a subscription is re-checked at a deployment, never on a timer".
  • OpcUaServer.Tests: 493 passed / 1 skipped.

T8 — wire the timer into OtOpcUaServerHostedService

  • RED observed: error CS1061: 'OtOpcUaServerHostedService' does not contain a definition for 'StartRevalidationTimer' and error CS1729: … does not contain a constructor that takes 13 arguments (the two new ctor parameters).
  • DEVIATION (test seam, and the reason for it): the plan says "extend the hosted-service tests". There are none — grep -rln OtOpcUaServerHostedService tests/ matched nothing, because StartAsync boots the real OPC UA SDK (PKI store, listening socket). A new suite, OtOpcUaServerHostedServiceRevalidationTests, drives the timer's lifecycle through an internal StartRevalidationTimer() (new InternalsVisibleTo on the Host csproj, the convention six driver projects already use). Everything the timer's lifecycle touches — the options, the clock, the disposal — is exercised exactly as production reaches it; only the SDK boot is skipped.
  • DEVIATION (assertion not made): the plan asks the test to assert disposal happens before SetDataPlaneAuthorizer(null). That is not observable: both detaches go through _server?.…, and OtOpcUaSdkServer.NodeManager is created only by the SDK's own CreateMasterNodeManager during a real boot, so with no server both calls are no-ops. The ordering is instead guaranteed structurally — the dispose is the first statement of StopAsync, with a comment saying why — and what IS asserted is that StopAsync (and DisposeAsync) dispose the timer, with a non-vacuity check that it was live beforehand. Reviewer: this is the one plan assertion downgraded rather than met.
  • The node manager is resolved per tick (() => _server?.NodeManager?.Revalidate…) rather than captured, so a restart cannot leave the tick walking a dead manager's item map.
  • Host.Tests: 132 green.

T9 — over-the-wire expiry test (key-carrying session)

  • RED observed (build): error CS1503: Argument 2: cannot convert from 'string' to 'byte[]' — the 1.5.378 UserNameIdentityToken.DecryptedPassword is raw bytes, not a string (the host already decodes UTF-8; the token had to be built the same way).
  • FALSIFICATION (the plan's step 2, run and recorded): deleting the single rig.TickRevalidation() call from An_expired_key_stops_a_live_subscription_with_no_deploy makes it [FAIL] — the expired key goes on streaming. So the denial is produced by the T8 tick and by nothing else. Restored, both cases green.
  • DEVIATION (fake choice): the plan calls for "an ApiKeyOpcUaUserAuthenticator over a fake IApiKeyStore". The rig instead uses a 6-line KeyPresentingAuthenticator returning OpcUaUserAuthResult.AllowKey(principal, [], keyId). The credential-verification path (parse, peppered HMAC, store lookup, deny shapes) is already covered in the Host suite and changes nothing about what this file asks — whether the SDK hands a monitored item back an identity that still carries the key id. Grants come from the deployed snapshot, so the authenticator returns no roles.
  • The tick is driven through the real AclRevalidationTimer, which required a new ProjectReference from the integration test project to ZB.MOM.WW.OtOpcUa.Host (the timer lives there). Without it the test would have called the node manager's pass directly and could not have falsified T8.
  • OpcUaServer.IntegrationTests: 20 green (was 18).

T10 — docs

  • docs/security.md store table: the "Revocation takes effect" column is now two columns — new activation vs already-open session — with an explicit ⚠️ paragraph saying the old single column was wrong and why (an open session re-checks the artifact snapshot, never IApiKeyStore), and that expiry is the exception on both node kinds.
  • docs/security.md new subsection "Bounding the key store" — the lock hazard, both knobs, what a timeout looks like in the log, the counter to watch, why a timeout denies, and why faults are never cached.
  • docs/security.md revocation bullets: split into the deployment axis and the clock axis; the live-subscription bullet now names two triggers, the timer's key and default, the silenced-not-closed semantics with re-activation recovery, and the event-monitored-item residual.
  • CLAUDE.md: the "asymmetric revocation" sentence corrected to activation-scoped (naming the old wording as wrong), a new bullet for the S18 bound, and the stage-6 "Revocation reaches live subscriptions" bullet extended with the second trigger and the pass-local-memo constraint.
  • Beyond the plan's file list: docs/Configuration.md gained rows for all three new keys. It carries the field-by-field tables for Security:ApiKeys and Security:Acl, so shipping the knobs without them would have been the exact doc drift this plan's cross-cutting requirement is about.

T12 — residuals filed

  • R1 → Gitea #562 — ACL: event/alarm monitored items are revalidated by neither trigger (https://gitea.dohertylan.com/dohertj2/lmxopcua/issues/562).
  • R2 → Gitea #563 — ACL: an expired API key keeps its session's roles while Security:Acl:Enabled is false (https://gitea.dohertylan.com/dohertj2/lmxopcua/issues/563).
  • R3 (activation-time revocation now bounded by StoreCacheSeconds) needs no issue — it is documented behaviour after T10, in both docs/security.md and docs/Configuration.md.

T11 — LIVE GATE

Left pending for the orchestrator, per the execution brief. Nothing in this branch has been run against the docker-dev rig; the nine-step recipe in Finding 2 → Live gate is unchanged and still applies. Two notes for whoever runs it:

  • Security:Acl:RevalidationIntervalSeconds defaults to 60, so step 6's "within one revalidation interval" is ≤ 60 s unless the rig overrides it. Setting it to 15 shortens the wall-clock wait without changing what is proven.
  • Step 8's cache-hit leg reads otopcua.opcua.apikey_store.bounded{outcome=cache_hit}; the instrument only exists on an admin/fused node, since the artifact store is not wrapped.

Summary of what a reviewer should look at

  1. Program.cs → DataPlaneAuthRegistration (T1): a verbatim move of the authorizer + key-store branches out of top-level statements so they could be guarded. Confirm no branch changed.
  2. OtOpcUaServerHostedServiceRevalidationTests (T8): the plan's dispose-before-SetDataPlaneAuthorizer(null) ordering is guaranteed structurally but not asserted — see T8's notes for why it is unobservable.
  3. BoundedApiKeyStore.IsExpired (T3): a two-line fail-closed addition beyond the plan, so the TTL cannot resurrect a clock-expired key.
  4. New public members added purely so wiring could be pinned by reference: DataPlaneAuthorizer.Clock, BoundedApiKeyStore.Inner / .Clock.

Review outcome — FIX-FIRST, applied 2026-08-20

Coordinator verdict on the branch was FIX-FIRST with four items. All four applied; Host.Tests and OpcUaServer.Tests rerun green.

  1. [Important] IsExpired had no test — the review was right, and the gap was mine. T3's execution note called the guard a "fail-closed refinement" and shipped it with no case that fails without it: deleting && !IsExpired(cached, utcNow) left all 14 BoundedApiKeyStoreTests green. That is the unchanged-observable trap — the guard's whole effect is invisible unless a cached entry outlives its key's ExpiresUtc inside the TTL, which no existing case constructed. Added A_cached_key_that_expires_by_the_clock_is_not_served: key expiring at start + 2 s, TTL 60 s, first FindAsync non-null (and now cached); flip the inner store to Revoked; advance the fake clock 5 s — past ExpiresUtc, far inside the TTL; assert null and inner.Calls == 2. The Revoked flip is deliberate: it makes "served from cache" and "re-read" differ in the answer, not only in a call count, so a build that re-read and ignored the result cannot pass either. RED confirmed by falsification: with the guard removed, exactly 1 failed / 14 passed — only the new case — then restored, 15/15 green.
  2. [Minor] cache_hit was metered before the expiry guard could reject. TryServe counted a hit for an entry that then fell through to a store read, so the counter over-reported. The Add moved to the two call sites, after the guard, behind a MeterCacheHit() helper; TryServe is now purely a lookup. This matters operationally — cache_hit is the ratio §"Bounding the key store" tells an operator to watch.
  3. [Minor] Added A_node_with_neither_role_registers_the_deny_all_authenticator — asserts the IOpcUaUserAuthenticator singleton instance is NullOpcUaUserAuthenticator.Instance and that no IApiKeyStore is registered. It closes the third branch of the role conditional, which the guard suite had left unpinned even though that branch is the one whose absence stops a node booting.
  4. [Minor] StartRevalidationTimer now disposes before overwriting. StartAsync can run again when a host restarts the service around the same instance, and the overwritten field would have left the previous timer armed with nothing left to dispose it — the dispose-before-overwrite rule the driver reconnect paths already carry.

Verification after the fixes: solution build 0 errors · Host.Tests 134 passed / 0 failed (was 132) · OpcUaServer.Tests 493 passed / 1 skipped. T11 remains pending for the orchestrator.


Live gate result (2026-08-20) — S19 PASS, S18 FAIL on docker-dev

Rig: docker-dev/docker-compose.yml on OrbStack (Mac), image otopcua-host:dev = b344565cad3d (master e280db37) on all six containers (parity asserted via docker inspect --format '{{.Image}}'), AdminUI on Security__Auth__DisableLogin=true. Deploys via ./docker-dev/deploy.sh (POST /api/deployments, X-Api-Key). Subscriptions via Client.CLI against opc.tcp://localhost:4840 (central-1, fused admin+driver — the only node kind that carries BoundedApiKeyStore). The node under test was the Modbus fixture's auto-increment register, raw tag MAIN-modbus-hr100 → ns=2;s=pymodbus/plc/HR100 (added for R3-03's gate; it changes every second, so "still streaming" is observable).

Recipe correction — the plan's step 1 does not do what it says. OTOPCUA_ACL_ENABLED in docker-dev/docker-compose.yml is wired only to site-a-1 / site-a-2 (:605, :722), not to the centrals — OTOPCUA_ACL_ENABLED=true docker compose up -d central-1 central-2 is a no-op ("Running", no recreate, no env). The gate used a scratch compose override (services.central-1/central-2.environment.Security__Acl__Enabled: "true") passed as a second -f, verified in-container (Security__Acl__Enabled=true ×2), and dropped at restore. Worth fixing in the compose (or the plan) before the next ACL gate.

Setup (steps 2-4)

  • /apikeys: minted name/principal expiry-probe, key id c81b4e50705281a1 (secret captured from the one-time banner, not recorded here).
  • Grant by SQL (the scope picker flow is stage-7-gated and not what this gate tests): NodeAcl('acl-expiry-probe-hr100', 'MAIN', 'expiry-probe', ScopeKind='Tag', ScopeId='MAIN-modbus-hr100', PermissionFlags=7) = Browse|Read|Subscribe on that one tag.
  • Enforcement confirmed live before anything else: an anonymous read of HR100 → ServiceResultException [801F0000] (BadUserAccessDenied) with AclGate: Read withheld 1 of 1 node(s) for principal anonymous on central-1 (×3 probes); a keyed read/subscribe → values. Both AclRevalidationTimer: live monitored items will be re-authorized every 00:01:00 lines present (default interval, deliberately not shortened).

Step 7 first — negative control (1 h expiry): PASS

ExpiresUtc = 18:03:25Z set by SQL, deployed (6df8f967…; central-1 data-plane authority generation 2 installed (… keys=1, grants=1, resolvedScopes=1) at 17:03:26). Keyed subscription from 17:03:45Z to 17:09:02Z (5 min 17 s): 279 Good notifications, 0 non-Good, and zero RevalidateSubscriptions withheld lines on either central across the window. (The stream has one 57 s gap inside it — caused by the S18 leg below, not by the timer.)

Steps 5-6 — the timer leg (3 min expiry, no deploy in between): PASS

  • UPDATE ApiKey SET ExpiresUtc = DATEADD(minute, 3, GETUTCDATE()) → ExpiresUtc = 17:12:20.987Z (GETUTCDATE() read back 17:09:20.990Z). Deployed f35664e5… at 17:09:21; central-1 data-plane authority generation 3 installed (… keys=1, grants=1 …) at 17:09:21. No later deployment exists (SELECT TOP 1 CreatedAtUtc FROM Deployment = 17:09:21.072 at the end of the leg).
  • New keyed subscription from 17:09:26Z: Good values at 1/s (40623 → 40970).
  • Last Good: 40970 @ 17:12:20.1518Z. Next and final notification: ns=2;s=pymodbus/plc/HR100 = (0x801F0000) @ 17:12:21.0729Z = BadUserAccessDenied, 0.09 s after ExpiresUtc, then nothing further (checked at 17:13:37). central-1: [17:12:21 INF] AclGate: RevalidateSubscriptions withheld 2 of 2 node(s) for principal (none) (the second item is the still-open session from the negative control, whose CLI process had been killed but whose OPC UA session had not yet timed out). The timer tick (armed 17:02:21, 60 s period → 17:12:21) happened to fall within a second of the expiry; the bound being proven is "≤ one RevalidationIntervalSeconds", and the observation is inside it.

Step 8 — S18 bounded store: cache-hit half PASS, pause half FAIL

  • Cache-hit counter (PASS): after one keyed subscribe activation and three keyed read connects within ~2 s, central-1's /metrics (port 9000 in-container) read otopcua_opcua_apikey_store_bounded_total{outcome="cache_hit"} 2 — first read a miss (the subscribe activation was

    StoreCacheSeconds=5 earlier), reads 2 and 3 hits. The decorator is wired and the TTL is on. The instrument is exported via the Host's Prometheus endpoint; http://localhost:9200/metrics round-robins through Traefik so it must be read on the specific central.

  • Paused-SQL activation (FAIL — both sub-assertions): with the negative-control subscription streaming, docker pause otopcua-dev-sql-1 at 17:04:49, then (3 s later, past the 5 s TTL) a keyed read connect.
    • The connect did not deny in ~2 s: the CLI ran 42.9 s and failed client-side with [80860000] (BadRequestTimeout). The server's denial line [17:05:35 WRN] ApiKeyStore: the key store did not answer within 2000 ms — denying this activation … was logged at 17:05:35 — the instant the container was unpaused, not at 17:04:55. The stack under it is ConfigDbApiKeyStore.FindAsync → BoundedApiKeyStore.BoundAsync line 226 (return await call(cts.Token)), and a neighbouring Failed executing DbCommand (35,009ms) shows SqlClient sat on the black-holed socket until its own internal timeout. StoreTimeoutMs is a cooperative deadline — it is observed only when the inner call chooses to observe the token, and Microsoft.Data.SqlClient does not while a stalled connection/login is in flight. outcome="timeout" did increment to 1, but 40 s late.
    • The already-open subscription on the same node stalled behind it: HR100 notifications stop at 17:04:54.107Z and resume at 17:05:49.105Z (57 s, i.e. for the whole time the activation was wedged plus ~14 s of SqlClient recovery). This is precisely the SessionManager-lock serialization S18 describes.
    • Falsification control (run immediately after, required to attribute the stall): docker pause the SQL container again for 40 s (17:06:21 → 17:07:01) with no new activation ⇒ the same subscription streamed through it without a single missed second (17:06:00 … 17:07:21, 1/s throughout). So the stall is not "SQL paused hurts the data plane" in general — it is the activation's store lookup holding the lock past its advertised bound.
  • Consequence: on a fused/admin node, a ConfigDb that goes unresponsive (as opposed to refusing connections) still wedges every session activation and every open session's publish for tens of seconds per attempt. The bound needs to be a hard race (Task.WhenAny against the deadline, abandoning the in-flight SqlClient call and letting it die on its own), or the lookup must be moved off the lock — the T3 TTL cache helps only for keys seen in the last 5 s. Filed as a follow-up under the S18 slice; not fixed in this gate.

Restore

Gate rows deleted (NodeAcl row, the expiry-probe key, nothing else had been added for this gate), centrals recreated without the override (Security__Acl__Enabled absent from both env lists, i.e. default false), parity b344565cad3d ×6 re-asserted, readiness back, final deploy 1c666be3… Sealed, anonymous read of HR100 answers a value again. Rig left UP on the master image with DisableLogin=true.

S18 hard-deadline follow-up (2026-08-20)

The gate's S18 finding is fixed on fix/r3-05-s18-hard-deadline (branched off master 9242630c), plus the compose recipe defect the same leg exposed. Not yet live re-gated — the paused-SQL leg has to be re-run.

What was wrong

BoundedApiKeyStore.BoundAsync bounded the inner store with a CancellationTokenSource(TimeSpan, TimeProvider) linked into the call. That bounds the call only if the callee chooses to observe the token, and Microsoft.Data.SqlClient does not while a connect or login is in flight on a black-holed socket. So the deadline never returned control: the gate's keyed activation ran 42.9 s against a 2000 ms bound, the "did not answer within 2000 ms" Warning was emitted only at unpause, and because the whole thing sits inside the SDK's synchronous SessionManager-lock-holding impersonation callback, an unrelated already-open subscription stalled 57 s behind it. The T3 TTL cache cannot help: a miss is exactly what the outage produces.

Red before green — the observation, not just the claim

Every pre-existing deadline test drives GatedApiKeyStore, whose waits take the token (Task.Delay(Timeout.Infinite, ct), gate.WaitAsync(ct)). A cooperative CancelAfter unblocks those, so all 15 of them passed against the defective build — the suite could not see the gate's failure at all. The new fake, UncooperativeApiKeyStore, ignores ct entirely and completes only when the test releases it, modelling SqlClient. Against the unmodified BoundedApiKeyStore the three new tests fail — with a test-side WaitAsync(2 s), so the red is a failure, not a hang:

Failed! - Failed: 3, Passed: 15, Skipped: 0, Total: 18
  An_uncancellable_store_is_abandoned_at_the_deadline                [FAIL]
  An_abandoned_lookup_that_completes_later_is_not_cached             [FAIL]
  An_abandoned_lookup_that_faults_later_is_observed_and_swallowed    [FAIL]
Shouldly.ShouldAssertException : … should throw System.OperationCanceledException but threw System.TimeoutException

TimeoutException rather than OperationCanceledException is the finding, restated as an assertion: with a 200 ms bound the decorator had still not returned 2 s later.

The fix

BoundAsync now races the inner call against Task.Delay(timeout, _time, …) — the injected clock, so the deadline stays on the same TimeProvider as the authorizer and the revalidation timer — with Task.WhenAny:

  • Inner wins ⇒ stop the delay timer, dispose, await inner (faults and a genuine caller cancellation propagate exactly as before).
  • Delay wins ⇒ cancel the linked token anyway (a cooperative inner store should still stop), hand the abandoned task to ObserveAbandoned, meter outcome=timeout, log the (now honest) Warning, and throw OperationCanceledException — the same exception the authenticator's catch-all already maps to the standard opaque denial. Fail-closed, unchanged: nothing here can admit.
  • ObserveAbandoned attaches a continuation that disposes the CTS, touches finished.Exception (the observation itself, so a late SqlClient fault can never become an UnobservedTaskException), and logs at Debug. Debug rather than Warning because the timeout is already reported at Warning — a ConfigDb outage should not produce two lines per activation for one event.
  • An abandoned answer is never cached. It was taken before the deadline and would be served after it, so a key revoked or expired in the meantime could be admitted from a lookup nobody was still waiting for. Store(...) is simply unreachable on the timeout path; the test asserts it by call count (a cached late answer would make the next lookup a hit and leave Calls at 1).
  • StoreTimeoutMs ≤ 0 still short-circuits to the unbounded call, byte-for-byte — the documented rollback.

Files: src/Server/ZB.MOM.WW.OtOpcUa.Host/OpcUa/BoundedApiKeyStore.cs, tests/Server/ZB.MOM.WW.OtOpcUa.Host.Tests/OpcUa/BoundedApiKeyStoreTests.cs, docs/security.md §"Bounding the key store". Host.Tests 137/137 green (was 134; +3).

Recipe defect fixed: OTOPCUA_ACL_ENABLED never reached the centrals

The gate's own correction note, actioned. Security__Acl__Enabled: "${OTOPCUA_ACL_ENABLED:-false}" was wired to site-a-1 / site-a-2 only, so OTOPCUA_ACL_ENABLED=true docker compose up -d central-1 central-2 was a silent no-op — on the one node kind that carries BoundedApiKeyStore at all (only a fused admin+driver node wraps ConfigDbApiKeyStore). The key is now on central-1 and central-2 too, same env var, same default-false, so today's rig behaviour is unchanged and the plan's step 1 does what it says. site-b stays hardcoded OFF as the A/B control.

Live re-gate — still owed

Re-run step 8's paused-SQL leg only (setup steps 2-4 unchanged, and step 1 now needs no scratch override). Expected, versus what the gate saw:

Observation Gate (defective) Expected now
Keyed read connect with SQL paused 42.9 s, client BadRequestTimeout denies in ≈ StoreTimeoutMs (2 s)
"did not answer within 2000 ms" Warning at unpause, ~40 s late at the deadline, while SQL is still paused
Already-open subscription on the same node 57 s gap no gap beyond the ~2 s of the activation
apikey_store_bounded{outcome="timeout"} 1, ~40 s late 1, promptly
Falsification control (pause with no activation) streamed clean must still stream clean

⚠️ The Debug abandonment line is not visible at the rig's default log level — it is not part of the gate's pass criteria; the Warning and the restored subscription are.