diff --git a/CLAUDE.md b/CLAUDE.md index ff22131d..76034579 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -814,7 +814,12 @@ Two things to know before relying on any of this: item is queued `BadUserAccessDenied` and then **detached from its `MonitoredNode2` fan-out — NOT set `MonitoringMode.Disabled`**: disabling discards the item's queued notifications, so the client is silenced without ever being told why (caught only by the over-the-wire test). Detaching is also - reversible, so restoring the grant resumes the feed with no client action. + reversible, so restoring the grant resumes the feed with no client action. ⚠️ **`Changed` is no longer + the only trigger (arch-review 03/S19):** `AclRevalidationTimer`, armed by `OtOpcUaServerHostedService` + at `Security:Acl:RevalidationIntervalSeconds` (default 60 s, `0` disables, inert while + `Security:Acl:Enabled` is false), runs the same pass on the clock — because `ExpiresUtc` passes with + the clock, not with a deployment. The pass's verdict memo is **pass-local and must stay that way**; + hoisting it across ticks would serve an expiry that landed between two ticks the earlier tick's allow. - **Anonymous is a normal grantable principal**, not a bypass. `OpcUa:AllowAnonymous` (default `true`, preserving today's behaviour) controls *admission*; when admitted the session authenticates as the reserved principal `anonymous` and gets roles from `NodeAcl` rows naming it, arriving at the gates as @@ -826,9 +831,28 @@ Two things to know before relying on any of this: deliberately *not* in the artifact — and `DeploymentArtifact.ParseDataPlaneAuthority` decodes them node-side alongside the cluster's `NodeAcl` rows. `OpcUaPublishActor` installs both on `IDataPlaneAuthoritySource` on every apply, **before** its `plan.IsEmpty` early-return, so a deployment - that only revokes a key still lands. A node with a ConfigDb keeps `ConfigDbApiKeyStore` (per-request - read); a driver-only node gets `ArtifactApiKeyStore`. ⚠️ **The consequence is asymmetric revocation: - immediate on a ConfigDb node, next-deployment on a driver-only one.** + that only revokes a key still lands. A node with a ConfigDb keeps `ConfigDbApiKeyStore` (per-activation + read, wrapped — see below); a driver-only node gets `ArtifactApiKeyStore`. ⚠️ **The asymmetry is + ACTIVATION-scoped, not session-scoped — this line used to say "immediate on a ConfigDb node" and that was + wrong.** Clearing `Enabled` in the ConfigDb stops the **next session activation** on an admin/fused node + (bounded by `Security:ApiKeys:StoreCacheSeconds`, default 5 s) and takes until the next deployment on a + driver-only one — but an **already-open** session re-checks its key against the in-memory *artifact + snapshot* (`DataPlaneAuthorizer.Evaluate` → `snapshot.FindUsableKey`), never against `IApiKeyStore`, so on + **both** node kinds an `Enabled` revocation reaches an open session only at the next deployment. + **Expiry is the exception**: `FindUsableKey` compares `ExpiresUtc` against the authorizer's clock on every + call, so it is immediate everywhere — and since arch-review 03/S19 it reaches live subscriptions too, on + the host's `Security:Acl:RevalidationIntervalSeconds` timer (default 60 s; the session is **silenced with + `BadUserAccessDenied`, not closed**, and recovers on re-activation). ⚠️ Event/alarm monitored items are + revalidated by neither trigger — pre-existing, and a named residual. +- ⚠️ **The activation lookup is BOUNDED (arch-review 03/S18).** The SDK raises impersonation synchronously + under a `SessionManager`-wide lock, so a ConfigDb that is *slow* rather than *refusing* stalled **every** + session activation on the server, Anonymous and X.509 included. `BoundedApiKeyStore` wraps the ConfigDb + store — and only it, never `ArtifactApiKeyStore`, which is two in-memory dictionary probes — with + `Security:ApiKeys:StoreTimeoutMs` (2000; exceeding it **denies**, via the authenticator's existing + catch-all) plus a `StoreCacheSeconds` (5) read-through cache. Faults and timeouts are never cached, a + cached hit whose `ExpiresUtc` has passed is re-read rather than served, and a timeout logs a distinct + Warning + `otopcua.opcua.apikey_store.bounded{outcome=timeout}`. Both knobs at `0` restore the old + behaviour exactly. - ⚠️ **The auth generation is a node-local COUNTER driven by the `RevisionHash`, not derived from its bits.** `PermissionTrieCache` treats the highest `GenerationId` as current, so a generation taken from hash bytes would install a *lower* number about half the time and the cache would go on serving the diff --git a/archreview/plans/R3-05-acl-hot-path-expiry-plan.md b/archreview/plans/R3-05-acl-hot-path-expiry-plan.md index 2577b436..12d4c14b 100644 --- a/archreview/plans/R3-05-acl-hot-path-expiry-plan.md +++ b/archreview/plans/R3-05-acl-hot-path-expiry-plan.md @@ -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. diff --git a/archreview/plans/R3-05-acl-hot-path-expiry-plan.md.tasks.json b/archreview/plans/R3-05-acl-hot-path-expiry-plan.md.tasks.json index 77023481..82045b5b 100644 --- a/archreview/plans/R3-05-acl-hot-path-expiry-plan.md.tasks.json +++ b/archreview/plans/R3-05-acl-hot-path-expiry-plan.md.tasks.json @@ -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": [] } ] diff --git a/docs/Configuration.md b/docs/Configuration.md index 2bccc843..5858bccb 100644 --- a/docs/Configuration.md +++ b/docs/Configuration.md @@ -79,12 +79,15 @@ The checked-in `appsettings*.json` files are deliberately thin: they carry only |---|---|---|---| | `Username` | string | `apikey` | The fixed username a key-bearing client sends in its OPC UA UserName token. Carries no information — the key identifies the principal — but is checked rather than ignored so a typo is an explicit denial. | | `Pepper` | string | `""` | Server-side pepper for the stored key hashes. **Supply via `Security__ApiKeys__Pepper` or `${secret:}` — never commit it.** No default on purpose: an empty pepper still hashes "successfully", so `ApiKeySecret.ComputeHash` throws on one rather than producing a deployment with none of the protection it believes it has. Rotating it invalidates every existing key. | +| `StoreTimeoutMs` | int | `2000` | Hard per-call deadline on the API-key store lookup that runs inline on session activation (arch-review 03/S18). Exceeding it **denies** the activation, logs a distinct Warning and meters `otopcua.opcua.apikey_store.bounded{outcome=timeout}`. `≤ 0` disables the deadline — do not, on a node with a ConfigDb: the SDK holds a `SessionManager`-wide lock across that lookup, so an unanswered query stalls **every** session activation on the server. Applies only to `ConfigDbApiKeyStore`; the in-memory artifact store on a driver-only node is not wrapped. | +| `StoreCacheSeconds` | int | `5` | TTL of the read-through cache in front of that store, so a burst of activations costs one round trip. `0` disables it (a store read per activation). Bounds how long an `Enabled` revocation authored in the ConfigDb takes to reach a **new** activation; it has no effect on already-open sessions (those re-check the artifact snapshot) and cannot delay an `ExpiresUtc` — a cached hit that has since expired is re-read rather than served. | **`Security:Acl`** key fields (`AclOptions`): | Key | Type | Default | Meaning | |---|---|---|---| | `Enabled` | bool | `false` | `true` makes scoped `NodeAcl` grants decide **write**, **alarm-command** and **HistoryRead** authorization, *replacing* the server-wide `WriteOperate` / `AlarmAck` role checks. `false` keeps those role checks and leaves HistoryRead ungated — byte-for-byte the pre-stage-5 behaviour. Read, Browse and Subscribe are ungated either way. | +| `RevalidationIntervalSeconds` | int | `60` | How often live monitored items are re-authorized against the clock, in addition to on every deployment (arch-review 03/S19). A key's `ExpiresUtc` passes with the clock, so without this an expired key kept feeding a live subscription until a deployment happened to land — indefinitely on a fleet deployed once and left running. `0` disables the timer (deploy-triggered only). **Inert while `Enabled` is false**, since the pass early-returns when enforcement is off. An affected session is silenced with `BadUserAccessDenied`, **not closed**, and recovers on re-activation. | > **Flipping `Security:Acl:Enabled` is a config change, not a redeploy** — both paths are wired in both modes (the node builds its permission trie on every deploy regardless), the same dark-switch discipline as [`MeshTransport`](#meshtransport), [`ConfigSource`](#configsource) and [`Telemetry`](#telemetry). It defaults off because the first deployment carrying the code would otherwise start denying every client that has no key, **including ScadaBridge's Data Connection Layer, which consumes this address space from another repo.** Enforcement fails closed on every unresolvable case, so a wiring or config mistake presents as *everything denied* — check the `OpcUaPublish: data-plane authority generation N installed` line for a non-zero grant count before debugging an individual rule. See [`security.md` § Node ACL enforcement](security.md#node-acl-enforcement-securityaclenabled). diff --git a/docs/security.md b/docs/security.md index 6872fbaf..512016c6 100644 --- a/docs/security.md +++ b/docs/security.md @@ -177,16 +177,59 @@ When no authenticator is supplied, `OpcUaApplicationHost` falls back to `NullOpc the Host wires the real `ApiKeyOpcUaUserAuthenticator` as a singleton in `Program.cs`. Which **`IApiKeyStore`** backs it depends on the node's role: -| Node | Store | Revocation takes effect | -|---|---|---| -| `admin` (including fused `admin,driver`) | `ConfigDbApiKeyStore` — reads the config database per request | On the **next request** | -| `driver`-only | `ArtifactApiKeyStore` — reads the keys carried by the deployment artifact, held in memory | On the **next deployment** | -| neither role | `NullOpcUaUserAuthenticator` — denies every UserName token | n/a | +| Node | Store | An `Enabled = 0` revocation reaches a **new activation** | …and an **already-open session** | +|---|---|---|---| +| `admin` (including fused `admin,driver`) | `ConfigDbApiKeyStore` — reads the config database per activation, wrapped in `BoundedApiKeyStore` | On the **next activation**, bounded by `Security:ApiKeys:StoreCacheSeconds` (default 5 s) | On the **next deployment** | +| `driver`-only | `ArtifactApiKeyStore` — reads the keys carried by the deployment artifact, held in memory | On the **next deployment** | On the **next deployment** | +| neither role | `NullOpcUaUserAuthenticator` — denies every UserName token | n/a | n/a | + +⚠️ **The last column is the correction, and it used to be missing.** This table once said a ConfigDb node +revokes "on the next request", which is true only of a **new session activation**. An operation on a +session that is *already open* does not re-read `IApiKeyStore` at all — `DataPlaneAuthorizer.Evaluate` +re-checks the key against the in-memory **artifact snapshot** (`DataPlaneAuthority.FindUsableKey`), which +`OpcUaPublishActor` installs on every apply. So clearing `Enabled` in the ConfigDb stops the *next* +connect on an admin node and nothing else until a deployment lands, on **both** node kinds. + +**Expiry is the exception, and it is immediate on both.** `FindUsableKey` compares `ExpiresUtc` against +the authorizer's clock on every call, so an `ExpiresUtc` that has already passed denies at once on an +open session — and since arch-review 03/S19 it reaches a live **subscription** too, on a timer (see +[Data-Plane Authorization](#data-plane-authorization)). The deny-all is registered **explicitly**, not left absent: `OtOpcUaServerHostedService` takes `IOpcUaUserAuthenticator` as a required constructor parameter, so an unregistered seam fails DI and the node does not boot at all. +### Bounding the key store (`StoreTimeoutMs` / `StoreCacheSeconds`) + +The SDK raises impersonation **synchronously, holding a `SessionManager`-wide event lock**, so the key +lookup does not stall one activation — it serializes **every** session activation on the server, +Anonymous and X.509 included. On a driver-only node that is harmless (the artifact store is two +dictionary probes). On an admin or fused node it is two EF round trips to central SQL, so a ConfigDb that +is *slow or hung* rather than *refusing* could hold that lock indefinitely. `BoundedApiKeyStore` +(`src/Server/ZB.MOM.WW.OtOpcUa.Host/OpcUa/BoundedApiKeyStore.cs`) wraps the ConfigDb store — and only it — +with two `Security:ApiKeys` knobs: + +| Key | Default | Meaning | +|---|---|---| +| `StoreTimeoutMs` | `2000` | Hard per-call deadline on the store. Exceeding it **denies** the activation. `≤ 0` disables the deadline (the pre-R3-05 unbounded behaviour — do not do this on an admin node). | +| `StoreCacheSeconds` | `5` | TTL of a read-through cache in front of the store, so a burst of activations costs one round trip. `0` disables it (a read per activation). | + +- **A timeout denies; it never admits.** The deadline surfaces as an `OperationCanceledException` into + `ApiKeyOpcUaUserAuthenticator`'s existing catch-all, which returns the same opaque denial as a bad + credential. There is no second denial path to get wrong, and the decorator can only ever return what + the inner store returned or throw. +- **What a timeout looks like:** a distinct Warning — *"ApiKeyStore: the key store did not answer within + N ms — denying this activation. Every OPC UA session activation on this server is serialized behind + this call; check ConfigDb health"* — plus + `otopcua.opcua.apikey_store.bounded{outcome=timeout}`. It is deliberately **not** the ordinary + *"no enabled, unexpired key with id X"* line, which would send an operator hunting for a key problem + that does not exist. +- **What to watch:** `otopcua.opcua.apikey_store.bounded{outcome=cache_hit}`. A busy fused central with no + `cache_hit` stream means `StoreCacheSeconds` is 0. +- **Faults and timeouts are never cached** — caching a deny would extend an outage past the outage. Only + a completed lookup (a hit or an authoritative miss) is stored, and a cached hit whose `ExpiresUtc` has + since passed is re-read rather than served, so the TTL can never resurrect an expired key. + ### LDAP bind flow (`OtOpcUaLdapAuthService`) LDAP is configured under the `Security:Ldap` section (bound to `LdapOptions`, `src/Server/ZB.MOM.WW.OtOpcUa.Security/Ldap/LdapOptions.cs`, `SectionName = "Security:Ldap"`). The app authenticator is `OtOpcUaLdapAuthService` — a thin wrapper around the shared `ZB.MOM.WW.Auth.Ldap` directory client that adds two app-only concerns the shared library deliberately does not model: the `Enabled` master switch and `DevStubMode`. Since #520 it serves the **Admin UI cookie login (`/auth/login`) only** — `LdapOpcUaUserAuthenticator` is deleted and the OPC UA UserName path authenticates API keys instead (see [API-key identity](#api-key-identity-opc-ua-data-plane)). @@ -411,17 +454,36 @@ Behaviours worth knowing before enabling it: - **A deploy takes effect on sessions that are already open.** The gate stamps each decision with the current authority generation rather than the one the session activated under, so an ACL edit lands without bouncing clients — in both directions, widening and narrowing. -- **Revocation also reaches an open session.** Every decision re-checks the authenticating key against - the current snapshot, so a key disabled or expired by a later deployment stops working immediately - rather than at the client's next reconnect. On a driver-only node the artifact is the only revocation - channel there is, so without this a revoked credential could keep its authority indefinitely. -- **Revocation reaches a live subscription too** (#520 stage 6). Every other operation re-asks on each - request; a subscription is authorized once and then streams, so it needed a push. A deployment - re-authorizes every live monitored item: one whose grant has gone is queued a `BadUserAccessDenied` - and then detached from its node's value fan-out, and a later deployment that restores the grant - re-attaches it. This matters more than it sounds — subscriptions are how machine consumers actually - read this server, so without it revoking a grant would close the operations nobody uses and leave the - feed running. +- **Revocation also reaches an open session — but read the two axes separately.** Every decision + re-checks the authenticating key against the current snapshot, so an open session is re-authorized on + every operation. What that check can *see* differs by axis: + - **The deployment axis** (a key's `Enabled` cleared, a grant edited or removed). The snapshot only + changes when an artifact is applied, so this lands **on the next deployment**, on both node kinds — + including on a fused admin node, whose ConfigDb store is consulted at *activation* only. On a + driver-only node the artifact is the only revocation channel there is. + - **The clock axis** (a key's `ExpiresUtc` passing). `FindUsableKey` compares it against the + authorizer's clock on every call, so this lands **immediately**, with no deployment. +- **Revocation reaches a live subscription too** (#520 stage 6, extended by arch-review 03/S19). Every + other operation re-asks on each request; a subscription is authorized once and then streams, so it + needed a push. A denied item is queued a `BadUserAccessDenied` and then detached from its node's value + fan-out; restoring its authority re-attaches it. There are **two triggers**: + - a **deployment**, which re-authorizes every live monitored item (the deployment axis); + - the host's **low-frequency timer**, `Security:Acl:RevalidationIntervalSeconds` (default 60 s, `0` + disables it, inert while `Security:Acl:Enabled` is false). Without it a key that expired by the + clock kept streaming until a deployment happened to land — **indefinitely** on a fleet that has been + deployed once and left running, which is exactly the shape a production fleet settles into. + + ⚠️ **An expired-key session is silenced, not disconnected.** Its items go `BadUserAccessDenied` and the + session stays open, deliberately: a clock-driven denial must look to a client exactly like a + deploy-driven one, OPC UA has no honest "expire this session" verb (deleting it server-side gives the + client a transport failure rather than an authorization status), and every other operation on that + session already denies. **A client that re-activates with a renewed key recovers on the next tick with + no reconnect** — closing the session instead would force a full reconnect-and-resubscribe on every + machine consumer in the fleet at once when a shared key expired. + + ⚠️ **Event/alarm monitored items are revalidated by neither trigger.** The live-item map is fed only + from data-change items, so an alarm subscription is gated at creation and never again. Pre-existing — + the deployment trigger has the same hole — and tracked as a residual of arch-review 03/S19. - **An ancestor of a granted node is browsable**, or no narrow grant would be reachable: a client arrives at a deep node by browsing its ancestors, and the trie otherwise inherits downward only. Note what this necessarily discloses — the *names* of the folders on the path to a granted node are visible diff --git a/src/Core/ZB.MOM.WW.OtOpcUa.Commons/Observability/OtOpcUaTelemetry.cs b/src/Core/ZB.MOM.WW.OtOpcUa.Commons/Observability/OtOpcUaTelemetry.cs index 09950d24..ba3a0441 100644 --- a/src/Core/ZB.MOM.WW.OtOpcUa.Commons/Observability/OtOpcUaTelemetry.cs +++ b/src/Core/ZB.MOM.WW.OtOpcUa.Commons/Observability/OtOpcUaTelemetry.cs @@ -110,6 +110,20 @@ public static class OtOpcUaTelemetry Meter.CreateCounter("otopcua.redundancy.primary_gate_denied", unit: "{denial}", description: "Operations denied by the Primary gate (site=write|ack|alarm-emit, reason=secondary|detached|role-unknown)."); + /// + /// API-key store lookups the 03/S18 bound intervened on (outcome=timeout|cache_hit). + /// + /// + /// One counter with an outcome tag rather than two: timeout is the alarm signal that + /// the ConfigDb is not answering the OPC UA impersonation callback, and cache_hit is the + /// ratio that says whether the TTL is doing anything. A missing cache_hit stream on a busy + /// fused central is how you notice Security:ApiKeys:StoreCacheSeconds was set to 0 by + /// accident. A plain read-through meters neither — the counter names interventions, not traffic. + /// + public static readonly Counter ApiKeyStoreBounded = + Meter.CreateCounter("otopcua.opcua.apikey_store.bounded", unit: "{lookup}", + description: "API-key store lookups the bound intervened on (outcome=timeout|cache_hit)."); + // ---------------- Convenience helpers ---------------- /// diff --git a/src/Core/ZB.MOM.WW.OtOpcUa.Core/Authorization/AclOptions.cs b/src/Core/ZB.MOM.WW.OtOpcUa.Core/Authorization/AclOptions.cs index 1efe641c..524fa93b 100644 --- a/src/Core/ZB.MOM.WW.OtOpcUa.Core/Authorization/AclOptions.cs +++ b/src/Core/ZB.MOM.WW.OtOpcUa.Core/Authorization/AclOptions.cs @@ -42,4 +42,23 @@ public sealed class AclOptions /// false, which preserves the pre-stage-5 role gates byte-for-byte. /// public bool Enabled { get; set; } + + /// + /// How often, in seconds, live monitored items are re-authorized against the clock — in addition + /// to on every deployment (arch-review 03/S19). + /// + /// + /// + /// A subscription is authorized once and then streams, so before this a key that expired by the + /// CLOCK kept feeding a live subscription until a deployment happened to advance the generation + /// — indefinitely on a fleet that has been deployed once and left running. Every other seam + /// re-asks per request and honours ExpiresUtc the moment it passes. + /// + /// + /// 0 or negative disables the timer, leaving revalidation deploy-triggered only (the + /// pre-R3-05 behaviour). The timer is inert while is false: the + /// pass early-returns when ACL enforcement is off. + /// + /// + public int RevalidationIntervalSeconds { get; set; } = 60; } diff --git a/src/Core/ZB.MOM.WW.OtOpcUa.Core/Authorization/ApiKeyOptions.cs b/src/Core/ZB.MOM.WW.OtOpcUa.Core/Authorization/ApiKeyOptions.cs index 47d122e4..ea2d64ee 100644 --- a/src/Core/ZB.MOM.WW.OtOpcUa.Core/Authorization/ApiKeyOptions.cs +++ b/src/Core/ZB.MOM.WW.OtOpcUa.Core/Authorization/ApiKeyOptions.cs @@ -27,4 +27,29 @@ public sealed class ApiKeyOptions /// ApiKeySecret.ComputeHash throws on one and this stays empty to make that fire early. /// public string Pepper { get; set; } = string.Empty; + + /// + /// Hard per-call deadline, in milliseconds, on the API-key store lookup that runs inline on OPC UA + /// session activation (arch-review 03/S18). Exceeding it denies the activation and emits a + /// distinct Warning plus otopcua.opcua.apikey_store.bounded{outcome=timeout}. + /// + /// + /// 0 or negative disables the deadline, restoring the pre-R3-05 unbounded behaviour. Do not + /// do that on a node with a ConfigDb: the SDK raises impersonation synchronously under a + /// SessionManager-wide lock, so an unanswered lookup stalls every session activation + /// in the server, not just its own. + /// + public int StoreTimeoutMs { get; set; } = 2000; + + /// + /// TTL, in seconds, of the read-through cache in front of the API-key store (arch-review 03/S18). + /// Bounds how long a revocation authored in the ConfigDb can take to reach a new session + /// activation. + /// + /// + /// 0 or negative disables the cache, restoring a store read per activation. The window it + /// introduces is strictly smaller than the one that already exists for already-open sessions + /// on every node kind, which is the next deployment — see docs/security.md. + /// + public int StoreCacheSeconds { get; set; } = 5; } diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.Host/Configuration/DataPlaneAuthRegistration.cs b/src/Server/ZB.MOM.WW.OtOpcUa.Host/Configuration/DataPlaneAuthRegistration.cs new file mode 100644 index 00000000..19d5385d --- /dev/null +++ b/src/Server/ZB.MOM.WW.OtOpcUa.Host/Configuration/DataPlaneAuthRegistration.cs @@ -0,0 +1,111 @@ +using Microsoft.EntityFrameworkCore; +using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.DependencyInjection.Extensions; +using Microsoft.Extensions.Logging; +using Microsoft.Extensions.Options; +using ZB.MOM.WW.OtOpcUa.Cluster; +using ZB.MOM.WW.OtOpcUa.Configuration; +using ZB.MOM.WW.OtOpcUa.Core.Authorization; +using ZB.MOM.WW.OtOpcUa.Host.OpcUa; +using ZB.MOM.WW.OtOpcUa.OpcUaServer.Security; + +namespace ZB.MOM.WW.OtOpcUa.Host.Configuration; + +/// +/// Composition of the OPC UA data-plane authentication + authorization seams (#520), extracted from +/// Program.cs so the role-conditional branches are reachable from a test. +/// +/// +/// A store decorator registered in one branch of a role conditional is exactly the +/// "built but never wired" shape, and Program.cs's top-level statements cannot be driven by a +/// test — so the wiring moved here rather than the guard being written against a comment. The +/// branches are unchanged from the ones that lived inline; see ApiKeyStoreRegistrationTests. +/// +public static class DataPlaneAuthRegistration +{ + /// + /// Registers the shared clock, the node-scoped ACL gate and the role-appropriate API-key store + + /// user authenticator. + /// + /// The service collection. + /// Whether this node carries the admin role (and therefore a ConfigDb). + /// Whether this node carries the driver role. + /// The same collection, for chaining. + public static IServiceCollection AddOtOpcUaDataPlaneAuth( + this IServiceCollection services, bool hasAdmin, bool hasDriver) + { + ArgumentNullException.ThrowIfNull(services); + + // ONE clock for the whole authorization surface. The authorizer, the key-store bound and the + // revalidation timer all read time, and DataPlaneAuthorizer already documents what a second, + // diverging clock costs: every request denied, with a verdict indistinguishable from "no grant + // matched". TryAdd so a test (or a future host) can substitute one before this runs. + services.TryAddSingleton(TimeProvider.System); + + // #520 stage 5 — the node-scoped ACL gate. Registered on EVERY node, not just driver ones, because + // OtOpcUaServerHostedService takes it as a constructor parameter and the built-in container resolves + // constructors by whether every parameter is registered, not by whether the parameter type is nullable. + // That is the same trap the IOpcUaUserAuthenticator and IDataPlaneAuthoritySource registrations below + // both carry a comment about; an absent registration is a node that will not start. + services.AddSingleton(sp => + new TriePermissionEvaluator(sp.GetRequiredService())); + services.AddSingleton(sp => new DataPlaneAuthorizer( + sp.GetRequiredService(), + sp.GetRequiredService(), + sp.GetRequiredService>().Value.Enabled, + sp.GetRequiredService().CreateLogger(), + sp.GetRequiredService())); + + if (hasAdmin) + { + // A node WITH a ConfigDb reads keys straight from it, which is what makes revocation take effect + // on the very next activation rather than on the next deployment. Preferred wherever it is + // available, including on a fused admin+driver node. + // The ClusterId confines the grants this node honours to its OWN cluster — the ConfigDb holds + // the whole fleet's ACL rows, unlike the artifact a driver-only node reads, which + // ParseDataPlaneAuthority already scoped. Taken from AkkaClusterOptions (pure configuration), + // NOT from IClusterRoleInfo: that depends on the ActorSystem, and the same options-not-services + // rule is what keeps the redundancy-singleton registration in Program.cs from recursing into + // ActorSystem construction. Null on a node with no cluster-{ClusterId} role ⇒ no filter. + // + // arch-review 03/S18 — and it is WRAPPED. The lookup runs inline in the SDK's synchronous + // impersonation callback, under a SessionManager-wide lock, with CancellationToken.None: an + // unanswered ConfigDb query therefore stalls every session activation in the server. The + // decorator adds the deadline that bounds that, plus a short TTL that turns a burst of + // activations into one round trip. It cannot admit anything the inner store did not. + services.AddSingleton(sp => new BoundedApiKeyStore( + new ConfigDbApiKeyStore( + sp.GetRequiredService>(), + ConfigDbApiKeyStore.ResolveClusterId( + sp.GetRequiredService>().Value)), + sp.GetRequiredService>(), + sp.GetRequiredService().CreateLogger(), + sp.GetRequiredService())); + services.AddSingleton(); + + // Minting surface for the AdminUI. Same node condition as the store above: both need a ConfigDb. + services.AddSingleton(sp => new ApiKeyAdminService( + sp.GetRequiredService>(), + sp.GetRequiredService>().Value.Pepper)); + } + else if (hasDriver) + { + // Phase 4 left a driver-only node with no ConfigDb, so its keys ride the deployment artifact. + // Before stage 4 this node fell through to NullOpcUaUserAuthenticator and denied EVERY UserName + // token — safe, but it meant no machine client could authenticate against a site node at all. + // Deliberately NOT wrapped by the S18 bound: the artifact store is two dictionary probes on an + // in-memory snapshot, so it can neither hang nor be worth caching. + services.AddSingleton(); + services.AddSingleton(); + } + else + { + // Neither role: nothing serves a data plane here. MUST be registered explicitly, not left absent — + // OtOpcUaServerHostedService takes IOpcUaUserAuthenticator as a REQUIRED constructor parameter, so + // an unregistered seam is not a quiet fallback to deny-all; it fails DI and the node does not start. + services.AddSingleton(NullOpcUaUserAuthenticator.Instance); + } + + return services; + } +} diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.Host/OpcUa/AclRevalidationTimer.cs b/src/Server/ZB.MOM.WW.OtOpcUa.Host/OpcUa/AclRevalidationTimer.cs new file mode 100644 index 00000000..233c9ddb --- /dev/null +++ b/src/Server/ZB.MOM.WW.OtOpcUa.Host/OpcUa/AclRevalidationTimer.cs @@ -0,0 +1,99 @@ +using Microsoft.Extensions.Logging; +using ZB.MOM.WW.OtOpcUa.Core.Authorization; + +namespace ZB.MOM.WW.OtOpcUa.Host.OpcUa; + +/// +/// 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). +/// +/// +/// +/// Every request-shaped operation re-checks the calling key's usability per call and so honours +/// ExpiresUtc the moment it passes. A subscription is authorized once and then streams, and +/// streaming is how machine consumers actually read this server — so before this the only thing +/// that could stand a subscription down was a deployment. +/// +/// +/// ⚠️ The tick must never throw. An unhandled exception on a timer callback thread takes +/// the process down. Every tick is wrapped; a faulting pass is logged and the timer keeps running, +/// because the next tick is the recovery. +/// +/// +/// ⚠️ Each tick gets a fresh memo, and that is the pass's own constraint, not this class's. +/// RevalidateMonitoredItems allocates its verdict dictionary per call — see the remarks on +/// OtOpcUaNodeManager.OnAuthorityChanged. Nothing here may hoist it across ticks, or an +/// expiry landing between two ticks would be served the earlier tick's allow. +/// +/// +/// It adds no decision logic: the verdict comes from the authorizer's own +/// , and the stand-down (queue BadUserAccessDenied, then detach) +/// and its restore are the pass's, unchanged. The pass early-returns unless ACL enforcement is on, +/// so this is inert on the default configuration. +/// +/// +public sealed class AclRevalidationTimer : IDisposable +{ + private readonly Action _revalidate; + private readonly ILogger _log; + private readonly ITimer? _timer; + private volatile bool _disposed; + + /// Initializes a new instance of the class. + /// The revalidation pass to run on each tick. + /// + /// How often to run it. Non-positive arms no timer at all — the deploy-triggered-only behaviour + /// that predates 03/S19. + /// + /// The shared clock; its CreateTimer is what arms the tick. + /// The logger. + public AclRevalidationTimer(Action revalidate, TimeSpan interval, TimeProvider time, ILogger log) + { + ArgumentNullException.ThrowIfNull(revalidate); + ArgumentNullException.ThrowIfNull(time); + ArgumentNullException.ThrowIfNull(log); + + _revalidate = revalidate; + _log = log; + + if (interval <= TimeSpan.Zero) + { + // Say so once. A disabled guard that logs nothing is indistinguishable from a guard that is + // running and finding nothing, which is precisely the confusion 03/S19 is about. + _log.LogInformation( + "AclRevalidationTimer: disabled — {Section}:RevalidationIntervalSeconds is not positive, " + + "so live monitored items are re-authorized only when a deployment lands", + AclOptions.SectionName); + return; + } + + _timer = time.CreateTimer(_ => Tick(), state: null, interval, interval); + _log.LogInformation( + "AclRevalidationTimer: live monitored items will be re-authorized every {Interval}", interval); + } + + /// Runs one revalidation pass, absorbing anything it throws. + /// Exposed so a test can drive the callback body deterministically rather than by waiting. + public void Tick() + { + if (_disposed) return; + + try + { + _revalidate(); + } + catch (Exception ex) + { + _log.LogError(ex, "AclRevalidationTimer: the monitored-item revalidation pass threw; " + + "the timer keeps running and the next tick is the recovery"); + } + } + + /// + public void Dispose() + { + _disposed = true; + _timer?.Dispose(); + } +} diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.Host/OpcUa/ApiKeyOpcUaUserAuthenticator.cs b/src/Server/ZB.MOM.WW.OtOpcUa.Host/OpcUa/ApiKeyOpcUaUserAuthenticator.cs index dc0d0f85..b4e67ee7 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.Host/OpcUa/ApiKeyOpcUaUserAuthenticator.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.Host/OpcUa/ApiKeyOpcUaUserAuthenticator.cs @@ -18,9 +18,18 @@ namespace ZB.MOM.WW.OtOpcUa.Host.OpcUa; /// This runs inline on session activation. The SDK raises ImpersonateUser /// synchronously while holding a SessionManager-wide event lock, so a slow authenticator does not /// stall one request — it serializes every session activation server-wide. That is why -/// verification is a single indexed lookup plus one HMAC, and why there is no directory call, -/// no retry, and no network I/O on this path at all. It is the main practical advantage of keys -/// over the LDAP bind this replaces. +/// verification is a single indexed lookup plus one HMAC, with no directory call and no retry — +/// the main practical advantage of keys over the LDAP bind this replaces. +/// +/// +/// ⚠️ "No network I/O" is TRUE ONLY ON A DRIVER-ONLY NODE (arch-review 03/S18). The claim +/// describes , which is two dictionary probes on an in-memory +/// snapshot. On an admin or fused node is +/// , and a UserName activation costs two EF round trips +/// (the key row, then the principal's NodeAcl rows) — inside this same lock. What bounds +/// that is , which wraps it in production; the deadline it applies +/// surfaces here as an ordinary throw and is denied by the catch-all below, like any other store +/// failure. /// /// /// Fail closed, and say little. Every rejection returns the same opaque message. The diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.Host/OpcUa/BoundedApiKeyStore.cs b/src/Server/ZB.MOM.WW.OtOpcUa.Host/OpcUa/BoundedApiKeyStore.cs new file mode 100644 index 00000000..bc63006e --- /dev/null +++ b/src/Server/ZB.MOM.WW.OtOpcUa.Host/OpcUa/BoundedApiKeyStore.cs @@ -0,0 +1,240 @@ +using System.Collections.Concurrent; +using Microsoft.Extensions.Logging; +using Microsoft.Extensions.Options; +using ZB.MOM.WW.OtOpcUa.Commons.Observability; +using ZB.MOM.WW.OtOpcUa.Configuration.Entities; +using ZB.MOM.WW.OtOpcUa.Core.Authorization; + +namespace ZB.MOM.WW.OtOpcUa.Host.OpcUa; + +/// +/// Bounds what an can cost the OPC UA impersonation callback (arch-review +/// 03/S18). Wraps on a node that has a ConfigDb; the in-memory +/// needs neither half and is deliberately left unwrapped. +/// +/// +/// +/// Why a deadline is the fix and the cache is not. The SDK raises impersonation +/// synchronously under a SessionManager-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. +/// +/// +/// ⚠️ Every bounded outcome DENIES. A deadline exceeded, a faulted inner store and a +/// cancelled call all surface as an exception into +/// 's catch-all, which returns the same opaque denial as +/// a bad credential. Nothing here can admit a session the inner store did not authorize — the +/// decorator can only return what the inner store returned, or throw. +/// +/// +/// ⚠️ Faults and timeouts are NEVER cached. 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. +/// +/// +public sealed class BoundedApiKeyStore : IApiKeyStore +{ + /// + /// Entries either cache may hold before it is cleared wholesale. + /// + /// + /// Wholesale rather than per-entry eviction, for the reason SessionAclDecisionCache already + /// documents: picking WHICH entry to drop needs knowledge the cache does not have, and a + /// repopulate is cheap and cannot be wrong. The ceiling exists so a key-id guessing flood is a + /// bounded memory event — negative results are cached deliberately, so that a flood of unknown ids + /// does not become a SQL flood. + /// + public const int MaxCacheEntries = 1024; + + private readonly IApiKeyStore _inner; + private readonly IOptions _options; + private readonly ILogger _log; + private readonly TimeProvider _time; + private readonly ConcurrentDictionary> _keys = new(StringComparer.Ordinal); + private readonly ConcurrentDictionary>> _grants = + new(StringComparer.Ordinal); + + /// Initializes a new instance of the class. + /// The store being bounded. + /// The Security:ApiKeys options carrying the deadline and TTL. + /// The logger. + /// + /// The shared clock. The same instance the authorizer reads — see + /// DataPlaneAuthRegistration for why a second clock here would be a hazard rather than a + /// detail. + /// + public BoundedApiKeyStore( + IApiKeyStore inner, + IOptions options, + ILogger log, + TimeProvider time) + { + ArgumentNullException.ThrowIfNull(inner); + ArgumentNullException.ThrowIfNull(options); + ArgumentNullException.ThrowIfNull(log); + ArgumentNullException.ThrowIfNull(time); + + _inner = inner; + _options = options; + _log = log; + _time = time; + } + + /// The store being bounded. + /// + /// Exposed so the composition root's wiring can be asserted. A decorator registered in one branch + /// of a role conditional is the "built but never wired" shape, and a guard that cannot see what is + /// inside the wrapper cannot tell a correctly-wired node from one wrapping the wrong store. + /// + public IApiKeyStore Inner => _inner; + + /// The clock this bound's deadline and TTL are measured on. + /// Same reason as : the shared-clock invariant is only pinnable by reference. + public TimeProvider Clock => _time; + + /// + /// + /// Keyed by alone. is deliberately not part + /// of the key — it changes on every call, so including it would defeat the cache entirely — but a + /// cached hit IS re-checked against it before being served, so the TTL can never resurrect a key + /// that has expired by the clock in the meantime. The TTL therefore bounds how stale an + /// Enabled revocation may be, and nothing else. + /// + public async Task FindAsync(string apiKeyId, DateTime utcNow, CancellationToken ct) + { + var ttl = Ttl; + if (ttl > TimeSpan.Zero + && TryServe(_keys, apiKeyId, ttl, out var cached) + && !IsExpired(cached, utcNow)) + { + // Metered HERE rather than inside TryServe: an entry that is inside the TTL but whose key has + // since expired is not a hit — it falls through to a store read — so counting it in TryServe + // would report cache hits that never happened. + MeterCacheHit(); + return cached; + } + + var fresh = await BoundAsync(token => _inner.FindAsync(apiKeyId, utcNow, token), ct) + .ConfigureAwait(false); + if (ttl > TimeSpan.Zero) Store(_keys, apiKeyId, fresh); + return fresh; + } + + /// + /// Keyed by principal — the anonymous activation path, which runs on every unauthenticated connect. + public async Task> FindGrantsAsync(string principal, CancellationToken ct) + { + var ttl = Ttl; + if (ttl > TimeSpan.Zero && TryServe(_grants, principal, ttl, out var cached)) + { + MeterCacheHit(); + return cached; + } + + var fresh = await BoundAsync(token => _inner.FindGrantsAsync(principal, token), ct) + .ConfigureAwait(false); + if (ttl > TimeSpan.Zero) Store(_grants, principal, fresh); + return fresh; + } + + /// The configured TTL; non-positive means the cache is off. + private TimeSpan Ttl => TimeSpan.FromSeconds(_options.Value.StoreCacheSeconds); + + /// Whether a cached lookup has since expired by the clock. + /// The cached lookup, possibly null (an authoritative miss). + /// The caller's current time. + /// when the cached key must not be served. + private static bool IsExpired(ApiKeyLookup? cached, DateTime utcNow) => + cached?.Key.ExpiresUtc is { } expires && expires <= utcNow; + + /// Counts one lookup the cache actually answered. + private static void MeterCacheHit() => + OtOpcUaTelemetry.ApiKeyStoreBounded.Add(1, new KeyValuePair("outcome", "cache_hit")); + + /// Serves an entry if one is present and still inside the TTL. + /// The cached value type. + /// The cache to read. + /// The cache key. + /// How long an entry stays servable. + /// The cached value, when this returns true. + /// on a hit. + private bool TryServe( + ConcurrentDictionary> cache, string key, TimeSpan ttl, out T value) + { + if (cache.TryGetValue(key, out var entry) && _time.GetUtcNow() - entry.StampedUtc < ttl) + { + value = entry.Value; + return true; + } + + value = default!; + return false; + } + + /// Stores a COMPLETED lookup, clearing wholesale first if the ceiling has been reached. + /// The cached value type. + /// The cache to write. + /// The cache key. + /// The value to cache. + private void Store(ConcurrentDictionary> cache, string key, T value) + { + if (cache.Count >= MaxCacheEntries) cache.Clear(); + cache[key] = new Entry(_time.GetUtcNow(), value); + } + + /// One cached lookup and the instant it was taken. + /// The cached value type. + /// When the inner store answered. + /// What it answered. + private readonly record struct Entry(DateTimeOffset StampedUtc, T Value); + + /// Runs one inner call under the configured deadline. + /// The inner call's result type. + /// The inner call, taking the deadline-linked token. + /// The caller's token — in production always None, see remarks. + /// Whatever the inner store returned. + /// + /// + /// The deadline is created HERE rather than received from the caller because there is no caller + /// to receive one from: OpcUaApplicationHost block-bridges the SDK's synchronous + /// impersonation callback with , so the ct the store + /// sees in production is always uncancellable. + /// + /// + /// ⚠️ It re-throws rather than returning . 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 below is + /// the honest signal; the denial itself is unchanged either way. + /// + /// + private async Task BoundAsync(Func> call, CancellationToken ct) + { + var timeoutMs = _options.Value.StoreTimeoutMs; + if (timeoutMs <= 0) return await call(ct).ConfigureAwait(false); + + // The deadline runs on the INJECTED clock (the TimeProvider ctor overload), not on a plain + // CancelAfter, so it is the same TimeProvider the authorizer and the revalidation timer read. + // Linked with the caller's token so a genuine caller cancellation is still honoured — and so the + // catch below can tell the two apart. + using var deadline = new CancellationTokenSource(TimeSpan.FromMilliseconds(timeoutMs), _time); + using var cts = CancellationTokenSource.CreateLinkedTokenSource(ct, deadline.Token); + + try + { + return await call(cts.Token).ConfigureAwait(false); + } + catch (OperationCanceledException) when (!ct.IsCancellationRequested) + { + OtOpcUaTelemetry.ApiKeyStoreBounded.Add( + 1, new KeyValuePair("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; + } + } +} diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.Host/OpcUa/ConfigDbApiKeyStore.cs b/src/Server/ZB.MOM.WW.OtOpcUa.Host/OpcUa/ConfigDbApiKeyStore.cs index 09178426..e3cdc004 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.Host/OpcUa/ConfigDbApiKeyStore.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.Host/OpcUa/ConfigDbApiKeyStore.cs @@ -14,8 +14,19 @@ namespace ZB.MOM.WW.OtOpcUa.Host.OpcUa; /// A driver-only node has no ConfigDb since per-cluster mesh Phase 4, so this store cannot be /// registered there; that node uses instead (#520 stage 4). /// -/// Reading per request is what makes revocation immediate here, which is why this store is -/// preferred wherever a ConfigDb exists — including on a fused admin+driver node. +/// Reading per activation is what makes revocation prompt here, which is why this store is +/// preferred wherever a ConfigDb exists — including on a fused admin+driver node. ⚠️ It reaches a +/// new session activation only: an already-open session re-checks its key against the +/// in-memory artifact snapshot on every operation, not against this store, so clearing +/// Enabled here does not stop an open session until the next deployment. See +/// docs/security.md. +/// +/// +/// ⚠️ In production this store is WRAPPED by (arch-review +/// 03/S18), because these queries run inline in the SDK's impersonation callback under a +/// SessionManager-wide lock. A call may therefore be cancelled by the wrapper's deadline even +/// though the caller passed — the resulting +/// is a denial, not a bug. /// /// /// ⚠️ The ConfigDb holds the WHOLE fleet's ACL rows, not just this node's cluster's. That is diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.Host/OpcUa/OtOpcUaServerHostedService.cs b/src/Server/ZB.MOM.WW.OtOpcUa.Host/OpcUa/OtOpcUaServerHostedService.cs index d05d5abf..adea5467 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.Host/OpcUa/OtOpcUaServerHostedService.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.Host/OpcUa/OtOpcUaServerHostedService.cs @@ -38,6 +38,8 @@ public sealed class OtOpcUaServerHostedService : IHostedService, IAsyncDisposabl private readonly IDataPlaneAuthoritySource _dataPlaneAuthority; private readonly DataPlaneAuthorizer? _dataPlaneAuthorizer; private readonly ServerHistorianOptions _serverHistorianOptions; + private readonly AclOptions _aclOptions; + private readonly TimeProvider _time; private readonly Func _actorSystemAccessor; private readonly ActorRegistry _actorRegistry; private readonly ILoggerFactory _loggerFactory; @@ -45,6 +47,7 @@ public sealed class OtOpcUaServerHostedService : IHostedService, IAsyncDisposabl private OpcUaApplicationHost? _appHost; private OtOpcUaSdkServer? _server; + private AclRevalidationTimer? _revalidationTimer; /// /// Initializes a new instance of the OtOpcUaServerHostedService class. @@ -77,6 +80,13 @@ public sealed class OtOpcUaServerHostedService : IHostedService, IAsyncDisposabl /// #520 stage 5 — the node-scoped ACL gate. Nullable, and null means /// "not wired" rather than "deny": an unregistered gate must not lock a node out of writes to protect a /// feature that is off by default. Once wired AND enabled, every unresolvable case inside it denies. + /// #520 / arch-review 03/S19 — the Security:Acl options, read for + /// . Required, not optional: the built-in DI + /// container does not fall back to a default for an unregistered service, the same trap the + /// IOpcUaUserAuthenticator and IDataPlaneAuthoritySource parameters carry comments + /// about. + /// The shared clock — the one instance the authorizer and the API-key store bound + /// also read. Its CreateTimer arms the revalidation tick. /// The logger factory for creating loggers. public OtOpcUaServerHostedService( IOptions options, @@ -89,9 +99,16 @@ public sealed class OtOpcUaServerHostedService : IHostedService, IAsyncDisposabl Func actorSystemAccessor, ActorRegistry actorRegistry, DataPlaneAuthorizer? dataPlaneAuthorizer, + IOptions aclOptions, + TimeProvider time, ILoggerFactory loggerFactory) { + ArgumentNullException.ThrowIfNull(aclOptions); + ArgumentNullException.ThrowIfNull(time); + _dataPlaneAuthorizer = dataPlaneAuthorizer; + _aclOptions = aclOptions.Value; + _time = time; _options = options.Value; _deferredSink = deferredSink; _deferredServiceLevel = deferredServiceLevel; @@ -269,9 +286,41 @@ public sealed class OtOpcUaServerHostedService : IHostedService, IAsyncDisposabl _loggerFactory.CreateLogger())); } + // arch-review 03/S19 — LAST, after the node manager is known non-null and after the ACL gate is + // attached, so the first tick cannot land on a half-wired server. + StartRevalidationTimer(); + _logger.LogInformation("OtOpcUaServerHostedService: SDK started, address-space + ServiceLevel sinks bound"); } + /// + /// Arms the clock-driven monitored-item revalidation tick (arch-review 03/S19). + /// + /// + /// Created unconditionally: the pass it drives early-returns unless the ACL gate is wired AND + /// Security:Acl:Enabled is on, so it stays inert on the default configuration, and + /// RevalidationIntervalSeconds = 0 arms no timer at all. + /// + /// The node manager is resolved per tick rather than captured: is + /// replaced across a restart, and a captured reference would go on revalidating a dead + /// manager's item map — the same detach hazard the authority-source setter documents. + /// + /// Separate from so the timer's lifecycle is testable without booting the SDK. + /// + internal void StartRevalidationTimer() + { + // Dispose before overwriting. StartAsync runs again when a host restarts the service around the + // same instance, and an overwritten field would leave the previous timer armed with nothing left + // to dispose it — the same dispose-before-overwrite rule the driver reconnect paths carry. + _revalidationTimer?.Dispose(); + + _revalidationTimer = new AclRevalidationTimer( + () => _server?.NodeManager?.RevalidateMonitoredItems(), + TimeSpan.FromSeconds(_aclOptions.RevalidationIntervalSeconds), + _time, + _loggerFactory.CreateLogger()); + } + /// /// Stops the OPC UA server asynchronously. /// @@ -279,6 +328,12 @@ public sealed class OtOpcUaServerHostedService : IHostedService, IAsyncDisposabl /// A task that represents the asynchronous operation. public Task StopAsync(CancellationToken cancellationToken) { + // FIRST, ahead of every detach below (arch-review 03/S19). A tick landing on a node manager whose + // authority and gate have already been cleared is the shape this whole teardown block exists to + // prevent — a stopping node sheds the trigger before it sheds what the trigger drives. + _revalidationTimer?.Dispose(); + _revalidationTimer = null; + // Revert to Null adapters so any in-flight writes from a poison-pilled actor don't hit a // half-disposed NodeManager. _deferredSink.SetSink(null); @@ -304,6 +359,11 @@ public sealed class OtOpcUaServerHostedService : IHostedService, IAsyncDisposabl /// A task that represents the asynchronous operation. public async ValueTask DisposeAsync() { + // Before the app host, and idempotent with StopAsync — a host that disposes without stopping must + // not leave a tick armed against a server that is about to go away. + _revalidationTimer?.Dispose(); + _revalidationTimer = null; + if (_appHost is not null) await _appHost.DisposeAsync(); } } diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.Host/Program.cs b/src/Server/ZB.MOM.WW.OtOpcUa.Host/Program.cs index 12eeee6b..e6e0fbbb 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.Host/Program.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.Host/Program.cs @@ -398,55 +398,10 @@ if (hasDriver) builder.Services.AddSingleton(); } - // #520 stage 5 — the node-scoped ACL gate. Registered on EVERY node, not just driver ones, because - // OtOpcUaServerHostedService takes it as a constructor parameter and the built-in container resolves - // constructors by whether every parameter is registered, not by whether the parameter type is nullable. - // That is the same trap the IOpcUaUserAuthenticator and IDataPlaneAuthoritySource registrations above - // both carry a comment about; an absent registration is a node that will not start. - builder.Services.AddSingleton(sp => - new TriePermissionEvaluator(sp.GetRequiredService())); - builder.Services.AddSingleton(sp => new DataPlaneAuthorizer( - sp.GetRequiredService(), - sp.GetRequiredService(), - sp.GetRequiredService>().Value.Enabled, - sp.GetRequiredService().CreateLogger())); - - if (hasAdmin) - { - // A node WITH a ConfigDb reads keys straight from it, which is what makes revocation take effect - // on the very next request rather than on the next deployment. Preferred wherever it is available, - // including on a fused admin+driver node. - // The ClusterId confines the grants this node honours to its OWN cluster — the ConfigDb holds - // the whole fleet's ACL rows, unlike the artifact a driver-only node reads, which - // ParseDataPlaneAuthority already scoped. Taken from AkkaClusterOptions (pure configuration), - // NOT from IClusterRoleInfo: that depends on the ActorSystem, and the same options-not-services - // rule is what keeps the redundancy-singleton registration below from recursing into ActorSystem - // construction. Null on a node with no cluster-{ClusterId} role ⇒ no filter (legacy behaviour). - builder.Services.AddSingleton(sp => new ConfigDbApiKeyStore( - sp.GetRequiredService>(), - ConfigDbApiKeyStore.ResolveClusterId(sp.GetRequiredService>().Value))); - builder.Services.AddSingleton(); - - // Minting surface for the AdminUI. Same node condition as the store above: both need a ConfigDb. - builder.Services.AddSingleton(sp => new ApiKeyAdminService( - sp.GetRequiredService>(), - sp.GetRequiredService>().Value.Pepper)); - } - else if (hasDriver) - { - // Phase 4 left a driver-only node with no ConfigDb, so its keys ride the deployment artifact. - // Before stage 4 this node fell through to NullOpcUaUserAuthenticator and denied EVERY UserName - // token — safe, but it meant no machine client could authenticate against a site node at all. - builder.Services.AddSingleton(); - builder.Services.AddSingleton(); - } - else - { - // Neither role: nothing serves a data plane here. MUST be registered explicitly, not left absent — - // OtOpcUaServerHostedService takes IOpcUaUserAuthenticator as a REQUIRED constructor parameter, so - // an unregistered seam is not a quiet fallback to deny-all; it fails DI and the node does not start. - builder.Services.AddSingleton(NullOpcUaUserAuthenticator.Instance); - } + // #520 + arch-review 03/S18 — the node-scoped ACL gate, the shared clock and the role-appropriate + // API-key store + user authenticator, all in one extension so the role-conditional branches are + // reachable from a test (Program.cs's top-level statements are not). See DataPlaneAuthRegistration. + builder.Services.AddOtOpcUaDataPlaneAuth(hasAdmin, hasDriver); // Bind + validate the OPC UA host options the same way (fail-fast at start via ValidateOnStart) // and let OtOpcUaServerHostedService consume the validated IOptions instance rather than diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.Host/ZB.MOM.WW.OtOpcUa.Host.csproj b/src/Server/ZB.MOM.WW.OtOpcUa.Host/ZB.MOM.WW.OtOpcUa.Host.csproj index 1819d602..cc80ef36 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.Host/ZB.MOM.WW.OtOpcUa.Host.csproj +++ b/src/Server/ZB.MOM.WW.OtOpcUa.Host/ZB.MOM.WW.OtOpcUa.Host.csproj @@ -13,6 +13,13 @@ $(NoWarn);NU1608 + + + + + diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/OtOpcUaNodeManager.cs b/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/OtOpcUaNodeManager.cs index b12a03bc..8a1c3d1a 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/OtOpcUaNodeManager.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/OtOpcUaNodeManager.cs @@ -317,12 +317,21 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2 /// Re-authorizes live subscriptions when a deployment installs a new authorization generation. /// /// - /// ⚠️ This is the only trigger, and the pass's memo depends on that. - /// memoizes key revocation and expiry for the span of one - /// pass — safe only while revalidation is deploy-triggered, because a pass is then a momentary - /// event whose verdicts cannot go stale within it. A future periodic trigger must give each tick a - /// fresh memo (it already does — the dictionary is pass-local) and must not be tempted to reuse one - /// across ticks, or an expiry landing between ticks would be served the previous tick's allow. + /// + /// There are TWO triggers (arch-review 03/S19): this one, the deployment axis; and the + /// host's low-frequency AclRevalidationTimer, the clock axis — a key's + /// ExpiresUtc passes with the clock, not with a deployment, so a deploy-only trigger + /// left an expired key streaming to a live subscription indefinitely on a fleet that had been + /// deployed once and left running. + /// + /// + /// ⚠️ The second trigger is safe only because the memo is pass-local. + /// memoizes key revocation and expiry for the span + /// of one pass, which is sound because a pass is a momentary event whose verdicts cannot go + /// stale within it — its dictionary is constructed per call. Hoisting it across ticks, to save + /// the walk, would serve an expiry that landed between two ticks the earlier tick's allow, and + /// re-open exactly the gap the timer closes. + /// /// /// The newly published snapshot; unused — the current state is re-read per item. private void OnAuthorityChanged(DataPlaneAuthoritySnapshot snapshot) => RevalidateMonitoredItems(); @@ -3311,9 +3320,10 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2 /// /// /// This memo does cover the key expiry check that the authorizer's own cache excludes, but only - /// for the duration of one pass — and a subscription is re-checked at a deployment, never on a - /// timer, so an expiry that lands inside a single pass was already going to be caught by the - /// next deployment rather than by this one. + /// for the duration of one pass. Since arch-review 03/S19 a subscription is re-checked on the + /// host's low-frequency timer as well as at a deployment, so an expiry that lands inside a + /// single pass is caught by the next TICK — one interval later, not one deployment later. What + /// makes that hold is that the memo is constructed per call and never reused across passes. /// /// private bool IsSubscriptionStillPermitted( diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/Security/DataPlaneAuthorizer.cs b/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/Security/DataPlaneAuthorizer.cs index 3f9a64a1..127e5778 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/Security/DataPlaneAuthorizer.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/Security/DataPlaneAuthorizer.cs @@ -108,6 +108,15 @@ public sealed class DataPlaneAuthorizer /// public bool Enabled { get; } + /// The clock this gate evaluates key expiry against. + /// + /// Exposed so the composition root's wiring can be asserted by reference. The remarks on + /// UserAuthorizationState record what a second, diverging clock costs: every request denied, + /// with a verdict indistinguishable from "no grant matched". One registered singleton threaded into + /// every consumer is the fix, and a test that cannot see which instance landed here cannot pin it. + /// + public TimeProvider Clock => _time; + /// /// Memoized grant walks for the request-rate seams (#520 stage 6). See /// for what it is and is not allowed to cache. diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.Host.Tests/Configuration/MeshRegistrationRoleScopingTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.Host.Tests/Configuration/MeshRegistrationRoleScopingTests.cs index 58d5d93f..b3e4d7b2 100644 --- a/tests/Server/ZB.MOM.WW.OtOpcUa.Host.Tests/Configuration/MeshRegistrationRoleScopingTests.cs +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.Host.Tests/Configuration/MeshRegistrationRoleScopingTests.cs @@ -236,8 +236,10 @@ public sealed class MeshRegistrationRoleScopingTests } /// - /// Pins the scanner's expected result shape against today's Program.cs — 6 markers, - /// 5 braced blocks + the 1 braceless ConfigDb gate, so 6 scanned ranges. Without this, a + /// Pins the scanner's expected result shape against today's Program.cs — 5 markers, + /// 4 braced blocks + the 1 braceless ConfigDb gate, so 5 scanned ranges (the data-plane + /// key-store branch moved into DataPlaneAuthRegistration for arch-review 03/S18, taking + /// one braced marker with it). Without this, a /// scanner bug that silently drops or merges a marker (as the pre-fix version did to the /// braceless marker — it fabricated a phantom range spanning an unrelated if-block and swallowed /// a real nested 'if (hasAdmin)' block inside it, yielding 5 ranges instead of 6) would not @@ -248,13 +250,13 @@ public sealed class MeshRegistrationRoleScopingTests { var source = ReadProgramSource(); - CountOccurrences(source, HasAdminMarker).ShouldBe(6, + CountOccurrences(source, HasAdminMarker).ShouldBe(5, "Program.cs's 'if (hasAdmin)' marker count changed. If a marker was added or removed, " + "update this pin — and check whether the new/removed occurrence is braced or braceless — " + "rather than letting the scanner silently mis-shape its result."); var ranges = FindHasAdminBlockRanges(source); - ranges.Count.ShouldBe(6, + ranges.Count.ShouldBe(5, "expected one scanned range per 'if (hasAdmin)' marker, including the braceless ConfigDb " + "gate. A mismatch means the scanner adopted an unrelated brace or otherwise merged/dropped " + "a marker instead of handling each one independently."); diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.Host.Tests/OpcUa/AclRevalidationTimerTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.Host.Tests/OpcUa/AclRevalidationTimerTests.cs new file mode 100644 index 00000000..fbe57857 --- /dev/null +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.Host.Tests/OpcUa/AclRevalidationTimerTests.cs @@ -0,0 +1,151 @@ +using Microsoft.Extensions.Logging; +using Microsoft.Extensions.Logging.Abstractions; +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.Host.OpcUa; + +namespace ZB.MOM.WW.OtOpcUa.Host.Tests.OpcUa; + +/// +/// Covers — the low-frequency trigger that makes key expiry reach +/// a live subscription without waiting for a deployment (arch-review 03/S19). +/// +public sealed class AclRevalidationTimerTests +{ + /// A positive interval arms one timer whose due time and period are both that interval. + [Fact] + public void A_positive_interval_creates_a_timer_with_that_period() + { + var clock = new RecordingTimeProvider(); + + using var _ = new AclRevalidationTimer( + () => { }, TimeSpan.FromSeconds(30), clock, NullLogger.Instance); + + clock.Created.Count.ShouldBe(1); + clock.Created[0].DueTime.ShouldBe(TimeSpan.FromSeconds(30)); + clock.Created[0].Period.ShouldBe(TimeSpan.FromSeconds(30)); + } + + /// + /// A non-positive interval arms nothing at all, and says so once — a disabled guard should be + /// visible in the log rather than inferred from an absence of activity. + /// + [Fact] + public void A_zero_interval_creates_no_timer() + { + var clock = new RecordingTimeProvider(); + var log = new CapturingLogger(); + + using var _ = new AclRevalidationTimer(() => { }, TimeSpan.Zero, clock, log); + + clock.Created.ShouldBeEmpty(); + log.Lines.ShouldContain(l => l.Contains("RevalidationIntervalSeconds", StringComparison.Ordinal)); + } + + /// + /// A pass that throws must not escape the tick. + /// + /// + /// An unhandled exception on a timer callback thread takes the process down. The next tick is the + /// recovery, so the fault is logged and the timer keeps running. + /// + [Fact] + public void A_throwing_pass_does_not_escape_the_tick() + { + var log = new CapturingLogger(); + using var timer = new AclRevalidationTimer( + () => throw new InvalidOperationException("pass blew up"), + TimeSpan.FromSeconds(30), + new RecordingTimeProvider(), + log); + + Should.NotThrow(timer.Tick); + log.Lines.ShouldContain(l => l.Contains("revalidation", StringComparison.OrdinalIgnoreCase)); + } + + /// A tick runs the pass — the control for the two "nothing happened" cases above. + [Fact] + public void A_tick_runs_the_pass() + { + var runs = 0; + using var timer = new AclRevalidationTimer( + () => runs++, TimeSpan.FromSeconds(30), new RecordingTimeProvider(), NullLogger.Instance); + + timer.Tick(); + timer.Tick(); + + runs.ShouldBe(2); + } + + /// Disposing disposes the underlying timer and stops further ticks. + [Fact] + public void Dispose_stops_further_ticks() + { + var clock = new RecordingTimeProvider(); + var runs = 0; + var timer = new AclRevalidationTimer( + () => runs++, TimeSpan.FromSeconds(30), clock, NullLogger.Instance); + + timer.Dispose(); + + clock.Created[0].Disposed.ShouldBeTrue(); + timer.Tick(); + runs.ShouldBe(0, "a disposed timer must not run the pass again"); + } + + // ---------- helpers ---------- + + /// + /// A that records the timers asked of it instead of arming real ones. + /// + private sealed class RecordingTimeProvider : TimeProvider + { + /// Every timer this provider was asked to create, in order. + public List Created { get; } = []; + + /// + public override ITimer CreateTimer(TimerCallback callback, object? state, TimeSpan dueTime, TimeSpan period) + { + var timer = new FakeTimer(dueTime, period); + Created.Add(timer); + return timer; + } + + /// A timer that does nothing but remember how it was asked for, and whether it was disposed. + internal sealed class FakeTimer(TimeSpan dueTime, TimeSpan period) : ITimer + { + public TimeSpan DueTime { get; } = dueTime; + + public TimeSpan Period { get; } = period; + + public bool Disposed { get; private set; } + + public bool Change(TimeSpan due, TimeSpan p) => true; + + public void Dispose() => Disposed = true; + + public ValueTask DisposeAsync() + { + Disposed = true; + return ValueTask.CompletedTask; + } + } + } + + /// Collects rendered log messages so a "said so once" claim can be asserted. + private sealed class CapturingLogger : ILogger + { + public List Lines { get; } = []; + + public IDisposable? BeginScope(TState state) where TState : notnull => null; + + public bool IsEnabled(LogLevel logLevel) => true; + + public void Log( + LogLevel logLevel, + EventId eventId, + TState state, + Exception? exception, + Func formatter) => Lines.Add(formatter(state, exception)); + } +} diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.Host.Tests/OpcUa/ApiKeyStoreRegistrationTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.Host.Tests/OpcUa/ApiKeyStoreRegistrationTests.cs new file mode 100644 index 00000000..e425a12e --- /dev/null +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.Host.Tests/OpcUa/ApiKeyStoreRegistrationTests.cs @@ -0,0 +1,133 @@ +using Microsoft.EntityFrameworkCore; +using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Logging; +using Microsoft.Extensions.Logging.Abstractions; +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.Cluster; +using ZB.MOM.WW.OtOpcUa.Configuration; +using ZB.MOM.WW.OtOpcUa.Core.Authorization; +using ZB.MOM.WW.OtOpcUa.Host.OpcUa; +using ZB.MOM.WW.OtOpcUa.Host.Configuration; +using ZB.MOM.WW.OtOpcUa.OpcUaServer.Security; + +namespace ZB.MOM.WW.OtOpcUa.Host.Tests.OpcUa; + +/// +/// Guards the role-conditional data-plane authentication wiring (arch-review 03/S18). A decorator +/// registered in one branch of a role conditional is the "built but never wired" shape, so the branch +/// each node role lands in is asserted rather than assumed. +/// +public sealed class ApiKeyStoreRegistrationTests +{ + /// + /// The authorizer's clock and the registered clock must be the SAME instance. + /// + /// + /// already documents that a divergence between two clocks denies + /// every request with a verdict indistinguishable from "no grant matched" — found once by a test + /// clock two hours ahead. One registered singleton threaded into every consumer makes the + /// divergence unrepresentable instead of merely unlikely, so the registration is what is pinned. + /// + [Fact] + public void The_authorizer_and_the_key_store_share_one_TimeProvider() + { + using var sp = BuildNode(hasAdmin: true, hasDriver: true); + + var registered = sp.GetRequiredService(); + var authorizer = sp.GetRequiredService(); + + registered.ShouldBeSameAs(Probe.Instance); + authorizer.Clock.ShouldBeSameAs(registered); + } + + /// + /// An admin (or fused admin+driver) node's key store is the + /// decorator, wrapping the ConfigDb store. + /// + /// + /// The decorator lives in ONE branch of a role conditional, which is the "built but never wired" + /// shape OVERALL theme 3 names. Unwired, every unit test in BoundedApiKeyStoreTests still + /// passes and the deadline protects nothing. + /// + [Fact] + public void An_admin_node_wraps_the_ConfigDb_store_in_the_bound() + { + using var sp = BuildNode(hasAdmin: true, hasDriver: true); + + var store = sp.GetRequiredService().ShouldBeOfType(); + store.Inner.ShouldBeOfType(); + store.Clock.ShouldBeSameAs(sp.GetRequiredService()); + } + + /// + /// A driver-only node's key store is the bare artifact store — deliberately NOT wrapped. + /// + /// + /// It is two dictionary probes on an in-memory snapshot, so it can neither hang nor be worth + /// caching; wrapping it would add a deadline to a call that cannot miss one and a second copy of + /// data the snapshot already holds. + /// + [Fact] + public void A_driver_only_node_gets_the_bare_artifact_store() + { + using var sp = BuildNode(hasAdmin: false, hasDriver: true); + + sp.GetRequiredService().ShouldBeOfType(); + } + + /// + /// A node carrying neither role registers the deny-all authenticator explicitly, and no key + /// store at all. + /// + /// + /// Explicitly, not by absence: OtOpcUaServerHostedService takes + /// as a required constructor parameter, and the built-in + /// container does not fall back to a default for an unregistered service — it throws, and the node + /// does not start. The assertion is on the singleton INSTANCE, so a future registration that + /// happened to resolve some other deny-all type still surfaces here. + /// + [Fact] + public void A_node_with_neither_role_registers_the_deny_all_authenticator() + { + using var sp = BuildNode(hasAdmin: false, hasDriver: false); + + sp.GetRequiredService().ShouldBeSameAs(NullOpcUaUserAuthenticator.Instance); + sp.GetService().ShouldBeNull("a node that serves no data plane has no keys to read"); + } + + /// Builds the data-plane authentication wiring for one node role combination. + /// Whether the node carries the admin role. + /// Whether the node carries the driver role. + /// The built provider; the caller disposes it. + internal static ServiceProvider BuildNode(bool hasAdmin, bool hasDriver) + { + var services = new ServiceCollection(); + services.AddLogging(b => b.AddProvider(NullLoggerProvider.Instance)); + services.AddSingleton(Probe.Instance); + services.Configure(_ => { }); + services.Configure(_ => { }); + services.Configure(o => o.Roles = ["admin", "driver", "cluster-MAIN"]); + services.AddSingleton(EmptyDataPlaneAuthoritySource.Instance); + services.AddSingleton(); + services.AddSingleton(); + + if (hasAdmin) + { + services.AddDbContextFactory( + o => o.UseInMemoryDatabase($"apikey-reg-{Guid.NewGuid():N}")); + } + + services.AddOtOpcUaDataPlaneAuth(hasAdmin, hasDriver); + return services.BuildServiceProvider(); + } + + /// A clock distinguishable from by reference. + private sealed class Probe : TimeProvider + { + public static readonly Probe Instance = new(); + + /// + public override DateTimeOffset GetUtcNow() => new(2026, 8, 20, 12, 0, 0, TimeSpan.Zero); + } +} diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.Host.Tests/OpcUa/BoundedApiKeyStoreTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.Host.Tests/OpcUa/BoundedApiKeyStoreTests.cs new file mode 100644 index 00000000..9a4357dc --- /dev/null +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.Host.Tests/OpcUa/BoundedApiKeyStoreTests.cs @@ -0,0 +1,533 @@ +using System.Diagnostics.Metrics; +using Microsoft.Extensions.Logging.Abstractions; +using Microsoft.Extensions.Options; +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.Commons.Observability; +using ZB.MOM.WW.OtOpcUa.Configuration.Entities; +using ZB.MOM.WW.OtOpcUa.Core.Authorization; +using ZB.MOM.WW.OtOpcUa.Host.OpcUa; +using ZB.MOM.WW.OtOpcUa.OpcUaServer.Security; + +namespace ZB.MOM.WW.OtOpcUa.Host.Tests.OpcUa; + +/// +/// Covers — the deadline and TTL cache the OPC UA impersonation +/// callback's key lookup runs under (arch-review 03/S18). +/// +/// +/// +/// The deadline cases are driven through the real +/// rather than the store directly, because the fail-closed conversion — a throw becoming the +/// standard opaque denial — is part of what is being asserted, and it lives in the authenticator's +/// catch-all rather than in the decorator. +/// +/// +/// ⚠️ Deadline cases use a REAL short timeout; TTL cases use the fake clock. The hand-rolled +/// overrides GetUtcNow() only, so CreateTimer falls through to +/// the base implementation and produces a real system timer — advancing the fake clock does not +/// fire a CancelAfter. A deadline test "improved" to advance the fake clock would pass for +/// the wrong reason: the inner task simply never completing. +/// +/// +public sealed class BoundedApiKeyStoreTests +{ + private const string Pepper = "unit-test-pepper"; + private const string Principal = "scadabridge-dcl"; + + /// + /// A store that never answers must not hold the activation open: the deadline fires and the + /// activation is denied. + /// + /// + /// The SDK raises impersonation under a SessionManager-wide lock, so a lookup that does not + /// return stalls every session activation in the server — not just this one. That is the whole of + /// 03/S18, and it is why the bound is a deadline rather than a cache. + /// + [Fact] + public async Task A_blocked_store_denies_without_hanging_the_activation() + { + var (presented, inner) = SetUp(); + inner.BlockForever = true; + var auth = Auth(Bounded(inner, timeoutMs: 150, cacheSeconds: 0)); + + var result = await auth.AuthenticateUserNameAsync("apikey", presented, CancellationToken.None) + .WaitAsync(TimeSpan.FromSeconds(5)); + + result.Success.ShouldBeFalse(); + result.Error.ShouldBe(ApiKeyOpcUaUserAuthenticator.DenyMessage); + } + + /// + /// The positive control for : + /// the same fake, answering normally, still authenticates. + /// + /// + /// Without it a build whose decorator denied everything would pass every deadline case in + /// this class, and the bound would look correct while having locked the fleet out. + /// + [Fact] + public async Task A_reachable_store_still_authenticates() + { + var (presented, inner) = SetUp(); + var auth = Auth(Bounded(inner, timeoutMs: 150, cacheSeconds: 0)); + + var result = await auth.AuthenticateUserNameAsync("apikey", presented, CancellationToken.None); + + result.Success.ShouldBeTrue(); + result.Principal.ShouldBe(Principal); + } + + /// + /// A lookup that completes with a perfectly valid key after the deadline still denies — + /// nothing races back in past the bound. + /// + [Fact] + public async Task A_deadline_never_admits() + { + var (presented, inner) = SetUp(); + var gate = new TaskCompletionSource(); + inner.Gate = gate.Task; + var auth = Auth(Bounded(inner, timeoutMs: 100, cacheSeconds: 0)); + + var pending = auth.AuthenticateUserNameAsync("apikey", presented, CancellationToken.None); + + // Let the deadline pass, THEN hand the inner store its (valid) answer. + await Task.Delay(400); + gate.SetResult(); + + var result = await pending.WaitAsync(TimeSpan.FromSeconds(5)); + result.Success.ShouldBeFalse(); + result.Error.ShouldBe(ApiKeyOpcUaUserAuthenticator.DenyMessage); + } + + /// + /// A non-positive StoreTimeoutMs disables the deadline, restoring the unbounded behaviour + /// byte-for-byte — the documented rollback. + /// + [Fact] + public async Task A_zero_timeout_disables_the_deadline() + { + var (presented, inner) = SetUp(); + var gate = new TaskCompletionSource(); + inner.Gate = gate.Task; + var auth = Auth(Bounded(inner, timeoutMs: 0, cacheSeconds: 0)); + + var pending = auth.AuthenticateUserNameAsync("apikey", presented, CancellationToken.None); + + // Far longer than any deadline this class configures; an active bound would have denied by now. + await Task.Delay(300); + pending.IsCompleted.ShouldBeFalse("a disabled deadline must not cancel a slow lookup"); + + gate.SetResult(); + (await pending.WaitAsync(TimeSpan.FromSeconds(5))).Success.ShouldBeTrue(); + } + + /// The anonymous activation path gets the same deadline, and the same denial. + /// + /// It runs on every unauthenticated connect, so leaving it unbounded would leave the + /// server-wide-stall hazard fully open on the busiest activation path. + /// + [Fact] + public async Task Anonymous_grant_resolution_is_bounded_too() + { + var (_, inner) = SetUp(); + inner.BlockForever = true; + var auth = Auth(Bounded(inner, timeoutMs: 150, cacheSeconds: 0)); + + var result = await auth.AuthenticateAnonymousAsync(CancellationToken.None) + .WaitAsync(TimeSpan.FromSeconds(5)); + + result.Success.ShouldBeFalse(); + } + + // ---------- TTL read-through cache ---------- + + /// A second activation inside the TTL is served from the cache and never reaches the store. + [Fact] + public async Task A_second_activation_within_the_TTL_does_not_reach_the_store() + { + var (_, inner) = SetUp(); + var clock = new FakeClock(new DateTimeOffset(2026, 8, 20, 12, 0, 0, TimeSpan.Zero)); + var sut = Bounded(inner, timeoutMs: 0, cacheSeconds: 5, clock); + + (await sut.FindAsync(inner.KeyId, clock.GetUtcNow().UtcDateTime, default)).ShouldNotBeNull(); + clock.Advance(TimeSpan.FromSeconds(2)); + (await sut.FindAsync(inner.KeyId, clock.GetUtcNow().UtcDateTime, default)).ShouldNotBeNull(); + + inner.Calls.ShouldBe(1); + } + + /// + /// FALSIFICATION CONTROL. A key revoked in the store is denied once the TTL has passed — the + /// cache expires rather than merely existing. + /// + /// + /// A cache with no expiry at all passes every other case in this class: the hit test, the + /// read-through test and the fault test are all satisfied by "store once, serve forever". This is + /// the one that makes StoreCacheSeconds a bound rather than a claim. + /// + [Fact] + public async Task A_revoked_key_is_denied_once_the_TTL_has_passed() + { + var (_, inner) = SetUp(); + var clock = new FakeClock(new DateTimeOffset(2026, 8, 20, 12, 0, 0, TimeSpan.Zero)); + var sut = Bounded(inner, timeoutMs: 0, cacheSeconds: 5, clock); + + (await sut.FindAsync(inner.KeyId, clock.GetUtcNow().UtcDateTime, default)) + .ShouldNotBeNull("non-vacuity: the key really was usable before the revocation"); + + inner.Revoked = true; + + // Still inside the TTL — the documented, accepted staleness window. + clock.Advance(TimeSpan.FromSeconds(3)); + (await sut.FindAsync(inner.KeyId, clock.GetUtcNow().UtcDateTime, default)).ShouldNotBeNull(); + + // Past it — the store is re-read and the revocation lands. + clock.Advance(TimeSpan.FromSeconds(3)); + (await sut.FindAsync(inner.KeyId, clock.GetUtcNow().UtcDateTime, default)).ShouldBeNull(); + inner.Calls.ShouldBe(2); + } + + /// A non-positive TTL disables the cache: every call reads through, today's behaviour. + [Fact] + public async Task TTL_zero_reads_through_every_time() + { + var (_, inner) = SetUp(); + var clock = new FakeClock(new DateTimeOffset(2026, 8, 20, 12, 0, 0, TimeSpan.Zero)); + var sut = Bounded(inner, timeoutMs: 0, cacheSeconds: 0, clock); + + for (var i = 0; i < 3; i++) + await sut.FindAsync(inner.KeyId, clock.GetUtcNow().UtcDateTime, default); + + inner.Calls.ShouldBe(3); + } + + /// + /// A fault is never cached: the next call reaches the store, so an outage cannot outlive itself. + /// + /// + /// Caching a deny would be fail-closed and is still wrong — it would go on denying valid keys for + /// a further TTL after the database came back. + /// + [Fact] + public async Task A_faulted_store_is_not_cached() + { + var (_, inner) = SetUp(); + var clock = new FakeClock(new DateTimeOffset(2026, 8, 20, 12, 0, 0, TimeSpan.Zero)); + var sut = Bounded(inner, timeoutMs: 0, cacheSeconds: 60, clock); + inner.ThrowsRemaining = 1; + + await Should.ThrowAsync( + () => sut.FindAsync(inner.KeyId, clock.GetUtcNow().UtcDateTime, default)); + + // Same instant — a cached fault would still be "fresh" here, so the clock cannot rescue this. + (await sut.FindAsync(inner.KeyId, clock.GetUtcNow().UtcDateTime, default)).ShouldNotBeNull(); + inner.Calls.ShouldBe(2); + } + + /// The anonymous grant lookup is cached on the same terms, keyed by principal. + [Fact] + public async Task Anonymous_grants_are_cached_too() + { + var (_, inner) = SetUp(); + var clock = new FakeClock(new DateTimeOffset(2026, 8, 20, 12, 0, 0, TimeSpan.Zero)); + var sut = Bounded(inner, timeoutMs: 0, cacheSeconds: 5, clock); + + await sut.FindGrantsAsync(OpcUaPrincipals.Anonymous, default); + await sut.FindGrantsAsync(OpcUaPrincipals.Anonymous, default); + inner.Calls.ShouldBe(1); + + clock.Advance(TimeSpan.FromSeconds(6)); + await sut.FindGrantsAsync(OpcUaPrincipals.Anonymous, default); + inner.Calls.ShouldBe(2); + } + + /// + /// Past the entry ceiling the cache clears wholesale and keeps answering — a key-id guessing flood + /// is a bounded memory event, not an unbounded one. + /// + /// + /// Wholesale rather than per-entry eviction for the reason SessionAclDecisionCache already + /// documents: choosing WHICH entry to drop needs knowledge the cache does not have, and a + /// repopulate is cheap and cannot be wrong. + /// + [Fact] + public async Task Cache_overflow_clears_wholesale_and_keeps_serving() + { + var (_, inner) = SetUp(); + var clock = new FakeClock(new DateTimeOffset(2026, 8, 20, 12, 0, 0, TimeSpan.Zero)); + var sut = Bounded(inner, timeoutMs: 0, cacheSeconds: 600, clock); + + // Seed the real key, then flood past the ceiling with ids that do not exist. + (await sut.FindAsync(inner.KeyId, clock.GetUtcNow().UtcDateTime, default)).ShouldNotBeNull(); + for (var i = 0; i < BoundedApiKeyStore.MaxCacheEntries + 1; i++) + await sut.FindAsync($"ghost-{i}", clock.GetUtcNow().UtcDateTime, default); + + var callsAfterFlood = inner.Calls; + + // Still correct after the clear — and the real key was evicted, so it is re-read rather than + // served from a cache the flood was supposed to have emptied. + (await sut.FindAsync(inner.KeyId, clock.GetUtcNow().UtcDateTime, default)).ShouldNotBeNull(); + inner.Calls.ShouldBe(callsAfterFlood + 1); + } + + /// + /// A cached key whose ExpiresUtc passes inside the TTL is not served: the store is + /// re-read instead. + /// + /// + /// + /// Without this the TTL would resurrect an expired key for up to StoreCacheSeconds — + /// the exact axis 03/S19 exists to close, re-opened by 03/S18's own cache. The cache key is + /// apiKeyId alone (utcNow changes every call and would defeat the cache), so the + /// clock check has to happen on the way OUT of the cache. + /// + /// + /// The inner store is flipped to Revoked first, so "served from cache" and "re-read" + /// give different ANSWERS rather than only different call counts — a build that re-read and + /// then ignored the result could not pass. + /// + /// + [Fact] + public async Task A_cached_key_that_expires_by_the_clock_is_not_served() + { + var start = new DateTimeOffset(2026, 8, 20, 12, 0, 0, TimeSpan.Zero); + var (_, inner) = SetUp(expiresUtc: start.UtcDateTime.AddSeconds(2)); + var clock = new FakeClock(start); + var sut = Bounded(inner, timeoutMs: 0, cacheSeconds: 60, clock); + + (await sut.FindAsync(inner.KeyId, clock.GetUtcNow().UtcDateTime, default)) + .ShouldNotBeNull("non-vacuity: the key was usable and IS now cached"); + + inner.Revoked = true; + + // Past ExpiresUtc (+2 s) but far inside the 60 s TTL — the entry is still "fresh" to the cache. + clock.Advance(TimeSpan.FromSeconds(5)); + + (await sut.FindAsync(inner.KeyId, clock.GetUtcNow().UtcDateTime, default)).ShouldBeNull(); + inner.Calls.ShouldBe(2, "the expired entry must be re-read, not served"); + } + + // ---------- metering ---------- + + /// A deadline that fires meters outcome=timeout — the alarm signal for a sick ConfigDb. + [Fact] + public async Task A_timeout_meters_outcome_timeout() + { + var (_, inner) = SetUp(); + inner.BlockForever = true; + using var meter = new MeterRecorder(); + var sut = Bounded(inner, timeoutMs: 100, cacheSeconds: 0); + + await Should.ThrowAsync( + () => sut.FindAsync(inner.KeyId, DateTime.UtcNow, default).WaitAsync(TimeSpan.FromSeconds(5))); + + meter.WithOutcome("timeout").ShouldBe(1); + meter.WithOutcome("cache_hit").ShouldBe(0); + } + + /// + /// A cache hit meters outcome=cache_hit — the ratio that tells an operator the TTL is + /// doing something. A missing stream on a busy fused central is how you notice + /// StoreCacheSeconds was set to 0 by accident. + /// + [Fact] + public async Task A_cache_hit_meters_outcome_cache_hit() + { + var (_, inner) = SetUp(); + var clock = new FakeClock(new DateTimeOffset(2026, 8, 20, 12, 0, 0, TimeSpan.Zero)); + using var meter = new MeterRecorder(); + var sut = Bounded(inner, timeoutMs: 0, cacheSeconds: 5, clock); + + await sut.FindAsync(inner.KeyId, clock.GetUtcNow().UtcDateTime, default); + meter.WithOutcome("cache_hit").ShouldBe(0, "the first call is a miss"); + + await sut.FindAsync(inner.KeyId, clock.GetUtcNow().UtcDateTime, default); + meter.WithOutcome("cache_hit").ShouldBe(1); + } + + /// A plain read-through — no deadline hit, no cache hit — meters nothing at all. + /// + /// The counter names lookups the bound INTERVENED on. Metering every lookup would bury the two + /// outcomes that matter under the ordinary traffic they have to be spotted in. + /// + [Fact] + public async Task A_plain_read_through_meters_neither() + { + var (_, inner) = SetUp(); + using var meter = new MeterRecorder(); + var sut = Bounded(inner, timeoutMs: 2000, cacheSeconds: 0); + + await sut.FindAsync(inner.KeyId, DateTime.UtcNow, default); + + meter.Total.ShouldBe(0); + } + + // ---------- helpers ---------- + + private static BoundedApiKeyStore Bounded( + IApiKeyStore inner, int timeoutMs, int cacheSeconds, TimeProvider? clock = null) => new( + inner, + Options.Create(new ApiKeyOptions + { + Username = "apikey", + Pepper = Pepper, + StoreTimeoutMs = timeoutMs, + StoreCacheSeconds = cacheSeconds, + }), + NullLogger.Instance, + clock ?? TimeProvider.System); + + private static ApiKeyOpcUaUserAuthenticator Auth(IApiKeyStore store) => new( + store, + Options.Create(new ApiKeyOptions { Username = "apikey", Pepper = Pepper }), + NullLogger.Instance); + + private static (string Presented, GatedApiKeyStore Store) SetUp(DateTime? expiresUtc = null) + { + var (presented, apiKeyId, hash) = ApiKeySecret.Generate(Pepper); + var key = new ApiKey + { + ApiKeyId = apiKeyId, + Name = "test key", + SecretHash = hash, + Principal = Principal, + Enabled = true, + CreatedUtc = DateTime.UtcNow, + ExpiresUtc = expiresUtc, + }; + return (presented, new GatedApiKeyStore(key)); + } + + /// + /// An inner store whose answer can be withheld — a hang the EF in-memory provider cannot + /// represent — and whose calls are counted. + /// + private sealed class GatedApiKeyStore(ApiKey key) : IApiKeyStore + { + /// The id of the key this store holds. + public string KeyId => key.ApiKeyId; + + /// When set, every call awaits this before answering. + public Task? Gate { get; set; } + + /// When true, no call ever completes. + public bool BlockForever { get; set; } + + /// How many calls reached this store (i.e. were not served from the cache). + public int Calls => _calls; + + /// When true, the key lookup answers "no usable key" — a revocation. + public bool Revoked { get; set; } + + /// Number of leading calls that throw before the store starts answering. + public int ThrowsRemaining { get; set; } + + private int _calls; + + public async Task FindAsync(string apiKeyId, DateTime utcNow, CancellationToken ct) + { + await WaitAsync(ct).ConfigureAwait(false); + if (Revoked) return null; + return string.Equals(apiKeyId, key.ApiKeyId, StringComparison.Ordinal) + ? new ApiKeyLookup(key, []) + : null; + } + + public async Task> FindGrantsAsync(string principal, CancellationToken ct) + { + await WaitAsync(ct).ConfigureAwait(false); + return []; + } + + private async Task WaitAsync(CancellationToken ct) + { + Interlocked.Increment(ref _calls); + + if (ThrowsRemaining > 0) + { + ThrowsRemaining--; + throw new InvalidOperationException("store is down"); + } + + if (BlockForever) await Task.Delay(Timeout.Infinite, ct).ConfigureAwait(false); + if (Gate is { } gate) await gate.WaitAsync(ct).ConfigureAwait(false); + } + } + + /// Collects otopcua.opcua.apikey_store.bounded measurements by their outcome tag. + /// + /// ⚠️ Filtered by instrument name only, which is normally the metrics-test leak (a listener sees + /// a PARALLEL test's measurements too). It is safe HERE for two checkable reasons: + /// is the only thing in the product that writes this instrument, + /// and this assembly declares CollectionBehavior(DisableTestParallelization = true) in + /// AssemblyInfo.cs, so there is no parallel test to leak from. Removing that attribute + /// means adding a discriminating tag here. + /// + private sealed class MeterRecorder : IDisposable + { + private readonly MeterListener _listener; + private readonly List[]> _tagSets = []; + private readonly Lock _gate = new(); + private long _total; + + public MeterRecorder() + { + _listener = new MeterListener + { + InstrumentPublished = (instrument, listener) => + { + if (instrument.Meter.Name == OtOpcUaTelemetry.MeterName + && instrument.Name == "otopcua.opcua.apikey_store.bounded") + { + listener.EnableMeasurementEvents(instrument); + } + }, + }; + _listener.SetMeasurementEventCallback((_, value, tags, _) => + { + lock (_gate) + { + _total += value; + _tagSets.Add(tags.ToArray()); + } + }); + _listener.Start(); + } + + /// Every measurement seen, regardless of tags. + public long Total + { + get { lock (_gate) return _total; } + } + + /// How many measurements carried outcome=. + /// The outcome tag value to count. + /// The measurement count. + public int WithOutcome(string outcome) + { + lock (_gate) + return _tagSets.Count(set => set.Any(t => t.Key == "outcome" && Equals(t.Value, outcome))); + } + + /// + public void Dispose() => _listener.Dispose(); + } + + /// + /// A hand-advanced clock. Overrides GetUtcNow() only — see the class remarks for why that + /// confines it to the TTL cases. + /// + private sealed class FakeClock(DateTimeOffset utcNow) : TimeProvider + { + private DateTimeOffset _now = utcNow; + + /// + public override DateTimeOffset GetUtcNow() => _now; + + /// Moves the clock forward. + /// How far forward to move. + public void Advance(TimeSpan by) => _now = _now.Add(by); + } +} diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.Host.Tests/OpcUa/OtOpcUaServerHostedServiceRevalidationTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.Host.Tests/OpcUa/OtOpcUaServerHostedServiceRevalidationTests.cs new file mode 100644 index 00000000..96c989f8 --- /dev/null +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.Host.Tests/OpcUa/OtOpcUaServerHostedServiceRevalidationTests.cs @@ -0,0 +1,144 @@ +using Akka.Actor; +using Akka.Hosting; +using Microsoft.Extensions.Configuration; +using Microsoft.Extensions.Logging.Abstractions; +using Microsoft.Extensions.Options; +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.Commons.OpcUa; +using ZB.MOM.WW.OtOpcUa.Core.Abstractions; +using ZB.MOM.WW.OtOpcUa.Core.Authorization; +using ZB.MOM.WW.OtOpcUa.Host.OpcUa; +using ZB.MOM.WW.OtOpcUa.OpcUaServer; +using ZB.MOM.WW.OtOpcUa.OpcUaServer.Security; + +namespace ZB.MOM.WW.OtOpcUa.Host.Tests.OpcUa; + +/// +/// Pins the ACL revalidation timer's lifecycle on the hosted service (arch-review 03/S19) — that it is +/// armed at the configured interval, that 0 arms nothing, and that stopping disposes it. +/// +/// +/// The timer is armed through rather +/// than by running StartAsync, because StartAsync boots the real OPC UA SDK (certificate +/// store, listening socket) and none of that is what this is about. Everything the timer's lifecycle +/// touches — the options, the clock, the disposal — is exercised exactly as production reaches it. +/// +public sealed class OtOpcUaServerHostedServiceRevalidationTests +{ + /// The configured interval is what the timer is armed at. + [Fact] + public void A_configured_interval_arms_one_timer_at_that_period() + { + var clock = new RecordingTimeProvider(); + var sut = Service(clock, intervalSeconds: 30); + + sut.StartRevalidationTimer(); + + clock.Periods.ShouldBe([TimeSpan.FromSeconds(30)]); + } + + /// + /// RevalidationIntervalSeconds = 0 arms nothing — the documented rollback to + /// deploy-triggered-only revalidation. + /// + [Fact] + public void A_zero_interval_arms_nothing() + { + var clock = new RecordingTimeProvider(); + var sut = Service(clock, intervalSeconds: 0); + + sut.StartRevalidationTimer(); + + clock.Periods.ShouldBeEmpty(); + } + + /// + /// StopAsync disposes the timer, so no tick can land on a node manager that is being torn + /// down. + /// + /// + /// The disposal is the FIRST thing StopAsync does, ahead of + /// SetDataPlaneAuthority(null) / SetDataPlaneAuthorizer(null) — a stopping node sheds + /// the trigger before it sheds the thing the trigger drives. + /// + [Fact] + public async Task StopAsync_disposes_the_timer() + { + var clock = new RecordingTimeProvider(); + var sut = Service(clock, intervalSeconds: 30); + sut.StartRevalidationTimer(); + + clock.Timers[0].Disposed.ShouldBeFalse("non-vacuity: the timer is live before the stop"); + + await sut.StopAsync(CancellationToken.None); + + clock.Timers[0].Disposed.ShouldBeTrue(); + } + + /// Disposing the service disposes the timer too, for a host that never called StopAsync. + [Fact] + public async Task DisposeAsync_disposes_the_timer() + { + var clock = new RecordingTimeProvider(); + var sut = Service(clock, intervalSeconds: 30); + sut.StartRevalidationTimer(); + + await sut.DisposeAsync(); + + clock.Timers[0].Disposed.ShouldBeTrue(); + } + + // ---------- helpers ---------- + + private static OtOpcUaServerHostedService Service(TimeProvider clock, int intervalSeconds) => new( + Options.Create(new OpcUaApplicationHostOptions()), + new DeferredAddressSpaceSink(), + new DeferredServiceLevelPublisher(), + NullOpcUaUserAuthenticator.Instance, + NullHistorianDataSource.Instance, + EmptyDataPlaneAuthoritySource.Instance, + new ConfigurationBuilder().Build(), + () => throw new NotSupportedException("no ActorSystem is needed for the timer lifecycle"), + new ActorRegistry(), + dataPlaneAuthorizer: null, + Options.Create(new AclOptions { RevalidationIntervalSeconds = intervalSeconds }), + clock, + NullLoggerFactory.Instance); + + /// Records the timers asked of it instead of arming real ones. + private sealed class RecordingTimeProvider : TimeProvider + { + /// Every timer this provider was asked to create, in order. + public List Timers { get; } = []; + + /// The period each of those timers was armed at. + public IReadOnlyList Periods => [.. Timers.Select(t => t.Period)]; + + /// + public override ITimer CreateTimer(TimerCallback callback, object? state, TimeSpan dueTime, TimeSpan period) + { + var timer = new FakeTimer(period); + Timers.Add(timer); + return timer; + } + + /// A timer that remembers its period and whether it was disposed. + internal sealed class FakeTimer(TimeSpan period) : ITimer + { + public TimeSpan Period { get; } = period; + + public bool Disposed { get; private set; } + + public bool Change(TimeSpan dueTime, TimeSpan p) => true; + + public void Dispose() => Disposed = true; + + public ValueTask DisposeAsync() + { + Disposed = true; + return ValueTask.CompletedTask; + } + } + } +} diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.IntegrationTests/AclKeyExpiryEnforcementTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.IntegrationTests/AclKeyExpiryEnforcementTests.cs new file mode 100644 index 00000000..cb97261d --- /dev/null +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.IntegrationTests/AclKeyExpiryEnforcementTests.cs @@ -0,0 +1,370 @@ +using System.Net; +using System.Net.Sockets; +using System.Text; +using Microsoft.Extensions.Logging.Abstractions; +using Opc.Ua; +using Opc.Ua.Client; +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.Commons.OpcUa; +using ZB.MOM.WW.OtOpcUa.Configuration.Entities; +using ZB.MOM.WW.OtOpcUa.Configuration.Enums; +using ZB.MOM.WW.OtOpcUa.Core.Authorization; +using ZB.MOM.WW.OtOpcUa.Host.OpcUa; +using ZB.MOM.WW.OtOpcUa.OpcUaServer.Security; + +namespace ZB.MOM.WW.OtOpcUa.OpcUaServer.IntegrationTests; + +/// +/// arch-review 03/S19 over the wire — a key that expires by the CLOCK stops a live subscription, with +/// no deployment anywhere in the run. +/// +/// +/// +/// This cannot be reasoned into existence for the same reason +/// AclSubscriptionEnforcementTests gives: whether a monitored item can recover the identity +/// of the session that created it is an assumption about SDK behaviour. Here it must recover an +/// identity carrying an API key id — the field the per-call expiry re-check keys on — and +/// the existing rig has never exercised one, because it connects anonymously. +/// +/// +/// The tick is driven through the real rather than by calling +/// the node manager's pass directly, so what is asserted here is the whole T8 path: revert the +/// host's timer and the denial below cannot be produced by anything except a deployment. +/// +/// +public sealed class AclKeyExpiryEnforcementTests +{ + private const string Cluster = "SITE-A"; + private const string Device = "Plant/Modbus/dev1"; + private const string Tag = "Plant/Modbus/dev1/Speed"; + private const string Principal = "expiry-probe"; + private const string KeyId = "k-expiry"; + + private static readonly DateTime Start = new(2026, 8, 20, 12, 0, 0, DateTimeKind.Utc); + + /// + /// THE test this file exists for: the key's ExpiresUtc passes, a revalidation tick runs, and + /// the client is told BadUserAccessDenied — with DataPlaneAuthorityState.Apply called + /// exactly once, in setup. + /// + [Fact] + public async Task An_expired_key_stops_a_live_subscription_with_no_deploy() + { + var ct = TestContext.Current.CancellationToken; + var rig = await Rig.StartAsync("AclKeyExpiry", ct); + try + { + rig.Enforce(expiresUtc: Start.AddMinutes(10)); + rig.Materialise(); + rig.Write(1); + + using var session = await rig.ConnectAsync(ct); + var (subscription, received) = await rig.SubscribeAsync(session, ct); + + // Baseline: the key-carrying session's feed is live. + await WaitUntilAsync(() => received.Any(v => Equals(v.Value, 1f)), TimeSpan.FromSeconds(10)); + received.Clear(); + + // Only the clock moves. No deployment, no new generation, no grant change. + rig.Advance(TimeSpan.FromMinutes(20)); + rig.TickRevalidation(); + + await WaitUntilAsync( + () => received.Any(v => v.StatusCode.Code == StatusCodes.BadUserAccessDenied), + TimeSpan.FromSeconds(10)); + + // And the data really has stopped. + received.Clear(); + rig.Write(2); + await Task.Delay(TimeSpan.FromSeconds(2), ct); + received.ShouldNotContain(v => Equals(v.Value, 2f)); + + await subscription.DeleteAsync(true, ct); + } + finally { await rig.DisposeAsync(); } + } + + /// + /// The negative control: identical run, clock advanced but not past ExpiresUtc ⇒ values keep + /// arriving. + /// + /// + /// Without it the test above also passes on a build whose tick detaches every item it sees, which + /// would silence every subscription in the fleet once a minute. + /// + [Fact] + public async Task An_unexpired_key_keeps_streaming_across_a_tick() + { + var ct = TestContext.Current.CancellationToken; + var rig = await Rig.StartAsync("AclKeyLive", ct); + try + { + rig.Enforce(expiresUtc: Start.AddHours(4)); + rig.Materialise(); + rig.Write(1); + + using var session = await rig.ConnectAsync(ct); + var (subscription, received) = await rig.SubscribeAsync(session, ct); + await WaitUntilAsync(() => received.Any(v => Equals(v.Value, 1f)), TimeSpan.FromSeconds(10)); + + received.Clear(); + rig.Advance(TimeSpan.FromMinutes(20)); + rig.TickRevalidation(); + + rig.Write(2); + await WaitUntilAsync(() => received.Any(v => Equals(v.Value, 2f)), TimeSpan.FromSeconds(10)); + received.ShouldNotContain(v => v.StatusCode.Code == StatusCodes.BadUserAccessDenied); + + await subscription.DeleteAsync(true, ct); + } + finally { await rig.DisposeAsync(); } + } + + // ───────────────────────────── helpers ───────────────────────────── + + private static readonly IReadOnlyDictionary> NoScopes = + new Dictionary>(); + + private static async Task WaitUntilAsync(Func condition, TimeSpan timeout) + { + var deadline = DateTime.UtcNow + timeout; + while (DateTime.UtcNow < deadline) + { + if (condition()) return; + await Task.Delay(50); + } + condition().ShouldBeTrue("condition not met within timeout"); + } + + /// A booted server, a key-carrying client session, and a hand-driven revalidation tick. + private sealed class Rig : IAsyncDisposable + { + private readonly string _pkiRoot; + private readonly FakeClock _clock = new(Start); + private OpcUaApplicationHost _host = null!; + private OtOpcUaSdkServer _server = null!; + private AclRevalidationTimer? _timer; + private int _port; + + private Rig(string pkiRoot) => _pkiRoot = pkiRoot; + + private OtOpcUaNodeManager Nm => _server.NodeManager!; + + public static async Task StartAsync(string name, CancellationToken ct) + { + var rig = new Rig(Path.Combine(Path.GetTempPath(), $"otopcua-{name}-{Guid.NewGuid():N}")); + rig._port = AllocateFreePort(); + rig._server = new OtOpcUaSdkServer(); + rig._host = new OpcUaApplicationHost( + new OpcUaApplicationHostOptions + { + ApplicationName = $"OtOpcUa.{name}", + ApplicationUri = $"urn:OtOpcUa.{name}.{Guid.NewGuid():N}", + OpcUaPort = rig._port, + PublicHostname = "127.0.0.1", + PkiStoreRoot = rig._pkiRoot, + EnabledSecurityProfiles = [OpcUaSecurityProfile.None], + AutoAcceptUntrustedClientCertificates = true, + }, + NullLogger.Instance, + new KeyPresentingAuthenticator()); + + await rig._host.StartAsync(rig._server, ct); + return rig; + } + + /// + /// Switches enforcement on with one cluster-wide grant and one key expiring at + /// , and arms the revalidation timer over the same clock. + /// + /// When the key stops being usable. + public void Enforce(DateTime expiresUtc) + { + var key = new ApiKey + { + ApiKeyId = KeyId, + Name = KeyId, + SecretHash = "hash", + Principal = Principal, + Enabled = true, + CreatedUtc = Start.AddDays(-1), + ExpiresUtc = expiresUtc, + }; + var grant = new NodeAcl + { + NodeAclId = "acl-cluster", + ClusterId = Cluster, + Principal = Principal, + ScopeKind = NodeAclScopeKind.Cluster, + PermissionFlags = NodePermissions.Read | NodePermissions.Browse | NodePermissions.Subscribe, + }; + + var state = new DataPlaneAuthorityState(); + + // The ONLY Apply in the whole test. Everything after this moves the clock. + state.Apply(Cluster, "rev-1", [key], [grant], NoScopes); + + Nm.DataPlaneAuthority = state; + Nm.Authorizer = new DataPlaneAuthorizer( + state, new TriePermissionEvaluator(state.Tries), enabled: true, NullLogger.Instance, _clock); + + // A long interval: the tick is driven by hand, so a real timer firing underneath would only + // make the test non-deterministic. What matters is that the pass is reached through the same + // component the hosted service arms. + _timer = new AclRevalidationTimer( + () => _server.NodeManager?.RevalidateMonitoredItems(), + TimeSpan.FromHours(1), + TimeProvider.System, + NullLogger.Instance); + } + + /// Moves the authorizer's clock forward — the only thing that changes mid-test. + /// How far forward. + public void Advance(TimeSpan by) => _clock.Advance(by); + + /// Runs one revalidation tick through the host's own timer component. + public void TickRevalidation() => _timer!.Tick(); + + public void Materialise() + { + Nm.EnsureFolder("Plant", null, "Plant", AddressSpaceRealm.Raw); + Nm.EnsureFolder("Plant/Modbus", "Plant", "Modbus", AddressSpaceRealm.Raw); + Nm.EnsureFolder(Device, "Plant/Modbus", "dev1", AddressSpaceRealm.Raw); + Nm.EnsureVariable(Tag, Device, "Speed", "Float", writable: false, AddressSpaceRealm.Raw); + } + + public void Write(float value) => + Nm.WriteValue(Tag, value, OpcUaStatus.Good, DateTime.UtcNow, AddressSpaceRealm.Raw); + + public ushort RawNamespace(ISession session) + { + var ns = (ushort)session.NamespaceUris.GetIndex(V3NodeIds.RawNamespaceUri); + ns.ShouldBeGreaterThan((ushort)0); + return ns; + } + + public NodeId TagNodeId(ISession session) => new(Tag, RawNamespace(session)); + + public async Task<(Subscription Subscription, List Received)> SubscribeAsync( + ISession session, CancellationToken ct) + { + var received = new List(); + var gate = new object(); + + var subscription = new Subscription(session.DefaultSubscription) { PublishingInterval = 100 }; + session.AddSubscription(subscription); + await subscription.CreateAsync(ct); + + var item = new MonitoredItem(subscription.DefaultItem) + { + StartNodeId = TagNodeId(session), + AttributeId = Attributes.Value, + SamplingInterval = 50, + QueueSize = 10, + }; + item.Notification += (mi, _) => + { + foreach (var v in mi.DequeueValues()) lock (gate) received.Add(v); + }; + subscription.AddItem(item); + await subscription.ApplyChangesAsync(ct); + + item.Status.Error.ShouldBeNull(); + return (subscription, received); + } + + /// Connects presenting the fixed apikey username with an otua_ credential. + /// The cancellation token. + /// The connected session. + public async Task ConnectAsync(CancellationToken ct) + { + var appConfig = new ApplicationConfiguration + { + ApplicationName = "OtOpcUa.AclKeyClient", + ApplicationUri = $"urn:OtOpcUa.AclKeyClient.{Guid.NewGuid():N}", + ApplicationType = ApplicationType.Client, + SecurityConfiguration = TestClientSecurity.Build(TestClientSecurity.AllocatePkiRoot()), + ClientConfiguration = new ClientConfiguration { DefaultSessionTimeout = 60_000 }, + }; + await appConfig.ValidateAsync(ApplicationType.Client, ct); + appConfig.CertificateValidator.CertificateValidation += (_, e) => e.Accept = true; + + var endpoint = await CoreClientUtils.SelectEndpointAsync( + appConfig, $"opc.tcp://127.0.0.1:{_port}/OtOpcUa", false, DefaultTelemetry.Create(_ => { }), ct); + + return await new DefaultSessionFactory(DefaultTelemetry.Create(_ => { })).CreateAsync( + appConfig, + new ConfiguredEndpoint(null, endpoint, EndpointConfiguration.Create(appConfig)), + updateBeforeConnect: false, + checkDomain: false, + sessionName: "AclKeyExpiryEnforcementTests", + sessionTimeout: 60_000, + identity: new UserIdentity(new UserNameIdentityToken + { + UserName = "apikey", + // 1.5.378 carries the UserName token password as raw bytes; the host decodes UTF-8. + DecryptedPassword = Encoding.UTF8.GetBytes($"otua_{KeyId}_secret"), + }), + preferredLocales: null, + ct: ct); + } + + public async ValueTask DisposeAsync() + { + _timer?.Dispose(); + await _host.DisposeAsync(); + if (Directory.Exists(_pkiRoot)) + { + try { Directory.Delete(_pkiRoot, recursive: true); } + catch (IOException) { /* best effort */ } + } + } + + private static int AllocateFreePort() + { + var listener = new TcpListener(IPAddress.Loopback, 0); + listener.Start(); + var port = ((IPEndPoint)listener.LocalEndpoint).Port; + listener.Stop(); + return port; + } + + /// + /// Admits the fixed apikey username as , carrying + /// . + /// + /// + /// Deliberately not the production ApiKeyOpcUaUserAuthenticator over a fake store: the + /// credential-verification path (parse, peppered HMAC, store lookup, deny shapes) is covered by + /// ApiKeyAuthenticationTests and BoundedApiKeyStoreTests in the Host suite, and + /// none of it changes what this file is about — whether the SDK hands a monitored item back an + /// identity that still carries the key id. Grants come from the deployed snapshot, so this + /// returns no roles. + /// + private sealed class KeyPresentingAuthenticator : IOpcUaUserAuthenticator + { + /// + public Task AuthenticateUserNameAsync( + string username, string password, CancellationToken ct) => + Task.FromResult( + string.Equals(username, "apikey", StringComparison.Ordinal) + && password.StartsWith($"otua_{KeyId}_", StringComparison.Ordinal) + ? OpcUaUserAuthResult.AllowKey(Principal, [], KeyId) + : OpcUaUserAuthResult.Deny("Invalid API key.")); + } + + /// A hand-advanced clock; the authorizer reads it on every decision. + private sealed class FakeClock(DateTime utcNow) : TimeProvider + { + private DateTimeOffset _now = new(utcNow, TimeSpan.Zero); + + /// + public override DateTimeOffset GetUtcNow() => _now; + + /// Moves the clock forward. + /// How far forward. + public void Advance(TimeSpan by) => _now = _now.Add(by); + } + } +} diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.IntegrationTests/ZB.MOM.WW.OtOpcUa.OpcUaServer.IntegrationTests.csproj b/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.IntegrationTests/ZB.MOM.WW.OtOpcUa.OpcUaServer.IntegrationTests.csproj index 65b05bdd..1ea5dbac 100644 --- a/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.IntegrationTests/ZB.MOM.WW.OtOpcUa.OpcUaServer.IntegrationTests.csproj +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.IntegrationTests/ZB.MOM.WW.OtOpcUa.OpcUaServer.IntegrationTests.csproj @@ -25,6 +25,10 @@ + + diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/NodeManagerStage6EnforcementTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/NodeManagerStage6EnforcementTests.cs index 6d74b601..e4d0b926 100644 --- a/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/NodeManagerStage6EnforcementTests.cs +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/NodeManagerStage6EnforcementTests.cs @@ -615,8 +615,156 @@ public sealed class NodeManagerStage6EnforcementTests : IDisposable finally { await host.DisposeAsync(); } } + /// + /// A key that expires by the CLOCK stands its live monitored item down, with no deployment + /// anywhere in the test (arch-review 03/S19). + /// + /// + /// + /// The first pass is the falsification control, and it is not optional: without it a build + /// whose pass detached everything it saw — unconditionally, or because the authorizer denied + /// for some unrelated reason — would pass the second half and prove nothing about expiry. + /// + /// + /// DataPlaneAuthorityState.Apply is called exactly once here, in setup. Nothing + /// advances the generation between the two passes, so the only thing that changed is the + /// clock — which is the whole claim. + /// + /// + [Fact] + public async Task An_expired_key_stands_down_a_live_item_with_no_deploy() + { + var (host, server) = await BootAsync(); + try + { + var nm = server.NodeManager!; + var clock = new FakeTimeProvider(new DateTime(2026, 8, 20, 12, 0, 0, DateTimeKind.Utc)); + nm.Authorizer = KeyedSubscribeAuthorizer( + clock, out _, expiresUtc: new DateTime(2026, 8, 20, 12, 10, 0, DateTimeKind.Utc)); + + var item = TrackItem(nm, Variable(nm, "Plant/Modbus/dev1/Speed"), KeyedIdentity()); + + nm.RevalidateMonitoredItems(); + item.Denials.ShouldBe(0, "the key is still valid, so nothing may stand down"); + + clock.Advance(TimeSpan.FromMinutes(20)); + nm.RevalidateMonitoredItems(); + + item.Denials.ShouldBe(1); + } + finally { await host.DisposeAsync(); } + } + + /// + /// A key renewed with a later expiry re-attaches its item on the next pass — the clock axis + /// inherits the deploy axis's reversibility. + /// + /// + /// The restore is asserted by standing the item down a SECOND time: the pass skips an item that is + /// already suppressed, so a second denial can only be queued if the item was genuinely restored + /// in between. That is why this pins the design decision not to close the session — a client that + /// renews its key recovers with no action of its own. + /// + [Fact] + public async Task A_renewed_key_re_attaches_the_item_on_the_next_pass() + { + var (host, server) = await BootAsync(); + try + { + var nm = server.NodeManager!; + var clock = new FakeTimeProvider(new DateTime(2026, 8, 20, 12, 0, 0, DateTimeKind.Utc)); + nm.Authorizer = KeyedSubscribeAuthorizer( + clock, out var state, expiresUtc: new DateTime(2026, 8, 20, 12, 10, 0, DateTimeKind.Utc)); + + var item = TrackItem(nm, Variable(nm, "Plant/Modbus/dev1/Speed"), KeyedIdentity()); + + clock.Advance(TimeSpan.FromMinutes(20)); + nm.RevalidateMonitoredItems(); + item.Denials.ShouldBe(1, "non-vacuity: the item really was stood down before the renewal"); + + // Renew: same key id, a later expiry. + state.Apply( + Cluster, "rev-2", + [Key(new DateTime(2026, 8, 20, 14, 0, 0, DateTimeKind.Utc))], + [GrantOn(NodePermissions.Subscribe)], + Scopes()); + nm.RevalidateMonitoredItems(); + + // Expire it again. A still-suppressed item is skipped, so a second denial proves the restore. + clock.Advance(TimeSpan.FromHours(3)); + nm.RevalidateMonitoredItems(); + + item.Denials.ShouldBe(2); + } + finally { await host.DisposeAsync(); } + } + + /// A clock that moves but does not reach the expiry stands nothing down. + [Fact] + public async Task An_unexpired_key_is_untouched_by_a_clock_only_pass() + { + var (host, server) = await BootAsync(); + try + { + var nm = server.NodeManager!; + var clock = new FakeTimeProvider(new DateTime(2026, 8, 20, 12, 0, 0, DateTimeKind.Utc)); + nm.Authorizer = KeyedSubscribeAuthorizer( + clock, out _, expiresUtc: new DateTime(2026, 8, 20, 13, 0, 0, DateTimeKind.Utc)); + + var item = TrackItem(nm, Variable(nm, "Plant/Modbus/dev1/Speed"), KeyedIdentity()); + + clock.Advance(TimeSpan.FromMinutes(30)); + nm.RevalidateMonitoredItems(); + + item.Denials.ShouldBe(0); + } + finally { await host.DisposeAsync(); } + } + // ───────────────────────────── helpers ───────────────────────────── + /// The public id of the key the clock-driven expiry cases authenticate with. + private const string ExpiringKeyId = "k-expiry"; + + /// + /// A cluster-wide Subscribe grant PLUS a key that expires at , so the + /// verdict moves with the clock rather than with a deployment. + /// + /// The authorizer's clock — the one Evaluate checks expiry against. + /// The authority state, so a test can renew the key without a second helper. + /// When the key stops being usable. + /// The gate, enforcing. + private static DataPlaneAuthorizer KeyedSubscribeAuthorizer( + TimeProvider clock, out DataPlaneAuthorityState state, DateTime expiresUtc) + { + state = new DataPlaneAuthorityState(); + state.Apply(Cluster, "rev-1", [Key(expiresUtc)], [GrantOn(NodePermissions.Subscribe)], Scopes()); + return new DataPlaneAuthorizer( + state, new TriePermissionEvaluator(state.Tries), enabled: true, NullLogger.Instance, clock); + } + + /// The expiring key, at whatever expiry a case needs. + /// When the key stops being usable. + /// The key row. + private static ApiKey Key(DateTime expiresUtc) => new() + { + ApiKeyId = ExpiringKeyId, + Name = ExpiringKeyId, + SecretHash = "hash", + Principal = Principal, + Enabled = true, + CreatedUtc = new DateTime(2026, 8, 20, 0, 0, 0, DateTimeKind.Utc), + ExpiresUtc = expiresUtc, + }; + + /// + /// An identity carrying the key id, which is what makes the per-call expiry re-check apply — an + /// identity with no ApiKeyId skips it entirely. + /// + /// The identity. + private static RoleCarryingUserIdentity KeyedIdentity() => + new(new UserNameIdentityToken { UserName = "apikey" }, [], Principal, ExpiringKeyId); + /// A cluster-wide Subscribe grant — what CreateMonitoredItems resolves to. private static DataPlaneAuthorizer SubscribeAuthorizer(TimeProvider clock) {