From 3a1980cb4c1e989a7525796db82fa86064cbce4d Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Fri, 10 Jul 2026 05:12:29 -0400 Subject: [PATCH] fix(management): elide DataConnection config secrets from responses and audit (arch-review C3) Claude-Session: https://claude.ai/code/session_01MtdgwpEeCUn6cUA5f1LMPj --- .../Component-ManagementService.md | 2 + .../ManagementActor.cs | 48 +++++-- .../SecretProjectionTests.cs | 134 ++++++++++++++++++ 3 files changed, 175 insertions(+), 9 deletions(-) create mode 100644 tests/ZB.MOM.WW.ScadaBridge.ManagementService.Tests/SecretProjectionTests.cs diff --git a/docs/requirements/Component-ManagementService.md b/docs/requirements/Component-ManagementService.md index c29d2c88..99450fe2 100644 --- a/docs/requirements/Component-ManagementService.md +++ b/docs/requirements/Component-ManagementService.md @@ -134,6 +134,8 @@ Both endpoints honour any site-scope rules attached to the caller's audit role b - **CreateDataConnection** / **UpdateDataConnection** / **DeleteDataConnection**: Manage data connection definitions. - **AssignDataConnectionToSite** / **UnassignDataConnectionFromSite**: Manage site assignments. +> **Secret elision (arch-review C3).** The `PrimaryConfiguration`/`BackupConfiguration` JSON blobs can embed OPC UA user and certificate passwords, yet List/Get are readable by any authenticated user. All command responses **and** audit `afterState` are projected through a secret-elided shape (`DataConnectionPublicShape`): secret-bearing values inside the config JSON are replaced with a redaction sentinel and a `hasSecrets` flag is surfaced (mirrors the SMTP/SMS `HasCredentials` pattern). **Update** merges the sentinel via `ConfigSecretScrubber.MergeSentinels` — a client that round-trips a scrubbed config keeps the stored secret rather than wiping it. + ### Deployments - **DeployInstance**: Deploy configuration to a specific instance (includes pre-deployment validation). diff --git a/src/ZB.MOM.WW.ScadaBridge.ManagementService/ManagementActor.cs b/src/ZB.MOM.WW.ScadaBridge.ManagementService/ManagementActor.cs index c773c540..57fd228a 100644 --- a/src/ZB.MOM.WW.ScadaBridge.ManagementService/ManagementActor.cs +++ b/src/ZB.MOM.WW.ScadaBridge.ManagementService/ManagementActor.cs @@ -1445,12 +1445,37 @@ public class ManagementActor : ReceiveActor // Data Connection handlers // ======================================================================== + /// + /// Secret-elided projection of a DataConnection (arch-review C3): the raw + /// PrimaryConfiguration/BackupConfiguration JSON can embed OPC UA user and + /// certificate passwords, and List/Get are readable by any authenticated user. + /// Mirrors . The secret-bearing config blobs + /// are scrubbed to the redaction sentinel; every non-config field is projected + /// verbatim so callers keep the full shape they had before. + /// + private static object DataConnectionPublicShape(DataConnection c) + { + var (primary, s1) = ConfigSecretScrubber.Scrub(c.PrimaryConfiguration); + var (backup, s2) = ConfigSecretScrubber.Scrub(c.BackupConfiguration); + return new + { + c.Id, + c.SiteId, + c.Name, + c.Protocol, + c.FailoverRetryCount, + PrimaryConfiguration = primary, + BackupConfiguration = backup, + HasSecrets = s1 || s2, + }; + } + private static async Task HandleListDataConnections(IServiceProvider sp, ListDataConnectionsCommand cmd) { var repo = sp.GetRequiredService(); if (cmd.SiteId.HasValue) - return await repo.GetDataConnectionsBySiteIdAsync(cmd.SiteId.Value); - return await repo.GetAllDataConnectionsAsync(); + return (await repo.GetDataConnectionsBySiteIdAsync(cmd.SiteId.Value)).Select(DataConnectionPublicShape).ToList(); + return (await repo.GetAllDataConnectionsAsync()).Select(DataConnectionPublicShape).ToList(); } private static async Task HandleGetDataConnection(IServiceProvider sp, GetDataConnectionCommand cmd, AuthenticatedUser user) @@ -1459,7 +1484,7 @@ public class ManagementActor : ReceiveActor var conn = await repo.GetDataConnectionByIdAsync(cmd.DataConnectionId); if (conn != null) EnforceSiteScope(user, conn.SiteId); - return conn; + return conn is null ? null : DataConnectionPublicShape(conn); } private static async Task HandleCreateDataConnection(IServiceProvider sp, CreateDataConnectionCommand cmd, string user) @@ -1473,8 +1498,9 @@ public class ManagementActor : ReceiveActor }; await repo.AddDataConnectionAsync(conn); await repo.SaveChangesAsync(); - await AuditAsync(sp, user, "Create", "DataConnection", conn.Id.ToString(), conn.Name, conn); - return conn; + var publicShape = DataConnectionPublicShape(conn); + await AuditAsync(sp, user, "Create", "DataConnection", conn.Id.ToString(), conn.Name, publicShape); + return publicShape; } private static async Task HandleUpdateDataConnection(IServiceProvider sp, UpdateDataConnectionCommand cmd, string user) @@ -1484,13 +1510,17 @@ public class ManagementActor : ReceiveActor ?? throw new ManagementCommandException($"DataConnection with ID {cmd.DataConnectionId} not found."); conn.Name = cmd.Name; conn.Protocol = cmd.Protocol; - conn.PrimaryConfiguration = cmd.PrimaryConfiguration; - conn.BackupConfiguration = cmd.BackupConfiguration; + // Sentinel-preserving merge: a client that round-trips a scrubbed config + // (secrets replaced with the redaction sentinel) keeps the stored secret + // rather than wiping it. Non-sentinel values overwrite as usual. + conn.PrimaryConfiguration = ConfigSecretScrubber.MergeSentinels(cmd.PrimaryConfiguration, conn.PrimaryConfiguration); + conn.BackupConfiguration = ConfigSecretScrubber.MergeSentinels(cmd.BackupConfiguration, conn.BackupConfiguration); conn.FailoverRetryCount = cmd.FailoverRetryCount; await repo.UpdateDataConnectionAsync(conn); await repo.SaveChangesAsync(); - await AuditAsync(sp, user, "Update", "DataConnection", conn.Id.ToString(), conn.Name, conn); - return conn; + var publicShape = DataConnectionPublicShape(conn); + await AuditAsync(sp, user, "Update", "DataConnection", conn.Id.ToString(), conn.Name, publicShape); + return publicShape; } private static async Task HandleDeleteDataConnection(IServiceProvider sp, DeleteDataConnectionCommand cmd, string user) diff --git a/tests/ZB.MOM.WW.ScadaBridge.ManagementService.Tests/SecretProjectionTests.cs b/tests/ZB.MOM.WW.ScadaBridge.ManagementService.Tests/SecretProjectionTests.cs new file mode 100644 index 00000000..e474f1dc --- /dev/null +++ b/tests/ZB.MOM.WW.ScadaBridge.ManagementService.Tests/SecretProjectionTests.cs @@ -0,0 +1,134 @@ +using Akka.Actor; +using Akka.TestKit.Xunit2; +using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Logging.Abstractions; +using NSubstitute; +using ZB.MOM.WW.ScadaBridge.Commons.Entities.ExternalSystems; +using ZB.MOM.WW.ScadaBridge.Commons.Entities.Sites; +using ZB.MOM.WW.ScadaBridge.Commons.Interfaces.Repositories; +using ZB.MOM.WW.ScadaBridge.Commons.Interfaces.Services; +using ZB.MOM.WW.ScadaBridge.Commons.Messages.Management; +using ZB.MOM.WW.ScadaBridge.Commons.Types; + +namespace ZB.MOM.WW.ScadaBridge.ManagementService.Tests; + +/// +/// Secret-elision projection tests (arch-review C3): connection-config secrets +/// (OPC UA passwords, external-system auth config, DB connection strings) must +/// never leave the management boundary via List/Get responses or audit afterState, +/// and sentinel/omitted round-trips through Update must preserve the stored secret. +/// +public class SecretProjectionTests : TestKit, IDisposable +{ + private readonly IAuditService _auditService; + private readonly ServiceCollection _services; + + public SecretProjectionTests() + { + _auditService = Substitute.For(); + _services = new ServiceCollection(); + _services.AddScoped(_ => _auditService); + } + + private IActorRef CreateActor() + { + var sp = _services.BuildServiceProvider(); + return Sys.ActorOf(Props.Create(() => new ManagementActor( + sp, NullLogger.Instance))); + } + + private static ManagementEnvelope Envelope(object command, params string[] roles) => + new(new AuthenticatedUser("testuser", "Test User", roles, Array.Empty()), + command, Guid.NewGuid().ToString("N")); + + void IDisposable.Dispose() => Shutdown(); + + // ======================================================================== + // Task 7 — DataConnection config secret elision + // ======================================================================== + + private ISiteRepository AddSiteRepoWithConnection(DataConnection conn) + { + var siteRepo = Substitute.For(); + siteRepo.GetDataConnectionByIdAsync(conn.Id, Arg.Any()).Returns(conn); + siteRepo.GetAllDataConnectionsAsync(Arg.Any()) + .Returns(new List { conn }); + _services.AddScoped(_ => siteRepo); + return siteRepo; + } + + [Fact] + public void GetDataConnection_ResponseOmitsPasswords() + { + AddSiteRepoWithConnection(new DataConnection("c1", "OpcUa", 1) + { + Id = 1, + PrimaryConfiguration = """{"Endpoint":"opc.tcp://x","UserIdentity":{"Password":"opc-secret"}}""", + }); + + var actor = CreateActor(); + actor.Tell(Envelope(new GetDataConnectionCommand(1), "Viewer")); + + var resp = ExpectMsg(TimeSpan.FromSeconds(5)); + Assert.DoesNotContain("opc-secret", resp.JsonData); + Assert.Contains("hasSecrets", resp.JsonData); // camelCase serializer + } + + [Fact] + public void ListDataConnections_ResponseOmitsPasswords() + { + AddSiteRepoWithConnection(new DataConnection("c1", "OpcUa", 1) + { + Id = 1, + PrimaryConfiguration = """{"UserIdentity":{"Password":"opc-secret"}}""", + }); + + var actor = CreateActor(); + actor.Tell(Envelope(new ListDataConnectionsCommand(null), "Viewer")); + + var resp = ExpectMsg(TimeSpan.FromSeconds(5)); + Assert.DoesNotContain("opc-secret", resp.JsonData); + Assert.Contains("hasSecrets", resp.JsonData); + } + + [Fact] + public async Task UpdateDataConnection_SentinelConfig_PreservesStoredSecret() + { + var stored = new DataConnection("c1", "OpcUa", 1) + { + Id = 1, + PrimaryConfiguration = """{"Endpoint":"opc.tcp://old","UserIdentity":{"Password":"opc-secret"}}""", + }; + var siteRepo = AddSiteRepoWithConnection(stored); + + var actor = CreateActor(); + var incoming = """{"Endpoint":"opc.tcp://new","UserIdentity":{"Password":"***REDACTED***"}}"""; + actor.Tell(Envelope(new UpdateDataConnectionCommand(1, "c1", "OpcUa", incoming, null, 3), "Designer")); + ExpectMsg(TimeSpan.FromSeconds(5)); + + await siteRepo.Received(1).UpdateDataConnectionAsync( + Arg.Is(c => c.PrimaryConfiguration!.Contains("opc-secret") + && !c.PrimaryConfiguration.Contains("***REDACTED***")), + Arg.Any()); + } + + [Fact] + public void UpdateDataConnection_AuditAfterState_OmitsPasswords() + { + var stored = new DataConnection("c1", "OpcUa", 1) + { + Id = 1, + PrimaryConfiguration = """{"Password":"opc-secret"}""", + }; + AddSiteRepoWithConnection(stored); + + var actor = CreateActor(); + actor.Tell(Envelope(new UpdateDataConnectionCommand(1, "c1", "OpcUa", """{"Password":"newpw"}""", null, 3), "Designer")); + ExpectMsg(TimeSpan.FromSeconds(5)); + + _auditService.Received().LogAsync(Arg.Any(), "Update", "DataConnection", + Arg.Any(), Arg.Any(), + Arg.Is(o => !ManagementActor.SerializeResult(o).Contains("newpw")), + Arg.Any()); + } +}