Merge #835/#836 review fixes (owner check on unsaved endpoints, credential-use audit)

2026-11-11 04:13:02 -05:00
parent 2fc9e02b9a 9072777422
commit 214813bbc8
21 changed files with 790 additions and 32 deletions
@@ -0,0 +1,212 @@
+using System.Security.Claims;
+using Microsoft.AspNetCore.Components.Authorization;
+using Microsoft.EntityFrameworkCore;
+using Microsoft.Extensions.Logging.Abstractions;
+using Shouldly;
+using Xunit;
+using ZB.MOM.WW.Audit;
+using ZB.MOM.WW.OtOpcUa.AdminUI.Audit;
+using ZB.MOM.WW.OtOpcUa.AdminUI.Browsing;
+using ZB.MOM.WW.OtOpcUa.AdminUI.Clients;
+using ZB.MOM.WW.OtOpcUa.AdminUI.Tests.Uns;
+using ZB.MOM.WW.OtOpcUa.Commons.Browsing;
+using ZB.MOM.WW.OtOpcUa.Commons.Interfaces;
+using ZB.MOM.WW.OtOpcUa.Commons.Messages.Admin;
+using ZB.MOM.WW.OtOpcUa.Configuration;
+using ZB.MOM.WW.OtOpcUa.Configuration.Entities;
+using ZB.MOM.WW.OtOpcUa.Core.Abstractions;
+
+namespace ZB.MOM.WW.OtOpcUa.AdminUI.Tests.Browsing;
+
+/// <summary>
+///     lmxopcua#835 security review: I-1 (an UNSAVED endpoint may not borrow the named owner's own secrets; every
+///     credential-carrying Test Connect / browse is audited) and L-2 (a ConfigDb fault in the browse check is a refusal,
+///     not an exception).
+/// </summary>
+[Trait("Category", "Unit")]
+public sealed class CredentialUseReviewTests
+{
+    private static CancellationToken Ct => TestContext.Current.CancellationToken;
+
+    private const string StoredEndpoint = "http://gw-victim:5001";
+
+    private static string Galaxy(string endpoint, string reference) =>
+        """{"Gateway":{"Endpoint":"EP","ApiKeySecretRef":"REF"}}""".Replace("EP", endpoint).Replace("REF", reference);
+
+    /// <summary>The victim's driver stores no endpoint; its device stores <see cref="StoredEndpoint"/> (v3 layout).</summary>
+    private static IDbContextFactory<OtOpcUaConfigDbContext> VictimFactory()
+    {
+        var name = $"i1-{Guid.NewGuid():N}";
+        using (var db = RawTreeTestDb.CreateNamed(name))
+        {
+            db.DriverInstances.Add(new DriverInstance
+            {
+                DriverInstanceId = "DRV-victim", ClusterId = "c", Name = "Victim", DriverType = DriverTypeNames.Galaxy,
+                DriverConfig = """{"Gateway":{"ApiKeySecretRef":"secret:galaxy/DRV-victim/apikey"}}""",
+            });
+            db.Devices.Add(new Device
+            {
+                DeviceId = "DEV-victim", DriverInstanceId = "DRV-victim", Name = "gw",
+                DeviceConfig = Galaxy(StoredEndpoint, "secret:galaxy/DRV-victim/apikey"),
+            });
+            db.SaveChanges();
+        }
+
+        return RawTreeTestDb.Factory(name);
+    }
+
+    private static BrowserSessionService Service(
+        RecordingBrowser browser, IDbContextFactory<OtOpcUaConfigDbContext>? factory, IAuditWriter? audit = null) =>
+        new([browser], new BrowseSessionRegistry(), NullLogger<BrowserSessionService>.Instance, new NoUniversal(), null!,
+            new FixedUser("designer1"), factory, audit);
+
+    [Fact]
+    public async Task Browse_from_the_owners_device_with_an_unsaved_endpoint_cannot_use_the_owners_secret()
+    {
+        var browser = new RecordingBrowser();
+        var result = await Service(browser, VictimFactory()).OpenAsync(DriverTypeNames.Galaxy,
+            Galaxy("http://attacker:5001", "secret:galaxy/DRV-victim/apikey"), "DRV-victim", Ct);
+
+        result.Ok.ShouldBeFalse();
+        result.Message.ShouldNotBeNull().ShouldContain("lmxopcua#835");
+        result.Message.ShouldContain("save the change first");
+        browser.Opens.ShouldBe(0);
+    }
+
+    [Fact]
+    public async Task Browse_with_the_saved_endpoint_may_use_the_owners_secret()
+    {
+        var browser = new RecordingBrowser();
+        await Service(browser, VictimFactory()).OpenAsync(DriverTypeNames.Galaxy,
+            Galaxy(StoredEndpoint, "secret:galaxy/DRV-victim/apikey"), "DRV-victim", Ct);
+        browser.Opens.ShouldBe(1);
+    }
+
+    [Fact]
+    public async Task Browse_naming_an_owner_of_another_type_gets_no_owner()
+    {
+        var browser = new RecordingBrowser();
+        var result = await Service(browser, VictimFactory()).OpenAsync(DriverTypeNames.Galaxy,
+            Galaxy(StoredEndpoint, "secret:galaxy/DRV-victim/apikey"), "DRV-does-not-exist", Ct);
+        result.Ok.ShouldBeFalse();
+        browser.Opens.ShouldBe(0);
+    }
+
+    [Fact]
+    public async Task A_config_db_fault_in_the_browse_check_is_a_refusal_not_an_exception()
+    {
+        var browser = new RecordingBrowser();
+        var result = await Service(browser, new ThrowingFactory()).OpenAsync(DriverTypeNames.Galaxy,
+            Galaxy(StoredEndpoint, "secret:galaxy/shared/apikey"), "DRV-victim", Ct);
+
+        result.Ok.ShouldBeFalse();
+        result.Message.ShouldNotBeNull().ShouldContain("could not be read");
+        browser.Opens.ShouldBe(0);
+    }
+
+    [Fact]
+    public async Task Every_credential_carrying_browse_is_audited_with_user_driver_host_and_outcome()
+    {
+        var audit = new RecordingAudit();
+        var browser = new RecordingBrowser();
+        var svc = Service(browser, VictimFactory(), audit);
+
+        await svc.OpenAsync(DriverTypeNames.Galaxy, Galaxy("http://attacker:5001", "secret:galaxy/DRV-victim/apikey"), "DRV-victim", Ct);
+        await svc.OpenAsync(DriverTypeNames.Galaxy, Galaxy(StoredEndpoint, "env:OTOPCUA_GALAXY_KEY"), "DRV-victim", Ct);
+        await svc.OpenAsync(DriverTypeNames.Galaxy, Galaxy(StoredEndpoint, "dev:literal"), "DRV-victim", Ct); // no reference ⇒ no row
+
+        audit.Events.Count.ShouldBe(2);
+        var refused = audit.Events[0];
+        refused.Category.ShouldBe(CredentialUseAudit.Category);
+        refused.Action.ShouldBe(CredentialUseAudit.BrowseAction);
+        refused.Actor.ShouldBe("designer1");
+        refused.Target.ShouldBe("DRV-victim");
+        refused.Outcome.ShouldBe(AuditOutcome.Failure);
+        refused.DetailsJson.ShouldNotBeNull().ShouldContain("attacker:5001");
+        refused.DetailsJson.ShouldNotContain("DRV-victim/apikey"); // the reference itself is never recorded
+        audit.Events[1].DetailsJson.ShouldNotBeNull().ShouldContain("gw-victim:5001");
+    }
+
+    [Fact]
+    public async Task Every_credential_carrying_test_connect_is_audited()
+    {
+        var audit = new RecordingAudit();
+        var probe = new AdminProbeService(
+            new FixedReplyClient(new TestDriverConnectResult(false, "Test Connect refused: x", null, Guid.Empty)),
+            audit, new FixedUser("designer2"), NullLogger<AdminProbeService>.Instance);
+
+        await probe.TestAsync(DriverTypeNames.Galaxy, Galaxy("http://attacker:5001", "secret:galaxy/DRV-victim/apikey"), 5, Ct, "DRV-victim");
+        await probe.TestAsync(DriverTypeNames.Modbus, """{"Host":"plc"}""", 5, Ct);
+
+        var e = audit.Events.ShouldHaveSingleItem();
+        e.Action.ShouldBe(CredentialUseAudit.TestConnectAction);
+        e.Actor.ShouldBe("designer2");
+        e.Target.ShouldBe("DRV-victim");
+        e.Outcome.ShouldBe(AuditOutcome.Failure);
+        e.DetailsJson.ShouldNotBeNull().ShouldContain("attacker:5001");
+    }
+
+    [Fact]
+    public void Hosts_drop_uri_user_info()
+    {
+        CredentialUseAudit.Hosts(DriverTypeNames.OpcUaClient, """{"EndpointUrl":"opc.tcp://user:pw@plc1:4840/x"}""")
+            .ShouldHaveSingleItem().ShouldBe("plc1:4840");
+    }
+
+    private sealed class RecordingBrowser : IDriverBrowser
+    {
+        public int Opens { get; private set; }
+
+        public string DriverType => DriverTypeNames.Galaxy;
+
+        public Task<IBrowseSession> OpenAsync(string configJson, CancellationToken cancellationToken)
+        {
+            Opens++;
+            throw new InvalidOperationException("opened");
+        }
+    }
+
+    private sealed class NoUniversal : IUniversalDriverBrowser
+    {
+        public bool CanBrowse(string driverType, string configJson) => false;
+
+        public Task<IBrowseSession> OpenAsync(string driverType, string configJson, CancellationToken ct) =>
+            throw new InvalidOperationException("opened");
+    }
+
+    private sealed class FixedUser(string name) : AuthenticationStateProvider
+    {
+        public override Task<AuthenticationState> GetAuthenticationStateAsync() =>
+            Task.FromResult(new AuthenticationState(new ClaimsPrincipal(new ClaimsIdentity([new Claim(ClaimTypes.Name, name)], "test"))));
+    }
+
+    private sealed class RecordingAudit : IAuditWriter
+    {
+        public List<AuditEvent> Events { get; } = [];
+
+        public Task WriteAsync(AuditEvent evt, CancellationToken ct = default)
+        {
+            Events.Add(evt);
+            return Task.CompletedTask;
+        }
+    }
+
+    private sealed class ThrowingFactory : IDbContextFactory<OtOpcUaConfigDbContext>
+    {
+        public OtOpcUaConfigDbContext CreateDbContext() => throw new InvalidOperationException("ConfigDb unreachable");
+    }
+
+    private sealed class FixedReplyClient(TestDriverConnectResult reply) : IAdminOperationsClient
+    {
+        public Task<StartDeploymentResult> StartDeploymentAsync(string createdBy, CancellationToken ct) => throw new NotSupportedException();
+
+        public Task<AcknowledgeAlarmResult> AcknowledgeAlarmAsync(string alarmId, string user, string? comment, CancellationToken ct) =>
+            throw new NotSupportedException();
+
+        public Task<ShelveAlarmResult> ShelveAlarmAsync(
+            string alarmId, string user, ShelveKind kind, DateTime? unshelveAtUtc, string? comment, CancellationToken ct) =>
+            throw new NotSupportedException();
+
+        public Task<T> AskAsync<T>(object message, CancellationToken ct) => Task.FromResult((T)(object)reply);
+    }
+}
@@ -124,7 +124,10 @@ public sealed class OpcUaClientDriverPageFormSerializationTests
        result.AuthType.ShouldBe(OpcUaAuthType.Username);
        result.Username.ShouldBe("opcuser");
        result.Password.ShouldBe("p@ssw0rd");
