dohertj2/lmxopcua
1
0
Fork 0
Code Issues 32 Pull Requests Actions Packages Projects Releases Wiki Activity

docs(archreview): round-3 plan R3-05 (ACL hot-path deadline + clock-driven key expiry); index task count

Browse Source
This commit is contained in:
Joseph Doherty
2026-08-20 09:28:53 -04:00
parent 5fd65d190e
commit dd7f495432
3 changed files with 682 additions and 1 deletions
Download Patch File Download Diff File Expand all files Collapse all files
archreview/plans/00-INDEX.md
+1 -1
View File
@@ -24,7 +24,7 @@ co-located `.tasks.json`.
| 2 | [R3-02 Stale-Good family](R3-02-stale-good-family-plan.md) | 03/S15, 05/STAB-19, 05/STAB-18, 02/S14 | TBD | **fable** | opus | opus |
| 3 | [R3-03 Restart re-subscribe + watchdog `max()`](R3-03-restart-resubscribe-watchdog-plan.md) | 03/S16, S17, U6 | 9 | opus | opus (upgraded from sonnet: the plan redesigns the spawn path + adds a supervision-restart hook) | opus |
| 4 | [R3-04 One role authority](R3-04-one-role-authority-plan.md) | 01/S-12 = 03/C5, 01/C-9 | 13 | opus | sonnet | opus |
| 5 | [R3-05 ACL hot path + key expiry](R3-05-acl-hot-path-expiry-plan.md) | 03/S18, S19 | TBD | opus | opus | opus |
| 5 | [R3-05 ACL hot path + key expiry](R3-05-acl-hot-path-expiry-plan.md) | 03/S18, S19 | 12 | opus | opus | opus |
**Model rationale.** Plan-writing is where design judgment lives, so every plan is opus or better; R3-02
spans four seams with one shared contract and a known vacuity trap (value-dedup), so it gets fable. Execution
archreview/plans/R3-05-acl-hot-path-expiry-plan.md
+580
View File
@@ -0,0 +1,580 @@
# 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.
```csharp
// 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`:
```csharp
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`):**
```csharp
/// <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:**
```csharp
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:
```csharp
// 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`):
```csharp
// 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`):
```bash
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:
```bash
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:
```bash
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.
archreview/plans/R3-05-acl-hot-path-expiry-plan.md.tasks.json
+101
View File
@@ -0,0 +1,101 @@
{
"planPath": "archreview/plans/R3-05-acl-hot-path-expiry-plan.md",
"lastUpdated": "2026-08-20",
"tasks": [
{
"id": "T1",
"subject": "Register TimeProvider in DI (shared by authorizer, key store and revalidation timer) + ApiKeyOptions.StoreTimeoutMs/StoreCacheSeconds + AclOptions.RevalidationIntervalSeconds (03/S18, 03/S19)",
"status": "pending",
"blockedBy": []
},
{
"id": "T2",
"subject": "BoundedApiKeyStore: hard per-call deadline on the key store under the SessionManager lock, fail-closed (03/S18)",
"status": "pending",
"blockedBy": [
"T1"
]
},
{
"id": "T3",
"subject": "BoundedApiKeyStore: short TTL read-through cache + falsification control (a revoked key is denied once the TTL has passed)",
"status": "pending",
"blockedBy": [
"T2"
]
},
{
"id": "T4",
"subject": "otopcua.opcua.apikey_store.bounded counter (outcome=timeout|cache_hit) + MeterListener assertions",
"status": "pending",
"blockedBy": [
"T3"
]
},
{
"id": "T5",
"subject": "Wire BoundedApiKeyStore on admin/fused nodes in Program.cs + registration guard test; correct the authenticator's 'no network I/O at all' remark",
"status": "pending",
"blockedBy": [
"T4"
]
},
{
"id": "T6",
"subject": "AclRevalidationTimer (pure, TimeProvider-driven, non-throwing tick) + unit tests (03/S19)",
"status": "pending",
"blockedBy": [
"T1"
]
},
{
"id": "T7",
"subject": "Node-manager expiry tests (FakeTimeProvider advanced past ExpiresUtc, no deploy) + correct the ':316-328' only-trigger xmldoc",
"status": "pending",
"blockedBy": [
"T1"
]
},
{
"id": "T8",
"subject": "Wire the revalidation timer into OtOpcUaServerHostedService (create after StartAsync, dispose FIRST in StopAsync) + ordering test",
"status": "pending",
"blockedBy": [
"T6",
"T7"
]
},
{
"id": "T9",
"subject": "Over-the-wire AclKeyExpiryEnforcementTests: key-carrying session, clock advanced past expiry, item goes BadUserAccessDenied with no deploy (+ negative control)",
"status": "pending",
"blockedBy": [
"T8"
]
},
{
"id": "T10",
"subject": "Docs: docs/security.md store table + revocation bullets, and the CLAUDE.md 'asymmetric revocation' sentence (activation vs already-open session)",
"status": "pending",
"blockedBy": [
"T5",
"T8"
]
},
{
"id": "T11",
"subject": "LIVE GATE: docker-dev with OTOPCUA_ACL_ENABLED=true — subscription goes BadUserAccessDenied within one interval of ExpiresUtc with no deploy (+ long-expiry negative control, + bounded-store leg)",
"status": "pending",
"blockedBy": [
"T5",
"T9"
]
},
{
"id": "T12",
"subject": "File residuals R1 (event monitored items revalidated by nothing) and R2 (expired key retains roles while Security:Acl:Enabled=false)",
"status": "pending",
"blockedBy": []
}
]
}