fix(security): preflight and /raw banner check the merged driver+device config (#850)

The deploy gate's merged-config path check is now one helper, CredentialReferenceDeployChecks.MergedOnlyProblems (problems the runtime merge reveals that the per-config checks do not), shared by the gate and CredentialReferencePreflight.Scan - so an AuthType switch in one config and a UserCertificatePath in the other is listed as a Draft finding '(merged with its devices)' on the banner before a deploy refuses it. Co-Authored-By: Claude <noreply@anthropic.com>
2026-11-11 06:24:59 -05:00
parent d08e59eea7
commit 3dbfe6ec0a
3 changed files with 89 additions and 22 deletions
@@ -74,35 +74,44 @@ public static class CredentialReferenceDeployChecks
            foreach (var problem in DriverCredentialPolicy.DescribeProblems(driver.DriverType, driver.DriverConfig))
                errors.Add(new(Code, $"{driver.DriverType} driver '{driver.Name}': {problem}{MigrationHint}", driver.DriverInstanceId));

-            var reported = new HashSet<string>(StringComparer.Ordinal);
-            foreach (var problem in DriverCredentialPolicy.DescribeProblems(driver.DriverType, driver.DriverConfig))
-                reported.Add(problem);
            foreach (var device in devices)
            {
                foreach (var problem in DriverCredentialPolicy.DescribeProblems(driver.DriverType, device.DeviceConfig))
-                {
-                    reported.Add(problem);
                    errors.Add(new(Code, $"{driver.DriverType} device '{device.Name}': {problem}{MigrationHint}", device.DeviceId));
-                }
            }

-            // #836 review L-1: a path field is exempt only in an object that explicitly switches it off (OpcUaClient
-            // AuthType ≠ Certificate), and the switch and the path may sit in different configs (driver vs device). Check
-            // the config exactly as the runtime merges it, so a split can never deploy a path the driver will load.
-            if (rules.PathFieldList.Count > 0)
-            {
-                var merged = DriverDeviceConfigMerger.Merge(
-                    driver.DriverConfig,
-                    devices.Select(d => new DriverDeviceConfigMerger.DeviceRow(d.Name, d.DeviceConfig)).ToList(),
-                    []);
-                foreach (var problem in DriverCredentialPolicy.DescribeProblems(driver.DriverType, merged))
-                {
-                    if (reported.Add(problem))
-                        errors.Add(new(Code, $"{driver.DriverType} driver '{driver.Name}' (merged with its devices): {problem}{MigrationHint}", driver.DriverInstanceId));
-                }
-            }
+            foreach (var problem in MergedOnlyProblems(
+                         driver.DriverType, driver.DriverConfig, devices.Select(d => (d.Name, (string?)d.DeviceConfig)).ToList()))
+                errors.Add(new(Code, $"{driver.DriverType} driver '{driver.Name}' (merged with its devices): {problem}{MigrationHint}", driver.DriverInstanceId));
        }

        return errors;
    }
+
+    /// <summary>
+    ///     #836 review L-1 / lmxopcua#850: a path field is exempt only in an object that explicitly switches it off
+    ///     (OpcUaClient <c>AuthType</c> ≠ Certificate), and the switch and the path may sit in different configs (driver vs
+    ///     device). This checks the config exactly as the runtime merges it (<see cref="DriverDeviceConfigMerger"/>) and
+    ///     returns the problems the merge reveals that the driver and device configs, checked one by one, do not — so a
+    ///     split can never hide a path the driver will load. The ONE implementation the deploy gate and the upgrade
+    ///     preflight / <c>/raw</c> banner share. Empty for a type without path fields.
+    /// </summary>
+    /// <param name="driverType">The driver type.</param>
+    /// <param name="driverConfig">The stored DriverConfig.</param>
+    /// <param name="devices">Its devices (name, DeviceConfig).</param>
+    /// <returns>The merged-only problems.</returns>
+    public static IReadOnlyList<string> MergedOnlyProblems(
+        string? driverType, string? driverConfig, IReadOnlyList<(string Name, string? DeviceConfig)> devices)
+    {
+        ArgumentNullException.ThrowIfNull(devices);
+        if (!DriverCredentialPolicy.TryGet(driverType, out var rules) || rules.PathFieldList.Count == 0) return [];
+        var reported = new HashSet<string>(DriverCredentialPolicy.DescribeProblems(driverType, driverConfig), StringComparer.Ordinal);
+        foreach (var (_, deviceConfig) in devices)
+            reported.UnionWith(DriverCredentialPolicy.DescribeProblems(driverType, deviceConfig));
+        var merged = DriverDeviceConfigMerger.Merge(
+            driverConfig ?? "{}",
+            devices.Select(d => new DriverDeviceConfigMerger.DeviceRow(d.Name, d.DeviceConfig ?? "{}")).ToList(),
+            []);
+        return DriverCredentialPolicy.DescribeProblems(driverType, merged).Where(reported.Add).ToList();
+    }
 }
@@ -5,6 +5,7 @@ using Microsoft.Extensions.Logging;
 using ZB.MOM.WW.OtOpcUa.Configuration;
 using ZB.MOM.WW.OtOpcUa.Configuration.Entities;
 using ZB.MOM.WW.OtOpcUa.Configuration.Enums;
