diff --git a/CLAUDE.md b/CLAUDE.md index fe799856..fe6087a6 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -610,15 +610,14 @@ the data-plane authenticator when you see LDAP wiring in `Program.cs` — it is Two things to know before relying on any of this: -- ⚠️ **Node ACLs are enforced only behind `Security:Acl:Enabled` (default `false`), and only for three - operations.** With the flag **off**, a key's grants collapse into the two server-wide role gates - (`WriteOperate` on Value writes, `AlarmAck` on Part 9 methods) and **only `Cluster`-scoped grants - count** — `ApiKeyInterimRoleMapper` deliberately under-grants, because honouring a narrower grant's - flags while unable to honour its *scope* would turn "WriteOperate on Equipment X" into server-wide - write authority. With the flag **on** (stage 5, `DataPlaneAuthorizer`), scoped grants decide **write, - alarm commands and HistoryRead** — the last of which had no authorization check of any kind before. - **Read, Browse and CreateMonitoredItems stay ungated in BOTH modes**, for any admitted session - including Anonymous; that is stage 6. +- ⚠️ **Node ACLs are enforced only behind `Security:Acl:Enabled` (default `false`).** With the flag + **off**, a key's grants collapse into the two server-wide role gates (`WriteOperate` on Value writes, + `AlarmAck` on Part 9 methods), **only `Cluster`-scoped grants count** — `ApiKeyInterimRoleMapper` + deliberately under-grants, because honouring a narrower grant's flags while unable to honour its + *scope* would turn "WriteOperate on Equipment X" into server-wide write authority — and **read, + browse, subscribe and HistoryRead are ungated for any admitted session including Anonymous**. With the + flag **on** (`DataPlaneAuthorizer`), scoped grants decide **every** data-plane operation: write and + alarm commands and HistoryRead (stage 5), read and browse and subscription creation (stage 6). - ⚠️ **The ACL decision REPLACES the role check — the two are never chained.** Chained, a grant would have to confer the server-wide role *as well as* pass the scope check, and only `Cluster`-scoped grants confer a role, so every narrow grant would stay inert and the flag would appear to do @@ -633,9 +632,28 @@ Two things to know before relying on any of this: *current* generation rather than the one the session activated under, and re-checks the authenticating key — so revoking a key or a grant stops an open session immediately instead of at the client's next reconnect. On a driver-only node the artifact is the only revocation channel. - - `UserAuthorizationState` is built **per call, not per session** — its freshness/staleness predicates - are LDAP-shaped and cannot fire here. Caching belongs at stage 6's publish-path seams, keyed - per-`(session, node)`. + - `UserAuthorizationState` is built **per call, not per session**, and its LDAP-shaped + freshness/staleness predicates are switched off EXPLICITLY (`AuthCacheMaxStaleness = MaxValue`). + ⚠️ Relying on "the stamp is always fresh" was not safe: `MembershipResolvedUtc` comes from + `DataPlaneAuthorizer`'s `TimeProvider` while `IsStale` is evaluated against + `TriePermissionEvaluator`'s, so **any divergence between the two clocks denies every request** with a + verdict indistinguishable from "no grant matched". Found by a test clock two hours ahead. + - **Stage 6's hot seams (read/browse/subscribe) go through a per-`(session, node, operation, realm)` + decision cache**, flushed when the generation advances. ⚠️ It caches the **grant walk only** — key + revocation and expiry are re-checked every call, because expiry moves with the clock rather than + with a deployment. + - **Browse needs ancestor visibility, and it must NOT be a synthesized grant on the ancestors.** A + grant deep in the tree is reached by browsing the nodes above it, and the trie inherits downward + only, so `PermissionTrieNode.BrowseTransitPrincipals` marks the path. Planting a real `Browse` grant + on each ancestor instead would be collected by the additive downward walk and leak `Browse` onto + every sibling subtree beneath that ancestor. + - **Revocation reaches live subscriptions** (stage 6): every other operation re-asks per request, but a + subscription is authorized once and then streams — and streaming is how machine consumers actually + read this server. `DataPlaneAuthorityState.Changed` fires `RevalidateMonitoredItems`. ⚠️ A denied + 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. - **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 diff --git a/docker-dev/docker-compose.yml b/docker-dev/docker-compose.yml index 89b9af91..924f1bfb 100644 --- a/docker-dev/docker-compose.yml +++ b/docker-dev/docker-compose.yml @@ -522,6 +522,12 @@ services: ConfigSource__CentralFetchEndpoints__0: "http://central-1:4055" ConfigSource__CentralFetchEndpoints__1: "http://central-2:4055" ConfigSource__ApiKey: "configserve-docker-dev-key" + # #520 stage 6 — node ACL enforcement, the dark switch. Default OFF everywhere, exactly as in + # production; the site-a pair is the demo/live-gate subject, so it reads an env override: + # OTOPCUA_ACL_ENABLED=true docker compose -f docker-dev/docker-compose.yml up -d site-a-1 site-a-2 + # With it on, EVERY data-plane operation needs a matching NodeAcl grant — a principal with no + # grant connects and sees nothing. site-b stays hardcoded OFF as the A/B control. + Security__Acl__Enabled: "${OTOPCUA_ACL_ENABLED:-false}" # Per-cluster mesh Phase 5/6 (gRPC live-telemetry stream) — see central-1/central-2. This # node carries cluster-SITE-A + driver, so SplitTopologyTransportValidator requires # Telemetry:Mode=Grpc (mandatory, not a dark switch, as of Phase 6). The dedicated :4056 @@ -631,6 +637,12 @@ services: ConfigSource__CentralFetchEndpoints__0: "http://central-1:4055" ConfigSource__CentralFetchEndpoints__1: "http://central-2:4055" ConfigSource__ApiKey: "configserve-docker-dev-key" + # #520 stage 6 — node ACL enforcement, the dark switch. Default OFF everywhere, exactly as in + # production; the site-a pair is the demo/live-gate subject, so it reads an env override: + # OTOPCUA_ACL_ENABLED=true docker compose -f docker-dev/docker-compose.yml up -d site-a-1 site-a-2 + # With it on, EVERY data-plane operation needs a matching NodeAcl grant — a principal with no + # grant connects and sees nothing. site-b stays hardcoded OFF as the A/B control. + Security__Acl__Enabled: "${OTOPCUA_ACL_ENABLED:-false}" # Per-cluster mesh Phase 5/6 (gRPC live-telemetry stream) — see central-1/site-a-1. # Telemetry:Mode=Grpc is mandatory as of Phase 6 (cluster-SITE-A + driver); the dedicated # :4056 listener binds regardless. diff --git a/docs/plans/2026-07-31-apikey-acl-enforcement-design.md b/docs/plans/2026-07-31-apikey-acl-enforcement-design.md index 5eaa6665..20a9742d 100644 --- a/docs/plans/2026-07-31-apikey-acl-enforcement-design.md +++ b/docs/plans/2026-07-31-apikey-acl-enforcement-design.md @@ -235,7 +235,42 @@ Each stage is one commit, builds clean, and leaves the tree shippable. trie's matching key was being dropped at the identity boundary, and the audit `ClientUserId` on a rejected write recorded `apikey` for every consuming system. `Principal` and `ApiKeyId` are now carried explicitly. -6. **Enforce read/browse/subscribe** — the new seam, with per-session decision caching. +6. **Enforce read/browse/subscribe.** ✅ **Done.** `Read`, `Browse`, `TranslateBrowsePath`, + `CreateMonitoredItems`, `SubscribeToEvents` and `SetMonitoringMode` are gated at the node manager; + `SessionAclDecisionCache` memoizes the grant walk per `(session, node, operation, realm)` and flushes + on a generation change. + + Four things went differently from this line, all deliberate: + + - **Browse needed a new trie concept: ancestor visibility.** The trie inherits downward, and a client + reaches a deep node by browsing the nodes above it — so without this, every grant narrower than the + cluster was authored, stored, shipped and *unreachable*. ⚠️ The obvious implementation, planting a + synthetic `Browse` grant on each ancestor, is a silent widening: the additive downward walk carries + it from `Plant` onto `Plant/Siemens/dev9` and every other sibling subtree. It is a separate, + non-inheriting marker (`BrowseTransitPrincipals`) checked at the exact node only, set only by grants + that actually carry `Browse`, and conferring only `Browse`. The consequence to accept: the *names* + of the folders on the path to a granted node are visible to whoever holds that grant. + - **The cache holds the grant walk, not the decision.** Grants and the trie are fixed for a + generation; a key's `ExpiresUtc` passes with the clock. Caching the whole decision would have let an + expired key keep working until the next deploy — the stage-5 open-session hole in a subtler form. + - **Revocation had to reach LIVE subscriptions, which needed a push.** Every other operation re-asks + per request; a subscription is authorized once and streams. Since subscriptions are how machine + consumers actually read this server, stopping only the re-asking operations would have made + revocation close to cosmetic. `IDataPlaneAuthoritySource` gained a `Changed` event and the node + manager re-authorizes every live item on it. ⚠️ A denied item is queued `BadUserAccessDenied` and + then **detached from its node's fan-out, not set `MonitoringMode.Disabled`** — disabling *discards + queued notifications*, silencing the client without telling it why. Caught by the over-the-wire test + rather than by reasoning; the unit-level path reports the item as stood down either way. Detaching + is also reversible, so a restored grant resumes the feed with no client action. + - **`TranslateBrowsePath` is gated too**, without which browse filtering is decorative: a client + refused a browse would ask `TranslateBrowsePathsToNodeIds` for the same path and get the NodeId. + + Also found and fixed en route: **`UserAuthorizationState`'s staleness ceiling couples two clocks.** + `MembershipResolvedUtc` is stamped from `DataPlaneAuthorizer`'s `TimeProvider` while `IsStale` is + evaluated against `TriePermissionEvaluator`'s. They are the same instance in production and diverge + the moment anything injects a test clock — at which point the evaluator denies *everything* with a + verdict indistinguishable from "no grant matched". Stage 5's claim that these predicates "can never + fire" was accidentally true, not true by construction; they are now disabled explicitly. 7. **AdminUI.** Key management (create/revoke, show-once) ✅ **done early, in stage 4's predecessor**; ACL authoring against principals, and **remove the two "not enforced" warning banners** — which is the honest signal that this is done. 8. **Rollout.** Docs, the ScadaBridge key hand-off, and flipping `Security:Acl:Enabled` on docker-dev @@ -261,16 +296,59 @@ checked against something other than itself. Rig left clean (0 keys, 0 ACL rows, redeployed, all six nodes healthy). -## Live gate (stage 8) +## Live gate — PASSED (docker-dev, 2026-08-01) -On docker-dev, with `Security:Acl:Enabled=true`: +Subject: **site-a-1** (`driver,cluster-SITE-A`, no ConfigDb, `ConfigSource:Mode=FetchAndCache`) with +`Security:Acl:Enabled=true`. Control: **central-1**, same artifact, flag unset. All six nodes were +rebuilt onto the gate commit first and image parity asserted per container, because `:9200` +round-robins central-1/central-2 and a stale central-2 rejected the deploy outright (`Cannot convert +string value 'RawDevice' … enum` — its image predated the stage-3 scope kinds). -1. A key granted `ReadOnly` on one equipment reads its tags and is denied a tag outside the grant. -2. The same key is denied a write; a key granted `Operator` writes successfully. -3. Anonymous with the setting off cannot activate a session; with it on and a `ReadOnly` grant, it - reads and is denied writes. -4. Revoking a key denies the next request **after a deploy** — see the caveat below. -5. A raw-realm grant (`NodeHierarchyKind.Raw`) and a UNS grant both evaluate, proving blocker #1. +The key was minted with an **independent Python HMAC**, so the verifier was checked against something +other than itself. One grant throughout: `RawDevice` → `SITEA-dev`, i.e. `raw/sa-modbus2/plc`. + +| # | Leg | Result | +|---|---|---| +| 0 | The grant is *placed*, not dropped | `generation 2 installed (keys=1, grants=1, resolvedScopes=1)`, no dropped-rule warning. This is the leg that distinguishes working enforcement from a trie that silently denies everything | +| 1 | Read INSIDE the grant | `Value: 16100, Status: 0x00000000` | +| 2 | Read OUTSIDE it (sibling device `plc2`) | `BadUserAccessDenied` | +| 3 | Browse the driver, gated | `plc` only — `plc2` filtered out | +| 4 | Browse the same driver on the **control** | `plc` **and** `plc2` — the A/B that shows the filtering is the ACL and not the address space | +| 5 | Browse the ungranted device directly | `BadUserAccessDenied` (the throw path) | +| 6 | Anonymous on the gated node | `BadUserAccessDenied` — principal `anonymous` holds no grant | +| 7 | Write with a grant lacking `WriteOperate` | `0x801F0000` | +| 8 | Same write after granting `WriteOperate` (+redeploy) | `Write successful` | +| 9 | Wrong secret | `BadIdentityTokenRejected` at activation | +| 10 | **Revoke mid-subscription** | see below | +| 11 | **Restore the grant** | see below | + +Legs 10–11 are the ones worth reading as a transcript, because they are what stage 6 added and what +no offline test could settle: + +``` +13:49:39 SiteAOnly = 53 (0x00000000) ← streaming, ~1/s +13:49:40 SiteAOnly = (0x801F0000) ← the revoking deployment lands; the client is TOLD + … 25 seconds of complete silence — ~25 updates that did not arrive … +13:50:05 SiteAOnly = 157 (0x00000000) ← the restoring deployment lands; the feed resumes +``` + +Note `53 → 157`: the device counter ran throughout, so it was **delivery** that stopped, not the +source. The `0x801F0000` frame is the half that the `MonitoringMode.Disabled` implementation would +have swallowed. + +**Not proven by this gate:** a UNS-realm grant (the rig's SITE-A cluster has no UNS equipment), and +alarm-command scoping (no alarms configured on SITE-A). Both are covered offline; neither has been +seen live. Reads on the *control* node returned `BadNodeIdUnknown` rather than a value — central does +not materialise another cluster's tag variables — so leg 4's browse is the meaningful A/B, not a read. + +**Found en route, unrelated to ACLs:** the rig's `SITEA-modbus` driver had an empty `DriverConfig`, so +its factory rejected the config and the driver ran as a stub serving nothing. Fixed on the rig +(`Host`/`Port` added) — without it every read returns `BadWaitingForInitialData` and the allow legs +cannot be told from a denial. + +Rig left as found: key, ACL row, the second device and its tag all deleted and redeployed; site-a +returned to `Security:Acl:Enabled` unset, confirmed by the absence of the startup banner and an +anonymous read succeeding again. ## Known consequence to design around diff --git a/docs/security.md b/docs/security.md index f69b1b2e..d5920caa 100644 --- a/docs/security.md +++ b/docs/security.md @@ -265,13 +265,13 @@ the one request making it. One key remains: ## Data-Plane Authorization -> ⚠️ **Read this first (updated 2026-08-01, #520 stage 5).** Whether the `NodeAcl` / `PermissionTrie` +> ⚠️ **Read this first (updated 2026-08-01, #520 stages 5–6).** Whether the `NodeAcl` / `PermissionTrie` > subsystem runs depends on one flag. With **`Security:Acl:Enabled` false — the default** — it does not: > the only authorization in the server is the coarse role checks in -> [What is enforced with the flag off](#what-is-enforced-with-the-flag-off). With it **true**, those role -> checks are *replaced* by scoped ACL decisions for write, alarm commands and HistoryRead — see -> [Node ACL enforcement](#node-acl-enforcement-securityaclenabled). **Read, Browse and -> CreateMonitoredItems are ungated in both modes.** +> [What is enforced with the flag off](#what-is-enforced-with-the-flag-off), and read, browse, subscribe +> and HistoryRead are ungated for any admitted session including Anonymous. With it **true**, those role +> checks are *replaced* by scoped ACL decisions and **every** data-plane operation is gated — see +> [Node ACL enforcement](#node-acl-enforcement-securityaclenabled). ### What is enforced with the flag off @@ -318,7 +318,13 @@ takes no realm and no node. | Key | Default | Effect | |---|---|---| -| `Security:Acl:Enabled` | `false` | `true` makes scoped `NodeAcl` grants decide write, alarm-command and HistoryRead authorization | +| `Security:Acl:Enabled` | `false` | `true` makes scoped `NodeAcl` grants decide **every** data-plane operation — read, browse, subscribe, write, alarm commands and HistoryRead | + +**Live-gated on docker-dev 2026-08-01** (site-a-1 with the flag on, central-1 as the same-artifact +control): scoped read/browse/write behave as described below, and revoking a grant stopped a running +subscription mid-stream — the client received `BadUserAccessDenied`, then 25 seconds of silence, then +the feed resumed by itself when the grant was restored. Evidence + what the gate did *not* cover (UNS +grants, alarm scoping) in `docs/plans/2026-07-31-apikey-acl-enforcement-design.md`. **It is a config change, not a redeploy.** Both paths are wired in both modes — the node always builds its permission trie on every deploy and always constructs the gate; the flag only chooses which decision @@ -333,14 +339,16 @@ the seams consult. Same dark-switch discipline as `MeshTransport`, `ConfigSource | Alarm Confirm | role `AlarmAck` | `AlarmConfirm` | | Alarm OneShotShelve / TimedShelve / Unshelve | role `AlarmAck` | `AlarmShelve` | | HistoryRead (all four variants) | **nothing** | `HistoryRead` on each node's scope, decided **per node in the batch** | -| Read, Browse, CreateMonitoredItems | nothing | **still nothing** — stage 6 | +| Read (every attribute, not just Value) | nothing | `Read` on that node's scope, decided per node in the batch | +| Browse / TranslateBrowsePath | nothing | `Browse` on the source node, **and** on each returned reference | +| CreateMonitoredItems / SubscribeToEvents | nothing | `Subscribe` on that node's scope, decided per item | ⚠️ **The ACL decision REPLACES the role check; the two are not chained.** Chaining them would require a grant to confer the server-wide role *as well as* pass the scope check, and only `Cluster`-scoped grants confer a role (`ApiKeyInterimRoleMapper`) — so every narrow grant would remain inert and the flag would look like it did nothing. Narrow grants working is the whole point of the stage. -Two behaviours worth knowing before enabling it: +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 @@ -349,6 +357,35 @@ Two behaviours worth knowing before enabling it: 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. +- **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 + to whoever holds that grant. It is conferred only by grants that actually carry `Browse`, and it + confers nothing but `Browse`. +- **A denied node reports `BadUserAccessDenied` rather than `BadNodeIdUnknown`**, because the gate runs + before node resolution. That is the information-hiding direction — error codes can no longer be used + to enumerate the address space — but it does mean "access denied" is not evidence the node exists. +- **The two namespace root folders remain visible** to any admitted session. They are referenced from + the ns=0 `Objects` folder, whose browse the SDK's own node manager serves; a session with no grants + sees the two roots and is refused on entry. + +#### Cost + +Read, Browse and subscription creation ask the same question many times per request — one Browse of a +device folder asks once per child. Those seams go through a per-`(session, node, operation, realm)` +decision cache, flushed wholesale whenever the authority generation advances. + +⚠️ **The cache holds the grant walk, not the decision.** Grants, principals and the trie are all fixed +for the life of a generation, so memoizing the walk is sound. A key's `ExpiresUtc` passes with the +*clock*, not with a deployment — so revocation and expiry are re-checked on every call, outside the +cache. Caching them would have let an expired key keep working until the next deploy. #### Everything denies when something is missing @@ -380,11 +417,9 @@ What is real about it: generation as the snapshot. Deliberately one call rather than two wirings: an uninstalled trie denies everything, which is indistinguishable from working enforcement until someone checks that an ALLOW is possible at all. -- `DataPlaneAuthorizer` **evaluates** them at the three seams above, when the flag is on. - -So an operator can still author a grant, deploy it green, and have it change nothing at the node — but -the grant now reaches the node rather than being dropped at its door. That is the behaviour to expect -until the tracking issue below is closed. +- `DataPlaneAuthorizer` **evaluates** them at every data-plane seam (#520 stages 5–6), when the flag is + on. With the flag off an operator can still author a grant, deploy it green, and have it change nothing + beyond the two coarse role gates — that is the documented default, not an unfinished state. **The four things blocking a wire-up** (all verified, none of them merely "call the evaluator"): diff --git a/src/Core/ZB.MOM.WW.OtOpcUa.Core/Authorization/DataPlaneAuthority.cs b/src/Core/ZB.MOM.WW.OtOpcUa.Core/Authorization/DataPlaneAuthority.cs index 234ec915..122e4e01 100644 --- a/src/Core/ZB.MOM.WW.OtOpcUa.Core/Authorization/DataPlaneAuthority.cs +++ b/src/Core/ZB.MOM.WW.OtOpcUa.Core/Authorization/DataPlaneAuthority.cs @@ -179,6 +179,25 @@ public interface IDataPlaneAuthoritySource { /// The snapshot in force right now. Never null; before the first apply. DataPlaneAuthoritySnapshot Current { get; } + + /// + /// Raised after a new snapshot is published, so consumers holding state derived from the previous + /// one can re-derive it. + /// + /// + /// + /// Most consumers do not need this — they read when a request arrives and + /// pick up the new generation for free. It exists for the one consumer that has no request to + /// ride: a live subscription. A monitored item authorized under an old generation keeps + /// streaming with nothing ever asking again, so revoking a grant would stop future reads while + /// leaving the data flowing on the path most clients actually use. + /// + /// + /// ⚠️ Handlers run synchronously on the deploy thread, inside the apply. Keep them short + /// and non-throwing; a handler that blocks stalls the deployment. + /// + /// + event Action? Changed; } /// @@ -194,6 +213,14 @@ public sealed class EmptyDataPlaneAuthoritySource : IDataPlaneAuthoritySource /// public DataPlaneAuthoritySnapshot Current => DataPlaneAuthoritySnapshot.Empty; + + /// + /// Never raised — this source is permanently empty, so nothing derived from it can go stale. + public event Action? Changed + { + add { } + remove { } + } } /// @@ -255,6 +282,9 @@ public sealed class DataPlaneAuthorityState : IDataPlaneAuthoritySource, IDataPl /// public DataPlaneAuthoritySnapshot Current => Volatile.Read(ref _current); + /// + public event Action? Changed; + /// public DataPlaneAuthoritySnapshot Apply( string? clusterId, @@ -265,6 +295,8 @@ public sealed class DataPlaneAuthorityState : IDataPlaneAuthoritySource, IDataPl { ArgumentNullException.ThrowIfNull(revisionHash); + DataPlaneAuthoritySnapshot published; + lock (_gate) { var previous = _current; @@ -291,10 +323,47 @@ public sealed class DataPlaneAuthorityState : IDataPlaneAuthoritySource, IDataPl // reachable during a deploy — precisely the kind of defect that never reproduces. var dropped = BuildAndInstallTrie(clusterId, generation, grants, scopePaths); - var next = DataPlaneAuthoritySnapshot.Create( + published = DataPlaneAuthoritySnapshot.Create( clusterId, revisionHash, generation, keys, grants, scopePaths, dropped); - Volatile.Write(ref _current, next); - return next; + Volatile.Write(ref _current, published); + } + + // Outside the lock, deliberately. Subscribers walk live subscriptions and take the SDK's own + // node-manager lock to do it; calling out while holding _gate would put two locks in play in an + // order nothing else guarantees. The snapshot is already published, so a subscriber that reads + // Current mid-notification sees the generation it is being told about. + Notify(published); + return published; + } + + /// + /// Raises , swallowing handler faults. + /// + /// + /// A handler that throws must not fail the deployment. The subscriber this event exists for + /// re-validates live monitored items, and the fail-safe direction there is emphatically not "abort + /// the apply": the artifact has already been installed by this point, so throwing would seal a + /// deployment red that in fact succeeded, and leave the operator redeploying an artifact that is + /// already in force. + /// + /// The newly published snapshot. + private void Notify(DataPlaneAuthoritySnapshot snapshot) + { + var handlers = Changed; + if (handlers is null) return; + + foreach (var handler in handlers.GetInvocationList().Cast>()) + { + try + { + handler(snapshot); + } + catch (Exception ex) + { + // Per-handler, so one bad subscriber cannot stop the others being told. + System.Diagnostics.Trace.TraceError( + $"DataPlaneAuthorityState: a Changed handler threw for generation {snapshot.GenerationId}: {ex}"); + } } } diff --git a/src/Core/ZB.MOM.WW.OtOpcUa.Core/Authorization/PermissionTrie.cs b/src/Core/ZB.MOM.WW.OtOpcUa.Core/Authorization/PermissionTrie.cs index 2ad1666c..ec8d1b1d 100644 --- a/src/Core/ZB.MOM.WW.OtOpcUa.Core/Authorization/PermissionTrie.cs +++ b/src/Core/ZB.MOM.WW.OtOpcUa.Core/Authorization/PermissionTrie.cs @@ -68,6 +68,63 @@ public sealed class PermissionTrie return matches; } + /// + /// Whether is an ancestor of some node one of + /// holds a Browse grant on — i.e. whether it must be + /// browsable purely so the granted node can be reached. + /// + /// + /// + /// Without this, every narrow Browse grant is unreachable and therefore useless: a grant on + /// Plant/Modbus/dev1 is only arrived at by browsing Plant then Plant/Modbus, + /// and a downward-inheriting trie confers nothing at either. The operator authors a working-looking + /// grant and the client cannot navigate to it. + /// + /// + /// ⚠️ Transit is checked at the EXACT node and never accumulates down the walk, which is + /// the whole reason it is a separate marker instead of a synthesized Browse grant planted + /// on each ancestor. A planted grant would be collected by and — the + /// walk being purely additive downward — would cascade from Plant onto + /// Plant/Siemens/dev9 and every other unrelated subtree beneath it. That is a silent + /// widening of exactly the kind the ordinal-comparer note below records. + /// + /// + /// Only grants that actually carry Browse mark their ancestors. A principal granted + /// Read alone on a deep node can still read it by NodeId — OPC UA does not require Browse + /// to Read — but cannot discover it by navigating, which is the fail-closed reading and the one + /// that keeps "Browse" meaning something. + /// + /// + /// The node scope being browsed. + /// The principal names carried by the session. + /// when the node is on the path to a Browse-granted descendant. + public bool HasBrowseTransit(NodeScope scope, IEnumerable principals) + { + ArgumentNullException.ThrowIfNull(scope); + ArgumentNullException.ThrowIfNull(principals); + + // Level 1 is the realm, exactly as in CollectMatches — a scope with no segments addresses the + // realm root, which is itself an ancestor of everything granted in that realm. + var realm = NodeAclRealms.SegmentFor(scope.Kind); + if (!Root.Children.TryGetValue(realm, out var node)) return false; + + foreach (var segment in scope.Segments) + { + if (segment is null || !node.Children.TryGetValue(segment, out var next)) return false; + node = next; + } + + return MatchesTransit(node, principals); + } + + private static bool MatchesTransit(PermissionTrieNode node, IEnumerable principals) + { + if (node.BrowseTransitPrincipals.Count == 0) return false; + foreach (var principal in principals) + if (principal is not null && node.BrowseTransitPrincipals.Contains(principal)) return true; + return false; + } + private static void CollectAtLevel(PermissionTrieNode node, HashSet groups, List matches) { foreach (var grant in node.Grants) @@ -101,6 +158,19 @@ public sealed class PermissionTrieNode /// nothing instead of granting something adjacent. /// public Dictionary Children { get; } = new(StringComparer.Ordinal); + + /// + /// Principals holding a Browse grant somewhere strictly below this node, so this node + /// must be browsable for that grant to be reachable. See + /// for why this is a separate marker rather than a + /// synthesized grant, and why it must never be accumulated during the walk. + /// + /// + /// Compared OrdinalIgnoreCase: this holds principal names, which are identifiers, and the + /// repo's rule (#533) is that identifiers compare case-insensitively while path segments + /// — the keys — compare ordinally. + /// + public HashSet BrowseTransitPrincipals { get; } = new(StringComparer.OrdinalIgnoreCase); } /// Projection of a row into the trie. diff --git a/src/Core/ZB.MOM.WW.OtOpcUa.Core/Authorization/PermissionTrieBuilder.cs b/src/Core/ZB.MOM.WW.OtOpcUa.Core/Authorization/PermissionTrieBuilder.cs index 77b6726e..137bf598 100644 --- a/src/Core/ZB.MOM.WW.OtOpcUa.Core/Authorization/PermissionTrieBuilder.cs +++ b/src/Core/ZB.MOM.WW.OtOpcUa.Core/Authorization/PermissionTrieBuilder.cs @@ -118,7 +118,16 @@ public static class PermissionTrieBuilder var node = trie.Root; foreach (var segment in path.Segments) + { node = EnsureChild(node, segment); + + // Mark every node ON THE WAY DOWN — i.e. every strict ancestor of the target, plus the + // target itself, which is harmless because the target also carries the real grant. A + // Browse grant that nobody can navigate to is not a grant, and the trie inherits + // downward only, so without this the operator's rule silently does nothing. + if ((row.PermissionFlags & NodePermissions.Browse) != 0) + node.BrowseTransitPrincipals.Add(row.Principal); + } node.Grants.Add(grant); } } diff --git a/src/Core/ZB.MOM.WW.OtOpcUa.Core/Authorization/TriePermissionEvaluator.cs b/src/Core/ZB.MOM.WW.OtOpcUa.Core/Authorization/TriePermissionEvaluator.cs index 2c401a5b..f8f1373a 100644 --- a/src/Core/ZB.MOM.WW.OtOpcUa.Core/Authorization/TriePermissionEvaluator.cs +++ b/src/Core/ZB.MOM.WW.OtOpcUa.Core/Authorization/TriePermissionEvaluator.cs @@ -49,15 +49,24 @@ public sealed class TriePermissionEvaluator : IPermissionEvaluator } var matches = trie.CollectMatches(scope, session.Principals); - if (matches.Count == 0) return AuthorizationDecision.NotGranted(); - var required = MapOperationToPermission(operation); - var granted = NodePermissions.None; - foreach (var m in matches) granted |= m.PermissionFlags; - return (granted & required) == required - ? AuthorizationDecision.Allowed(matches) - : AuthorizationDecision.NotGranted(); + if (matches.Count > 0) + { + var granted = NodePermissions.None; + foreach (var m in matches) granted |= m.PermissionFlags; + + if ((granted & required) == required) return AuthorizationDecision.Allowed(matches); + } + + // Browse alone falls back to ancestor visibility: a node is browsable when it merely leads to + // something the principal may browse. This is checked AFTER the ordinary grant walk so a real + // grant still supplies the provenance, and it applies to no other operation — transit confers + // navigation, never read, write or subscribe. + if (operation == OpcUaOperation.Browse && trie.HasBrowseTransit(scope, session.Principals)) + return AuthorizationDecision.Allowed([]); + + return AuthorizationDecision.NotGranted(); } /// Maps each to the bit required to grant it. diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Pages/Clusters/AclEdit.razor b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Pages/Clusters/AclEdit.razor index fe57d457..487fc605 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Pages/Clusters/AclEdit.razor +++ b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Pages/Clusters/AclEdit.razor @@ -46,12 +46,18 @@ else equipment" into server-wide write authority.
With it on: the scope is honoured, and the scoped decision replaces the - server-wide role rather than adding to it. Writes, alarm commands and HistoryRead are gated; the - alarm methods separate into AlarmAcknowledge / - AlarmConfirm / AlarmShelve. + server-wide role rather than adding to it. Every data-plane operation is gated — + Read, Browse, + Subscribe, WriteOperate, + HistoryRead and the alarm methods, which separate into + AlarmAcknowledge / AlarmConfirm / + AlarmShelve. So a grant that omits + Read denies reads, and one that omits + Browse hides the node from navigation.
- Read, Browse and Subscribe are not gated in either mode, for any admitted - session, including anonymous. + Granting Browse on a node also makes the folders above it + browsable — otherwise the grant could never be navigated to — so it reveals their names. + Revoking this grant stops any live subscription it was authorizing, on the next deployment.
Principal is an API-key principal — or the reserved anonymous principal — not an LDAP group: a client presents an OPC UA diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Pages/Clusters/ClusterAcls.razor b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Pages/Clusters/ClusterAcls.razor index bc4ef360..f8bac332 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Pages/Clusters/ClusterAcls.razor +++ b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Pages/Clusters/ClusterAcls.razor @@ -39,13 +39,18 @@ else
With it on: every scope kind is honoured, and the scoped decision replaces the server-wide role rather than adding to it — so a narrow grant works, and a - Cluster-scoped one no longer confers authority beyond that cluster. Writes, alarm commands and - HistoryRead are gated; the alarm methods separate into - AlarmAcknowledge / AlarmConfirm / - AlarmShelve. + Cluster-scoped one no longer confers authority beyond that cluster. + Every data-plane operation is gated — Read, + Browse, Subscribe, + WriteOperate, HistoryRead and the alarm + methods, which separate into AlarmAcknowledge / + AlarmConfirm / AlarmShelve. A principal with + no grant at all can connect and see nothing.
- Read, Browse and Subscribe are not gated in either mode, for any admitted - session, including anonymous. + A node that merely leads to something you may browse stays browsable, or a narrow grant + could never be navigated to — so granting Browse deep in the tree also + reveals the names of the folders above it. Revoking a grant stops live subscriptions on the next + deployment, not just future requests.
Identity is an API key, not LDAP: a client presents an OPC UA UserName token with username apikey and password 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 e5760e43..d05d5abf 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.Host/OpcUa/OtOpcUaServerHostedService.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.Host/OpcUa/OtOpcUaServerHostedService.cs @@ -243,8 +243,9 @@ public sealed class OtOpcUaServerHostedService : IHostedService, IAsyncDisposabl if (_dataPlaneAuthorizer is { Enabled: true }) { _logger.LogWarning( - "OtOpcUaServerHostedService: node ACL enforcement is ENABLED — writes, alarm commands and " - + "HistoryRead now require a matching NodeAcl grant. Read, Browse and Subscribe remain ungated."); + "OtOpcUaServerHostedService: node ACL enforcement is ENABLED — EVERY data-plane operation " + + "(read, browse, subscribe, write, alarm commands, HistoryRead) now requires a matching " + + "NodeAcl grant. A principal with no grant can connect and see nothing."); } // Propagate the configured tie-cluster over-fetch bound onto the node manager so HistoryRead-Raw diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/OtOpcUaNodeManager.cs b/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/OtOpcUaNodeManager.cs index 2dc2e608..ed397b0a 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/OtOpcUaNodeManager.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/OtOpcUaNodeManager.cs @@ -270,11 +270,48 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2 /// public IDataPlaneAuthoritySource DataPlaneAuthority { - get => _dataPlaneAuthority; - set => _dataPlaneAuthority = value ?? EmptyDataPlaneAuthoritySource.Instance; + get => Volatile.Read(ref _dataPlaneAuthority); + set + { + var next = value ?? EmptyDataPlaneAuthoritySource.Instance; + var previous = Interlocked.Exchange(ref _dataPlaneAuthority, next); + if (ReferenceEquals(previous, next)) return; + + // ⚠️ The DETACH is load-bearing, for the same reason it is on IRediscoverable: this manager is + // rebuilt across the server's lifetime while the authority source is a process singleton that + // outlives it, so a missing -= leaks a dead node manager per rebuild AND has every stale one + // walking its own dead item map on each deploy. + previous.Changed -= OnAuthorityChanged; + next.Changed += OnAuthorityChanged; + } } - private volatile IDataPlaneAuthoritySource _dataPlaneAuthority = EmptyDataPlaneAuthoritySource.Instance; + // Not `volatile`: the setter swaps it with Interlocked.Exchange (which cannot take a volatile ref), and + // the getter reads it with Volatile.Read, so both accesses are ordered explicitly. + private IDataPlaneAuthoritySource _dataPlaneAuthority = EmptyDataPlaneAuthoritySource.Instance; + + /// + /// Re-authorizes live subscriptions when a deployment installs a new authorization generation. + /// + /// The newly published snapshot; unused — the current state is re-read per item. + private void OnAuthorityChanged(DataPlaneAuthoritySnapshot snapshot) => RevalidateMonitoredItems(); + + /// + /// + /// Detaches from the authority source. The source is a process singleton that outlives this manager, + /// so without this a disposed manager stays subscribed and re-walks its dead item map on every + /// subsequent deploy — the same leak shape the setter's swap guards against. + /// + protected override void Dispose(bool disposing) + { + if (disposing) + { + Volatile.Read(ref _dataPlaneAuthority).Changed -= OnAuthorityChanged; + _liveItems.Clear(); + } + + base.Dispose(disposing); + } /// /// The node-scoped ACL gate (#520 stage 5), or null when none is wired. Consulted by the @@ -2591,6 +2628,423 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2 } } + // --------------------------------------------------------------------------------------------- + // #520 stage 6 — Read / Browse / TranslateBrowsePath / subscribe gates. + // + // These are the request-rate seams, so every decision here goes through the authorizer's CACHED + // entry point: one Browse of a device folder asks the same question once per child, and a client + // reading a thousand nodes asks it a thousand times. + // + // The gates resolve a node purely from its NodeId — namespace index for ownership, `s=` identifier + // for the scope — and never call GetManagerHandle. That is not only cheaper: it means a DENIED node + // is never resolved, never read, and never touches a driver. + // + // ⚠️ A consequence worth knowing: a NodeId in one of our namespaces that does not exist now answers + // BadUserAccessDenied instead of BadNodeIdUnknown while enforcement is on, because the gate runs + // before resolution. That is the information-hiding direction — an ungranted client can no longer + // use error codes to enumerate which nodes exist — but it does mean "access denied" is not proof + // that the node is there. + // --------------------------------------------------------------------------------------------- + + /// True when is minted in one of this manager's two namespaces. + /// The node id to test. + /// when this manager owns the node's namespace. + private bool OwnsNamespace(NodeId? nodeId) => + nodeId is not null + && (nodeId.NamespaceIndex == NamespaceIndexes[0] || nodeId.NamespaceIndex == NamespaceIndexes[1]); + + /// Pulls the ACL identity off an SDK operation context. + /// The SDK context carrying the session identity. + /// The role-carrying identity, or null when the session has none. + private static RoleCarryingUserIdentity? IdentityOf(OperationContext context) => + context.UserIdentity as RoleCarryingUserIdentity; + + /// Pulls the ACL identity off a system context (the protected SDK seams). + /// The SDK system context. + /// The role-carrying identity, or null when the session has none. + private static RoleCarryingUserIdentity? IdentityOf(ServerSystemContext context) => + (context as ISessionOperationContext)?.UserIdentity as RoleCarryingUserIdentity + ?? context.OperationContext?.UserIdentity as RoleCarryingUserIdentity; + + /// Whether the session may perform on . + /// The calling session's identity. + /// The operation being attempted. + /// The target node. + /// when permitted, or when the node is not ours to gate. + private bool Permits(RoleCarryingUserIdentity? identity, OpcUaOperation operation, NodeId nodeId) => + !OwnsNamespace(nodeId) + || _authorizer!.IsAllowedCached(identity, operation, nodeId, RealmOf(nodeId)); + + /// + /// + /// Denied slots are answered and marked before the base + /// runs, so a refused read never resolves the node or samples its value. Every attribute of a node + /// is gated by the single Read permission rather than only Value: the finer split + /// would let an ungranted session harvest BrowseNames, DataTypes and descriptions of nodes it + /// cannot read, which is most of what the address space discloses. + /// + public override void Read( + OperationContext context, + double maxAge, + IList nodesToRead, + IList values, + IList errors) + { + if (AclEnforced) + { + var identity = IdentityOf(context); + var denied = 0; + + for (var ii = 0; ii < nodesToRead.Count; ii++) + { + var nodeToRead = nodesToRead[ii]; + if (nodeToRead.Processed || !OwnsNamespace(nodeToRead.NodeId)) continue; + if (Permits(identity, OpcUaOperation.Read, nodeToRead.NodeId)) continue; + + nodeToRead.Processed = true; + errors[ii] = StatusCodes.BadUserAccessDenied; + values[ii] = new DataValue(StatusCodes.BadUserAccessDenied); + denied++; + } + + LogAclFiltering("Read", identity, denied, nodesToRead.Count); + } + + base.Read(context, maxAge, nodesToRead, values, errors); + } + + /// + /// + /// + /// Two distinct checks: the browsed node itself must be reachable, and every reference the base + /// returns is filtered against the target's own grant. Filtering only the source would list + /// children the session may not see; refusing only the source would make every narrow grant + /// unreachable, since a client arrives at a deep node by browsing its ancestors. + /// + /// + /// Reachability comes from PermissionTrie.HasBrowseTransit — a node is browsable when it + /// merely leads somewhere the principal may browse. Note what this necessarily discloses: the + /// names of the folders on the path to a granted node are visible to anyone holding + /// that grant, because navigation requires them. + /// + /// + /// ⚠️ The two namespace root folders are referenced from the ns=0 Objects folder, whose + /// browse is served by the SDK's own node manager and is not gated here. A session with no + /// grants at all therefore still sees that the two roots exist, and is refused on entry. + /// + /// + public override void Browse( + OperationContext context, + ref ContinuationPoint continuationPoint, + IList references) + { + if (!AclEnforced) + { + base.Browse(context, ref continuationPoint, references); + return; + } + + var identity = IdentityOf(context); + // NodeToBrowse holds the MANAGER HANDLE the master node manager resolved — a NodeHandle for this + // manager — not a NodeState. The declared type is `object`, so casting it to the wrong thing + // compiles cleanly and silently yields null, which would skip the source gate entirely and leave + // only the reference filtering below. + var source = (continuationPoint?.NodeToBrowse as NodeHandle)?.NodeId; + + if (source is not null && !Permits(identity, OpcUaOperation.Browse, source)) + throw new ServiceResultException(StatusCodes.BadUserAccessDenied); + + // Only the references THIS call appends are ours to filter — on a BrowseNext the list already + // carries the earlier pages, and re-examining them would both waste work and skew the summary. + var before = references.Count; + base.Browse(context, ref continuationPoint, references); + var returned = references.Count - before; + + var filtered = 0; + for (var ii = references.Count - 1; ii >= before; ii--) + { + // ExpandedNodeId → NodeId only makes sense for a reference into our own server; a reference + // to another server's node is left alone, as this manager has no grants that could speak to it. + var target = references[ii].NodeId; + if (target is null || target.IsAbsolute) continue; + + var targetId = ExpandedNodeId.ToNodeId(target, Server.NamespaceUris); + if (targetId is null || !OwnsNamespace(targetId)) continue; + if (Permits(identity, OpcUaOperation.Browse, targetId)) continue; + + references.RemoveAt(ii); + filtered++; + } + + LogAclFiltering("Browse", identity, filtered, returned); + } + + /// + /// + /// Gated for the same reason Browse is, and it is not optional: without it a client denied a browse + /// simply asks TranslateBrowsePathsToNodeIds for the same path and gets the NodeId back, which + /// would make the reference filtering above decorative. + /// + public override void TranslateBrowsePath( + OperationContext context, + object sourceHandle, + RelativePathElement relativePath, + IList targetIds, + IList unresolvedTargetIds) + { + if (!AclEnforced) + { + base.TranslateBrowsePath(context, sourceHandle, relativePath, targetIds, unresolvedTargetIds); + return; + } + + var identity = IdentityOf(context); + + if (sourceHandle is NodeHandle handle + && !Permits(identity, OpcUaOperation.Browse, handle.NodeId)) + { + // Silence rather than an error: the SDK's contract here is "add what you resolved", and a + // path that resolves to nothing is the same answer the client gets for a path that does not + // exist — which is the answer we want an ungranted session to receive. + return; + } + + var before = targetIds.Count; + base.TranslateBrowsePath(context, sourceHandle, relativePath, targetIds, unresolvedTargetIds); + + for (var ii = targetIds.Count - 1; ii >= before; ii--) + { + var targetId = ExpandedNodeId.ToNodeId(targetIds[ii], Server.NamespaceUris); + if (targetId is null || !OwnsNamespace(targetId)) continue; + if (Permits(identity, OpcUaOperation.Browse, targetId)) continue; + + targetIds.RemoveAt(ii); + } + } + + /// + /// + /// Per item, not per batch — OPC UA Part 4 gives each item its own result, and a client subscribing + /// to ten nodes of which one is off-limits should get nine subscriptions and one denial. + /// + public override void CreateMonitoredItems( + OperationContext context, + uint subscriptionId, + double publishingInterval, + TimestampsToReturn timestampsToReturn, + IList itemsToCreate, + IList errors, + IList filterErrors, + IList monitoredItems, + bool createDurable, + MonitoredItemIdFactory monitoredItemIdFactory) + { + if (AclEnforced) + { + var identity = IdentityOf(context); + var denied = 0; + + for (var ii = 0; ii < itemsToCreate.Count; ii++) + { + var item = itemsToCreate[ii]; + var target = item.ItemToMonitor?.NodeId; + if (item.Processed || !OwnsNamespace(target)) continue; + if (Permits(identity, OpcUaOperation.CreateMonitoredItems, target!)) continue; + + item.Processed = true; + errors[ii] = StatusCodes.BadUserAccessDenied; + denied++; + } + + LogAclFiltering("CreateMonitoredItems", identity, denied, itemsToCreate.Count); + } + + base.CreateMonitoredItems( + context, subscriptionId, publishingInterval, timestampsToReturn, itemsToCreate, errors, + filterErrors, monitoredItems, createDurable, monitoredItemIdFactory); + } + + /// + /// + /// Event subscriptions are gated on the notifier they attach to. This is the alarm-delivery path: + /// without it, a session denied every value on an equipment could still subscribe to its event + /// notifier and receive every alarm the equipment raises, including the condition fields. + /// + public override ServiceResult SubscribeToEvents( + OperationContext context, + object sourceId, + uint subscriptionId, + IEventMonitoredItem monitoredItem, + bool unsubscribe) + { + // ⚠️ sourceId is the MANAGER HANDLE, not a NodeState — the same `object`-typed trap as + // ContinuationPoint.NodeToBrowse. Casting it to NodeState compiles, always yields null, and + // silently disables this gate; IsHandleInNamespace is the SDK's own resolver and cannot be + // wrong about it. + // + // Only creation is gated. Refusing an UNSUBSCRIBE would strand the item on a node the session can + // no longer reach — the client could never detach it, and the server would keep delivering. + if (AclEnforced && !unsubscribe + && IsHandleInNamespace(sourceId) is { } source + && !Permits(IdentityOf(context), OpcUaOperation.CreateMonitoredItems, source.NodeId)) + { + return StatusCodes.BadUserAccessDenied; + } + + return base.SubscribeToEvents(context, sourceId, subscriptionId, monitoredItem, unsubscribe); + } + + /// + /// + /// Re-checked on every mode change so a client cannot re-enable an item that + /// disabled when its grant was revoked. Disabling is always + /// allowed: a session that has lost its grant must still be able to stand its items down. + /// + protected override ServiceResult SetMonitoringMode( + ServerSystemContext context, + IMonitoredItem monitoredItem, + MonitoringMode monitoringMode, + NodeHandle handle) + { + if (AclEnforced && monitoringMode != MonitoringMode.Disabled + && !Permits(IdentityOf(context), OpcUaOperation.CreateMonitoredItems, handle.NodeId)) + { + return StatusCodes.BadUserAccessDenied; + } + + return base.SetMonitoringMode(context, monitoredItem, monitoringMode, handle); + } + + /// + protected override void OnMonitoredItemCreated( + ServerSystemContext context, NodeHandle handle, ISampledDataChangeMonitoredItem monitoredItem) + { + base.OnMonitoredItemCreated(context, handle, monitoredItem); + if (OwnsNamespace(handle.NodeId)) _liveItems[monitoredItem] = new LiveItem(handle); + } + + /// + protected override void OnMonitoredItemDeleted( + ServerSystemContext context, NodeHandle handle, ISampledDataChangeMonitoredItem monitoredItem) + { + _liveItems.TryRemove(monitoredItem, out _); + base.OnMonitoredItemDeleted(context, handle, monitoredItem); + } + + /// + /// Live data-change monitored items on nodes this manager owns, so a grant revoked by a deployment + /// can reach the subscriptions it was authorizing. + /// + /// + /// Keyed by the item itself; the value is the node it monitors. Maintained by + /// / , which the SDK calls + /// for every create and delete including subscription teardown, so the map cannot outlive the items. + /// + private readonly ConcurrentDictionary _liveItems = new(); + + /// A tracked monitored item: the handle it was created against, and whether we have silenced it. + /// The node handle, which carries the fan-out list. + private sealed record LiveItem(NodeHandle Handle) + { + /// True while the item has been detached from its node's fan-out for want of a grant. + public bool Suppressed { get; set; } + } + + /// + /// Re-authorizes every live monitored item against the current generation, standing down the ones + /// whose grant has gone. + /// + /// + /// + /// Why this is not optional. Stage 5 made revocation reach an open session for write, + /// alarm commands and HistoryRead, because each of those re-asks on every request. A + /// subscription never re-asks: it is authorized once at creation and then streams. Subscriptions + /// are also how this server is actually consumed — ScadaBridge's Data Connection Layer + /// subscribes rather than polls — so without this, revoking a grant would stop the operations + /// nobody uses and leave the data flowing on the one everybody does. + /// + /// + /// A denied item is told, then detached: queued a BadUserAccessDenied so the client + /// learns why its data stopped rather than watching a feed go quiet, then removed from its node's + /// fan-out so no further value reaches it. + /// + /// + /// ⚠️ Detaching, not — and the order matters. The + /// obvious implementation is to queue the denial and then disable the item; it does not work, + /// and it fails in the quiet direction. Disabling an item discards its queued notifications, + /// so the client is silenced without ever being told why. This was caught by the over-the-wire + /// test, not by reasoning: the unit-level path reports the item as stood down either way. + /// + /// + /// Detaching is also reversible, which disabling is not from the server's side. A later + /// deployment that restores the grant re-attaches the item and the feed resumes on its own — a + /// disabled item would have needed the client to notice and re-enable it, which a machine + /// consumer will never do. + /// + /// + /// Items are not deleted. A node manager has no way to remove an item from a client's + /// subscription, and inventing one would leave the client's own bookkeeping describing items the + /// server had silently discarded. + /// + /// + public void RevalidateMonitoredItems() + { + if (!AclEnforced || _liveItems.IsEmpty) return; + + var now = DateTime.UtcNow; + var stoodDown = 0; + var restored = 0; + + foreach (var (item, tracked) in _liveItems) + { + // The item carries the identity of the session that created it, which is what must be + // re-checked — the deploy thread running this has no session of its own. + if (item is not IMonitoredItem live) continue; + + var nodeId = tracked.Handle.NodeId; + var permitted = _authorizer!.IsAllowedCached( + live.EffectiveIdentity as RoleCarryingUserIdentity, + OpcUaOperation.CreateMonitoredItems, nodeId, RealmOf(nodeId)); + + if (permitted) + { + if (!tracked.Suppressed) continue; + + lock (Lock) tracked.Handle.MonitoredNode?.Add(item); + tracked.Suppressed = false; + restored++; + continue; + } + + if (tracked.Suppressed) continue; + + // Queue BEFORE detaching: the notification has to be in the item's queue while the item is + // still live, or the client is cut off with no explanation. + item.QueueValue( + new DataValue(StatusCodes.BadUserAccessDenied) { SourceTimestamp = now, ServerTimestamp = now }, + new ServiceResult(StatusCodes.BadUserAccessDenied), + ignoreFilters: true); + + lock (Lock) tracked.Handle.MonitoredNode?.Remove(item); + tracked.Suppressed = true; + stoodDown++; + } + + if (stoodDown > 0 || restored > 0) + { + _authorizer!.LogFiltered( + "RevalidateSubscriptions", principal: null, stoodDown, _liveItems.Count); + } + } + + /// Emits the one operator-facing summary line per gated request. + /// The service being filtered. + /// The calling session's identity. + /// How many nodes or references were withheld. + /// How many were considered. + private void LogAclFiltering(string operation, RoleCarryingUserIdentity? identity, int denied, int total) => + _authorizer?.LogFiltered(operation, identity?.Principal, denied, total); + // --------------------------------------------------------------------------------------------- // Phase C — OPC UA HistoryRead over historized variable nodes. // 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 1969620d..f9cf8447 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/Security/DataPlaneAuthorizer.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/Security/DataPlaneAuthorizer.cs @@ -32,15 +32,24 @@ namespace ZB.MOM.WW.OtOpcUa.OpcUaServer.Security; /// session's principals out of band — the principal comes from the key, and keys and grants both /// ride the deployment artifact — so the only real invalidation signal is the generation stamp, /// which a per-call construction picks up for free. The cost is one small record per write, alarm -/// command or HistoryRead, none of which are hot. The publish path is hot, and it is stage 6's -/// problem: that seam needs a per-(session, node) decision cache, not this. +/// command or HistoryRead, none of which are hot. /// /// -/// A consequence worth stating: because the state is rebuilt each call against the current -/// generation, and -/// can never fire on this path. They are -/// vestigial here rather than load-bearing, and a reader should not conclude from their presence -/// that a freshness timer is doing anything. +/// Stage 6's request-rate seams — Read, Browse, subscription creation — go through +/// instead, which memoizes the grant walk in +/// . That is a cache of the walk, not of the decision: the key's +/// revocation and expiry are re-checked on every call regardless, because expiry moves with the +/// clock rather than with a generation. +/// +/// +/// and +/// are vestigial here rather than load-bearing, +/// and a reader should not conclude from their presence that a freshness timer is doing anything. +/// They are switched off explicitly (AuthCacheMaxStaleness = TimeSpan.MaxValue) rather +/// than left to never be reached — see the comment at that line. Relying on "the stamp is always +/// fresh" was not safe: the stamp comes from this class's clock and the ceiling is checked against +/// the evaluator's, so any divergence between the two denied every request with a verdict that +/// looks exactly like "no grant matched". /// /// public sealed class DataPlaneAuthorizer @@ -80,6 +89,72 @@ public sealed class DataPlaneAuthorizer /// public bool Enabled { get; } + /// + /// Memoized grant walks for the request-rate seams (#520 stage 6). See + /// for what it is and is not allowed to cache. + /// + public SessionAclDecisionCache Decisions { get; } = new(); + + /// + /// Authorizes on through the decision cache — + /// the entry point for Read, Browse and subscription creation, which ask the same question many + /// times per request. + /// + /// + /// Identical in outcome to ; it differs only in memoizing the grant walk and + /// in logging denials at Debug rather than Information. The quieter log is deliberate: a browse of a + /// large folder by an ungranted principal would otherwise emit one Information line per child. The + /// per-request summary the node manager logs is the operator-facing signal; this is the detail behind + /// it. + /// + /// The role-carrying identity off the SDK context, or null when the session carries none. + /// The operation being attempted. + /// The target node. + /// The namespace the node was resolved in. + /// null to proceed; otherwise the veto to return to the SDK. + public ServiceResult? AuthorizeCached( + RoleCarryingUserIdentity? identity, + OpcUaOperation operation, + NodeId nodeId, + AddressSpaceRealm realm) + => Evaluate(identity, operation, nodeId, realm, cached: true); + + /// + /// Emits the one operator-facing line per gated request, summarising what a hot seam withheld. + /// + /// + /// This exists because the per-node denials behind it are logged at Debug: a Browse of a large + /// folder by an ungranted principal would otherwise emit one Information line per child, and an + /// operator would be reading a flood rather than a fact. It lives on the authorizer rather than the + /// node manager purely because the node manager is a CustomNodeManager2 and carries no + /// ILogger — everything it logs itself goes through the SDK's obsolete static trace. + /// + /// The service being filtered, e.g. Browse. + /// The calling principal, or null when the session resolved none. + /// How many nodes or references were withheld. + /// How many were considered. + public void LogFiltered(string operation, string? principal, int withheld, int total) + { + if (withheld == 0) return; + + _log.LogInformation( + "AclGate: {Operation} withheld {Withheld} of {Total} node(s) for principal {Principal}", + operation, withheld, total, principal ?? "(none)"); + } + + /// Whether may perform , as a bool. + /// The role-carrying identity off the SDK context. + /// The operation being attempted. + /// The target node. + /// The namespace the node was resolved in. + /// when the operation is permitted. + public bool IsAllowedCached( + RoleCarryingUserIdentity? identity, + OpcUaOperation operation, + NodeId nodeId, + AddressSpaceRealm realm) + => AuthorizeCached(identity, operation, nodeId, realm) is null; + /// /// Authorizes on for the session behind /// . @@ -96,16 +171,24 @@ public sealed class DataPlaneAuthorizer OpcUaOperation operation, NodeId nodeId, AddressSpaceRealm realm) + => Evaluate(identity, operation, nodeId, realm, cached: false); + + private ServiceResult? Evaluate( + RoleCarryingUserIdentity? identity, + OpcUaOperation operation, + NodeId nodeId, + AddressSpaceRealm realm, + bool cached) { if (identity is null) - return Deny(operation, nodeId, principal: null, "the session carries no role-carrying identity"); + return Deny(operation, nodeId, principal: null, "the session carries no role-carrying identity", cached); if (string.IsNullOrEmpty(identity.Principal)) { // An authenticator that predates the principal model, or a code path that built the identity // through the legacy two-argument constructor. Guessing a principal here — from the display // name, say — would silently authorize as whatever the token's username happened to be. - return Deny(operation, nodeId, principal: null, "the session resolved no ACL principal"); + return Deny(operation, nodeId, principal: null, "the session resolved no ACL principal", cached); } var snapshot = _authority.Current; @@ -113,41 +196,81 @@ public sealed class DataPlaneAuthorizer if (string.IsNullOrWhiteSpace(snapshot.ClusterId)) { return Deny(operation, nodeId, identity.Principal, - "this node has no cluster identity, so no permission trie exists to evaluate against"); + "this node has no cluster identity, so no permission trie exists to evaluate against", cached); } // Re-check revocation on every call. The key was verified at session activation, but a deployment // since then may have disabled or expired it — and on a driver-only node the artifact is the ONLY // revocation channel. Without this a revoked key keeps its open session's authority indefinitely, // which on a machine-to-machine data plane means until the client chooses to reconnect. + // ⚠️ Deliberately OUTSIDE the decision cache, and it must stay there. Grants and principals are + // fixed for a generation, so memoizing the grant walk is sound; a key's expiry passes with the + // CLOCK, so a cached allow would keep an expired key working until the next deploy. var now = _time.GetUtcNow().UtcDateTime; + var sessionKey = identity.ApiKeyId ?? identity.Principal; if (identity.ApiKeyId is { Length: > 0 } keyId && snapshot.FindUsableKey(keyId, now) is null) { return Deny(operation, nodeId, identity.Principal, - $"key {keyId} is no longer enabled or has expired in generation {snapshot.GenerationId}"); + $"key {keyId} is no longer enabled or has expired in generation {snapshot.GenerationId}", cached); } if (!TryBuildScope(nodeId, realm, snapshot.ClusterId, out var scope)) { return Deny(operation, nodeId, identity.Principal, - "the node id carries no string identifier, so it addresses no ACL scope"); + "the node id carries no string identifier, so it addresses no ACL scope", cached); } + var allowed = cached + ? Decisions.GetOrAdd( + snapshot.GenerationId, sessionKey, operation, realm, (string)nodeId.Identifier, + () => Walk(snapshot, identity, operation, scope, now)) + : Walk(snapshot, identity, operation, scope, now); + + if (allowed) return null; + + return Deny(operation, nodeId, identity.Principal, + $"no grant in generation {snapshot.GenerationId} confers {operation} here", cached); + } + + /// The grant walk itself — the only part of a decision that is safe to memoize. + /// The authority snapshot the decision is taken against. + /// The calling identity. + /// The operation being attempted. + /// The resolved node scope. + /// The current time, for the authorization state's resolution stamp. + /// when a grant confers the operation. + private bool Walk( + DataPlaneAuthoritySnapshot snapshot, + RoleCarryingUserIdentity identity, + OpcUaOperation operation, + NodeScope scope, + DateTime now) + { var session = new UserAuthorizationState { SessionId = identity.ApiKeyId ?? identity.Principal, - ClusterId = snapshot.ClusterId, + ClusterId = snapshot.ClusterId!, Principals = [identity.Principal], MembershipResolvedUtc = now, AuthGenerationId = snapshot.GenerationId, MembershipVersion = snapshot.GenerationId, + + // ⚠️ Disables the LDAP-era freshness ceiling explicitly, rather than relying on it never + // being reached. Two separate reasons, and the second is the one that bites: + // + // 1. It is meaningless here. The ceiling exists so a long-lived session re-resolves group + // membership a directory changed out of band. Nothing changes out of band under API keys + // — principals come from the key and both keys and grants ride the artifact — so a + // "stale membership" is not a state this model can be in. + // 2. It couples two clocks. MembershipResolvedUtc is stamped from THIS class's TimeProvider, + // while IsStale is evaluated against the EVALUATOR's. They are the same instance in + // production and diverge the moment anything injects a test clock — at which point the + // evaluator silently denies everything with a verdict indistinguishable from "no grant". + // That is how this was found: a fake clock two hours ahead made every decision NotGranted. + AuthCacheMaxStaleness = TimeSpan.MaxValue, }; - var decision = _evaluator.Authorize(session, operation, scope); - if (decision.IsAllowed) return null; - - return Deny(operation, nodeId, identity.Principal, - $"no grant in generation {snapshot.GenerationId} confers {operation} here"); + return _evaluator.Authorize(session, operation, scope).IsAllowed; } /// @@ -184,12 +307,18 @@ public sealed class DataPlaneAuthorizer return true; } - private ServiceResult Deny(OpcUaOperation operation, NodeId? nodeId, string? principal, string reason) + private ServiceResult Deny( + OpcUaOperation operation, NodeId? nodeId, string? principal, string reason, bool cached = false) { // Logged, never returned. The client gets an undifferentiated BadUserAccessDenied so the server // cannot be used to probe which nodes exist or which grants a principal holds; the operator // debugging a denial reads the reason here. Same posture as the authenticator's DenyMessage. - _log.LogInformation( + // + // The hot seams log at Debug because a single Browse of an ungranted folder would otherwise emit + // one Information line per child. Their operator-facing signal is the per-request summary the node + // manager logs, which says how many references were filtered and for whom. + _log.Log( + cached ? LogLevel.Debug : LogLevel.Information, "AclGate: denied {Operation} on {NodeId} for principal {Principal} — {Reason}", operation, nodeId, principal ?? "(none)", reason); return new ServiceResult(StatusCodes.BadUserAccessDenied); diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/Security/SessionAclDecisionCache.cs b/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/Security/SessionAclDecisionCache.cs new file mode 100644 index 00000000..545b481c --- /dev/null +++ b/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/Security/SessionAclDecisionCache.cs @@ -0,0 +1,112 @@ +using System.Collections.Concurrent; +using ZB.MOM.WW.OtOpcUa.Commons.OpcUa; +using ZB.MOM.WW.OtOpcUa.Core.Abstractions; + +namespace ZB.MOM.WW.OtOpcUa.OpcUaServer.Security; + +/// +/// Memoizes the grant-walk half of an ACL decision per (session, node, operation) for one +/// authorization generation (#520 stage 6). +/// +/// +/// +/// Why this exists. Stage 5 gated write, alarm commands and HistoryRead — all of which are +/// request-rate operations where rebuilding the decision each time costs nothing. Stage 6 gates +/// Read, Browse and subscription creation, which are not: one Browse of a device folder asks the +/// same question once per child, and a client that reads a thousand nodes asks it a thousand times. +/// The trie walk is cheap individually and the repetition is what makes it worth avoiding. +/// +/// +/// ⚠️ Only the grant walk is cached — never the key check. A cached verdict is valid because +/// grants, principals and the trie are all fixed for the life of a generation, so nothing the walk +/// reads can change without advancing. That reasoning does not +/// extend to the calling key's usability: a key's ExpiresUtc passes with the clock, not with +/// a deployment, so a cached allow would keep an expired key working until the next deploy — +/// re-introducing, in a subtler form, exactly the open-session hole stage 5 closed. +/// therefore runs revocation and expiry on every call and consults +/// this only afterwards. +/// +/// +/// Invalidation is wholesale, by generation. Per-entry invalidation would need to know which +/// nodes a changed grant covers, which is the trie walk run backwards; dropping everything on a +/// deploy costs one repopulation and cannot be wrong. Deployments are rare relative to reads. +/// +/// +public sealed class SessionAclDecisionCache +{ + /// + /// Entry ceiling, past which the cache clears rather than growing. Large enough that a realistic + /// address space browsed by a handful of machine consumers never reaches it, small enough that a + /// pathological client cannot use it to exhaust memory. + /// + /// + /// Clearing beats evicting here: an LRU needs per-entry bookkeeping on the hot path to save a trie + /// walk that is itself cheap, so the eviction policy would cost more than the miss it prevents. The + /// failure mode of clearing is a burst of recomputation, which is exactly the steady state of not + /// having a cache at all. + /// + public const int MaxEntries = 100_000; + + private readonly ConcurrentDictionary _entries = new(); + private long _generation = -1; + + /// The authorization generation the cached decisions were taken under. + public long Generation => Interlocked.Read(ref _generation); + + /// Number of decisions currently memoized. Diagnostics and tests. + public int Count => _entries.Count; + + /// + /// Returns the memoized decision for this key, computing and storing it when absent or when the + /// generation has moved on. + /// + /// The authorization generation the decision is being taken under. + /// Stable identity of the calling session — its key id, else its principal. + /// The operation being attempted. + /// The realm the node was resolved in. + /// The node's s= identifier. + /// The grant walk to run on a miss. + /// when the operation is permitted. + public bool GetOrAdd( + long generation, + string sessionKey, + OpcUaOperation operation, + AddressSpaceRealm realm, + string nodeIdentifier, + Func compute) + { + ArgumentNullException.ThrowIfNull(compute); + + InvalidateIfStale(generation); + + var key = new DecisionKey(sessionKey, operation, realm, nodeIdentifier); + if (_entries.TryGetValue(key, out var cached)) return cached; + + var decided = compute(); + + // Re-read the generation AFTER computing. A deploy landing mid-walk means the verdict may have been + // taken against either generation, and storing it under the new one would pin a decision the new + // trie never made. Dropping it costs one recomputation on the next call. + if (Generation != generation) return decided; + + if (_entries.Count >= MaxEntries) _entries.Clear(); + _entries[key] = decided; + return decided; + } + + /// Drops every memoized decision. Called when the authorization generation advances. + public void Clear() => _entries.Clear(); + + private void InvalidateIfStale(long generation) + { + var seen = Interlocked.Read(ref _generation); + if (seen == generation) return; + + // Only the thread that actually moves the stamp clears, so a burst of concurrent requests arriving + // just after a deploy repopulates once rather than each clearing the others' fresh entries. + if (Interlocked.CompareExchange(ref _generation, generation, seen) == seen) _entries.Clear(); + } + + private readonly record struct DecisionKey( + string SessionKey, OpcUaOperation Operation, AddressSpaceRealm Realm, string NodeIdentifier); +} diff --git a/tests/Core/ZB.MOM.WW.OtOpcUa.Core.Tests/Authorization/PermissionTrieBrowseTransitTests.cs b/tests/Core/ZB.MOM.WW.OtOpcUa.Core.Tests/Authorization/PermissionTrieBrowseTransitTests.cs new file mode 100644 index 00000000..f42e5592 --- /dev/null +++ b/tests/Core/ZB.MOM.WW.OtOpcUa.Core.Tests/Authorization/PermissionTrieBrowseTransitTests.cs @@ -0,0 +1,195 @@ +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.Configuration.Entities; +using ZB.MOM.WW.OtOpcUa.Configuration.Enums; +using ZB.MOM.WW.OtOpcUa.Core.Abstractions; +using ZB.MOM.WW.OtOpcUa.Core.Authorization; + +namespace ZB.MOM.WW.OtOpcUa.Core.Tests.Authorization; + +/// +/// #520 stage 6 — ancestor visibility for Browse. +/// +/// +/// +/// The trie inherits downward: a grant applies to the node it sits on and everything beneath it. +/// Browse needs the opposite relation as well, because a client reaches a deep node by browsing +/// its ancestors — so without transit, every grant narrower than the cluster is authored, stored, +/// shipped, and unreachable. +/// +/// +/// The tempting implementation is to plant a synthetic Browse grant on each ancestor. It is +/// wrong, and is the test that says so: +/// a planted grant is collected by the ordinary walk, and the ordinary walk is additive downward, +/// so a grant planted on Plant to reach Plant/Modbus/dev1 would also confer Browse on +/// Plant/Siemens/dev9 — silently widening the operator's rule to the whole tree. +/// +/// +[Trait("Category", "Unit")] +public sealed class PermissionTrieBrowseTransitTests +{ + private const string Cluster = "c1"; + private const string Principal = "p"; + + /// An ancestor of a Browse-granted node is browsable, so the grant can be navigated to. + [Fact] + public void An_ancestor_of_a_browse_grant_is_browsable() + { + var trie = TrieGranting(NodePermissions.Browse, "Plant", "Modbus", "dev1"); + + trie.HasBrowseTransit(RawScope("Plant"), [Principal]).ShouldBeTrue(); + trie.HasBrowseTransit(RawScope("Plant", "Modbus"), [Principal]).ShouldBeTrue(); + } + + /// + /// THE test. Transit must reach the granted node's ancestors and no further — never sideways into + /// a sibling subtree hanging off the same ancestor. + /// + /// + /// ⚠️ This goes through the evaluator, not through HasBrowseTransit directly, and the + /// distinction is the whole point. A sibling like Plant/Siemens does not exist in the trie at + /// all, so HasBrowseTransit answers false at the first missing child whatever the + /// implementation — probing it directly would pass against the broken design too. The leak the + /// naive "plant a Browse grant on each ancestor" implementation produces is in + /// , which collects grants at every level it walks and + /// would return Plant's synthetic grant on the way to a sibling. Only the evaluator sees + /// both halves. + /// + [Fact] + public void Transit_does_not_leak_into_a_sibling_subtree() + { + var trie = TrieGranting(NodePermissions.Browse, "Plant", "Modbus", "dev1"); + var evaluator = new TriePermissionEvaluator(CacheOf(trie)); + var session = Session(); + + evaluator.Authorize(session, OpcUaOperation.Browse, RawScope("Plant")).IsAllowed + .ShouldBeTrue("the ancestor must be navigable"); + + evaluator.Authorize(session, OpcUaOperation.Browse, RawScope("Plant", "Siemens")).IsAllowed + .ShouldBeFalse("an unrelated subtree under the same ancestor must not inherit the transit"); + evaluator.Authorize(session, OpcUaOperation.Browse, RawScope("Plant", "Siemens", "dev9")).IsAllowed + .ShouldBeFalse(); + } + + /// + /// Transit confers navigation only. An ancestor made visible so a deep grant can be reached does + /// not thereby become readable. + /// + [Fact] + public void Transit_confers_browse_and_nothing_else() + { + var trie = TrieGranting(NodePermissions.Browse | NodePermissions.Read, "Plant", "Modbus", "dev1"); + var evaluator = new TriePermissionEvaluator(CacheOf(trie)); + var session = Session(); + + evaluator.Authorize(session, OpcUaOperation.Browse, RawScope("Plant")).IsAllowed.ShouldBeTrue(); + + // Read on the ancestor is refused even though the descendant grant carries Read: the descendant's + // Read applies to the descendant, and transit is not a grant. + evaluator.Authorize(session, OpcUaOperation.Read, RawScope("Plant")).IsAllowed.ShouldBeFalse(); + } + + /// + /// A grant that does not carry Browse makes nothing browsable. Read-by-NodeId still works — + /// OPC UA does not require Browse to Read — but the node cannot be discovered by navigating, which + /// is what keeps the Browse bit meaningful. + /// + [Fact] + public void A_grant_without_the_browse_bit_marks_no_ancestors() + { + var trie = TrieGranting(NodePermissions.Read, "Plant", "Modbus", "dev1"); + + trie.HasBrowseTransit(RawScope("Plant"), [Principal]).ShouldBeFalse(); + trie.HasBrowseTransit(RawScope("Plant", "Modbus"), [Principal]).ShouldBeFalse(); + } + + /// Transit is per principal — another principal's grant does not open the path. + [Fact] + public void Transit_belongs_to_the_granted_principal_only() + { + var trie = TrieGranting(NodePermissions.Browse, "Plant", "Modbus", "dev1"); + + trie.HasBrowseTransit(RawScope("Plant"), ["someone-else"]).ShouldBeFalse(); + } + + /// + /// Realms are separate trees, so a raw-realm grant opens no path through the UNS realm even for an + /// identical segment sequence. + /// + [Fact] + public void Transit_does_not_cross_realms() + { + var trie = TrieGranting(NodePermissions.Browse, "Plant", "Modbus", "dev1"); + + trie.HasBrowseTransit( + new NodeScope { ClusterId = Cluster, Kind = NodeHierarchyKind.Uns, Segments = ["Plant"] }, + [Principal]).ShouldBeFalse(); + } + + /// + /// End to end through the evaluator: the granted node is browsable on its own grant, its ancestors + /// on transit, and its sibling on neither. This is the shape an operator actually authors. + /// + [Fact] + public void A_narrow_browse_grant_is_navigable_end_to_end() + { + var trie = TrieGranting(NodePermissions.Browse, "Plant", "Modbus", "dev1"); + var evaluator = new TriePermissionEvaluator(CacheOf(trie)); + var session = Session(); + + evaluator.Authorize(session, OpcUaOperation.Browse, RawScope("Plant")).IsAllowed.ShouldBeTrue(); + evaluator.Authorize(session, OpcUaOperation.Browse, RawScope("Plant", "Modbus")).IsAllowed.ShouldBeTrue(); + evaluator.Authorize(session, OpcUaOperation.Browse, RawScope("Plant", "Modbus", "dev1")).IsAllowed.ShouldBeTrue(); + + // Beneath the grant: ordinary downward inheritance, not transit. + evaluator.Authorize(session, OpcUaOperation.Browse, RawScope("Plant", "Modbus", "dev1", "Speed")) + .IsAllowed.ShouldBeTrue(); + + evaluator.Authorize(session, OpcUaOperation.Browse, RawScope("Plant", "Siemens")).IsAllowed.ShouldBeFalse(); + } + + // ───────────────────────────── helpers ───────────────────────────── + + private static NodeScope RawScope(params string[] segments) => + new() { ClusterId = Cluster, Kind = NodeHierarchyKind.Raw, Segments = segments }; + + private static UserAuthorizationState Session() => new() + { + SessionId = "s", + ClusterId = Cluster, + Principals = [Principal], + MembershipResolvedUtc = DateTime.UtcNow, + AuthGenerationId = 1, + MembershipVersion = 1, + }; + + private static PermissionTrieCache CacheOf(PermissionTrie trie) + { + var cache = new PermissionTrieCache(); + cache.Install(trie); + return cache; + } + + /// A trie with one raw-realm grant at the supplied path. + private static PermissionTrie TrieGranting(NodePermissions flags, params string[] segments) + { + var row = new NodeAcl + { + NodeAclRowId = Guid.NewGuid(), + NodeAclId = "acl-1", + ClusterId = Cluster, + Principal = Principal, + ScopeKind = NodeAclScopeKind.RawDevice, + ScopeId = "dev-1", + PermissionFlags = flags, + }; + + var scopePaths = new Dictionary> + { + [new AclScopeRef(NodeAclScopeKind.RawDevice, "dev-1")] = + [new NodeAclPath([NodeAclRealms.Raw, .. segments])], + }; + + return PermissionTrieBuilder.Build(Cluster, generationId: 1, [row], scopePaths); + } +} diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.IntegrationTests/AclSubscriptionEnforcementTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.IntegrationTests/AclSubscriptionEnforcementTests.cs new file mode 100644 index 00000000..a161e59b --- /dev/null +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.IntegrationTests/AclSubscriptionEnforcementTests.cs @@ -0,0 +1,448 @@ +using System.Net; +using System.Net.Sockets; +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.OpcUaServer.Security; + +namespace ZB.MOM.WW.OtOpcUa.OpcUaServer.IntegrationTests; + +/// +/// #520 stage 6 over the wire — browse filtering, subscription denial, and the one that could not be +/// settled from documentation: whether revoking a grant actually stops a live subscription. +/// +/// +/// +/// These are here rather than in the unit project because the mechanism under test lives in the SDK. +/// RevalidateMonitoredItems re-authorizes each item using +/// IMonitoredItem.EffectiveIdentity — the identity of the session that created it — and then +/// disables the item. Both halves are assumptions about SDK behaviour, and both fail silently in the +/// dangerous direction if wrong: an unrecoverable identity denies every item, which would disable +/// every subscription on the fleet the first time anyone deployed with the flag on. +/// +/// +/// The client connects anonymously, which under #520's D3 is a normal grantable principal +/// (anonymous) rather than a bypass — so these tests exercise the real grant path with no +/// key-minting ceremony. +/// +/// Run note: heavy in-process server+client tests; part of the serial integration pass. +/// +public sealed class AclSubscriptionEnforcementTests +{ + private const string Cluster = "SITE-A"; + private const string Device = "Plant/Modbus/dev1"; + private const string Tag = "Plant/Modbus/dev1/Speed"; + + /// + /// THE test this file exists for: a deployment that removes the grant stops an already-running + /// subscription, and tells the client why. + /// + /// + /// Without it, revocation would close the operations nobody uses (a one-shot read, an occasional + /// write) and leave the streaming feed — which is how ScadaBridge and every other machine consumer + /// actually reads this server — running indefinitely on a grant that no longer exists. + /// + [Fact] + public async Task Revoking_a_grant_stops_a_live_subscription() + { + var ct = TestContext.Current.CancellationToken; + var rig = await Rig.StartAsync("AclRevoke", ct); + try + { + var state = rig.Enforce(Grant(NodePermissions.Read | NodePermissions.Browse | NodePermissions.Subscribe)); + rig.Materialise(); + rig.Write(1); + + using var session = await rig.ConnectAsync(ct); + var (subscription, received) = await rig.SubscribeAsync(session, ct); + + // Baseline: the feed is live. + await WaitUntilAsync(() => received.Any(v => Equals(v.Value, 1f)), TimeSpan.FromSeconds(10)); + + received.Clear(); + + // The next deployment carries no grants at all. + state.Apply(Cluster, "rev-2", [], [], NoScopes); + + // The client is TOLD, rather than watching a feed go quiet for no stated reason. + await WaitUntilAsync( + () => received.Any(v => v.StatusCode.Code == StatusCodes.BadUserAccessDenied), + TimeSpan.FromSeconds(10)); + + // And the data really has stopped: a fresh value never arrives. + 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 positive control for the test above, and the one that would catch the failure mode that + /// worries me most: if the identity behind a monitored item could not be recovered, every item would + /// be denied and the fleet's subscriptions would all stop on the first deploy. Here a re-deploy that + /// KEEPS the grant must leave the feed running. + /// + [Fact] + public async Task A_redeploy_that_keeps_the_grant_leaves_the_subscription_running() + { + var ct = TestContext.Current.CancellationToken; + var rig = await Rig.StartAsync("AclKeep", ct); + try + { + var grant = Grant(NodePermissions.Read | NodePermissions.Browse | NodePermissions.Subscribe); + var state = rig.Enforce(grant); + 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(); + state.Apply(Cluster, "rev-2", [], [grant], NoScopes); + + 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(); } + } + + /// + /// A grant restored by a later deployment brings the feed back on its own, without the client + /// doing anything. + /// + /// + /// This is why a denied item is detached from its node's fan-out rather than set + /// MonitoringMode.Disabled. A disabled item can only be re-enabled by the client, and a + /// machine consumer has no reason to ever try — so an operator who revoked a grant by mistake and + /// put it straight back would have left the subscription dead until someone restarted the consumer. + /// + [Fact] + public async Task Restoring_a_grant_brings_the_subscription_back() + { + var ct = TestContext.Current.CancellationToken; + var rig = await Rig.StartAsync("AclRestore", ct); + try + { + var grant = Grant(NodePermissions.Read | NodePermissions.Browse | NodePermissions.Subscribe); + var state = rig.Enforce(grant); + 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)); + + state.Apply(Cluster, "rev-2", [], [], NoScopes); + await WaitUntilAsync( + () => received.Any(v => v.StatusCode.Code == StatusCodes.BadUserAccessDenied), + TimeSpan.FromSeconds(10)); + + // The operator puts the grant back. + received.Clear(); + state.Apply(Cluster, "rev-3", [], [grant], NoScopes); + + rig.Write(3); + await WaitUntilAsync(() => received.Any(v => Equals(v.Value, 3f)), TimeSpan.FromSeconds(10)); + + await subscription.DeleteAsync(true, ct); + } + finally { await rig.DisposeAsync(); } + } + + /// A principal with no Subscribe bit cannot create a monitored item at all. + [Fact] + public async Task Subscribing_without_the_permission_is_refused() + { + var ct = TestContext.Current.CancellationToken; + var rig = await Rig.StartAsync("AclNoSub", ct); + try + { + // Read and Browse, deliberately NOT Subscribe — so a failure here cannot be "the session was + // denied everything". + rig.Enforce(Grant(NodePermissions.Read | NodePermissions.Browse)); + rig.Materialise(); + rig.Write(1); + + using var session = await rig.ConnectAsync(ct); + + // The read works, proving the session is admitted and granted. + var read = await session.ReadValueAsync(rig.TagNodeId(session), ct); + read.StatusCode.Code.ShouldNotBe(StatusCodes.BadUserAccessDenied); + + var (subscription, _) = await rig.SubscribeAsync(session, ct, expectFailure: true); + subscription.MonitoredItems.First().Status.Error?.StatusCode.Code + .ShouldBe(StatusCodes.BadUserAccessDenied); + + await subscription.DeleteAsync(true, ct); + } + finally { await rig.DisposeAsync(); } + } + + /// + /// Browse returns only what the principal may see: a grant on one device makes that device visible + /// under its parent folder and its sibling invisible. + /// + /// + /// The parent folder itself must remain browsable — that is the ancestor-visibility ("transit") + /// rule. If it were not, the grant on the device below would be unreachable and the operator's rule + /// would silently do nothing. + /// + [Fact] + public async Task Browse_returns_only_the_granted_subtree() + { + var ct = TestContext.Current.CancellationToken; + var rig = await Rig.StartAsync("AclBrowse", ct); + try + { + var scoped = new NodeAcl + { + NodeAclId = "acl-dev1", + ClusterId = Cluster, + Principal = OpcUaPrincipals.Anonymous, + ScopeKind = NodeAclScopeKind.RawDevice, + ScopeId = "dev-1", + PermissionFlags = NodePermissions.Browse | NodePermissions.Read, + }; + var scopes = new Dictionary> + { + [new AclScopeRef(NodeAclScopeKind.RawDevice, "dev-1")] = + [new NodeAclPath([NodeAclRealms.Raw, "Plant", "Modbus", "dev1"])], + }; + + rig.Enforce(scoped, scopes); + rig.MaterialiseTwoDevices(); + + using var session = await rig.ConnectAsync(ct); + var ns = rig.RawNamespace(session); + + var children = await BrowseChildrenAsync(session, new NodeId("Plant/Modbus", ns), ct); + + children.ShouldContain(n => n == "Plant/Modbus/dev1"); + children.ShouldNotContain(n => n == "Plant/Modbus/dev9"); + } + finally { await rig.DisposeAsync(); } + } + + // ───────────────────────────── helpers ───────────────────────────── + + private static readonly IReadOnlyDictionary> NoScopes = + new Dictionary>(); + + private static NodeAcl Grant(NodePermissions flags) => new() + { + NodeAclId = "acl-cluster", + ClusterId = Cluster, + Principal = OpcUaPrincipals.Anonymous, + ScopeKind = NodeAclScopeKind.Cluster, + PermissionFlags = flags, + }; + + private static async Task> BrowseChildrenAsync( + ISession session, NodeId parent, CancellationToken ct) + { + var response = await session.BrowseAsync( + requestHeader: null, + view: null, + requestedMaxReferencesPerNode: 0, + nodesToBrowse: + [ + new BrowseDescription + { + NodeId = parent, + BrowseDirection = BrowseDirection.Forward, + ReferenceTypeId = ReferenceTypeIds.HierarchicalReferences, + IncludeSubtypes = true, + NodeClassMask = 0, + ResultMask = (uint)BrowseResultMask.All, + }, + ], + ct: ct); + + return response.Results[0].References + .Select(r => r.NodeId.Identifier as string ?? string.Empty) + .ToList(); + } + + 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 plus the ACL wiring, so each test states only what it is actually about. + private sealed class Rig : IAsyncDisposable + { + private readonly string _pkiRoot; + private OpcUaApplicationHost _host = null!; + private OtOpcUaSdkServer _server = null!; + 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); + + await rig._host.StartAsync(rig._server, ct); + return rig; + } + + /// Switches enforcement on with one grant, returning the state so a test can redeploy. + public DataPlaneAuthorityState Enforce( + NodeAcl grant, IReadOnlyDictionary>? scopes = null) + { + var state = new DataPlaneAuthorityState(); + state.Apply(Cluster, "rev-1", [], [grant], scopes ?? NoScopes); + + // BOTH must be set: Authorizer decides, and DataPlaneAuthority is what carries the Changed + // signal that reaches live subscriptions. Wiring only the first passes every test that makes a + // request and none that revokes. + Nm.DataPlaneAuthority = state; + Nm.Authorizer = new DataPlaneAuthorizer( + state, new TriePermissionEvaluator(state.Tries), enabled: true, NullLogger.Instance); + return state; + } + + 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 MaterialiseTwoDevices() + { + Materialise(); + Nm.EnsureFolder("Plant/Modbus/dev9", "Plant/Modbus", "dev9", AddressSpaceRealm.Raw); + Nm.EnsureVariable( + "Plant/Modbus/dev9/Torque", "Plant/Modbus/dev9", "Torque", "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, bool expectFailure = false) + { + 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); + + if (!expectFailure) item.Status.Error.ShouldBeNull(); + + // The list is only ever appended under `gate` and read from the test thread between awaits; + // a snapshot-on-read wrapper would hide ordering rather than help. + return (subscription, received); + } + + public async Task ConnectAsync(CancellationToken ct) + { + var appConfig = new ApplicationConfiguration + { + ApplicationName = "OtOpcUa.AclClient", + ApplicationUri = $"urn:OtOpcUa.AclClient.{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: "AclSubscriptionEnforcementTests", + sessionTimeout: 60_000, + identity: new UserIdentity(new AnonymousIdentityToken()), + preferredLocales: null, + ct: ct); + } + + public async ValueTask DisposeAsync() + { + 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; + } + } +} diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/NodeManagerStage6EnforcementTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/NodeManagerStage6EnforcementTests.cs new file mode 100644 index 00000000..595c677f --- /dev/null +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/NodeManagerStage6EnforcementTests.cs @@ -0,0 +1,384 @@ +using Microsoft.Extensions.Logging.Abstractions; +using Opc.Ua; +using Opc.Ua.Server; +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.Abstractions; +using ZB.MOM.WW.OtOpcUa.Core.Authorization; +using ZB.MOM.WW.OtOpcUa.OpcUaServer.Security; + +namespace ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests; + +/// +/// #520 stage 6 — the Read seam, through a REAL booted and the SDK's own +/// dispatch, plus the cache properties that only show up at a request-rate seam. +/// +/// +/// Browse filtering, subscription denial and revocation-reaching-a-live-subscription are proven over +/// the wire in AclSubscriptionEnforcementTests in the integration project instead — they need a +/// real client session, and the parts of them most likely to be wrong (whether a monitored item can +/// even recover the identity that created it) cannot be observed without one. +/// +public sealed class NodeManagerStage6EnforcementTests : IDisposable +{ + private const string Cluster = "SITE-A"; + private const string Principal = "scadabridge"; + + private static CancellationToken Ct => TestContext.Current.CancellationToken; + + private readonly string _pkiRoot = Path.Combine( + Path.GetTempPath(), $"otopcua-acl6-{Guid.NewGuid():N}"); + + /// + /// With enforcement off, Read is exactly what it always was. This is the flag's promise and the + /// assertion that catches an accidental enforce-by-default. + /// + [Fact] + public async Task Read_is_ungated_while_enforcement_is_off() + { + var (host, server) = await BootAsync(); + try + { + var nm = server.NodeManager!; + nm.Authorizer = Authorizer(enabled: false, GrantOn(NodePermissions.None)); + var nodeId = Variable(nm, "Plant/Modbus/dev1/Speed"); + + var (values, errors) = Read(nm, identity: null, nodeId); + + errors[0].StatusCode.Code.ShouldNotBe(StatusCodes.BadUserAccessDenied); + values[0].StatusCode.Code.ShouldNotBe(StatusCodes.BadUserAccessDenied); + } + finally { await host.DisposeAsync(); } + } + + /// + /// With enforcement on, a session whose grant lacks the Read bit is refused — the check Read never + /// had. The identity is real and the grant non-empty, so this cannot pass on the null-identity + /// branch alone. + /// + [Fact] + public async Task Read_is_denied_without_the_permission() + { + var (host, server) = await BootAsync(); + try + { + var nm = server.NodeManager!; + nm.Authorizer = Authorizer(enabled: true, GrantOn(NodePermissions.Browse)); + var nodeId = Variable(nm, "Plant/Modbus/dev1/Speed"); + + var (values, errors) = Read(nm, Identity(), nodeId); + + errors[0].StatusCode.Code.ShouldBe(StatusCodes.BadUserAccessDenied); + values[0].StatusCode.Code.ShouldBe(StatusCodes.BadUserAccessDenied); + } + finally { await host.DisposeAsync(); } + } + + /// The positive control: a granted principal reads normally, so the gate is not denying all. + [Fact] + public async Task Read_succeeds_for_a_granted_principal() + { + var (host, server) = await BootAsync(); + try + { + var nm = server.NodeManager!; + nm.Authorizer = Authorizer(enabled: true, GrantOn(NodePermissions.Read)); + var nodeId = Variable(nm, "Plant/Modbus/dev1/Speed"); + + var (values, errors) = Read(nm, Identity(), nodeId); + + errors[0].StatusCode.Code.ShouldNotBe(StatusCodes.BadUserAccessDenied); + values[0].StatusCode.Code.ShouldNotBe(StatusCodes.BadUserAccessDenied); + } + finally { await host.DisposeAsync(); } + } + + /// + /// Every attribute is gated, not just Value. Allowing an ungranted session to read BrowseName, + /// DataType and Description would disclose most of what the address space actually reveals. + /// + [Fact] + public async Task A_denied_node_discloses_no_attribute() + { + var (host, server) = await BootAsync(); + try + { + var nm = server.NodeManager!; + nm.Authorizer = Authorizer(enabled: true, GrantOn(NodePermissions.Browse)); + var nodeId = Variable(nm, "Plant/Modbus/dev1/Speed"); + + var (values, _) = Read( + nm, Identity(), + (nodeId, Attributes.Value), + (nodeId, Attributes.BrowseName), + (nodeId, Attributes.DataType), + (nodeId, Attributes.Description)); + + values.ShouldAllBe(v => v.StatusCode.Code == StatusCodes.BadUserAccessDenied); + } + finally { await host.DisposeAsync(); } + } + + /// + /// A mixed batch is decided per node — the granted node answers and the ungranted one is denied in + /// the same request, rather than the whole read failing. + /// + [Fact] + public async Task A_mixed_read_batch_denies_only_the_ungranted_node() + { + var (host, server) = await BootAsync(); + try + { + var nm = server.NodeManager!; + var granted = Variable(nm, "Plant/Modbus/dev1/Speed"); + var ungranted = Variable(nm, "Plant/Siemens/dev9/Torque"); + + nm.Authorizer = Authorizer( + enabled: true, + ScopedGrant(NodePermissions.Read, "dev-1"), + Scopes(("dev-1", ["Plant", "Modbus", "dev1"]))); + + var (_, errors) = Read(nm, Identity(), granted, ungranted); + + errors[0].StatusCode.Code.ShouldNotBe(StatusCodes.BadUserAccessDenied); + errors[1].StatusCode.Code.ShouldBe(StatusCodes.BadUserAccessDenied); + } + finally { await host.DisposeAsync(); } + } + + /// + /// ⚠️ The cache must not memoize the key check. Grants are fixed for a generation, so caching the + /// grant walk is sound; a key's expiry passes with the CLOCK, so a cached allow would keep an + /// expired key reading until the next deployment. + /// + /// + /// The first read is what makes this test meaningful: it populates the cache with an allow, + /// so the second read is answered by a code path that has a cached "yes" available and must refuse + /// to use it. Without the priming read, a build that cached the whole decision would still pass. + /// + [Fact] + public async Task An_expired_key_is_denied_even_though_its_allow_is_cached() + { + var (host, server) = await BootAsync(); + try + { + var nm = server.NodeManager!; + var clock = new FakeTimeProvider(new DateTime(2026, 8, 1, 12, 0, 0, DateTimeKind.Utc)); + + var state = new DataPlaneAuthorityState(); + var key = new ApiKey + { + ApiKeyId = "k1", + Name = "k1", + SecretHash = "hash", + Principal = Principal, + Enabled = true, + CreatedUtc = new DateTime(2026, 8, 1, 0, 0, 0, DateTimeKind.Utc), + ExpiresUtc = new DateTime(2026, 8, 1, 13, 0, 0, DateTimeKind.Utc), + }; + state.Apply(Cluster, "rev-1", [key], [GrantOn(NodePermissions.Read)], Scopes()); + + nm.Authorizer = new DataPlaneAuthorizer( + state, new TriePermissionEvaluator(state.Tries), enabled: true, NullLogger.Instance, clock); + + var nodeId = Variable(nm, "Plant/Modbus/dev1/Speed"); + var identity = new RoleCarryingUserIdentity( + new UserNameIdentityToken { UserName = "apikey" }, [], Principal, apiKeyId: "k1"); + + // Prime the cache with an allow. + Read(nm, identity, nodeId).Errors[0].StatusCode.Code.ShouldNotBe(StatusCodes.BadUserAccessDenied); + nm.Authorizer.Decisions.Count.ShouldBeGreaterThan(0, "the allow must actually be cached"); + + clock.Advance(TimeSpan.FromHours(2)); + + Read(nm, identity, nodeId).Errors[0].StatusCode.Code.ShouldBe(StatusCodes.BadUserAccessDenied); + } + finally { await host.DisposeAsync(); } + } + + /// + /// A deployment that removes a grant reaches an already-open session's reads, because the new + /// generation invalidates the cached allow rather than the session carrying its old verdict. + /// + [Fact] + public async Task A_revoked_grant_reaches_an_open_sessions_reads() + { + var (host, server) = await BootAsync(); + try + { + var nm = server.NodeManager!; + var state = new DataPlaneAuthorityState(); + state.Apply(Cluster, "rev-1", [], [GrantOn(NodePermissions.Read)], Scopes()); + nm.Authorizer = new DataPlaneAuthorizer( + state, new TriePermissionEvaluator(state.Tries), enabled: true, NullLogger.Instance); + + var nodeId = Variable(nm, "Plant/Modbus/dev1/Speed"); + + Read(nm, Identity(), nodeId).Errors[0].StatusCode.Code + .ShouldNotBe(StatusCodes.BadUserAccessDenied); + + // The next deployment carries no grants at all. + state.Apply(Cluster, "rev-2", [], [], Scopes()); + + Read(nm, Identity(), nodeId).Errors[0].StatusCode.Code.ShouldBe(StatusCodes.BadUserAccessDenied); + } + finally { await host.DisposeAsync(); } + } + + + /// + /// Event subscriptions are gated, and the gate resolves its source through the SDK's own + /// IsHandleInNamespace rather than a cast. + /// + /// + /// + /// ⚠️ This exists because the first version of that gate read sourceId as NodeState. + /// sourceId is the manager handle, so the cast compiled, always yielded null, and + /// silently disabled the gate — the same object-typed trap as + /// ContinuationPoint.NodeToBrowse. Nothing about the code looked wrong. + /// + /// + /// The monitored item is deliberately : a denial must be reached without + /// the gate touching the item at all. That also makes the test a discriminator rather than a + /// restatement — against the broken cast the gate falls through to the base implementation, + /// which does dereference the item, so the test fails loudly instead of passing. + /// + /// + [Fact] + public async Task Event_subscriptions_are_gated_on_the_notifier() + { + var (host, server) = await BootAsync(); + try + { + var nm = server.NodeManager!; + nm.Authorizer = Authorizer(enabled: true, GrantOn(NodePermissions.Read)); + var nodeId = Variable(nm, "Plant/Modbus/dev1/Speed"); + + var context = new OperationContext( + new RequestHeader(), secureChannelContext: null, RequestType.CreateMonitoredItems, Identity()); + + var result = nm.SubscribeToEvents( + context, nm.GetManagerHandle(nodeId), subscriptionId: 1, monitoredItem: null!, unsubscribe: false); + + result.StatusCode.Code.ShouldBe(StatusCodes.BadUserAccessDenied); + } + finally { await host.DisposeAsync(); } + } + + // ───────────────────────────── helpers ───────────────────────────── + + private static NodeId Variable(OtOpcUaNodeManager nm, string rawPath) + { + nm.EnsureVariable(rawPath, parentFolderNodeId: null, displayName: rawPath, dataType: "Float", + writable: false, historianTagname: null, realm: AddressSpaceRealm.Raw); + return nm.TryGetVariable(rawPath, AddressSpaceRealm.Raw)!.NodeId; + } + + private static (List Values, List Errors) Read( + OtOpcUaNodeManager nm, IUserIdentity? identity, params NodeId[] nodeIds) => + Read(nm, identity, nodeIds.Select(id => (id, Attributes.Value)).ToArray()); + + private static (List Values, List Errors) Read( + OtOpcUaNodeManager nm, IUserIdentity? identity, params (NodeId Node, uint Attribute)[] reads) + { + var context = new OperationContext( + new RequestHeader(), secureChannelContext: null, RequestType.Read, identity); + + var nodesToRead = reads + .Select(r => new ReadValueId { NodeId = r.Node, AttributeId = r.Attribute }) + .ToList(); + var values = nodesToRead.Select(_ => new DataValue()).ToList(); + var errors = Enumerable.Repeat(null!, nodesToRead.Count).ToList(); + + nm.Read(context, 0, nodesToRead, values, errors); + return (values, errors); + } + + private static DataPlaneAuthorizer Authorizer( + bool enabled, + NodeAcl grant, + IReadOnlyDictionary>? scopePaths = null) + { + var state = new DataPlaneAuthorityState(); + state.Apply(Cluster, "rev-1", [], [grant], scopePaths ?? Scopes()); + return new DataPlaneAuthorizer( + state, new TriePermissionEvaluator(state.Tries), enabled, NullLogger.Instance); + } + + private static IReadOnlyDictionary> Scopes( + params (string ScopeId, string[] Segments)[] entries) => + entries.ToDictionary( + e => new AclScopeRef(NodeAclScopeKind.RawDevice, e.ScopeId), + e => (IReadOnlyList)new[] { new NodeAclPath([NodeAclRealms.Raw, .. e.Segments]) }); + + private static NodeAcl GrantOn(NodePermissions flags) => new() + { + NodeAclId = "acl-cluster", + ClusterId = Cluster, + Principal = Principal, + ScopeKind = NodeAclScopeKind.Cluster, + PermissionFlags = flags, + }; + + private static NodeAcl ScopedGrant(NodePermissions flags, string scopeId) => new() + { + NodeAclId = "acl-device", + ClusterId = Cluster, + Principal = Principal, + ScopeKind = NodeAclScopeKind.RawDevice, + ScopeId = scopeId, + PermissionFlags = flags, + }; + + private static RoleCarryingUserIdentity Identity() => + new(new UserNameIdentityToken { UserName = "apikey" }, [], Principal, apiKeyId: null); + + private async Task<(OpcUaApplicationHost Host, OtOpcUaSdkServer Server)> BootAsync() + { + var host = new OpcUaApplicationHost( + new OpcUaApplicationHostOptions + { + ApplicationName = "OtOpcUa.Acl6Test", + ApplicationUri = $"urn:OtOpcUa.Acl6Test:{Guid.NewGuid():N}", + OpcUaPort = AllocateFreePort(), + PublicHostname = "localhost", + PkiStoreRoot = _pkiRoot, + }, + NullLogger.Instance); + + var server = new OtOpcUaSdkServer(); + await host.StartAsync(server, Ct); + return (host, server); + } + + private static int AllocateFreePort() + { + using var listener = new System.Net.Sockets.TcpListener(System.Net.IPAddress.Loopback, 0); + listener.Start(); + var port = ((System.Net.IPEndPoint)listener.LocalEndpoint).Port; + listener.Stop(); + return port; + } + + private sealed class FakeTimeProvider(DateTime utcNow) : TimeProvider + { + private DateTimeOffset _now = new(utcNow, TimeSpan.Zero); + + public override DateTimeOffset GetUtcNow() => _now; + + public void Advance(TimeSpan by) => _now = _now.Add(by); + } + + /// Cleans up the PKI root directory. + public void Dispose() + { + if (Directory.Exists(_pkiRoot)) + { + try { Directory.Delete(_pkiRoot, recursive: true); } + catch (IOException) { /* best effort */ } + } + } +} diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/SessionAclDecisionCacheTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/SessionAclDecisionCacheTests.cs new file mode 100644 index 00000000..1b6eaba1 --- /dev/null +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/SessionAclDecisionCacheTests.cs @@ -0,0 +1,163 @@ +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.Commons.OpcUa; +using ZB.MOM.WW.OtOpcUa.Core.Abstractions; +using ZB.MOM.WW.OtOpcUa.OpcUaServer.Security; + +namespace ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests; + +/// +/// #520 stage 6 — the decision cache behind the request-rate seams. +/// +/// +/// The risk in a cache like this is not that it is slow; it is that it answers a question slightly +/// different from the one asked. Each test below pins one dimension the key must discriminate on, +/// because a key that collides across any of them hands one session another's authority. +/// +public sealed class SessionAclDecisionCacheTests +{ + private const string Session = "key-1"; + private const string Node = "Plant/Modbus/dev1/Speed"; + + /// The second identical question is answered from the cache. + [Fact] + public void A_repeated_question_is_not_recomputed() + { + var cache = new SessionAclDecisionCache(); + var calls = 0; + + for (var i = 0; i < 5; i++) + Ask(cache, generation: 1, compute: () => { calls++; return true; }).ShouldBeTrue(); + + calls.ShouldBe(1); + } + + /// + /// A new generation drops everything. This is the invalidation the whole design rests on: grants + /// only change by deployment, and a deployment always advances the generation. + /// + [Fact] + public void A_new_generation_invalidates_every_decision() + { + var cache = new SessionAclDecisionCache(); + + Ask(cache, generation: 1, compute: () => true).ShouldBeTrue(); + cache.Count.ShouldBe(1); + + // The same question, one deployment later, must be re-decided — and the new answer must win. + Ask(cache, generation: 2, compute: () => false).ShouldBeFalse(); + cache.Generation.ShouldBe(2); + } + + /// + /// Two sessions asking about the same node get their own answers. A key that omitted the session + /// would let the first caller's verdict decide for everyone. + /// + [Fact] + public void Two_sessions_do_not_share_a_decision() + { + var cache = new SessionAclDecisionCache(); + + Ask(cache, generation: 1, session: "granted-key", compute: () => true).ShouldBeTrue(); + Ask(cache, generation: 1, session: "ungranted-key", compute: () => false).ShouldBeFalse(); + } + + /// Read and Write on one node are separate questions. + [Fact] + public void Two_operations_do_not_share_a_decision() + { + var cache = new SessionAclDecisionCache(); + + Ask(cache, generation: 1, operation: OpcUaOperation.Read, compute: () => true).ShouldBeTrue(); + Ask(cache, generation: 1, operation: OpcUaOperation.WriteOperate, compute: () => false).ShouldBeFalse(); + } + + /// + /// The two realms are separate namespaces that can carry the SAME s= identifier, so the realm + /// is part of the key. Without it, a raw grant would answer for the UNS node of the same path — + /// which is exactly the cross-realm confusion the trie itself is careful to avoid. + /// + [Fact] + public void The_two_realms_do_not_share_a_decision() + { + var cache = new SessionAclDecisionCache(); + + Ask(cache, generation: 1, realm: AddressSpaceRealm.Raw, compute: () => true).ShouldBeTrue(); + Ask(cache, generation: 1, realm: AddressSpaceRealm.Uns, compute: () => false).ShouldBeFalse(); + } + + /// Different nodes are different questions. + [Fact] + public void Two_nodes_do_not_share_a_decision() + { + var cache = new SessionAclDecisionCache(); + + Ask(cache, generation: 1, node: "Plant/Modbus/dev1/Speed", compute: () => true).ShouldBeTrue(); + Ask(cache, generation: 1, node: "Plant/Modbus/dev1/Torque", compute: () => false).ShouldBeFalse(); + } + + /// + /// Node identifiers are path segments, so they compare ordinally — Speed and speed can + /// both exist under one device and must not collide in the cache when they do not collide in the + /// trie. + /// + [Fact] + public void Node_identifiers_are_case_sensitive() + { + var cache = new SessionAclDecisionCache(); + + Ask(cache, generation: 1, node: "Plant/dev1/Speed", compute: () => true).ShouldBeTrue(); + Ask(cache, generation: 1, node: "Plant/dev1/speed", compute: () => false).ShouldBeFalse(); + } + + /// + /// The cache is bounded. A client walking a huge address space must not be able to grow it without + /// limit; clearing at the ceiling degrades to recomputation, which is the no-cache steady state. + /// + [Fact] + public void The_cache_is_bounded() + { + var cache = new SessionAclDecisionCache(); + + for (var i = 0; i <= SessionAclDecisionCache.MaxEntries + 10; i++) + Ask(cache, generation: 1, node: $"Plant/dev1/Tag{i}", compute: () => true); + + cache.Count.ShouldBeLessThanOrEqualTo(SessionAclDecisionCache.MaxEntries); + } + + /// + /// A decision taken while a deployment lands mid-walk is returned but NOT stored, so the new + /// generation never inherits a verdict its own trie did not make. + /// + [Fact] + public void A_decision_overtaken_by_a_deploy_is_not_stored() + { + var cache = new SessionAclDecisionCache(); + + // The compute callback simulates the deploy landing while the walk is in flight. + var decided = cache.GetOrAdd( + generation: 1, Session, OpcUaOperation.Read, AddressSpaceRealm.Raw, Node, + () => + { + Ask(cache, generation: 2, node: "unrelated", compute: () => true); + return true; + }); + + decided.ShouldBeTrue("the caller still gets the answer its own walk produced"); + + // Nothing was filed under the superseded generation — the only entry is the unrelated one the + // callback itself made. + cache.Generation.ShouldBe(2); + cache.Count.ShouldBe(1); + } + + private static bool Ask( + SessionAclDecisionCache cache, + long generation, + Func compute, + string session = Session, + OpcUaOperation operation = OpcUaOperation.Read, + AddressSpaceRealm realm = AddressSpaceRealm.Raw, + string node = Node) + => cache.GetOrAdd(generation, session, operation, realm, node, compute); +}