From 00aa4866777fe47a726542cb930821a336faddfa Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Sat, 1 Aug 2026 09:22:12 -0400 Subject: [PATCH 1/5] feat(security): enforce node ACLs on read, browse and subscribe (#520 stage 6) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Completes data-plane enforcement behind Security:Acl:Enabled. Read (every attribute, not just Value), Browse, TranslateBrowsePath, CreateMonitoredItems, SubscribeToEvents and SetMonitoringMode now consult scoped NodeAcl grants. With the flag off nothing changes. Four things went beyond the stage as written, each because leaving it out would have made the feature look like it worked: - Browse needed 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, shipped and unreachable. It is a separate non-inheriting marker, NOT a synthesized Browse grant on each ancestor: the additive downward walk would carry such a grant from Plant onto Plant/Siemens/dev9 and every other sibling subtree. Pinned by a test that goes through the evaluator — probing the trie helper directly passes against the broken design, because a sibling is absent from the trie either way. - Revocation had to reach LIVE subscriptions. Every other operation re-asks per request; a subscription is authorized once and then streams, and streaming is how machine consumers actually read this server — so stopping only the re-asking operations would have made revocation close to cosmetic. IDataPlaneAuthoritySource gained a Changed event. 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. The over-the-wire test caught this; 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. - The decision 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, so caching the whole decision would let an expired key keep working until the next deploy — the stage-5 open-session hole in a subtler form. - TranslateBrowsePath is gated, without which browse filtering is decorative: a client refused a browse asks TranslateBrowsePathsToNodeIds for the same path. Found 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 cannot fire was accidentally true; they are now disabled explicitly. Also fixed: ContinuationPoint.NodeToBrowse is the manager HANDLE, not a NodeState. The declared type is object, so the wrong cast compiled cleanly and silently yielded null — skipping the browse source gate entirely. Falsified: disabling the transit marking reddens exactly the three tests that assert transit works; implementing transit as an ancestor grant reddens the sibling-leak test; skipping the fan-out detach reddens the revocation test. Full solution suite green. The stage 8 live gate has NOT run yet. Claude-Session: https://claude.ai/code/session_015p7wGqy3YpZNCpDzTpGMKo --- CLAUDE.md | 42 +- ...026-07-31-apikey-acl-enforcement-design.md | 37 +- docs/security.md | 55 ++- .../Authorization/DataPlaneAuthority.cs | 75 ++- .../Authorization/PermissionTrie.cs | 70 +++ .../Authorization/PermissionTrieBuilder.cs | 9 + .../Authorization/TriePermissionEvaluator.cs | 23 +- .../Components/Pages/Clusters/AclEdit.razor | 16 +- .../Pages/Clusters/ClusterAcls.razor | 17 +- .../OtOpcUaNodeManager.cs | 451 +++++++++++++++++- .../Security/DataPlaneAuthorizer.cs | 169 ++++++- .../Security/SessionAclDecisionCache.cs | 112 +++++ .../PermissionTrieBrowseTransitTests.cs | 195 ++++++++ .../AclSubscriptionEnforcementTests.cs | 448 +++++++++++++++++ .../NodeManagerStage6EnforcementTests.cs | 345 ++++++++++++++ .../SessionAclDecisionCacheTests.cs | 163 +++++++ 16 files changed, 2157 insertions(+), 70 deletions(-) create mode 100644 src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/Security/SessionAclDecisionCache.cs create mode 100644 tests/Core/ZB.MOM.WW.OtOpcUa.Core.Tests/Authorization/PermissionTrieBrowseTransitTests.cs create mode 100644 tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.IntegrationTests/AclSubscriptionEnforcementTests.cs create mode 100644 tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/NodeManagerStage6EnforcementTests.cs create mode 100644 tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/SessionAclDecisionCacheTests.cs 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/docs/plans/2026-07-31-apikey-acl-enforcement-design.md b/docs/plans/2026-07-31-apikey-acl-enforcement-design.md index 5eaa6665..37de7222 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 diff --git a/docs/security.md b/docs/security.md index f69b1b2e..c7038c70 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,7 @@ 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 | **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 +333,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 +351,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 +411,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.OpcUaServer/OtOpcUaNodeManager.cs b/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/OtOpcUaNodeManager.cs index 2dc2e608..13d733fa 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,414 @@ 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); + + var before = references.Count; + base.Browse(context, ref continuationPoint, references); + + 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, references.Count + filtered); + } + + /// + /// + /// 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) + { + // 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 && sourceId is NodeState 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..9c4256b2 --- /dev/null +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/NodeManagerStage6EnforcementTests.cs @@ -0,0 +1,345 @@ +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(); } + } + + + // ───────────────────────────── 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); +} From ce404df970df3d05ffeb12380b7cd13ab26d33f1 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Sat, 1 Aug 2026 09:39:51 -0400 Subject: [PATCH 2/5] fix(security): count only this page's references in the browse-filter summary MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit On a BrowseNext the references list already carries the earlier pages, so the summary's denominator counted them again. Log accuracy only — the filtering itself was already scoped to this call's appended range. Claude-Session: https://claude.ai/code/session_015p7wGqy3YpZNCpDzTpGMKo --- docker-dev/docker-compose.yml | 12 ++++++++++++ .../OtOpcUaNodeManager.cs | 5 ++++- 2 files changed, 16 insertions(+), 1 deletion(-) 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/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/OtOpcUaNodeManager.cs b/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/OtOpcUaNodeManager.cs index 13d733fa..ecbdc79c 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/OtOpcUaNodeManager.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/OtOpcUaNodeManager.cs @@ -2754,8 +2754,11 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2 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--) @@ -2773,7 +2776,7 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2 filtered++; } - LogAclFiltering("Browse", identity, filtered, references.Count + filtered); + LogAclFiltering("Browse", identity, filtered, returned); } /// From 8cdbcb982363c36170bee7f941cd111ec78f06c9 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Sat, 1 Aug 2026 09:42:32 -0400 Subject: [PATCH 3/5] fix(security): resolve the event-subscribe gate's source through IsHandleInNamespace MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit SubscribeToEvents' sourceId is the manager HANDLE, not a NodeState — the same object-typed trap as ContinuationPoint.NodeToBrowse, found the same way. The `sourceId as NodeState` cast compiled, always yielded null, and silently disabled the gate: an ungranted session could subscribe to an equipment's event notifier and receive every alarm it raises, including the condition fields, while every value on it was refused. Now uses the SDK's own IsHandleInNamespace, which cannot be wrong about the type. The new test passes a null monitored item deliberately: the gate must deny without touching the item, which also makes the test a discriminator rather than a restatement — against the broken cast the call falls through to the base implementation, which does dereference it, so the test fails loudly. Claude-Session: https://claude.ai/code/session_015p7wGqy3YpZNCpDzTpGMKo --- .../OtOpcUaNodeManager.cs | 8 +++- .../NodeManagerStage6EnforcementTests.cs | 39 +++++++++++++++++++ 2 files changed, 46 insertions(+), 1 deletion(-) diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/OtOpcUaNodeManager.cs b/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/OtOpcUaNodeManager.cs index ecbdc79c..ed397b0a 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/OtOpcUaNodeManager.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/OtOpcUaNodeManager.cs @@ -2877,9 +2877,15 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2 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 && sourceId is NodeState source + if (AclEnforced && !unsubscribe + && IsHandleInNamespace(sourceId) is { } source && !Permits(IdentityOf(context), OpcUaOperation.CreateMonitoredItems, source.NodeId)) { return StatusCodes.BadUserAccessDenied; diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/NodeManagerStage6EnforcementTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/NodeManagerStage6EnforcementTests.cs index 9c4256b2..595c677f 100644 --- a/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/NodeManagerStage6EnforcementTests.cs +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/NodeManagerStage6EnforcementTests.cs @@ -229,6 +229,45 @@ public sealed class NodeManagerStage6EnforcementTests : IDisposable } + /// + /// 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) From 2aa42f90eb55f354d5a099c9c7a2bdcd6b709ad7 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Sat, 1 Aug 2026 09:43:40 -0400 Subject: [PATCH 4/5] fix(security): the ACL startup warning described stage 5's scope, not stage 6's MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit It told the operator "Read, Browse and Subscribe remain ungated" — which stage 6 is precisely the change to. A warning that misstates what a security control does is worse than no warning, because it is read as authoritative. Claude-Session: https://claude.ai/code/session_015p7wGqy3YpZNCpDzTpGMKo --- .../OpcUa/OtOpcUaServerHostedService.cs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) 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 From c4f537a60c0c350333eb8fd5804de3fdb15eb1bc Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Sat, 1 Aug 2026 09:52:42 -0400 Subject: [PATCH 5/5] =?UTF-8?q?docs(security):=20record=20the=20stage=206?= =?UTF-8?q?=20live=20gate=20=E2=80=94=20PASSED=20on=20docker-dev?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit site-a-1 (driver-only, no ConfigDb) with Security:Acl:Enabled=true; central-1 on the same artifact as the A/B control. 11 legs. The two that only a live rig could settle, as a transcript: 13:49:39 SiteAOnly = 53 (0x00000000) streaming ~1/s 13:49:40 SiteAOnly = (0x801F0000) revoking deploy lands; client TOLD … 25 seconds of silence, ~25 updates that never arrived … 13:50:05 SiteAOnly = 157 (0x00000000) restoring deploy; feed resumes 53 -> 157 across the gap: the device counter ran throughout, so delivery stopped, not the source. The 0x801F0000 frame is exactly what the MonitoringMode.Disabled implementation would have swallowed. Also recorded: the leg that distinguishes working enforcement from a trie that denies everything is resolvedScopes=1 in the install log, not any denial. Stated plainly as NOT covered: a UNS-realm grant (the rig's SITE-A has no UNS equipment) and alarm-command scoping (no alarms there). Both covered offline, neither seen live. Two rig facts worth keeping: :9200 round-robins central-1/central-2, and a stale central-2 rejected the deploy outright because its image predated the stage-3 scope kinds — image parity must be asserted per container, not assumed from one build. And SITEA-modbus had an empty DriverConfig, so it ran as a stub serving nothing; until that was fixed every read returned BadWaitingForInitialData and no allow leg could be told from a denial. Rig left as found. Claude-Session: https://claude.ai/code/session_015p7wGqy3YpZNCpDzTpGMKo --- ...026-07-31-apikey-acl-enforcement-design.md | 59 ++++++++++++++++--- docs/security.md | 6 ++ 2 files changed, 57 insertions(+), 8 deletions(-) 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 37de7222..20a9742d 100644 --- a/docs/plans/2026-07-31-apikey-acl-enforcement-design.md +++ b/docs/plans/2026-07-31-apikey-acl-enforcement-design.md @@ -296,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 c7038c70..d5920caa 100644 --- a/docs/security.md +++ b/docs/security.md @@ -320,6 +320,12 @@ takes no realm and no node. |---|---|---| | `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 the seams consult. Same dark-switch discipline as `MeshTransport`, `ConfigSource` and `Telemetry`.