From 6b04a85f86c9fcd0a0f68e61331f572a6f6154ca Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Sat, 18 Apr 2026 13:01:01 -0400 Subject: [PATCH] =?UTF-8?q?Phase=203=20PR=2026=20=E2=80=94=20server-layer?= =?UTF-8?q?=20write=20authorization=20gating=20by=20role.=20Per=20the=20us?= =?UTF-8?q?er's=20ACL-at-server-layer=20directive=20(saved=20as=20feedback?= =?UTF-8?q?=5Facl=5Fat=5Fserver=5Flayer.md=20in=20memory),=20write=20autho?= =?UTF-8?q?rization=20is=20enforced=20in=20DriverNodeManager.OnWriteValue?= =?UTF-8?q?=20and=20never=20delegated=20to=20the=20driver=20or=20to=20driv?= =?UTF-8?q?er-specific=20auth=20(the=20v1=20Galaxy-provided=20security=20p?= =?UTF-8?q?ath=20is=20explicitly=20not=20part=20of=20v2=20=E2=80=94=20driv?= =?UTF-8?q?ers=20report=20SecurityClassification=20as=20discovery=20metada?= =?UTF-8?q?ta=20only).=20New=20WriteAuthzPolicy=20static=20class=20in=20Se?= =?UTF-8?q?rver/Security/=20maps=20SecurityClassification=20=E2=86=92=20re?= =?UTF-8?q?quired=20role=20per=20the=20table=20documented=20in=20docs/Conf?= =?UTF-8?q?iguration.md:=20FreeAccess=20=3D=20no=20role=20required=20(anon?= =?UTF-8?q?ymous=20sessions=20can=20write),=20Operate=20+=20SecuredWrite?= =?UTF-8?q?=20=3D=20WriteOperate,=20Tune=20=3D=20WriteTune,=20VerifiedWrit?= =?UTF-8?q?e=20+=20Configure=20=3D=20WriteConfigure,=20ViewOnly=20=3D=20de?= =?UTF-8?q?ny=20regardless=20of=20roles.=20Role=20matching=20is=20case-ins?= =?UTF-8?q?ensitive=20and=20role=20requirements=20do=20NOT=20cascade=20?= =?UTF-8?q?=E2=80=94=20a=20session=20with=20WriteConfigure=20can=20write?= =?UTF-8?q?=20Configure=20attributes=20but=20needs=20WriteOperate=20separa?= =?UTF-8?q?tely=20to=20write=20Operate=20attributes;=20this=20is=20deliber?= =?UTF-8?q?ate=20so=20escalation=20is=20an=20explicit=20LDAP=20group=20ass?= =?UTF-8?q?ignment,=20not=20a=20hierarchy=20the=20policy=20silently=20gran?= =?UTF-8?q?ts.=20DriverNodeManager=20gains=20a=20=5FsecurityByFullRef=20Di?= =?UTF-8?q?ctionary=20populated=20during=20Variable()=20registration=20(pa?= =?UTF-8?q?rallel=20to=20the=20existing=20=5FvariablesByFullRef)=20so=20On?= =?UTF-8?q?WriteValue=20can=20look=20up=20the=20classification=20in=20O(1)?= =?UTF-8?q?=20on=20the=20hot=20path.=20OnWriteValue=20casts=20the=20sessio?= =?UTF-8?q?n's=20context.UserIdentity=20to=20the=20new=20IRoleBearer=20int?= =?UTF-8?q?erface=20(implemented=20by=20OtOpcUaServer.RoleBasedIdentity=20?= =?UTF-8?q?from=20PR=2019)=20=E2=80=94=20empty=20Roles=20collection=20when?= =?UTF-8?q?=20the=20session=20is=20anonymous;=20the=20same=20WriteAuthzPol?= =?UTF-8?q?icy.IsAllowed=20check=20then=20either=20short-circuits=20true?= =?UTF-8?q?=20(FreeAccess),=20false=20(ViewOnly),=20or=20walks=20the=20rol?= =?UTF-8?q?es=20list=20looking=20for=20the=20required=20one.=20On=20deny,?= =?UTF-8?q?=20OnWriteValue=20logs=20'Write=20denied=20for=20{FullRef}:=20c?= =?UTF-8?q?lassification=3DX=20userRoles=3D[...]'=20at=20Information=20lev?= =?UTF-8?q?el=20(readable=20trail=20for=20operator=20complaints)=20and=20r?= =?UTF-8?q?eturns=20BadUserAccessDenied=20without=20touching=20IWritable.W?= =?UTF-8?q?riteAsync=20=E2=80=94=20drivers=20never=20see=20a=20request=20w?= =?UTF-8?q?e'd=20have=20refused.=20IRoleBearer=20kept=20as=20a=20minimal?= =?UTF-8?q?=20server-side=20interface=20rather=20than=20reusing=20some=20a?= =?UTF-8?q?bstraction=20from=20Core.Abstractions=20because=20the=20concept?= =?UTF-8?q?=20is=20OPC-UA-session-scoped=20and=20doesn't=20generalize=20(t?= =?UTF-8?q?he=20driver=20side=20has=20no=20notion=20of=20a=20user=20sessio?= =?UTF-8?q?n).=20Tests=20=E2=80=94=20WriteAuthzPolicyTests=20(17=20new=20c?= =?UTF-8?q?ases):=20FreeAccess=20allows=20write=20with=20empty=20role=20se?= =?UTF-8?q?t=20+=20arbitrary=20roles;=20ViewOnly=20denies=20write=20even?= =?UTF-8?q?=20with=20every=20role;=20Operate=20requires=20WriteOperate;=20?= =?UTF-8?q?role=20match=20is=20case-insensitive;=20Operate=20denies=20empt?= =?UTF-8?q?y=20role=20set=20+=20wrong=20role;=20SecuredWrite=20shares=20Op?= =?UTF-8?q?erate's=20requirement;=20Tune=20requires=20WriteTune;=20Tune=20?= =?UTF-8?q?denies=20WriteOperate-only=20(asserts=20roles=20don't=20cascade?= =?UTF-8?q?=20=E2=80=94=20this=20is=20the=20test=20that=20catches=20a=20fu?= =?UTF-8?q?ture=20regression=20where=20someone=20'helpfully'=20adds=20a=20?= =?UTF-8?q?role-escalation=20table);=20Configure=20requires=20WriteConfigu?= =?UTF-8?q?re;=20VerifiedWrite=20shares=20Configure's=20requirement;=20mul?= =?UTF-8?q?ti-role=20session=20allowed=20when=20any=20role=20matches;=20un?= =?UTF-8?q?related=20roles=20denied;=20RequiredRole=20theory=20covering=20?= =?UTF-8?q?all=205=20classified-and-mapped=20rows=20+=20null=20for=20FreeA?= =?UTF-8?q?ccess/ViewOnly=20special=20cases.=20lmx-followups.md=20follow-u?= =?UTF-8?q?p=20#2=20marked=20DONE=20with=20a=20back-reference=20to=20this?= =?UTF-8?q?=20PR=20and=20the=20memory=20note.=20Full=20Server.Tests=20Unit?= =?UTF-8?q?=20suite:=2038=20pass=20/=200=20fail=20(17=20new=20WriteAuthz?= =?UTF-8?q?=20+=2014=20SecurityConfiguration=20from=20PR=2019=20+=202=20No?= =?UTF-8?q?deBootstrap=20+=205=20others).=20Server.Tests=20Integration=20(?= =?UTF-8?q?Category=3DIntegration)=202=20pass=20=E2=80=94=20existing=20PR?= =?UTF-8?q?=2017=20anonymous-endpoint=20smoke=20tests=20stay=20green=20sin?= =?UTF-8?q?ce=20the=20read=20path=20doesn't=20hit=20OnWriteValue.?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-Authored-By: Claude Opus 4.7 (1M context) --- docs/v2/lmx-followups.md | 24 ++-- .../OpcUa/DriverNodeManager.cs | 24 ++++ .../OpcUa/OtOpcUaServer.cs | 2 +- .../Security/IRoleBearer.cs | 13 ++ .../Security/WriteAuthzPolicy.cs | 70 +++++++++ .../WriteAuthzPolicyTests.cs | 134 ++++++++++++++++++ 6 files changed, 253 insertions(+), 14 deletions(-) create mode 100644 src/ZB.MOM.WW.OtOpcUa.Server/Security/IRoleBearer.cs create mode 100644 src/ZB.MOM.WW.OtOpcUa.Server/Security/WriteAuthzPolicy.cs create mode 100644 tests/ZB.MOM.WW.OtOpcUa.Server.Tests/WriteAuthzPolicyTests.cs diff --git a/docs/v2/lmx-followups.md b/docs/v2/lmx-followups.md index e16c26f..878e681 100644 --- a/docs/v2/lmx-followups.md +++ b/docs/v2/lmx-followups.md @@ -27,21 +27,19 @@ only exposes `ReadRawAsync` + `ReadProcessedAsync`. - Integration test: OPC UA client calls `HistoryReadAtTime` / `HistoryReadEvents`, value flows through IPC to the Host's `HistorianDataSource`, back to the client. -## 2. Write-gating by role +## 2. Write-gating by role — **DONE (PR 26)** -**Status**: `RoleBasedIdentity.Roles` populated on the session (PR 19) but -`DriverNodeManager.OnWriteValue` doesn't consult it. +Landed in PR 26. `WriteAuthzPolicy` in `Server/Security/` maps +`SecurityClassification` → required role (`FreeAccess` → no role required, +`Operate`/`SecuredWrite` → `WriteOperate`, `Tune` → `WriteTune`, +`Configure`/`VerifiedWrite` → `WriteConfigure`, `ViewOnly` → deny regardless). +`DriverNodeManager` caches the classification per variable during discovery and +checks the session's roles (via `IRoleBearer`) in `OnWriteValue` before calling +`IWritable.WriteAsync`. Roles do not cascade — a session with `WriteOperate` +can't write a `Tune` attribute unless it also carries `WriteTune`. -CLAUDE.md defines the role set: `ReadOnly` / `WriteOperate` / `WriteTune` / -`WriteConfigure` / `AlarmAck`. Each `DriverAttributeInfo.SecurityClassification` -maps to a required role for writes. - -**To do**: -- Add a `RoleRequirements` table: `SecurityClassification` → required role. -- `OnWriteValue` reads `context.UserIdentity` → cast to `RoleBasedIdentity` - → check role membership before calling `IWritable.WriteAsync`. Return - `BadUserAccessDenied` on miss. -- Unit test against a fake `ISystemContext` with varying role sets. +See `feedback_acl_at_server_layer.md` in memory for the architectural directive +that authz stays at the server layer and never delegates to driver-specific auth. ## 3. Admin UI client-cert trust management diff --git a/src/ZB.MOM.WW.OtOpcUa.Server/OpcUa/DriverNodeManager.cs b/src/ZB.MOM.WW.OtOpcUa.Server/OpcUa/DriverNodeManager.cs index d6738e3..fcd09dd 100644 --- a/src/ZB.MOM.WW.OtOpcUa.Server/OpcUa/DriverNodeManager.cs +++ b/src/ZB.MOM.WW.OtOpcUa.Server/OpcUa/DriverNodeManager.cs @@ -3,6 +3,7 @@ using Microsoft.Extensions.Logging; using Opc.Ua; using Opc.Ua.Server; using ZB.MOM.WW.OtOpcUa.Core.Abstractions; +using ZB.MOM.WW.OtOpcUa.Server.Security; using DriverWriteRequest = ZB.MOM.WW.OtOpcUa.Core.Abstractions.WriteRequest; namespace ZB.MOM.WW.OtOpcUa.Server.OpcUa; @@ -35,6 +36,12 @@ public sealed class DriverNodeManager : CustomNodeManager2, IAddressSpaceBuilder private FolderState? _driverRoot; private readonly Dictionary _variablesByFullRef = new(StringComparer.OrdinalIgnoreCase); + // PR 26: SecurityClassification per variable, populated during Variable() registration. + // OnWriteValue looks up the classification here to gate the write by the session's roles. + // Drivers never enforce authz themselves — the classification is discovery-time metadata + // only (feedback_acl_at_server_layer.md). + private readonly Dictionary _securityByFullRef = new(StringComparer.OrdinalIgnoreCase); + // Active building folder — set per Folder() call so Variable() lands under the right parent. // A stack would support nested folders; we use a single current folder because IAddressSpaceBuilder // returns a child builder per Folder call and the caller threads nesting through those references. @@ -122,6 +129,7 @@ public sealed class DriverNodeManager : CustomNodeManager2, IAddressSpaceBuilder _currentFolder.AddChild(v); AddPredefinedNode(SystemContext, v); _variablesByFullRef[attributeInfo.FullName] = v; + _securityByFullRef[attributeInfo.FullName] = attributeInfo.SecurityClass; v.OnReadValue = OnReadValue; v.OnWriteValue = OnWriteValue; @@ -337,6 +345,22 @@ public sealed class DriverNodeManager : CustomNodeManager2, IAddressSpaceBuilder var fullRef = node.NodeId.Identifier as string; if (string.IsNullOrEmpty(fullRef)) return StatusCodes.BadNodeIdUnknown; + // PR 26: server-layer write authorization. Look up the attribute's classification + // (populated during Variable() in Discover) and check the session's roles against the + // policy table. Drivers don't participate in this decision — IWritable.WriteAsync + // never sees a request we'd have refused here. + if (_securityByFullRef.TryGetValue(fullRef!, out var classification)) + { + var roles = context.UserIdentity is IRoleBearer rb ? rb.Roles : []; + if (!WriteAuthzPolicy.IsAllowed(classification, roles)) + { + _logger.LogInformation( + "Write denied for {FullRef}: classification={Classification} userRoles=[{Roles}]", + fullRef, classification, string.Join(",", roles)); + return new ServiceResult(StatusCodes.BadUserAccessDenied); + } + } + try { var results = _writable.WriteAsync( diff --git a/src/ZB.MOM.WW.OtOpcUa.Server/OpcUa/OtOpcUaServer.cs b/src/ZB.MOM.WW.OtOpcUa.Server/OpcUa/OtOpcUaServer.cs index 04cde9a..8ccb660 100644 --- a/src/ZB.MOM.WW.OtOpcUa.Server/OpcUa/OtOpcUaServer.cs +++ b/src/ZB.MOM.WW.OtOpcUa.Server/OpcUa/OtOpcUaServer.cs @@ -97,7 +97,7 @@ public sealed class OtOpcUaServer : StandardServer /// managers can gate writes by role via session.Identity. Anonymous identity still /// uses the stack's default. /// - private sealed class RoleBasedIdentity : UserIdentity + private sealed class RoleBasedIdentity : UserIdentity, IRoleBearer { public IReadOnlyList Roles { get; } public string? Display { get; } diff --git a/src/ZB.MOM.WW.OtOpcUa.Server/Security/IRoleBearer.cs b/src/ZB.MOM.WW.OtOpcUa.Server/Security/IRoleBearer.cs new file mode 100644 index 0000000..1635bd5 --- /dev/null +++ b/src/ZB.MOM.WW.OtOpcUa.Server/Security/IRoleBearer.cs @@ -0,0 +1,13 @@ +namespace ZB.MOM.WW.OtOpcUa.Server.Security; + +/// +/// Minimal interface a implementation can expose so +/// can read the session's +/// resolved roles without a hard dependency on any specific identity subtype. Implemented +/// by OtOpcUaServer.RoleBasedIdentity; tests implement it with stub identities to +/// drive the authz policy under different role sets. +/// +public interface IRoleBearer +{ + IReadOnlyList Roles { get; } +} diff --git a/src/ZB.MOM.WW.OtOpcUa.Server/Security/WriteAuthzPolicy.cs b/src/ZB.MOM.WW.OtOpcUa.Server/Security/WriteAuthzPolicy.cs new file mode 100644 index 0000000..f7447c9 --- /dev/null +++ b/src/ZB.MOM.WW.OtOpcUa.Server/Security/WriteAuthzPolicy.cs @@ -0,0 +1,70 @@ +using ZB.MOM.WW.OtOpcUa.Core.Abstractions; + +namespace ZB.MOM.WW.OtOpcUa.Server.Security; + +/// +/// Server-layer write-authorization policy. ACL enforcement lives here — drivers report +/// as discovery metadata only; the server decides +/// whether a given session is allowed to write a given attribute by checking the session's +/// roles (resolved at login via ) against the required +/// role for the attribute's classification. +/// +/// +/// Matches the table in docs/Configuration.md: +/// +/// FreeAccess: no role required — anonymous sessions can write (matches v1 default). +/// Operate / SecuredWrite: WriteOperate role required. +/// Tune: WriteTune role required. +/// VerifiedWrite / Configure: WriteConfigure role required. +/// ViewOnly: no role grants write access. +/// +/// AlarmAck is checked at the alarm-acknowledge path, not here. +/// +public static class WriteAuthzPolicy +{ + public const string RoleWriteOperate = "WriteOperate"; + public const string RoleWriteTune = "WriteTune"; + public const string RoleWriteConfigure = "WriteConfigure"; + + /// + /// Decide whether a session with is allowed to write to an + /// attribute with the given . Returns true for + /// FreeAccess regardless of roles (including empty / anonymous sessions) and + /// false for ViewOnly regardless of roles. Every other classification requires + /// the session to carry the mapped role — case-insensitive match. + /// + public static bool IsAllowed(SecurityClassification classification, IReadOnlyCollection userRoles) + { + if (classification == SecurityClassification.FreeAccess) return true; + if (classification == SecurityClassification.ViewOnly) return false; + + var required = RequiredRole(classification); + if (required is null) return false; + + foreach (var r in userRoles) + { + if (string.Equals(r, required, StringComparison.OrdinalIgnoreCase)) + return true; + } + return false; + } + + /// + /// Required role for a classification, or null when no role grants access + /// () or no role is needed + /// ( — also returns null; callers use + /// which handles the special-cases rather than branching on + /// null themselves). + /// + public static string? RequiredRole(SecurityClassification classification) => classification switch + { + SecurityClassification.FreeAccess => null, // IsAllowed short-circuits + SecurityClassification.Operate => RoleWriteOperate, + SecurityClassification.SecuredWrite => RoleWriteOperate, + SecurityClassification.Tune => RoleWriteTune, + SecurityClassification.VerifiedWrite => RoleWriteConfigure, + SecurityClassification.Configure => RoleWriteConfigure, + SecurityClassification.ViewOnly => null, // IsAllowed short-circuits + _ => null, + }; +} diff --git a/tests/ZB.MOM.WW.OtOpcUa.Server.Tests/WriteAuthzPolicyTests.cs b/tests/ZB.MOM.WW.OtOpcUa.Server.Tests/WriteAuthzPolicyTests.cs new file mode 100644 index 0000000..15ef758 --- /dev/null +++ b/tests/ZB.MOM.WW.OtOpcUa.Server.Tests/WriteAuthzPolicyTests.cs @@ -0,0 +1,134 @@ +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.Core.Abstractions; +using ZB.MOM.WW.OtOpcUa.Server.Security; + +namespace ZB.MOM.WW.OtOpcUa.Server.Tests; + +[Trait("Category", "Unit")] +public sealed class WriteAuthzPolicyTests +{ + // --- FreeAccess and ViewOnly special-cases --- + + [Fact] + public void FreeAccess_allows_write_even_for_empty_role_set() + { + WriteAuthzPolicy.IsAllowed(SecurityClassification.FreeAccess, []).ShouldBeTrue(); + } + + [Fact] + public void FreeAccess_allows_write_for_arbitrary_roles() + { + WriteAuthzPolicy.IsAllowed(SecurityClassification.FreeAccess, ["SomeOtherRole"]).ShouldBeTrue(); + } + + [Fact] + public void ViewOnly_denies_write_even_with_every_role() + { + var allRoles = new[] { "WriteOperate", "WriteTune", "WriteConfigure", "AlarmAck" }; + WriteAuthzPolicy.IsAllowed(SecurityClassification.ViewOnly, allRoles).ShouldBeFalse(); + } + + // --- Operate tier --- + + [Fact] + public void Operate_requires_WriteOperate_role() + { + WriteAuthzPolicy.IsAllowed(SecurityClassification.Operate, ["WriteOperate"]).ShouldBeTrue(); + } + + [Fact] + public void Operate_role_match_is_case_insensitive() + { + WriteAuthzPolicy.IsAllowed(SecurityClassification.Operate, ["writeoperate"]).ShouldBeTrue(); + WriteAuthzPolicy.IsAllowed(SecurityClassification.Operate, ["WRITEOPERATE"]).ShouldBeTrue(); + } + + [Fact] + public void Operate_denies_empty_role_set() + { + WriteAuthzPolicy.IsAllowed(SecurityClassification.Operate, []).ShouldBeFalse(); + } + + [Fact] + public void Operate_denies_wrong_role() + { + WriteAuthzPolicy.IsAllowed(SecurityClassification.Operate, ["ReadOnly"]).ShouldBeFalse(); + } + + [Fact] + public void SecuredWrite_maps_to_same_WriteOperate_requirement_as_Operate() + { + WriteAuthzPolicy.IsAllowed(SecurityClassification.SecuredWrite, ["WriteOperate"]).ShouldBeTrue(); + WriteAuthzPolicy.IsAllowed(SecurityClassification.SecuredWrite, ["WriteTune"]).ShouldBeFalse(); + } + + // --- Tune tier --- + + [Fact] + public void Tune_requires_WriteTune_role() + { + WriteAuthzPolicy.IsAllowed(SecurityClassification.Tune, ["WriteTune"]).ShouldBeTrue(); + } + + [Fact] + public void Tune_denies_WriteOperate_only_session() + { + // Important: role roles do NOT cascade — a session with WriteOperate can't write a Tune + // attribute. Operators escalate by adding WriteTune to the session's roles, not by a + // hierarchy the policy infers on its own. + WriteAuthzPolicy.IsAllowed(SecurityClassification.Tune, ["WriteOperate"]).ShouldBeFalse(); + } + + // --- Configure tier --- + + [Fact] + public void Configure_requires_WriteConfigure_role() + { + WriteAuthzPolicy.IsAllowed(SecurityClassification.Configure, ["WriteConfigure"]).ShouldBeTrue(); + } + + [Fact] + public void VerifiedWrite_maps_to_same_WriteConfigure_requirement_as_Configure() + { + WriteAuthzPolicy.IsAllowed(SecurityClassification.VerifiedWrite, ["WriteConfigure"]).ShouldBeTrue(); + WriteAuthzPolicy.IsAllowed(SecurityClassification.VerifiedWrite, ["WriteOperate"]).ShouldBeFalse(); + } + + // --- Multi-role sessions --- + + [Fact] + public void Session_with_multiple_roles_is_allowed_when_any_matches() + { + var roles = new[] { "ReadOnly", "WriteTune", "AlarmAck" }; + WriteAuthzPolicy.IsAllowed(SecurityClassification.Tune, roles).ShouldBeTrue(); + } + + [Fact] + public void Session_with_only_unrelated_roles_is_denied() + { + var roles = new[] { "ReadOnly", "AlarmAck", "SomeCustomRole" }; + WriteAuthzPolicy.IsAllowed(SecurityClassification.Configure, roles).ShouldBeFalse(); + } + + // --- Mapping table --- + + [Theory] + [InlineData(SecurityClassification.Operate, WriteAuthzPolicy.RoleWriteOperate)] + [InlineData(SecurityClassification.SecuredWrite, WriteAuthzPolicy.RoleWriteOperate)] + [InlineData(SecurityClassification.Tune, WriteAuthzPolicy.RoleWriteTune)] + [InlineData(SecurityClassification.VerifiedWrite, WriteAuthzPolicy.RoleWriteConfigure)] + [InlineData(SecurityClassification.Configure, WriteAuthzPolicy.RoleWriteConfigure)] + public void RequiredRole_returns_expected_role_for_classification(SecurityClassification c, string expected) + { + WriteAuthzPolicy.RequiredRole(c).ShouldBe(expected); + } + + [Theory] + [InlineData(SecurityClassification.FreeAccess)] + [InlineData(SecurityClassification.ViewOnly)] + public void RequiredRole_returns_null_for_special_classifications(SecurityClassification c) + { + WriteAuthzPolicy.RequiredRole(c).ShouldBeNull(); + } +} -- 2.49.1