+using ZB.MOM.WW.OtOpcUa.Configuration.Validation;
 using ZB.MOM.WW.OtOpcUa.Core.Abstractions;
 using ZB.MOM.WW.OtOpcUa.Runtime.Drivers;

@@ -85,13 +86,28 @@ public sealed class CredentialReferencePreflight(IDbContextFactory<OtOpcUaConfig
                findings.Add(new(CredentialFindingSource.Draft, d.DriverType, d.Name, d.DriverInstanceId, null, p));
        }

-        foreach (var dev in devices)
+        var deviceList = devices.ToList();
+        foreach (var dev in deviceList)
        {
            if (!byId.TryGetValue(dev.DriverInstanceId, out var owner)) continue;
            foreach (var p in DriverCredentialPolicy.DescribeProblems(owner.DriverType, dev.DeviceConfig, owner.DriverInstanceId, draftInstances))
                findings.Add(new(CredentialFindingSource.Draft, owner.DriverType, owner.Name, owner.DriverInstanceId, dev.Name, p));
        }

+        // lmxopcua#850: what the deploy gate checks on the runtime-merged driver+device config (an AuthType switch in one
+        // config and a UserCertificatePath in the other) — the same helper, so the banner shows it before a deploy does.
+        // Device order follows DraftSnapshotFactory (the deploy gate's input): by DeviceId.
+        foreach (var d in driverList)
+        {
+            var own = deviceList
+                .Where(dev => string.Equals(dev.DriverInstanceId, d.DriverInstanceId, StringComparison.Ordinal))
+                .OrderBy(dev => dev.DeviceId, StringComparer.Ordinal)
+                .Select(dev => (dev.Name, (string?)dev.DeviceConfig))
+                .ToList();
+            foreach (var p in CredentialReferenceDeployChecks.MergedOnlyProblems(d.DriverType, d.DriverConfig, own))
+                findings.Add(new(CredentialFindingSource.Draft, d.DriverType, d.Name, d.DriverInstanceId, null, $"(merged with its devices) {p}"));
+        }
+
        var deployedList = deployed.ToList();
        var deployedInstances = deployedList.Select(s => new DriverInstanceIdentity(s.DriverInstanceId, s.Name, s.DriverType)).ToList();
        foreach (var s in deployedList)
@@ -52,4 +52,46 @@ public sealed class CredentialReferencePreflightInstanceAndPathTests
        f.Problem.ShouldContain("lmxopcua#836");
        f.Problem.ShouldNotContain("plant.pfx");
    }
+
+    /// <summary>
+    ///     lmxopcua#850: the AuthType switch in one config and the path in the other — each config passes alone, the
+    ///     runtime-merged config does not. The deploy gate catches it; the banner must show it before a deploy does.
+    /// </summary>
+    [Fact]
+    public void A_path_split_from_its_certificate_switch_is_listed_from_the_merged_config()
+    {
+        var findings = CredentialReferencePreflight.Scan(
+            [Drv("o", "Upstream", DriverTypeNames.OpcUaClient, """{"AuthType":"Anonymous","UserCertificatePath":"C:\\stolen\\host.pfx"}""")],
+            [new Device { DeviceId = "dev-o", DriverInstanceId = "o", Name = "d", DeviceConfig = """{"AuthType":"Certificate"}""" }],
+            []);
+
+        var f = findings.ShouldHaveSingleItem();
+        f.Source.ShouldBe(CredentialFindingSource.Draft);
+        f.DriverInstanceId.ShouldBe("o");
+        f.DeviceName.ShouldBeNull();
+        f.Problem.ShouldContain("merged with its devices");
+        f.Problem.ShouldContain("lmxopcua#836");
+        f.Problem.ShouldNotContain("host.pfx");
+    }
+
+    /// <summary>Control: a problem already reported on one config is not listed a second time from the merge.</summary>
+    [Fact]
+    public void A_path_already_reported_on_its_own_config_is_not_duplicated_by_the_merge() =>
+        CredentialReferencePreflight.Scan(
+                [Drv("o", "Upstream", DriverTypeNames.OpcUaClient, """{"AuthType":"Certificate","UserCertificatePath":"C:\\certs\\plant.pfx"}""")],
+                [new Device { DeviceId = "dev-o", DriverInstanceId = "o", Name = "d", DeviceConfig = "{}" }],
+                [])
+            .ShouldHaveSingleItem().Problem.ShouldNotContain("merged");
+
+    /// <summary>lmxopcua#849: an env: reference naming another instance is listed like a secret: one.</summary>
+    [Fact]
+    public void A_cross_instance_env_reference_is_listed() =>
+        CredentialReferencePreflight.Scan(
+                [
+                    Drv("DRV-a", "A", DriverTypeNames.Galaxy, "{}"),
+                    Drv("DRV-b", "B", DriverTypeNames.Galaxy, """{"Gateway":{"ApiKeySecretRef":"env:OTOPCUA_GALAXY_DRV_A_KEY"}}"""),
+                ],
+                [],
+                [])
+            .ShouldHaveSingleItem().Problem.ShouldContain("lmxopcua#849");
 }