-        result.UserCertificatePath.ShouldBe(@"C:\certs\user.pfx");
+        // #836 review L-1: hidden (AuthType ≠ Certificate) ⇒ not persisted.
+        result.UserCertificatePath.ShouldBeNull();
+        OpcUaClientDriverForm.OpcUaClientFormModel.FromRecord(original with { AuthType = OpcUaAuthType.Certificate, UserCertificatePath = "plant1/user.pfx" })
+            .ToRecord(endpointUrls).UserCertificatePath.ShouldBe("plant1/user.pfx");
        result.UserCertificatePassword.ShouldBe("certpass");
        result.PerEndpointConnectTimeout.ShouldBe(TimeSpan.FromSeconds(4));
        result.Timeout.ShouldBe(TimeSpan.FromSeconds(12));
@@ -194,6 +194,26 @@ public sealed class RawBrowseModalStaleOpenTests : IDisposable
        Field<string?>(modal, "_disabledReason").ShouldBeNull();
    }

+    /// <summary>
+    ///     #835 review L-2: a fault inside "Connect &amp; browse" (the browse service throwing — e.g. its ConfigDb owner check)
+    ///     is shown on the modal, never escapes as a circuit error.
+    /// </summary>
+    [Fact]
+    public async Task A_faulting_browse_open_becomes_a_message_not_a_circuit_error()
+    {
+        var svc = new FakeRawTreeService { OnLoadMergedProbeConfig = _ => ("AbCip", ConfigA) };
+        var modal = NewModal(svc, new FakeBrowserSessionService(), NullExportReader()); // its OpenAsync throws
+        SetParameters(modal, visible: true, deviceId: "dev-A", tagGroupId: null);
+        await OpenAsync(modal);
+
+        var open = typeof(RawBrowseModal).GetMethod("OpenBrowseAsync", BindingFlags.NonPublic | BindingFlags.Instance);
+        open.ShouldNotBeNull("the browse-open handler was renamed — retarget this test.");
+        await Should.NotThrowAsync(() => (Task)open!.Invoke(modal, null)!);
+
+        Field<string?>(modal, "_openError").ShouldNotBeNull().ShouldContain("Browse could not be opened");
+        Field<bool>(modal, "_opening").ShouldBeFalse();
+    }
+
    /// <inheritdoc/>
    public void Dispose()
    {
@@ -95,6 +95,28 @@ public sealed class AdminOperationsActorCredentialScopeTests : ControlPlaneActor
        probe.Calls.ShouldBe(1);
    }

