diff --git a/.claude/rules/data-plane-auth.md b/.claude/rules/data-plane-auth.md index 4b80ca976..5c645a617 100644 --- a/.claude/rules/data-plane-auth.md +++ b/.claude/rules/data-plane-auth.md @@ -204,6 +204,13 @@ Two things to know before relying on any of this: enforcement (`ServerEventRig.TagReferences()`); null ⇒ none (fail closed). Tests: `UnsFolderScopeWireTests.A_moved_alarm_stops_reaching_its_old_folders_subscribers…`, `NativeAlarmFolderEventAclWireTests`. + ⚠️ **Never report an event with the synchronous `NodeState.ReportEvent` while holding `Lock` (#838).** Each + notifier's `MonitoredNode2` drains a BOUNDED channel (4096, `FullMode = Wait`) and its delivery task takes `Lock` on a + verdict-cache miss (SDK `GetNodeMetadata` — flag off too — and #826's check); sync `ReportEvent` blocks on a full + channel ⇒ a >4096 burst under one `Lock` hold deadlocked the node manager (stalled at 4097). Condition events go + through `ReportEventWithoutBlocking` (async on overflow, ordered chain). Folder events follow Part 3's shape — + `ReceiveEvents` on EventType + SourceNode (SDK check, kept first) AND our decision; non-condition events on a + folder item fail closed under enforcement. Tests: `FolderEventFloodWireTests`. - **ns=0 standard nodes stay ungated by design, even with `Security:Acl:Enabled`** (#617) — `ServerStatus`, `NamespaceArray`, `ServiceLevel` etc. are served by the SDK's core node manager outside our authorizer and clients read them to connect and to judge redundancy health; they carry no plant data. Every raw/UNS node, diff --git a/docs/security.md b/docs/security.md index 9db3f91ea..6a5fc4d48 100644 --- a/docs/security.md +++ b/docs/security.md @@ -825,6 +825,41 @@ Behaviours worth knowing before enabling it: enforcement only while the installed generation carries that reference — a hand-built authority (tests) must supply `tagReferencingEquipment`. Inert with the flag off. + **Which permissions an event on a folder follows — checked against Part 3 (Gitea #838).** OPC UA Part 3's + `RolePermissions` model gives events their own permission, `ReceiveEvents` (PermissionType bit 11), and the pinned + SDK (2.0.1, `CustomNodeManager2.ValidateEventReceivePermissions`, documented there against Part 3 §8.55) applies it to + **both the event's EventType and its SourceNode** — not to the notifier the item happens to sit on. The notifier's own + permission only decides whether the item may be created/attached. So the spec's intent is *the folder AND the + source*, never "an event reported through a folder inherits the folder's permissions". This server keeps that shape: + the re-implemented `ValidateEventRolePermissions` runs the SDK's type + SourceNode check **first** and our decision + only on top (both must pass). For the event kinds that reach folder items: + - **Condition events** — SourceNode is the condition. Our decision is the condition rule above (the condition, or a + folder it legitimately notifies through for the session), which in this ACL model is the deliberate + generalisation of "the source": a `Tag` grant covers a native alarm's raw node and every UNS projection of it, and + an `Equipment` grant covers the alarms it owns or references. + - **A condition's Part 9 method audit events** (Acknowledge, Comment, Shelve…) — SourceNode is the condition, so they + are resolved to it and follow exactly the condition rule (plus the SDK's audit/SignAndEncrypt guard on the + Server-object path). + - **Any other event on a folder item** (none is produced today; `GeneralModelChangeEvent` and the rejected-write + `AuditWriteUpdateEvent` go to the Server object, #588) names no live condition and is **withheld** under + enforcement — fail closed, which is stricter than the folder's permission and never laxer than Part 3. + With the flag off only the SDK's own type + SourceNode `ReceiveEvents` check applies, as before. + + **Event bursts never block under the node-manager lock (Gitea #838).** Each notifier with event items has an SDK + `MonitoredNode2` whose delivery task drains a **bounded** channel (4096, `FullMode = Wait`), and the synchronous + `NodeState.ReportEvent` **blocks** while that channel is full. Condition events are reported under the node + manager's `Lock`, and the delivery task needs that same `Lock` on every per-event verdict-cache miss — in the SDK's + own `GetNodeMetadata` (so **with the flag off too**, i.e. this predates #826) and in the #826 check. A burst of more + than 4096 events under one hold of `Lock`, starting at a miss (a new item, or right after a deploy invalidated the + verdicts), therefore deadlocked the node manager: `FolderEventFloodWireTests` stalled at exactly 4097 events, flag on + and off. Condition events now go through `ReportEventWithoutBlocking`: `ReportEventAsync` completes inline while the + channels have room (the normal case, unchanged); when one is full that report continues asynchronously, holding no + thread and no lock, and later reports queue behind it so events still leave **in report order**. The deferred work + holds only frozen snapshots — bounded by the burst — and drains once `Lock` is released; what a client receives is + still bounded by its item's queue (oldest discarded). Pinned by `FolderEventFloodWireTests` (6000 events under one + `Lock` hold with publishing disabled: completes, deferred reports drain to zero, the item delivers at most its + revised `QueueSize` in increasing order ending with the last event, and the next event is delivered). + **Model-change and audit events on the Server object are gated too** (Gitea #588). The node manager's own server-wide events — `GeneralModelChangeEvent` (NodeAdded / NodeDeleted / DataTypeChanged) and the `AuditWriteUpdateEvent` of a rejected device write — used to go straight to `Server.ReportEvent`, where diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/OtOpcUaNodeManager.cs b/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/OtOpcUaNodeManager.cs index 62489d97c..84ee718bb 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/OtOpcUaNodeManager.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/OtOpcUaNodeManager.cs @@ -1362,7 +1362,7 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2, INodeManager3 // subscriptions, per item, through the ACL (see OnConditionReportEvent). var snapshot = new InstanceStateSnapshot(); snapshot.Initialize(SystemContext, alarm); - alarm.ReportEvent(SystemContext, snapshot); + ReportEventWithoutBlocking(alarm, snapshot); } catch (Exception ex) { @@ -1375,6 +1375,95 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2, INodeManager3 } } + /// Serializes . + private readonly object _eventReportGate = new(); + + /// The tail of the deferred condition-event reports (Gitea #838); completed when none is pending. + private Task _eventReportChain = Task.CompletedTask; + + private int _deferredEventReports; + + /// Condition-event reports currently deferred because a notifier's delivery channel was full (test observation). + internal int DeferredEventReports => Volatile.Read(ref _deferredEventReports); + + /// + /// Reports a condition event through the condition's notifier chain WITHOUT blocking the caller, in report + /// order (Gitea #838). + /// + /// The condition. + /// The frozen snapshot of the event. + /// + /// + /// Why not NodeState.ReportEvent. Each notifier with event items has an SDK + /// MonitoredNode2 whose delivery task drains a BOUNDED channel (4096, FullMode = Wait), and + /// the synchronous ReportEvent BLOCKS when that channel is full. Condition events are reported + /// under Lock, and the delivery task takes Lock on every per-event verdict-cache miss (the + /// SDK's own GetNodeMetadata, and #826's ). So a burst of more + /// than 4096 events under one hold of Lock, starting at a miss, parked the consumer on Lock + /// while the producer waited on the consumer: a deadlock that froze the whole node manager (pinned by + /// FolderEventFloodWireTests, which stalled at exactly 4097 events before this). + /// + /// + /// What this does. The common case is unchanged and synchronous: ReportEventAsync completes + /// inline while every channel has room. Only when a channel is full does that report continue + /// asynchronously (awaiting channel space, holding no thread and no lock), and every later report queues + /// behind it, so events still leave in the order they were reported. The deferred work holds only frozen + /// snapshots, so it is bounded by the burst that caused it and drains once Lock is released. + /// Delivery to clients stays bounded by each monitored item's own queue (oldest discarded). A deferred + /// report runs the condition's synchronous OnReportEvent hook () + /// off Lock; that hook takes Lock itself where it reads guarded state. + /// + /// + private void ReportEventWithoutBlocking(NodeState source, IFilterTarget e) + { + lock (_eventReportGate) + { + if (_eventReportChain.IsCompleted) + { + var report = source.ReportEventAsync(SystemContext, e); + if (report.IsCompletedSuccessfully) return; + Interlocked.Increment(ref _deferredEventReports); + _eventReportChain = ObserveEventReportAsync(report.AsTask(), source); + return; + } + + Interlocked.Increment(ref _deferredEventReports); + _eventReportChain = ReportAfterAsync(_eventReportChain, source, e); + } + } + + private async Task ReportAfterAsync(Task previous, NodeState source, IFilterTarget e) + { + await previous.ConfigureAwait(false); // never faults: every link observes its own failure + Task report; + try + { + report = source.ReportEventAsync(SystemContext, e).AsTask(); + } + catch (Exception ex) + { + report = Task.FromException(ex); + } + + await ObserveEventReportAsync(report, source).ConfigureAwait(false); + } + + private async Task ObserveEventReportAsync(Task report, NodeState source) + { + try + { + await report.ConfigureAwait(false); + } + catch (Exception ex) + { + _logger.LogError(ex, "OtOpcUaNodeManager: failed to report Part 9 condition event for {NodeId}", source.NodeId); + } + finally + { + Interlocked.Decrement(ref _deferredEventReports); + } + } + /// /// The gate-relevant slice of a Part 9 condition's state — exactly the fields that drive a /// condition event AND that an can change. As a record, two diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.IntegrationTests/FolderEventFloodWireTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.IntegrationTests/FolderEventFloodWireTests.cs new file mode 100644 index 000000000..60bc81b50 --- /dev/null +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.IntegrationTests/FolderEventFloodWireTests.cs @@ -0,0 +1,136 @@ +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.Abstractions; +using ZB.MOM.WW.OtOpcUa.Core.Authorization; + +namespace ZB.MOM.WW.OtOpcUa.OpcUaServer.IntegrationTests; + +/// +/// Gitea #838 (follow-up to #826): the per-folder event ACL runs on the SDK's MonitoredNode2 delivery task, +/// and needs the node manager's Lock on a verdict-cache miss. That delivery task drains a BOUNDED channel +/// (4096, FullMode = Wait), and the synchronous NodeState.ReportEvent the node manager calls under +/// Lock BLOCKS when the channel is full. So a burst of more than 4096 events reported under one hold of +/// Lock, starting at a verdict-cache miss, parks the consumer on Lock while the producer waits on the +/// consumer — a deadlock that also freezes every other operation needing Lock. Publishing is switched off +/// on the client so nothing downstream drains either (the slow / absent consumer case). +/// +public sealed class FolderEventFloodWireTests +{ + private const string EqA = ServerEventRig.EqA; + private const int FloodSize = 6000; // > the SDK's 4096-slot per-node channel + + /// + /// Per-folder ACL on or off. The hazard does NOT need #826: with enforcement off the SDK's own + /// ValidateEventRolePermissions still reaches GetNodeMetadata, which takes Lock, on the + /// first event to a new item. + /// + [Theory] + [InlineData(true)] + [InlineData(false)] + public async Task A_flood_beyond_the_channel_under_the_node_manager_lock_neither_deadlocks_nor_grows_unbounded(bool enforced) + { + var ct = TestContext.Current.CancellationToken; + var rig = await ServerEventRig.StartAsync("Folder838Flood", encrypted: false, ct); + var deadlocked = false; + ISession? keyed = null; + var reported = 0; + try + { + rig.KeyRoles = []; + rig.MaterialiseNative(); + NodeAcl[] grants = [KeyEquipmentGrant(EqA)]; + rig.Enforce(enforced, [ServerEventRig.Key()], grants); + + keyed = await rig.ConnectAsync(anonymous: false, ct); + var events = new RecordedEvents(); + var subscription = await RecordedEvents.SubscribeAsync(keyed, events, ct); + var onA = RecordedEvents.AddEventItem(subscription, ServerEventRig.UnsNodeId(keyed, EqA)); + await subscription.ApplyChangesAsync(ct); + onA.Status.Error.ShouldBeNull(); + + // No publishing: the server-side item queue (QueueSize 1000) is the only sink, as with a stalled client. + await subscription.SetPublishingModeAsync(false, ct); + + // The whole burst under ONE hold of the node manager's Lock — the shape of any multi-event apply/projection + // pass — starting from an uncached verdict (the item is new), so the delivery task needs Lock for event 1. + var flood = Task.Factory.StartNew(() => + { + lock (rig.Nm.Lock) + { + for (var i = 0; i < FloodSize; i++) + { + rig.Project(native: true, State($"f{i}")); + Volatile.Write(ref reported, i + 1); + } + } + }, ct, TaskCreationOptions.LongRunning, TaskScheduler.Default); + + try + { + await flood.WaitAsync(TimeSpan.FromSeconds(60), ct); + } + catch (TimeoutException) + { + deadlocked = true; + throw new ShouldAssertException( + $"reporting {FloodSize} folder events under the node manager's Lock stalled after " + + $"{Volatile.Read(ref reported)} — the SDK's per-node event channel filled while its consumer waited " + + "on Lock (deadlock)"); + } + + // The overflow was deferred rather than blocking, and drains once Lock is free. + await RecordedEvents.WaitUntilAsync(() => rig.Nm.DeferredEventReports == 0); + // …and the SDK's channel (up to 4096 accepted events) finishes draining into the item queue, still with + // publishing off, so the count below measures the item queue and not events in transit. + await Task.Delay(TimeSpan.FromSeconds(2), ct); + + // Delivery is bounded by the item's queue (QueueSize 1000, oldest discarded), not by the flood, and the + // events that survive arrive in report order, ending with the last one reported. + await subscription.SetPublishingModeAsync(true, ct); + var last = $"f{FloodSize - 1}"; + await RecordedEvents.WaitUntilAsync(() => events.Has(onA, last)); + await Task.Delay(TimeSpan.FromMilliseconds(500), ct); + var flooded = events.Of(onA) + .Select(RecordedEvents.Message) + .Where(m => m?.StartsWith('f') == true) + .Select(m => int.Parse(m![1..], System.Globalization.CultureInfo.InvariantCulture)) + .ToList(); + flooded.Count.ShouldBeGreaterThan(0); + flooded.Count.ShouldBeLessThanOrEqualTo((int)onA.Status.QueueSize, "delivery is bounded by the item's queue, not the flood"); + flooded.ShouldBe(flooded.Order().ToList(), "events leave in the order they were reported"); + flooded[^1].ShouldBe(FloodSize - 1); + + // And the node manager is live: a later event is decided by the per-folder ACL and delivered. + rig.Project(native: true, State("after")); + await RecordedEvents.WaitUntilAsync(() => events.Has(onA, "after")); + } + finally + { + // A deadlocked rig cannot be disposed (its Lock is held by a blocked thread) and neither can a session on it + // (CloseSession needs the server); leak both rather than hang the run. + if (!deadlocked) + { + keyed?.Dispose(); + await rig.DisposeAsync(); + } + } + } + + private static AlarmConditionSnapshot State(string message) => + new(true, false, false, true, AlarmShelvingKind.Unshelved, 700, message); + + private static NodeAcl KeyEquipmentGrant(string equipment) => new() + { + NodeAclId = $"acl-key-{equipment}", + ClusterId = ServerEventRig.Cluster, + Principal = ServerEventRig.KeyPrincipal, + ScopeKind = NodeAclScopeKind.Equipment, + ScopeId = $"eq-{equipment}", + PermissionFlags = NodePermissions.ReadOnly, + }; +}