feat(security): enforce node ACLs on read, browse and subscribe (#520 stage 6)
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
This commit is contained in:
Joseph Doherty
@@ -179,6 +179,25 @@ public interface IDataPlaneAuthoritySource
|
||||
{
|
||||
/// <summary>The snapshot in force right now. Never null; <see cref="DataPlaneAuthoritySnapshot.Empty"/> before the first apply.</summary>
|
||||
DataPlaneAuthoritySnapshot Current { get; }
|
||||
|
||||
/// <summary>
|
||||
/// Raised after a new snapshot is published, so consumers holding state derived from the previous
|
||||
/// one can re-derive it.
|
||||
/// </summary>
|
||||
/// <remarks>
|
||||
/// <para>
|
||||
/// Most consumers do not need this — they read <see cref="Current"/> 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 <b>live subscription</b>. 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.
|
||||
/// </para>
|
||||
/// <para>
|
||||
/// ⚠️ Handlers run <b>synchronously on the deploy thread</b>, inside the apply. Keep them short
|
||||
/// and non-throwing; a handler that blocks stalls the deployment.
|
||||
/// </para>
|
||||
/// </remarks>
|
||||
event Action<DataPlaneAuthoritySnapshot>? Changed;
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
@@ -194,6 +213,14 @@ public sealed class EmptyDataPlaneAuthoritySource : IDataPlaneAuthoritySource
|
||||
|
||||
/// <inheritdoc />
|
||||
public DataPlaneAuthoritySnapshot Current => DataPlaneAuthoritySnapshot.Empty;
|
||||
|
||||
/// <inheritdoc />
|
||||
/// <remarks>Never raised — this source is permanently empty, so nothing derived from it can go stale.</remarks>
|
||||
public event Action<DataPlaneAuthoritySnapshot>? Changed
|
||||
{
|
||||
add { }
|
||||
remove { }
|
||||
}
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
@@ -255,6 +282,9 @@ public sealed class DataPlaneAuthorityState : IDataPlaneAuthoritySource, IDataPl
|
||||
/// <inheritdoc />
|
||||
public DataPlaneAuthoritySnapshot Current => Volatile.Read(ref _current);
|
||||
|
||||
/// <inheritdoc />
|
||||
public event Action<DataPlaneAuthoritySnapshot>? Changed;
|
||||
|
||||
/// <inheritdoc />
|
||||
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;
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// Raises <see cref="Changed"/>, swallowing handler faults.
|
||||
/// </summary>
|
||||
/// <remarks>
|
||||
/// 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.
|
||||
/// </remarks>
|
||||
/// <param name="snapshot">The newly published snapshot.</param>
|
||||
private void Notify(DataPlaneAuthoritySnapshot snapshot)
|
||||
{
|
||||
var handlers = Changed;
|
||||
if (handlers is null) return;
|
||||
|
||||
foreach (var handler in handlers.GetInvocationList().Cast<Action<DataPlaneAuthoritySnapshot>>())
|
||||
{
|
||||
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}");
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@@ -68,6 +68,63 @@ public sealed class PermissionTrie
|
||||
return matches;
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// Whether <paramref name="scope"/> is an <b>ancestor</b> of some node one of
|
||||
/// <paramref name="principals"/> holds a <c>Browse</c> grant on — i.e. whether it must be
|
||||
/// browsable purely so the granted node can be reached.
|
||||
/// </summary>
|
||||
/// <remarks>
|
||||
/// <para>
|
||||
/// Without this, every narrow Browse grant is unreachable and therefore useless: a grant on
|
||||
/// <c>Plant/Modbus/dev1</c> is only arrived at by browsing <c>Plant</c> then <c>Plant/Modbus</c>,
|
||||
/// and a downward-inheriting trie confers nothing at either. The operator authors a working-looking
|
||||
/// grant and the client cannot navigate to it.
|
||||
/// </para>
|
||||
/// <para>
|
||||
/// ⚠️ <b>Transit is checked at the EXACT node and never accumulates down the walk</b>, which is
|
||||
/// the whole reason it is a separate marker instead of a synthesized <c>Browse</c> grant planted
|
||||
/// on each ancestor. A planted grant would be collected by <see cref="CollectMatches"/> and — the
|
||||
/// walk being purely additive downward — would cascade from <c>Plant</c> onto
|
||||
/// <c>Plant/Siemens/dev9</c> and every other unrelated subtree beneath it. That is a silent
|
||||
/// widening of exactly the kind the ordinal-comparer note below records.
|
||||
/// </para>
|
||||
/// <para>
|
||||
/// Only grants that actually carry <c>Browse</c> mark their ancestors. A principal granted
|
||||
/// <c>Read</c> 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.
|
||||
/// </para>
|
||||
/// </remarks>
|
||||
/// <param name="scope">The node scope being browsed.</param>
|
||||
/// <param name="principals">The principal names carried by the session.</param>
|
||||
/// <returns><see langword="true"/> when the node is on the path to a Browse-granted descendant.</returns>
|
||||
public bool HasBrowseTransit(NodeScope scope, IEnumerable<string> 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<string> 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<string> groups, List<MatchedGrant> matches)
|
||||
{
|
||||
foreach (var grant in node.Grants)
|
||||
@@ -101,6 +158,19 @@ public sealed class PermissionTrieNode
|
||||
/// nothing instead of granting something adjacent.
|
||||
/// </remarks>
|
||||
public Dictionary<string, PermissionTrieNode> Children { get; } = new(StringComparer.Ordinal);
|
||||
|
||||
/// <summary>
|
||||
/// Principals holding a <c>Browse</c> grant somewhere <b>strictly below</b> this node, so this node
|
||||
/// must be browsable for that grant to be reachable. See
|
||||
/// <see cref="PermissionTrie.HasBrowseTransit"/> for why this is a separate marker rather than a
|
||||
/// synthesized grant, and why it must never be accumulated during the walk.
|
||||
/// </summary>
|
||||
/// <remarks>
|
||||
/// Compared <c>OrdinalIgnoreCase</c>: this holds principal names, which are identifiers, and the
|
||||
/// repo's rule (#533) is that identifiers compare case-insensitively while <em>path segments</em>
|
||||
/// — the <see cref="Children"/> keys — compare ordinally.
|
||||
/// </remarks>
|
||||
public HashSet<string> BrowseTransitPrincipals { get; } = new(StringComparer.OrdinalIgnoreCase);
|
||||
}
|
||||
|
||||
/// <summary>Projection of a <see cref="Configuration.Entities.NodeAcl"/> row into the trie.</summary>
|
||||
|
||||
@@ -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);
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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();
|
||||
}
|
||||
|
||||
/// <summary>Maps each <see cref="OpcUaOperation"/> to the <see cref="NodePermissions"/> bit required to grant it.</summary>
|
||||
|
||||
Reference in New Issue
Block a user