+    /// <summary>
+    ///     #835 review I-1: Test Connect from the victim's OWN device modal with an unsaved endpoint (the modal merges the
+    ///     stored DriverConfig with the typed device config) must not send the victim's own secret to that endpoint.
+    /// </summary>
+    [Fact]
+    public void An_unsaved_endpoint_cannot_use_the_named_owners_own_secret()
+    {
+        var factory = NewInMemoryDbFactory();
+        SeedGalaxyDrivers(factory);
+        var probe = new RecordingProbe(DriverTypeNames.Galaxy);
+        var actor = Sys.ActorOf(AdminOperationsActor.Props(factory, CreateTestProbe("coord").Ref, new IDriverProbe[] { probe }));
+
+        actor.Tell(new TestDriverConnect(
+            DriverTypeNames.Galaxy, """{"Gateway":{"Endpoint":"http://attacker:5001","ApiKeySecretRef":"secret:galaxy/DRV-victim/apikey"}}""",
+            5, Guid.NewGuid(), "DRV-victim"));
+
+        var reply = ExpectMsg<TestDriverConnectResult>(PresenceBudget, cancellationToken: TestContext.Current.CancellationToken);
+        reply.Ok.ShouldBeFalse();
+        reply.Message.ShouldNotBeNull().ShouldContain("save the change first");
+        probe.Calls.ShouldBe(0);
+    }
+
    /// <summary>lmxopcua#836: an absolute OpcUaClient user-certificate path never reaches the probe.</summary>
    [Fact]
    public void An_absolute_user_certificate_path_never_reaches_the_probe()
@@ -118,7 +140,7 @@ public sealed class AdminOperationsActorCredentialScopeTests : ControlPlaneActor
        using var db = factory.CreateDbContext();
        db.DriverInstances.Add(new ZB.MOM.WW.OtOpcUa.Configuration.Entities.DriverInstance
        {
-            DriverInstanceId = "DRV-victim", ClusterId = "c", Name = "Victim", DriverType = DriverTypeNames.Galaxy, DriverConfig = "{}",
+            DriverInstanceId = "DRV-victim", ClusterId = "c", Name = "Victim", DriverType = DriverTypeNames.Galaxy, DriverConfig = """{"Gateway":{"Endpoint":"http://gw:5001"}}""",
        });
        db.DriverInstances.Add(new ZB.MOM.WW.OtOpcUa.Configuration.Entities.DriverInstance
        {