fix(acl): revalidate event/alarm subscriptions on both triggers; key-usability check on role gates (#562, #563)
#562: direct event (alarm) monitored items are now tracked in a second map (_eventItems, fed by the SubscribeToEvents override) and re-authorized by RevalidateMonitoredItems on both triggers (DataPlaneAuthorityState.Changed and AclRevalidationTimer). A denied item is detached via the SDK's own protected SubscribeToEvents(unsubscribe: true) - never MonitoringMode.Disabled - and re-attached on restore; a client delete of a stood-down item is absorbed. Over-the-wire tests: revoke -> stop (positive control on a still-granted notifier) -> restore -> resume; clock-driven expiry via the real timer with a control session; delete-while-stood-down is not resurrected; unexpired negative control. Server-object (all-events) subscriptions are documented as outside the ACL (observed over the wire). #563: DataPlaneAuthorizer.AuthorizeKeyUsable runs the FindUsableKey probe on the WriteOperate and AlarmAck role gates regardless of Security:Acl:Enabled, so an expired or deployment-disabled key loses its activation-time roles on its open session. Anonymous (no key id) is unaffected; reads/subscriptions stay ungated with the flag off. No new flag. Docs: docs/security.md and the CLAUDE.md API-key/ACL section. Claude-Session: https://claude.ai/code/session_01Ef2ozxS7QPanGT8uoATbTe
This commit is contained in:
Joseph Doherty
@@ -348,6 +348,7 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2
|
||||
{
|
||||
Volatile.Read(ref _dataPlaneAuthority).Changed -= OnAuthorityChanged;
|
||||
_liveItems.Clear();
|
||||
_eventItems.Clear();
|
||||
}
|
||||
|
||||
base.Dispose(disposing);
|
||||
@@ -357,7 +358,9 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2
|
||||
/// The node-scoped ACL gate (#520 stage 5), or <c>null</c> when none is wired. Consulted by the
|
||||
/// write, alarm-command and HistoryRead seams <b>only while
|
||||
/// <see cref="Security.DataPlaneAuthorizer.Enabled"/> is true</b>; otherwise each keeps its
|
||||
/// server-wide role check and behaviour is identical to before this stage.
|
||||
/// server-wide role check and behaviour is identical to before this stage — with one exception: the
|
||||
/// two role gates consult <see cref="Security.DataPlaneAuthorizer.AuthorizeKeyUsable"/> in either
|
||||
/// mode, so an expired or revoked key loses its activation-time roles (Gitea #563).
|
||||
/// </summary>
|
||||
/// <remarks>
|
||||
/// Null means "not wired" and is treated as not-enabled, not as deny-all. That looks like the wrong
|
||||
@@ -1595,6 +1598,12 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2
|
||||
identity, AlarmOperationFor(operation), condition.NodeId, RealmOf(condition.NodeId));
|
||||
}
|
||||
|
||||
// Gitea #563: the activation-time AlarmAck role is honoured only while the key that earned it is
|
||||
// still usable — an expired or deployment-disabled key loses it on its open session.
|
||||
var keyVeto = _authorizer?.AuthorizeKeyUsable(
|
||||
identity, AlarmOperationFor(operation), condition.NodeId);
|
||||
if (keyVeto is not null) return keyVeto;
|
||||
|
||||
// Fail closed: no role / no identity ⇒ veto. Returning a bad ServiceResult aborts the SDK's
|
||||
// state change and surfaces the status to the client; we never route or mutate.
|
||||
return identity is null
|
||||
@@ -1670,10 +1679,14 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2
|
||||
// #520 stage 5. With ACLs enforced the scoped decision REPLACES the server-wide WriteOperate role
|
||||
// check — see AclOptions for why chaining them would leave every narrow grant inert. The
|
||||
// gateway-wired check applies either way: it is an availability fact, not an authorization one.
|
||||
// With the flag off, Gitea #563: the activation-time WriteOperate role is honoured only while the
|
||||
// session's key is still usable — checked first, so an expired key reads as denied rather than as
|
||||
// "writes unavailable" on a node with no gateway.
|
||||
var gate = AclEnforced
|
||||
? _authorizer!.Authorize(identity, OpcUaOperation.WriteOperate, node.NodeId, RealmOf(node.NodeId))
|
||||
?? (gatewayWired ? null : new ServiceResult(StatusCodes.BadNotWritable, "writes unavailable"))
|
||||
: EvaluateEquipmentWriteGate(identity, gatewayWired);
|
||||
: _authorizer?.AuthorizeKeyUsable(identity, OpcUaOperation.WriteOperate, node.NodeId)
|
||||
?? EvaluateEquipmentWriteGate(identity, gatewayWired);
|
||||
if (gate is not null) return gate;
|
||||
|
||||
// Item A (synchronous structural fail-fast): reject a structurally-invalid write INLINE — return Bad
|
||||
@@ -3222,7 +3235,58 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2
|
||||
return StatusCodes.BadUserAccessDenied;
|
||||
}
|
||||
|
||||
return base.SubscribeToEvents(context, sourceId, subscriptionId, monitoredItem, unsubscribe);
|
||||
if (unsubscribe)
|
||||
{
|
||||
// Gitea #562: an item RevalidateMonitoredItems stood down is ALREADY off its notifier's fan-out.
|
||||
// Handing its delete to the SDK would make it remove an item that is not there — at best a
|
||||
// spurious BadNodeIdUnknown on the client's delete, at worst a NullReferenceException inside
|
||||
// MonitoredNode2.Remove when the node's event list has gone null. Checked and dropped under the
|
||||
// same Lock the revalidation pass re-attaches under, so the two cannot interleave into
|
||||
// re-attaching an item the client has just deleted.
|
||||
lock (Lock)
|
||||
{
|
||||
if (_eventItems.TryRemove(monitoredItem, out var stoodDown) && stoodDown.Suppressed)
|
||||
return ServiceResult.Good;
|
||||
}
|
||||
|
||||
return base.SubscribeToEvents(context, sourceId, subscriptionId, monitoredItem, unsubscribe);
|
||||
}
|
||||
|
||||
var result = base.SubscribeToEvents(context, sourceId, subscriptionId, monitoredItem, unsubscribe);
|
||||
TrackLiveEventItem(sourceId, monitoredItem);
|
||||
return result;
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// Starts (or refreshes) tracking a direct event subscription for revalidation (Gitea #562).
|
||||
/// </summary>
|
||||
/// <param name="sourceId">The manager handle the item subscribed through.</param>
|
||||
/// <param name="monitoredItem">The event item.</param>
|
||||
/// <remarks>
|
||||
/// <para>
|
||||
/// Only items the SDK actually attached are tracked — a subscribe the SDK refused (a source that
|
||||
/// is not an event notifier, an unknown node) has nothing to stand down. The SDK reports an item
|
||||
/// that was already attached (a <c>ModifyMonitoredItems</c> re-subscribe) as a non-Good status
|
||||
/// while having re-attached it anyway, so attachment is read off the fan-out list itself rather
|
||||
/// than inferred from the status the subscribe returned.
|
||||
/// </para>
|
||||
/// <para>
|
||||
/// A re-subscribe of an item that was stood down is the client re-asking, and it has just passed
|
||||
/// the creation gate above, so the entry is replaced with a live one.
|
||||
/// </para>
|
||||
/// </remarks>
|
||||
private void TrackLiveEventItem(object sourceId, IEventMonitoredItem monitoredItem)
|
||||
{
|
||||
if (IsHandleInNamespace(sourceId) is not { } handle || !OwnsNamespace(handle.NodeId)) return;
|
||||
|
||||
lock (Lock)
|
||||
{
|
||||
if (MonitoredNodes.TryGetValue(handle.NodeId, out var monitored)
|
||||
&& monitored.EventMonitoredItems?.Contains(monitoredItem) == true)
|
||||
{
|
||||
_eventItems[monitoredItem] = new LiveEventItem(handle);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
/// <inheritdoc />
|
||||
@@ -3293,6 +3357,54 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2
|
||||
public bool Suppressed { get; set; }
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// Live <b>event</b> monitored items subscribed directly to a notifier this manager owns (Gitea #562),
|
||||
/// the event-side twin of <see cref="_liveItems"/>.
|
||||
/// </summary>
|
||||
/// <remarks>
|
||||
/// <para>
|
||||
/// A second map because the SDK keeps the two kinds apart: a data-change item reaches this
|
||||
/// manager through <see cref="OnMonitoredItemCreated"/>, an event item only through
|
||||
/// <see cref="SubscribeToEvents(OperationContext, object, uint, IEventMonitoredItem, bool)"/>, and
|
||||
/// they hang off different lists of <see cref="MonitoredNode2"/>. Before #562 nothing tracked event
|
||||
/// items at all, so an alarm feed authorized at creation survived every later revocation.
|
||||
/// </para>
|
||||
/// <para>
|
||||
/// Maintained by that override's subscribe/unsubscribe arms — the SDK routes every create, modify
|
||||
/// and delete of a direct event item through it, subscription teardown included. Mutated under
|
||||
/// <c>Lock</c>, so the membership check the revalidation pass makes before re-attaching an item
|
||||
/// cannot race a client deleting it.
|
||||
/// </para>
|
||||
/// <para>
|
||||
/// ⚠️ <b>Server-object (all-events) subscriptions are not in this map</b>, because they are not
|
||||
/// authorized by this manager in the first place: the SDK fans them out through
|
||||
/// <c>SubscribeToAllEvents</c> and through the Server object's own notifier, which the core node
|
||||
/// manager owns. See <c>docs/security.md</c> § Data-Plane Authorization.
|
||||
/// </para>
|
||||
/// </remarks>
|
||||
private readonly ConcurrentDictionary<IEventMonitoredItem, LiveEventItem> _eventItems = new();
|
||||
|
||||
/// <summary>
|
||||
/// How many event items are attached to the event fan-out of nodes this manager owns. Test
|
||||
/// observability for Gitea #562: an item resurrected onto a notifier after its client deleted it is
|
||||
/// invisible to that client, so only the server side can show it.
|
||||
/// </summary>
|
||||
internal int AttachedEventItemCount
|
||||
{
|
||||
get
|
||||
{
|
||||
lock (Lock) return MonitoredNodes.Values.Sum(n => n.EventMonitoredItems?.Count ?? 0);
|
||||
}
|
||||
}
|
||||
|
||||
/// <summary>A tracked event item: the notifier it subscribed to, and whether we have stood it down.</summary>
|
||||
/// <param name="Handle">The notifier's handle; its <see cref="NodeHandle.Node"/> is what the item is attached to.</param>
|
||||
private sealed record LiveEventItem(NodeHandle Handle)
|
||||
{
|
||||
/// <summary>True while the item has been detached from its notifier's event fan-out for want of a grant.</summary>
|
||||
public bool Suppressed { get; set; }
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// Re-authorizes every live monitored item against the current generation, standing down the ones
|
||||
/// whose grant has gone.
|
||||
@@ -3346,7 +3458,7 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2
|
||||
/// </remarks>
|
||||
public void RevalidateMonitoredItems()
|
||||
{
|
||||
if (!AclEnforced || _liveItems.IsEmpty) return;
|
||||
if (!AclEnforced || (_liveItems.IsEmpty && _eventItems.IsEmpty)) return;
|
||||
|
||||
var now = DateTime.UtcNow;
|
||||
var stoodDown = 0;
|
||||
@@ -3391,11 +3503,83 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2
|
||||
stoodDown++;
|
||||
}
|
||||
|
||||
var (eventsStoodDown, eventsRestored) = RevalidateEventItems(verdicts);
|
||||
|
||||
if (stoodDown > 0 || restored > 0)
|
||||
{
|
||||
_authorizer!.LogFiltered(
|
||||
"RevalidateSubscriptions", principal: null, stoodDown, _liveItems.Count);
|
||||
}
|
||||
|
||||
if (eventsStoodDown > 0 || eventsRestored > 0)
|
||||
{
|
||||
_authorizer!.LogFiltered(
|
||||
"RevalidateEventSubscriptions", principal: null, eventsStoodDown, _eventItems.Count);
|
||||
}
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// The event-item half of <see cref="RevalidateMonitoredItems"/> (Gitea #562): re-authorizes every
|
||||
/// tracked direct event subscription against its notifier, standing down the denied ones and
|
||||
/// re-attaching the restored ones.
|
||||
/// </summary>
|
||||
/// <param name="verdicts">The pass-local memo shared with the data-change half.</param>
|
||||
/// <returns>How many items were stood down and how many restored.</returns>
|
||||
/// <remarks>
|
||||
/// <para>
|
||||
/// The verdict is the same question the creation gate in
|
||||
/// <see cref="SubscribeToEvents(OperationContext, object, uint, IEventMonitoredItem, bool)"/>
|
||||
/// asks — <c>Subscribe</c> on the notifier's scope, for the identity of the session that created
|
||||
/// the item — so an event feed is held to exactly the authority it was admitted under, re-asked on
|
||||
/// both triggers (a deployment, and the host's <c>AclRevalidationTimer</c>).
|
||||
/// </para>
|
||||
/// <para>
|
||||
/// ⚠️ <b>Detached through the SDK's own unsubscribe, never <see cref="MonitoringMode.Disabled"/>.</b>
|
||||
/// Same reason as the data-change half: a disabled item can only be re-enabled by the client, and
|
||||
/// a machine consumer will never try, so a revocation reversed by the next deployment would leave
|
||||
/// the feed dead. The stand-down is the SDK's protected <c>SubscribeToEvents(…, unsubscribe:
|
||||
/// true)</c> — precisely what the client's own delete does to the notifier's
|
||||
/// <see cref="MonitoredNode2"/> fan-out — and the restore is the same call with
|
||||
/// <c>unsubscribe: false</c>, so neither leaves the fan-out in a shape a client could not have
|
||||
/// produced. The item itself stays in the client's subscription untouched.
|
||||
/// </para>
|
||||
/// <para>
|
||||
/// <b>The client is not told.</b> Unlike a data-change item, an event item has no value slot to
|
||||
/// carry a <c>BadUserAccessDenied</c>: its notifications are event field lists, and inventing an
|
||||
/// event to say "access denied" would put a fabricated event in front of every alarm consumer. The
|
||||
/// feed goes quiet, and the operator-facing signal is the summary line this pass logs.
|
||||
/// </para>
|
||||
/// <para>
|
||||
/// Membership is re-checked under <c>Lock</c> immediately before each attach/detach: the walk is
|
||||
/// over a concurrent snapshot, and re-attaching an item the client deleted mid-pass would leave it
|
||||
/// on the notifier's fan-out with no owner and no way to remove it.
|
||||
/// </para>
|
||||
/// </remarks>
|
||||
private (int StoodDown, int Restored) RevalidateEventItems(
|
||||
Dictionary<(string Session, AddressSpaceRealm Realm, string Node), bool> verdicts)
|
||||
{
|
||||
var stoodDown = 0;
|
||||
var restored = 0;
|
||||
|
||||
foreach (var (item, tracked) in _eventItems)
|
||||
{
|
||||
var permitted = IsSubscriptionStillPermitted(
|
||||
item.EffectiveIdentity as RoleCarryingUserIdentity, tracked.Handle.NodeId, verdicts);
|
||||
if (permitted != tracked.Suppressed) continue;
|
||||
|
||||
lock (Lock)
|
||||
{
|
||||
if (!_eventItems.TryGetValue(item, out var current) || !ReferenceEquals(current, tracked)) continue;
|
||||
|
||||
SubscribeToEvents(SystemContext, tracked.Handle.Node, item, unsubscribe: !permitted);
|
||||
tracked.Suppressed = !permitted;
|
||||
}
|
||||
|
||||
if (permitted) restored++;
|
||||
else stoodDown++;
|
||||
}
|
||||
|
||||
return (stoodDown, restored);
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
|
||||
@@ -201,6 +201,44 @@ public sealed class DataPlaneAuthorizer
|
||||
AddressSpaceRealm realm)
|
||||
=> Evaluate(identity, operation, nodeId, realm, cached: false);
|
||||
|
||||
/// <summary>
|
||||
/// Whether the API key behind <paramref name="identity"/> is still usable — consulted by the two
|
||||
/// server-wide role gates (<c>WriteOperate</c>, <c>AlarmAck</c>) <b>regardless of
|
||||
/// <see cref="Enabled"/></b> (Gitea #563).
|
||||
/// </summary>
|
||||
/// <remarks>
|
||||
/// <para>
|
||||
/// With the flag off a session's roles are resolved once, at activation, so before this check a
|
||||
/// key that expired by the clock — or that a deployment disabled or removed — kept
|
||||
/// <c>WriteOperate</c> / <c>AlarmAck</c> on its open session until the client disconnected. With
|
||||
/// the flag on, <see cref="Authorize"/> already makes the identical check first thing, so the role
|
||||
/// gates call this only on the flag-off path and the two postures agree on key usability.
|
||||
/// </para>
|
||||
/// <para>
|
||||
/// It is the same <see cref="DataPlaneAuthoritySnapshot.FindUsableKey"/> probe against the same
|
||||
/// snapshot and the same clock as <see cref="Authorize"/>: one dictionary lookup, no I/O, so it is
|
||||
/// safe on the SDK's synchronous write and method-call threads. A session carrying <b>no</b> key id —
|
||||
/// Anonymous, or a legacy identity — has nothing that can expire and always passes; its roles stay
|
||||
/// exactly what activation resolved. A key absent from the snapshot denies, as it does with the flag
|
||||
/// on: on a driver-only node the artifact is the only key source there is, so "absent" means
|
||||
/// "removed by a deployment".
|
||||
/// </para>
|
||||
/// </remarks>
|
||||
/// <param name="identity">The role-carrying identity off the SDK context, or null.</param>
|
||||
/// <param name="operation">The operation being attempted, for the denial log.</param>
|
||||
/// <param name="nodeId">The target node, for the denial log.</param>
|
||||
/// <returns><c>null</c> to proceed to the role check; otherwise the veto to return to the SDK.</returns>
|
||||
public ServiceResult? AuthorizeKeyUsable(
|
||||
RoleCarryingUserIdentity? identity, OpcUaOperation operation, NodeId nodeId)
|
||||
{
|
||||
if (identity?.ApiKeyId is not { Length: > 0 } keyId) return null;
|
||||
|
||||
var snapshot = _authority.Current;
|
||||
return snapshot.FindUsableKey(keyId, _time.GetUtcNow().UtcDateTime) is null
|
||||
? DenyRevokedKey(operation, nodeId, identity.Principal, keyId, snapshot.GenerationId, cached: false)
|
||||
: null;
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// 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
|
||||
|
||||
@@ -32,6 +32,9 @@
|
||||
<!-- R2-08 (03/S2): the outage-sim integration test drives the internal static
|
||||
OpcUaApplicationHost.HandleImpersonation against the real LDAP library. -->
|
||||
<InternalsVisibleTo Include="ZB.MOM.WW.OtOpcUa.Host.IntegrationTests"/>
|
||||
<!-- Gitea #562: the over-the-wire event-revalidation test asserts the server-side fan-out, because a
|
||||
deleted item resurrected onto a notifier is invisible to the client that deleted it. -->
|
||||
<InternalsVisibleTo Include="ZB.MOM.WW.OtOpcUa.OpcUaServer.IntegrationTests"/>
|
||||
</ItemGroup>
|
||||
|
||||
</Project>
|
||||
|
||||
Reference in New Issue
Block a user