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 f9cf8447..68870287 100644
--- a/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/Security/DataPlaneAuthorizer.cs
+++ b/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/Security/DataPlaneAuthorizer.cs
@@ -54,6 +54,25 @@ namespace ZB.MOM.WW.OtOpcUa.OpcUaServer.Security;
///
public sealed class DataPlaneAuthorizer
{
+ ///
+ /// The single veto every denial returns. Logged, never explained to the client: the client gets an
+ /// undifferentiated BadUserAccessDenied so the server cannot be used to probe which nodes
+ /// exist or which grants a principal holds, and the operator debugging a denial reads the reason in
+ /// the log. Same posture as the authenticator's DenyMessage.
+ ///
+ ///
+ /// Shared rather than per-call, because is immutable to us.
+ /// Verified by reflection against the pinned Opc.Ua.Core 1.5.378.106: every one of its properties
+ /// (StatusCode, SymbolicId, NamespaceUri, LocalizedText,
+ /// AdditionalInfo, InnerResult) has a private setter, and its only non-getter
+ /// method — Append(StringBuilder) — formats rather than mutates, so no code outside the type
+ /// can change an instance. The SDK itself publishes ServiceResult.Good and
+ /// ServiceResult.Bad as shared static readonly singletons, which is its own
+ /// endorsement of the pattern. Every call site here treats the result as an opaque value — it is
+ /// stored into an SDK errors array or returned — and none compares by reference.
+ ///
+ private static readonly ServiceResult DeniedResult = new(StatusCodes.BadUserAccessDenied);
+
private readonly IDataPlaneAuthoritySource _authority;
private readonly IPermissionEvaluator _evaluator;
private readonly ILogger _log;
@@ -173,6 +192,23 @@ public sealed class DataPlaneAuthorizer
AddressSpaceRealm realm)
=> Evaluate(identity, operation, nodeId, realm, cached: false);
+ ///
+ /// The decision itself. Ordered so the cached seams reach their answer having allocated nothing:
+ /// every check that must run per call comes first, the memoized verdict is consulted next, and the
+ /// scope build and grant walk happen only on a miss.
+ ///
+ ///
+ /// The ordering is a correctness constraint before it is a performance one — see the key-revocation
+ /// comment inline, and Key_revocation_is_rechecked_on_a_cache_hit /
+ /// NodeManagerStage6EnforcementTests.An_expired_key_is_denied_even_though_its_allow_is_cached,
+ /// which fail if the cache lookup is moved above it.
+ ///
+ /// The role-carrying identity off the SDK context.
+ /// The operation being attempted.
+ /// The target node.
+ /// The namespace the node was resolved in.
+ /// Whether the grant walk may be memoized (the request-rate seams).
+ /// null to proceed; otherwise the veto to return to the SDK.
private ServiceResult? Evaluate(
RoleCarryingUserIdentity? identity,
OpcUaOperation operation,
@@ -203,33 +239,43 @@ public sealed class DataPlaneAuthorizer
// 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.
+ // ⚠️ Deliberately OUTSIDE the decision cache, and it must stay ABOVE the lookup below. 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 consulted first would keep an expired key working until
+ // the next deploy — the generation never moves, so nothing would drop the entry.
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}", cached);
- }
+ return DenyRevokedKey(operation, nodeId, identity.Principal, keyId, snapshot.GenerationId, cached);
- if (!TryBuildScope(nodeId, realm, snapshot.ClusterId, out var scope))
+ // The s= identifier, read once: it is both the cache key and the raw material the scope is split
+ // from, and pulling it up here absorbs what used to be TryBuildScope's own guard. A NodeId that
+ // carries no string addresses no ACL scope at all, so it can never be answered from the cache.
+ if (nodeId?.Identifier is not string identifier || identifier.Length == 0)
{
return Deny(operation, nodeId, identity.Principal,
"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);
+ var sessionKey = identity.ApiKeyId ?? identity.Principal;
- if (allowed) return null;
+ if (cached &&
+ Decisions.TryGet(snapshot.GenerationId, sessionKey, operation, realm, identifier, out var memoized))
+ {
+ // The whole point of the reorder: a hit returns HERE, having built no scope, no closure and no
+ // UserAuthorizationState. One Browse of a device folder asks this once per child.
+ return memoized
+ ? null
+ : DenyNoGrant(operation, nodeId, identity.Principal, snapshot.GenerationId, cached);
+ }
- return Deny(operation, nodeId, identity.Principal,
- $"no grant in generation {snapshot.GenerationId} confers {operation} here", cached);
+ var scope = BuildScope(identifier, realm, snapshot.ClusterId);
+ var allowed = Walk(snapshot, identity, operation, scope, now);
+
+ if (cached) Decisions.Set(snapshot.GenerationId, sessionKey, operation, realm, identifier, allowed);
+
+ return allowed
+ ? null
+ : DenyNoGrant(operation, nodeId, identity.Principal, snapshot.GenerationId, cached);
}
/// The grant walk itself — the only part of a decision that is safe to memoize.
@@ -274,7 +320,7 @@ public sealed class DataPlaneAuthorizer
}
///
- /// Projects an SDK onto the scope the permission trie is keyed by.
+ /// Projects a node's s= identifier onto the scope the permission trie is keyed by.
///
///
/// The s= identifier IS the path in both realms — a RawPath
@@ -286,41 +332,108 @@ public sealed class DataPlaneAuthorizer
/// Segments are compared ordinally, matching RawPaths.Comparer and OPC UA NodeId
/// identity — Speed and speed are different nodes and must be different scopes.
///
+ ///
+ /// Takes the identifier rather than the , and cannot fail: the "not a
+ /// string" guard this used to carry moved up into Evaluate, where it has to run before
+ /// the cache lookup anyway — the identifier is the cache key.
+ ///
///
- /// The target node.
+ /// The node's non-empty s= identifier.
/// The namespace the node was resolved in.
/// The node's cluster.
- /// The resolved scope.
- /// when a scope could be built.
- private static bool TryBuildScope(
- NodeId nodeId, AddressSpaceRealm realm, string clusterId, out NodeScope scope)
+ /// The scope to evaluate against.
+ private static NodeScope BuildScope(string identifier, AddressSpaceRealm realm, string clusterId) => new()
{
- scope = null!;
- if (nodeId?.Identifier is not string identifier || identifier.Length == 0) return false;
-
- scope = new NodeScope
- {
- ClusterId = clusterId,
- Kind = realm == AddressSpaceRealm.Raw ? NodeHierarchyKind.Raw : NodeHierarchyKind.Uns,
- Segments = identifier.Split('/', StringSplitOptions.RemoveEmptyEntries),
- };
- return true;
- }
+ ClusterId = clusterId,
+ Kind = realm == AddressSpaceRealm.Raw ? NodeHierarchyKind.Raw : NodeHierarchyKind.Uns,
+ Segments = identifier.Split('/', StringSplitOptions.RemoveEmptyEntries),
+ };
+ /// Denies with a constant reason — nothing to build, so nothing to defer.
+ /// The operation being attempted.
+ /// The target node.
+ /// The calling principal, when one resolved.
+ /// The operator-facing explanation.
+ /// Whether this came from a request-rate seam (which logs at Debug).
+ /// The veto to return to the SDK.
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.
- //
- // 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,
+ var level = LevelFor(cached);
+ if (_log.IsEnabled(level)) LogDenial(level, operation, nodeId, principal, reason);
+ return DeniedResult;
+ }
+
+ ///
+ /// Denies because the calling key is disabled or expired, composing the reason only if it will be
+ /// logged.
+ ///
+ /// The operation being attempted.
+ /// The target node.
+ /// The calling principal.
+ /// The key that is no longer usable.
+ /// The authorization generation the check ran under.
+ /// Whether this came from a request-rate seam (which logs at Debug).
+ /// The veto to return to the SDK.
+ private ServiceResult DenyRevokedKey(
+ OpcUaOperation operation, NodeId? nodeId, string? principal, string keyId, long generation, bool cached)
+ {
+ var level = LevelFor(cached);
+ if (_log.IsEnabled(level))
+ {
+ LogDenial(level, operation, nodeId, principal,
+ $"key {keyId} is no longer enabled or has expired in generation {generation}");
+ }
+
+ return DeniedResult;
+ }
+
+ ///
+ /// Denies because no grant conferred the operation — the hot denial, hit once per child of an
+ /// ungranted folder, so the reason string is built only when Debug is actually on.
+ ///
+ /// The operation being attempted.
+ /// The target node.
+ /// The calling principal.
+ /// The authorization generation the walk ran under.
+ /// Whether this came from a request-rate seam (which logs at Debug).
+ /// The veto to return to the SDK.
+ private ServiceResult DenyNoGrant(
+ OpcUaOperation operation, NodeId? nodeId, string? principal, long generation, bool cached)
+ {
+ var level = LevelFor(cached);
+ if (_log.IsEnabled(level))
+ {
+ LogDenial(level, operation, nodeId, principal,
+ $"no grant in generation {generation} confers {operation} here");
+ }
+
+ return DeniedResult;
+ }
+
+ ///
+ /// 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
+ /// emits, which says how many references were filtered and for whom.
+ ///
+ /// Whether the denial came from a request-rate seam.
+ /// The level to log the denial at.
+ private static LogLevel LevelFor(bool cached) => cached ? LogLevel.Debug : LogLevel.Information;
+
+ ///
+ /// Emits the denial detail. Callers guard with first — not for the
+ /// message template, which costs nothing, but because every caller's is a
+ /// composed string and two of them interpolate on the request-rate path.
+ ///
+ /// The level chosen by .
+ /// The operation being attempted.
+ /// The target node.
+ /// The calling principal, when one resolved.
+ /// The operator-facing explanation.
+ private void LogDenial(
+ LogLevel level, OpcUaOperation operation, NodeId? nodeId, string? principal, string reason)
+ => _log.Log(
+ level,
"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
index dd1c6fb8..15d004c6 100644
--- a/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/Security/SessionAclDecisionCache.cs
+++ b/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/Security/SessionAclDecisionCache.cs
@@ -94,9 +94,101 @@ public sealed class SessionAclDecisionCache
}
}
+ ///
+ /// Looks a decision up without allocating anything — the hot half, and the reason the store half is
+ /// a separate method rather than a parameter.
+ ///
+ ///
+ ///
+ /// Why split from . A closure-taking API forces its caller to
+ /// build the closure — and everything the closure captures — before it knows whether the
+ /// answer is already memoized, so the allocation the cache exists to avoid is paid on the hit
+ /// path too. Read and Browse ask this question once per node, which is precisely where paying
+ /// for a walk that never runs is worth removing. remains, delegating
+ /// here, for callers that genuinely have nothing cheaper to offer.
+ ///
+ ///
+ /// The generation is checked (and a stale cache dropped) here rather than in
+ /// alone, so a lookup landing first after a deploy cannot read the previous
+ /// generation's verdicts.
+ ///
+ ///
+ /// 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 memoized verdict, when one exists.
+ /// when a decision was memoized for this key.
+ public bool TryGet(
+ long generation,
+ string sessionKey,
+ OpcUaOperation operation,
+ AddressSpaceRealm realm,
+ string nodeIdentifier,
+ out bool decision)
+ {
+ InvalidateIfStale(generation);
+
+ decision = false;
+
+ // TryGetValue rather than GetOrAdd: a session that only ever hits must not mint a shard, and a
+ // session whose shard was just cleared must not resurrect one until it actually stores something.
+ return _shards.TryGetValue(sessionKey, out var shard)
+ && shard.Entries.TryGetValue(new ShardKey(operation, realm, nodeIdentifier), out decision);
+ }
+
+ ///
+ /// Memoizes a decision, subject to the two entry bounds and the overtaken-generation drop rule.
+ ///
+ /// The generation the decision was taken under — not re-read from the
+ /// caller, so a deploy landing mid-walk is detected here.
+ /// Stable identity of the calling session — its key id, else its principal.
+ /// The operation that was attempted.
+ /// The realm the node was resolved in.
+ /// The node's s= identifier.
+ /// The verdict the grant walk produced.
+ public void Set(
+ long generation,
+ string sessionKey,
+ OpcUaOperation operation,
+ AddressSpaceRealm realm,
+ string nodeIdentifier,
+ bool decision)
+ {
+ // Re-read the generation AFTER the walk. 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;
+
+ var shard = _shards.GetOrAdd(sessionKey, static _ => new Shard());
+
+ if (Volatile.Read(ref _globalCount) >= MaxEntries)
+ {
+ // The outer bound: no single session is over its own ceiling, so there is nothing narrower to
+ // drop. Re-acquire the shard — ClearAll drops the shard objects themselves.
+ ClearAll();
+ shard = _shards.GetOrAdd(sessionKey, static _ => new Shard());
+ }
+ else if (Volatile.Read(ref shard.ApproxCount) >= MaxEntriesPerSession)
+ {
+ shard.Entries.Clear();
+ Interlocked.Add(ref _globalCount, -Interlocked.Exchange(ref shard.ApproxCount, 0));
+ }
+
+ // TryAdd, not an indexer assignment: only a genuine insert may move the counters, or a racing pair
+ // of walks over the same key would inflate them and pull the ceilings in for good.
+ if (shard.Entries.TryAdd(new ShardKey(operation, realm, nodeIdentifier), decision))
+ {
+ Interlocked.Increment(ref shard.ApproxCount);
+ Interlocked.Increment(ref _globalCount);
+ }
+ }
+
///
/// Returns the memoized decision for this key, computing and storing it when absent or when the
- /// generation has moved on.
+ /// generation has moved on. A convenience over + for callers
+ /// that have no cheaper way to express the walk than a closure.
///
/// The authorization generation the decision is being taken under.
/// Stable identity of the calling session — its key id, else its principal.
@@ -115,40 +207,10 @@ public sealed class SessionAclDecisionCache
{
ArgumentNullException.ThrowIfNull(compute);
- InvalidateIfStale(generation);
-
- var shard = _shards.GetOrAdd(sessionKey, static _ => new Shard());
- var key = new ShardKey(operation, realm, nodeIdentifier);
- if (shard.Entries.TryGetValue(key, out var cached)) return cached;
+ if (TryGet(generation, sessionKey, operation, realm, nodeIdentifier, 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 (Volatile.Read(ref _globalCount) >= MaxEntries)
- {
- // The outer bound: no single session is over its own ceiling, so there is nothing narrower to
- // drop. Re-acquire the shard — ClearAll drops the shard objects themselves.
- ClearAll();
- shard = _shards.GetOrAdd(sessionKey, static _ => new Shard());
- }
- else if (Volatile.Read(ref shard.ApproxCount) >= MaxEntriesPerSession)
- {
- shard.Entries.Clear();
- Interlocked.Add(ref _globalCount, -Interlocked.Exchange(ref shard.ApproxCount, 0));
- }
-
- // TryAdd, not an indexer assignment: only a genuine insert may move the counters, or a racing pair
- // of walks over the same key would inflate them and pull the ceilings in for good.
- if (shard.Entries.TryAdd(key, decided))
- {
- Interlocked.Increment(ref shard.ApproxCount);
- Interlocked.Increment(ref _globalCount);
- }
-
+ Set(generation, sessionKey, operation, realm, nodeIdentifier, decided);
return decided;
}
diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/DataPlaneAuthorizerTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/DataPlaneAuthorizerTests.cs
index 77d6cfe7..405aa61b 100644
--- a/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/DataPlaneAuthorizerTests.cs
+++ b/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/DataPlaneAuthorizerTests.cs
@@ -320,6 +320,66 @@ public sealed class DataPlaneAuthorizerTests
Veto(gate.Authorize(Identity(), OpcUaOperation.WriteOperate, node, AddressSpaceRealm.Raw));
}
+ // ───────────────────────────── the cached hot path ─────────────────────────────
+
+ ///
+ /// A second identical question is answered from the decision cache without re-walking the grants —
+ /// which on the reordered hot path also means without rebuilding the the
+ /// walk would need, because the lookup happens before the scope is built at all.
+ ///
+ ///
+ /// The evaluator count is the observable half: a hit that still built a scope would have nothing to
+ /// do with it, so the only way to see the short-circuit from outside is that the walk never ran. The
+ /// cache size pins that the second call was a hit rather than a second miss that happened to
+ /// overwrite the first entry.
+ ///
+ [Fact]
+ public void A_cache_hit_does_not_rebuild_the_scope()
+ {
+ var gate = CountingGate(ClusterGrant(NodePermissions.Read), out var evaluator);
+ var node = RawNode("Plant/Modbus/dev1/Speed");
+
+ gate.AuthorizeCached(Identity(), OpcUaOperation.Read, node, AddressSpaceRealm.Raw).ShouldBeNull();
+ gate.AuthorizeCached(Identity(), OpcUaOperation.Read, node, AddressSpaceRealm.Raw).ShouldBeNull();
+
+ evaluator.Calls.ShouldBe(1);
+ gate.Decisions.Count.ShouldBe(1);
+ }
+
+ ///
+ /// Pins the ordering the hot path must keep: the key's usability is re-checked BEFORE the
+ /// decision cache is consulted, so a cached allow cannot outlive the key that earned it.
+ ///
+ ///
+ /// Expiry rather than a disabling redeploy is the falsifiable form of this. A redeploy advances the
+ /// generation, which drops the cache wholesale — so a gate that consulted the cache first would
+ /// still deny, and the test would pass against the broken ordering. Expiry moves with the CLOCK and
+ /// leaves the generation (and therefore the memoized allow) exactly where it was, so the denial can
+ /// only have come from the key check running ahead of the lookup.
+ ///
+ [Fact]
+ public void Key_revocation_is_rechecked_on_a_cache_hit()
+ {
+ var clock = new FakeTimeProvider(new DateTime(2026, 8, 1, 12, 0, 0, DateTimeKind.Utc));
+ var state = new DataPlaneAuthorityState();
+ var key = Key(enabled: true);
+ key.ExpiresUtc = new DateTime(2026, 8, 1, 13, 0, 0, DateTimeKind.Utc);
+ state.Apply(Cluster, "rev-1", [key], [ClusterGrant(NodePermissions.Read)], Scopes());
+
+ var gate = new DataPlaneAuthorizer(
+ state, new TriePermissionEvaluator(state.Tries), enabled: true, NullLogger.Instance, clock);
+ var node = RawNode("Plant/Modbus/dev1/Speed");
+
+ gate.AuthorizeCached(Identity(), OpcUaOperation.Read, node, AddressSpaceRealm.Raw).ShouldBeNull();
+ gate.Decisions.Count.ShouldBe(1, "the allow was memoized, so the next call takes the hit path");
+
+ clock.Advance(TimeSpan.FromHours(2));
+
+ Veto(gate.AuthorizeCached(Identity(), OpcUaOperation.Read, node, AddressSpaceRealm.Raw));
+ gate.Decisions.Generation.ShouldBe(state.Current.GenerationId);
+ gate.Decisions.Count.ShouldBe(1, "the memoized allow is untouched — the denial came from the key check");
+ }
+
/// The flag is carried faithfully; the seams read it to decide whether to consult the gate at all.
[Theory]
[InlineData(true)]
@@ -349,6 +409,18 @@ public sealed class DataPlaneAuthorizerTests
state, new TriePermissionEvaluator(state.Tries), enabled: true, NullLogger.Instance);
}
+ ///
+ /// The same gate builds, with the REAL evaluator wrapped in a call counter — the
+ /// chain under test stays end-to-end, and only the number of walks becomes observable.
+ ///
+ private static DataPlaneAuthorizer CountingGate(NodeAcl grant, out CountingEvaluator evaluator)
+ {
+ var state = new DataPlaneAuthorityState();
+ state.Apply(Cluster, "rev-1", [Key(enabled: true)], [grant], Scopes());
+ evaluator = new CountingEvaluator(new TriePermissionEvaluator(state.Tries));
+ return new DataPlaneAuthorizer(state, evaluator, enabled: true, NullLogger.Instance);
+ }
+
private static RoleCarryingUserIdentity Identity() =>
new(new UserNameIdentityToken { UserName = "apikey" }, [], Principal, KeyId);
@@ -381,6 +453,19 @@ public sealed class DataPlaneAuthorizerTests
return map;
}
+ /// Counts grant walks without changing any of their answers.
+ private sealed class CountingEvaluator(IPermissionEvaluator inner) : IPermissionEvaluator
+ {
+ public int Calls { get; private set; }
+
+ public AuthorizationDecision Authorize(
+ UserAuthorizationState session, OpcUaOperation operation, NodeScope scope)
+ {
+ Calls++;
+ return inner.Authorize(session, operation, scope);
+ }
+ }
+
/// Minimal controllable clock — the suite needs only "now" and "advance".
private sealed class FakeTimeProvider(DateTime utcNow) : TimeProvider
{