1044 lines
100 KiB
Markdown
1044 lines
100 KiB
Markdown
# 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.
|
||
|
||
---
|
||
|
||
## 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 | **Re-gate (2026-08-20, image `97cb5cefb6d2` = master `b9b3cf9a`)** |
|
||
|---|---|---|---|
|
||
| Keyed `read` connect with SQL paused | 42.9 s, client `BadRequestTimeout` | denies in ≈ `StoreTimeoutMs` (2 s) | **2.42 s wall clock**, client `ServiceResultException [80210000]` (`BadIdentityTokenRejected`) — **PASS** |
|
||
| *"did not answer within 2000 ms"* Warning | at unpause, ~40 s late | at the deadline, while SQL is still paused | `[18:11:54 WRN]` — connect started 18:11:52.1, SQL unpaused 18:11:59.7 — **PASS** |
|
||
| Already-open subscription on the same node | 57 s gap | no gap beyond the ~2 s of the activation | **max inter-notification gap 1.00 s** across the whole 18:11:33 → 18:13:xx window (98 Good notifications, 1/s; no gap > 1.6 s) — **PASS** |
|
||
| `apikey_store_bounded{outcome="timeout"}` | 1, ~40 s late | 1, promptly | `outcome="timeout"} 1` (+ `cache_hit` 5) read at 18:13:12 — **PASS** |
|
||
| Falsification control (pause with **no** activation) | streamed clean | must still stream clean | 40 s pause `18:12:19 → 18:12:59`, same subscription streamed 1/s throughout — **PASS** |
|
||
|
||
⚠️ 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.
|
||
|
||
## Live gate result (2026-08-20, S18 re-gate) — **PASS**; T11 now fully green
|
||
|
||
Rig rebuilt from master `b9b3cf9a` (contains `f665e8b8`, the hard-deadline fix, and the compose `OTOPCUA_ACL_ENABLED`
|
||
wiring for the centrals): `otopcua-host:dev` = `97cb5cefb6d2`, parity asserted ×6 via `docker inspect --format '{{.Image}}'`.
|
||
`OTOPCUA_ACL_ENABLED=true docker compose -f docker-dev/docker-compose.yml up -d central-1 central-2` now **does** what the plan's
|
||
step 1 says — `Security__Acl__Enabled=true` read back in both containers, no scratch override needed — and enforcement was live
|
||
before the leg ran (anonymous `browse` → `[801F0000]`).
|
||
|
||
Setup: key `s18-regate` (id `a1b2c3d4e5f60718`, secret hashed with the rig's committed pepper and inserted by SQL) +
|
||
`NodeAcl('acl-s18-cluster','MAIN','s18-regate',ScopeKind='Cluster',ScopeId=NULL,PermissionFlags=7)`, deployed (`3cb5415c…`,
|
||
Sealed). Subscription under test: `ns=2;s=pymodbus/plc/HR100` (the fixture's auto-increment register) at `-i 500`, keyed, on
|
||
central-1 (`opc.tcp://localhost:4840`) — the only node kind that carries `BoundedApiKeyStore`.
|
||
|
||
Timeline (UTC):
|
||
|
||
- `18:11:45.1` `docker pause otopcua-dev-sql-1` (the rig's SQL is local on OrbStack — not the shared host).
|
||
- `18:11:52.1` new keyed `read` connect (7 s after the pause, past the 5 s `StoreCacheSeconds` TTL, so a guaranteed miss).
|
||
- `18:11:54.5` client returns `ServiceResultException [80210000]` — **2.42 s** end to end (the 2000 ms bound plus the
|
||
OPC UA handshake). central-1 at the same second:
|
||
`[18:11:54 WRN] ApiKeyStore: the key store did not answer within 2000 ms — denying this activation. The lookup is abandoned and
|
||
its result, if it ever arrives, is discarded. …` followed by `[18:11:54 ERR] ApiKeyAuth: key store threw for key id a1b2c3d4e5f60718
|
||
System.OperationCanceledException: The API key store did not answer within 2000 ms; the lookup was abandoned.` (stack through
|
||
`BoundedApiKeyStore.BoundAsync` line 278 → `FindAsync` line 120) and `UserName auth denied for apikey: Invalid API key.` — the
|
||
standard opaque denial, fail-closed.
|
||
- `18:11:59.7` `docker unpause`. The open subscription's notifications: `… 18:11:52.7, 18:11:53.7, 18:11:54.7, 18:11:55.7 …` —
|
||
**no missed second** through the activation, the pause, or the unpause (first gate: a 57 s hole starting at the activation).
|
||
- After unpause: **zero** `Unobserved` lines on central-1. The abandoned SqlClient call surfaced only as the expected ConfigDb
|
||
health-check noise (`Health check configdb … 'Database connection failed.'`, `Connection Timeout Expired … pre-login handshake`)
|
||
and a `batch is aborted` inner exception attached to the *already-logged* activation error — nothing new, nothing unobserved. The
|
||
Debug abandonment line is below the rig's log level, as the follow-up note predicted.
|
||
- `18:12:19 → 18:12:59` falsification control: SQL paused 40 s with **no** activation; the subscription streamed 1/s throughout
|
||
(max gap over the whole session 1.00 s) — so a paused SQL still does not hurt the data plane by itself, and the first gate's 57 s
|
||
stall is attributable to the activation alone, now bounded.
|
||
- `18:13:12` `http://central-1:9000/metrics`: `otopcua_opcua_apikey_store_bounded_total{outcome="timeout"} 1`,
|
||
`{outcome="cache_hit"} 5`.
|
||
|
||
Restore: `NodeAcl` row + `ApiKey` row deleted, centrals recreated **without** the override (`Security__Acl__Enabled=false` ×2,
|
||
image `97cb5cefb6d2` ×2 re-asserted), deploy `2a97271f…` Sealed, anonymous reads answer values again. Rig left UP on the master
|
||
image; the R3-02 gate's rows (HR5/HR100 raw tags, UNS `a/l/e1` + mirror, `Calc` driver) are unrelated to this gate and are
|
||
described in that plan.
|
||
|
||
**T11 → `done`.** S19 (timer expiry) passed in the first gate; S18 (bounded store) passes now.
|