From a9b592aeb8b32a530a1a4076aff6fb2b44b149c9 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Thu, 20 Aug 2026 10:04:03 -0400 Subject: [PATCH] docs(security): correct revocation latency (activation vs open session) + S18/S19 knobs --- CLAUDE.md | 32 ++++++- .../plans/R3-05-acl-hot-path-expiry-plan.md | 19 ++++ ...-05-acl-hot-path-expiry-plan.md.tasks.json | 2 +- docs/Configuration.md | 3 + docs/security.md | 94 +++++++++++++++---- 5 files changed, 129 insertions(+), 21 deletions(-) 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 88487801..420f325c 100644 --- a/archreview/plans/R3-05-acl-hot-path-expiry-plan.md +++ b/archreview/plans/R3-05-acl-hot-path-expiry-plan.md @@ -730,3 +730,22 @@ for the orchestrator. 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. 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 1b459d39..46077638 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 @@ -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" 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