Merge branch 'fix/r3-05-acl-hot-path-expiry' — R3-05: bounded ConfigDb API-key lookup (deadline + TTL) and clock-driven key-expiry revalidation of live monitored items (archreview 03/S18, 03/S19)
This commit is contained in:
Joseph Doherty
@@ -578,3 +578,242 @@ 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.
|
||||
|
||||
@@ -5,13 +5,13 @@
|
||||
{
|
||||
"id": "T1",
|
||||
"subject": "Register TimeProvider in DI (shared by authorizer, key store and revalidation timer) + ApiKeyOptions.StoreTimeoutMs/StoreCacheSeconds + AclOptions.RevalidationIntervalSeconds (03/S18, 03/S19)",
|
||||
"status": "pending",
|
||||
"status": "done",
|
||||
"blockedBy": []
|
||||
},
|
||||
{
|
||||
"id": "T2",
|
||||
"subject": "BoundedApiKeyStore: hard per-call deadline on the key store under the SessionManager lock, fail-closed (03/S18)",
|
||||
"status": "pending",
|
||||
"status": "done",
|
||||
"blockedBy": [
|
||||
"T1"
|
||||
]
|
||||
@@ -19,7 +19,7 @@
|
||||
{
|
||||
"id": "T3",
|
||||
"subject": "BoundedApiKeyStore: short TTL read-through cache + falsification control (a revoked key is denied once the TTL has passed)",
|
||||
"status": "pending",
|
||||
"status": "done",
|
||||
"blockedBy": [
|
||||
"T2"
|
||||
]
|
||||
@@ -27,7 +27,7 @@
|
||||
{
|
||||
"id": "T4",
|
||||
"subject": "otopcua.opcua.apikey_store.bounded counter (outcome=timeout|cache_hit) + MeterListener assertions",
|
||||
"status": "pending",
|
||||
"status": "done",
|
||||
"blockedBy": [
|
||||
"T3"
|
||||
]
|
||||
@@ -35,7 +35,7 @@
|
||||
{
|
||||
"id": "T5",
|
||||
"subject": "Wire BoundedApiKeyStore on admin/fused nodes in Program.cs + registration guard test; correct the authenticator's 'no network I/O at all' remark",
|
||||
"status": "pending",
|
||||
"status": "done",
|
||||
"blockedBy": [
|
||||
"T4"
|
||||
]
|
||||
@@ -43,7 +43,7 @@
|
||||
{
|
||||
"id": "T6",
|
||||
"subject": "AclRevalidationTimer (pure, TimeProvider-driven, non-throwing tick) + unit tests (03/S19)",
|
||||
"status": "pending",
|
||||
"status": "done",
|
||||
"blockedBy": [
|
||||
"T1"
|
||||
]
|
||||
@@ -51,7 +51,7 @@
|
||||
{
|
||||
"id": "T7",
|
||||
"subject": "Node-manager expiry tests (FakeTimeProvider advanced past ExpiresUtc, no deploy) + correct the ':316-328' only-trigger xmldoc",
|
||||
"status": "pending",
|
||||
"status": "done",
|
||||
"blockedBy": [
|
||||
"T1"
|
||||
]
|
||||
@@ -59,7 +59,7 @@
|
||||
{
|
||||
"id": "T8",
|
||||
"subject": "Wire the revalidation timer into OtOpcUaServerHostedService (create after StartAsync, dispose FIRST in StopAsync) + ordering test",
|
||||
"status": "pending",
|
||||
"status": "done",
|
||||
"blockedBy": [
|
||||
"T6",
|
||||
"T7"
|
||||
@@ -68,7 +68,7 @@
|
||||
{
|
||||
"id": "T9",
|
||||
"subject": "Over-the-wire AclKeyExpiryEnforcementTests: key-carrying session, clock advanced past expiry, item goes BadUserAccessDenied with no deploy (+ negative control)",
|
||||
"status": "pending",
|
||||
"status": "done",
|
||||
"blockedBy": [
|
||||
"T8"
|
||||
]
|
||||
@@ -76,7 +76,7 @@
|
||||
{
|
||||
"id": "T10",
|
||||
"subject": "Docs: docs/security.md store table + revocation bullets, and the CLAUDE.md 'asymmetric revocation' sentence (activation vs already-open session)",
|
||||
"status": "pending",
|
||||
"status": "done",
|
||||
"blockedBy": [
|
||||
"T5",
|
||||
"T8"
|
||||
@@ -94,7 +94,7 @@
|
||||
{
|
||||
"id": "T12",
|
||||
"subject": "File residuals R1 (event monitored items revalidated by nothing) and R2 (expired key retains roles while Security:Acl:Enabled=false)",
|
||||
"status": "pending",
|
||||
"status": "done",
|
||||
"blockedBy": []
|
||||
}
|
||||
]
|
||||
|
||||
Reference in New Issue
Block a user