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(); + } +}