fix(security): per-instance credential references (#835) and confined OpcUaClient user-certificate path (#836)

#835: inside one driver namespace, a secret:<ns>/<X>/... reference whose first segment names ANOTHER existing same-type driver instance (id or name, case-insensitive; another-instance match beats an own-name match, so renaming one's driver after a victim's unlocks nothing) is refused at save (DriverConfigSaveGate.ValidateAsync), deploy (CredentialReferenceOtherInstance, Heidenhain included), Test Connect (TestDriverConnect.DriverInstanceId, ConfigDb read off the mailbox) and live browse (BrowserSessionService, fail-closed without a ConfigDb). One rule: DriverCredentialPolicy.DescribeOtherInstanceProblems + DriverInstanceIdentities. Shared names and the driver's own id stay valid; env: is not instance-scoped. Running drivers are unchanged (warn-only window: the deploy gate and the upgrade preflight / /raw banner list them). #836: OpcUaClient UserCertificatePath is a relative path inside Drivers:OpcUaClient:UserCertificateDirectory (default <LocalAppData>/OtOpcUa/pki/user-certificates). Syntactic rule (CredentialFilePaths, a DriverCredentialPolicy path field) at every #815 seam; at load time ResolveSecretRefsAsync joins, canonicalises and requires lexical AND link-resolved containment before the PFX password is resolved, and LoadCertificate refuses an unresolved path. A running driver's pre-#836 absolute path keeps loading with a rate-limited warning (LegacyCredentialReferenceTracker.ReportPath). Co-Authored-By: Claude <noreply@anthropic.com>
2026-11-11 03:52:48 -05:00
parent 481985e20b
commit 63331b0416
31 changed files with 1555 additions and 49 deletions
@@ -1,7 +1,10 @@
 using Microsoft.AspNetCore.Authorization;
 using Microsoft.AspNetCore.Components.Authorization;
+using Microsoft.EntityFrameworkCore;
 using Microsoft.Extensions.Logging;
 using ZB.MOM.WW.OtOpcUa.Commons.Browsing;
+using ZB.MOM.WW.OtOpcUa.Configuration;
+using ZB.MOM.WW.OtOpcUa.Configuration.Validation;
 using ZB.MOM.WW.OtOpcUa.Core.Abstractions;
 using ZB.MOM.WW.OtOpcUa.Security.Auth;

@@ -20,13 +23,18 @@ namespace ZB.MOM.WW.OtOpcUa.AdminUI.Browsing;
 /// <param name="universalBrowser">The Discover-backed fallback browser.</param>
 /// <param name="authorizationService">Policy evaluator for <see cref="RequestRebirthAsync"/>.</param>
 /// <param name="authenticationStateProvider">Supplies the calling circuit's user.</param>
+/// <param name="dbFactory">
+///     The config DB, for the cross-instance credential rule (lmxopcua#835). Null (test graphs) ⇒ a config carrying an
+///     in-scope <c>secret:</c> reference cannot be checked and is refused (fail-closed).
+/// </param>
 public sealed class BrowserSessionService(
    IEnumerable<IDriverBrowser> browsers,
    BrowseSessionRegistry registry,
    ILogger<BrowserSessionService> logger,
    IUniversalDriverBrowser universalBrowser,
    IAuthorizationService authorizationService,
-    AuthenticationStateProvider authenticationStateProvider) : IBrowserSessionService
+    AuthenticationStateProvider authenticationStateProvider,
+    IDbContextFactory<OtOpcUaConfigDbContext>? dbFactory = null) : IBrowserSessionService
 {
    /// <summary>Upper bound on a single root/expand/attributes call.</summary>
    public static readonly TimeSpan PerCallTimeout = TimeSpan.FromSeconds(20);
@@ -35,7 +43,11 @@ public sealed class BrowserSessionService(
        browsers.ToDictionary(b => b.DriverType, StringComparer.OrdinalIgnoreCase);

    /// <inheritdoc />
-    public async Task<BrowseOpenResult> OpenAsync(string driverType, string configJson, CancellationToken ct)
+    public Task<BrowseOpenResult> OpenAsync(string driverType, string configJson, CancellationToken ct) =>
+        OpenAsync(driverType, configJson, null, ct);
+
+    /// <inheritdoc />
+    public async Task<BrowseOpenResult> OpenAsync(string driverType, string configJson, string? driverInstanceId, CancellationToken ct)
    {
        // lmxopcua#815: a live browse resolves the config's credential references and sends them to the
        // author-chosen endpoint — refuse an out-of-namespace reference before any browser (or the
@@ -44,6 +56,19 @@ public sealed class BrowserSessionService(
        if (credentialProblems.Count > 0)
            return new(false, "Browse refused: " + string.Join(" ", credentialProblems), Guid.Empty);

+        // lmxopcua#835: nor may an in-namespace reference name ANOTHER driver instance's secret.
+        if (DriverCredentialPolicy.HasInstanceSegments(driverType, configJson))
+        {
+            if (dbFactory is null)
+                return new(false, "Browse refused: the driver-instance list needed to check this config's secret: "
+                                  + "references is unavailable (lmxopcua#835, fail-closed).", Guid.Empty);
+            await using var db = await dbFactory.CreateDbContextAsync(ct).ConfigureAwait(false);
+            var instanceProblems = await DriverInstanceIdentities
+                .DescribeOtherInstanceProblemsAsync(db, driverType, configJson, driverInstanceId, ct).ConfigureAwait(false);
+            if (instanceProblems.Count > 0)
+                return new(false, "Browse refused: " + string.Join(" ", instanceProblems), Guid.Empty);
+        }
+
        if (!_browsersByType.TryGetValue(driverType, out var browser))
            return await OpenUniversalAsync(driverType, configJson, ct).ConfigureAwait(false);
        try
@@ -27,6 +27,19 @@ public interface IBrowserSessionService
    /// <returns>The result of opening the session, including the registry token on success.</returns>
    Task<BrowseOpenResult> OpenAsync(string driverType, string configJson, CancellationToken ct);

+    /// <summary>
+    ///     As <see cref="OpenAsync(string, string, CancellationToken)"/>, naming the driver instance the config belongs to
+    ///     (lmxopcua#835: its own <c>secret:&lt;ns&gt;/&lt;id&gt;/…</c> references are allowed, another instance's are refused;
+    ///     null ⇒ a not-yet-created instance, which is stricter). The default forwards without the id (test doubles).
+    /// </summary>
+    /// <param name="driverType">The driver type to open a browse session against.</param>
+    /// <param name="configJson">The driver configuration as JSON.</param>
+    /// <param name="driverInstanceId">The owning driver instance, or null.</param>
+    /// <param name="ct">Cancellation token for the operation.</param>
+    /// <returns>The result of opening the session, including the registry token on success.</returns>
+    Task<BrowseOpenResult> OpenAsync(string driverType, string configJson, string? driverInstanceId, CancellationToken ct) =>
+        OpenAsync(driverType, configJson, ct);
+
    /// <summary>Returns the root nodes of an open session. Throws
    /// <see cref="BrowseSessionNotFoundException"/> if the token is unknown.</summary>
    /// <param name="token">Registry handle for the open browse session.</param>
@@ -25,15 +25,17 @@ public sealed class AdminProbeService
    /// <param name="configJson">Driver config as JSON (same shape as <c>DriverInstance.DriverConfig</c>).</param>
    /// <param name="timeoutSeconds">Per-probe timeout; actor clamps to [1, 60].</param>
    /// <param name="ct">Optional cancellation token from the caller.</param>
+    /// <param name="driverInstanceId">The owning driver instance (lmxopcua#835); null ⇒ a not-yet-created one.</param>
    /// <returns>The probe result, or a timeout failure if no reply arrives in time.</returns>
    public async Task<TestDriverConnectResult> TestAsync(
        string driverType,
        string configJson,
        int timeoutSeconds,
-        CancellationToken ct = default)
+        CancellationToken ct = default,
+        string? driverInstanceId = null)
    {
        var correlationId = Guid.NewGuid();
-        var msg = new TestDriverConnect(driverType, configJson, timeoutSeconds, correlationId);
+        var msg = new TestDriverConnect(driverType, configJson, timeoutSeconds, correlationId, driverInstanceId);

        // 65s outer guard — the actor's CTS clamps to 60s; if the Ask never returns we still want
        // a deterministic failure surface for the UI.
@@ -59,7 +59,7 @@
                        }

                        <div class="mt-3">
-                            <DriverTestConnectButton DriverType="@_driverType" GetConfigJson="@BuildMergedProbeConfig" />
+                            <DriverTestConnectButton DriverType="@_driverType" GetConfigJson="@BuildMergedProbeConfig" DriverInstanceId="@_resolvedDriverInstanceId" />
                        </div>

                        @if (DriverConfigSaveGate.Validate(_driverType, _deviceConfigJson) is { } gateError)
@@ -60,6 +60,12 @@
    /// </summary>
    [Parameter] public int TimeoutSeconds { get; set; } = TestConnectBudget.DefaultSeconds;

+    /// <summary>
+    ///     The driver instance the config belongs to (lmxopcua#835: its own <c>secret:&lt;ns&gt;/&lt;id&gt;/…</c> references are
+    ///     allowed, another instance's are refused). Null ⇒ treated as a not-yet-created instance (stricter).
+    /// </summary>
+    [Parameter] public string? DriverInstanceId { get; set; }
+
    private const int ClearAfterMs = 30_000;

    private bool _busy;
@@ -83,7 +89,7 @@
            var json = GetConfigJson() ?? "{}";
            var budget = TestConnectBudget.Resolve(json, TimeoutSeconds, DriverType);
            _lastBudgetSeconds = budget;
-            _result = await Probe.TestAsync(DriverType, json, budget);
+            _result = await Probe.TestAsync(DriverType, json, budget, driverInstanceId: DriverInstanceId);
        }
        catch (Exception ex)
        {
@@ -146,7 +146,8 @@
                <div class="col-md-6">
                    <label class="form-label">User certificate path (PFX/PEM)</label>
                    <InputText @bind-Value="_form.OpcUa.UserCertificatePath" @bind-Value:after="EmitAsync" class="form-control form-control-sm mono"
-                               placeholder="C:\certs\user.pfx" />
+                               placeholder="plant1/user.pfx" />
+                    <div class="form-text">Relative path inside the node's user-certificate directory (<code>Drivers:OpcUaClient:UserCertificateDirectory</code>, default <code>&lt;LocalAppData&gt;/OtOpcUa/pki/user-certificates</code>). Absolute paths and <code>..</code> are refused (#836).</div>
                </div>
                <div class="col-md-4">
                    <label class="form-label">Certificate password (if PFX-locked)</label>
@@ -463,7 +463,12 @@
        StateHasChanged();
        try
        {
-            var result = await BrowserService.OpenAsync(_driverType, _mergedConfig, default);
+            // lmxopcua#835: name the owning driver so its own secret:<ns>/<id>/… references are told apart from
+            // another instance's (an unresolvable owner is treated as a new driver — stricter).
+            var ownerDriverId = string.IsNullOrEmpty(_effectiveDeviceId)
+                ? null
+                : await Svc.LoadDeviceDriverInstanceIdAsync(_effectiveDeviceId);
+            var result = await BrowserService.OpenAsync(_driverType, _mergedConfig, ownerDriverId, default);
            // Host disposed, modal closed, or modal re-opened mid-open: close the session nobody will (R04 L4).
            if (BrowseOpenGuard.CloseIfOrphaned(result, _disposed || IsStale(generation), BrowserService)) { return; }
            if (result.Ok)
@@ -77,22 +77,26 @@ public sealed class CredentialReferencePreflight(IDbContextFactory<OtOpcUaConfig
        var findings = new List<CredentialReferenceFinding>();
        var driverList = drivers.ToList();
        var byId = driverList.ToDictionary(d => d.DriverInstanceId, StringComparer.Ordinal);
+        // lmxopcua#835: the cross-instance rule is checked against the instance set of the same source.
+        var draftInstances = driverList.Select(d => new DriverInstanceIdentity(d.DriverInstanceId, d.Name, d.DriverType)).ToList();
        foreach (var d in driverList)
        {
-            foreach (var p in DriverCredentialPolicy.DescribeProblems(d.DriverType, d.DriverConfig))
+            foreach (var p in DriverCredentialPolicy.DescribeProblems(d.DriverType, d.DriverConfig, d.DriverInstanceId, draftInstances))
                findings.Add(new(CredentialFindingSource.Draft, d.DriverType, d.Name, d.DriverInstanceId, null, p));
        }

        foreach (var dev in devices)
        {
            if (!byId.TryGetValue(dev.DriverInstanceId, out var owner)) continue;
-            foreach (var p in DriverCredentialPolicy.DescribeProblems(owner.DriverType, dev.DeviceConfig))
+            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));
        }

-        foreach (var s in deployed)
+        var deployedList = deployed.ToList();
+        var deployedInstances = deployedList.Select(s => new DriverInstanceIdentity(s.DriverInstanceId, s.Name, s.DriverType)).ToList();
+        foreach (var s in deployedList)
        {
-            foreach (var p in DriverCredentialPolicy.DescribeProblems(s.DriverType, s.DriverConfig))
+            foreach (var p in DriverCredentialPolicy.DescribeProblems(s.DriverType, s.DriverConfig, s.DriverInstanceId, deployedInstances))
                findings.Add(new(CredentialFindingSource.Deployed, s.DriverType, s.Name, s.DriverInstanceId, null, p));
        }

@@ -1,3 +1,5 @@
+using ZB.MOM.WW.OtOpcUa.Configuration;
+using ZB.MOM.WW.OtOpcUa.Configuration.Validation;
 using ZB.MOM.WW.OtOpcUa.Core.Abstractions;

 namespace ZB.MOM.WW.OtOpcUa.AdminUI.Uns;
@@ -26,4 +28,24 @@ public static class DriverConfigSaveGate
        var problems = DriverCredentialPolicy.DescribeProblems(driverType, configJson);
        return problems.Count == 0 ? null : string.Join(" ", problems);
    }
+
+    /// <summary>
+    ///     The authority check <see cref="RawTreeService"/> runs on every driver/device create + update: <see cref="Validate"/>
+    ///     plus the cross-instance rule (lmxopcua#835 — a <c>secret:galaxy/&lt;X&gt;/…</c> reference may not name ANOTHER
+    ///     existing instance of the same driver type), which needs the stored instance list.
+    /// </summary>
+    /// <param name="db">The config DB (read only when the config carries an in-scope <c>secret:</c> reference).</param>
+    /// <param name="driverType">The owning driver's type.</param>
+    /// <param name="configJson">The DriverConfig or DeviceConfig JSON about to be stored.</param>
+    /// <param name="ownDriverInstanceId">The owning driver; null when it is being created.</param>
+    /// <param name="ct">Cancellation.</param>
+    /// <returns>The refusal (never containing a credential value), or null.</returns>
+    public static async Task<string?> ValidateAsync(
+        OtOpcUaConfigDbContext db, string? driverType, string? configJson, string? ownDriverInstanceId, CancellationToken ct)
+    {
+        if (Validate(driverType, configJson) is { } refusal) return refusal;
+        var problems = await DriverInstanceIdentities
+            .DescribeOtherInstanceProblemsAsync(db, driverType, configJson, ownDriverInstanceId, ct).ConfigureAwait(false);
+        return problems.Count == 0 ? null : string.Join(" ", problems);
+    }
 }
@@ -394,4 +394,15 @@ public interface IRawTreeService
    /// <param name="ct">A token to cancel the load.</param>
    /// <returns>The parent driver's type + the merged config JSON, or null when the device/driver cannot be resolved.</returns>
    Task<(string DriverType, string MergedConfigJson)?> LoadMergedProbeConfigAsync(string deviceId, CancellationToken ct = default);
+
+    /// <summary>
+    ///     The driver instance owning <paramref name="deviceId"/>, or null when it cannot be resolved — handed to the live
+    ///     browse so the cross-instance credential rule (lmxopcua#835) can tell the driver's own secrets from another
+    ///     instance's. The default (test doubles) returns null, which is the stricter "not-yet-created" treatment.
+    /// </summary>
+    /// <param name="deviceId">The device.</param>
+    /// <param name="ct">A token to cancel the load.</param>
+    /// <returns>The owning driver instance id, or null.</returns>
+    Task<string?> LoadDeviceDriverInstanceIdAsync(string deviceId, CancellationToken ct = default) =>
+        Task.FromResult<string?>(null);
 }
@@ -488,6 +488,10 @@ public sealed class RawTreeService(IDbContextFactory<OtOpcUaConfigDbContext> dbF

        await using var db = await dbFactory.CreateDbContextAsync(ct);

+        // lmxopcua#835: a new driver owns no secrets yet — a reference naming any existing instance is refused.
+        if (await DriverConfigSaveGate.ValidateAsync(db, driverType, driverConfigJson, null, ct) is { } instanceRefusal)
+            return new UnsMutationResult(false, instanceRefusal);
+
        if (!await db.ServerClusters.AnyAsync(c => c.ClusterId == clusterId, ct))
            return new UnsMutationResult(false, $"Cluster '{clusterId}' not found.");

@@ -621,7 +625,7 @@ public sealed class RawTreeService(IDbContextFactory<OtOpcUaConfigDbContext> dbF
        var owner = await db.DriverInstances.FirstOrDefaultAsync(d => d.DriverInstanceId == driverInstanceId, ct);
        if (owner is null)
            return new UnsMutationResult(false, $"Driver '{driverInstanceId}' not found.");
-        if (DriverConfigSaveGate.Validate(owner.DriverType, deviceConfigJson) is { } configRefusal)
+        if (await DriverConfigSaveGate.ValidateAsync(db, owner.DriverType, deviceConfigJson, owner.DriverInstanceId, ct) is { } configRefusal)
            return new UnsMutationResult(false, configRefusal);

        // MTConnect owns one agent client per driver — mirror the deploy gate
@@ -1116,7 +1120,7 @@ public sealed class RawTreeService(IDbContextFactory<OtOpcUaConfigDbContext> dbF

        var entity = await db.DriverInstances.FirstOrDefaultAsync(d => d.DriverInstanceId == driverInstanceId, ct);
        if (entity is null) return new UnsMutationResult(false, "Row no longer exists.");
-        if (DriverConfigSaveGate.Validate(entity.DriverType, driverConfigJson) is { } configRefusal)
+        if (await DriverConfigSaveGate.ValidateAsync(db, entity.DriverType, driverConfigJson, entity.DriverInstanceId, ct) is { } configRefusal)
            return new UnsMutationResult(false, configRefusal);

        // Sibling uniqueness in the same scope RenameDriverAsync uses (cluster + folder).
@@ -1178,7 +1182,8 @@ public sealed class RawTreeService(IDbContextFactory<OtOpcUaConfigDbContext> dbF

        var entity = await db.Devices.FirstOrDefaultAsync(dev => dev.DeviceId == deviceId, ct);
        if (entity is null) return new UnsMutationResult(false, "Row no longer exists.");
-        if (DriverConfigSaveGate.Validate(await ResolveDriverTypeAsync(db, deviceId, ct), deviceConfigJson) is { } configRefusal)
+        if (await DriverConfigSaveGate.ValidateAsync(
+                db, await ResolveDriverTypeAsync(db, deviceId, ct), deviceConfigJson, entity.DriverInstanceId, ct) is { } configRefusal)
            return new UnsMutationResult(false, configRefusal);

        if (await db.Devices.AnyAsync(
@@ -1201,6 +1206,16 @@ public sealed class RawTreeService(IDbContextFactory<OtOpcUaConfigDbContext> dbF
        }
    }

+    /// <inheritdoc />
+    public async Task<string?> LoadDeviceDriverInstanceIdAsync(string deviceId, CancellationToken ct = default)
+    {
+        await using var db = await dbFactory.CreateDbContextAsync(ct);
+        return await db.Devices.AsNoTracking()
+            .Where(dev => dev.DeviceId == deviceId)
+            .Select(dev => dev.DriverInstanceId)
+            .FirstOrDefaultAsync(ct);
+    }
+
    /// <inheritdoc />
    public async Task<(string DriverType, string MergedConfigJson)?> LoadMergedProbeConfigAsync(
        string deviceId, CancellationToken ct = default)
@@ -611,6 +611,19 @@ public sealed class AdminOperationsActor : ReceiveActor
        var sw = System.Diagnostics.Stopwatch.StartNew();
        try
        {
+            // lmxopcua#835: a reference naming ANOTHER driver instance's secret (secret:galaxy/<other>/…) is refused
+            // before the probe resolves it — off the mailbox, since it reads the ConfigDb (only when the config
+            // carries an in-scope secret: reference at all).
+            if (DriverCredentialPolicy.HasInstanceSegments(msg.DriverType, msg.ConfigJson))
+            {
+                await using var db = await _dbFactory.CreateDbContextAsync(cts.Token);
+                var instanceProblems = await DriverInstanceIdentities.DescribeOtherInstanceProblemsAsync(
+                    db, msg.DriverType, msg.ConfigJson, msg.DriverInstanceId, cts.Token);
+                if (instanceProblems.Count > 0)
+                    return new TestDriverConnectResult(
+                        false, "Test Connect refused: " + string.Join(" ", instanceProblems), null, msg.CorrelationId);
+            }
+
            var result = await probe.ProbeAsync(msg.ConfigJson, timeout, cts.Token);
            sw.Stop();
            return new TestDriverConnectResult(
@@ -194,6 +194,10 @@ builder.Services.AddValidatedOptions<LdapOptions, LdapOptionsValidator>(builder.
 // Test Connect probe (admin role, which must probe the stack the driver would run). A bad value fails host
 // start here rather than silently falling back.
 builder.Services.AddAbCipDriverDefaults(builder.Configuration);
+// lmxopcua#836: the directory OpcUaClient UserCertificatePath (a relative path) is confined to. Unconditional for the
+// same reason — the driver (driver role) and the AdminUI live browse (admin role) both load user certificates.
+ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient.OpcUaUserCertificatePaths.Configure(
+    builder.Configuration[ZB.MOM.WW.OtOpcUa.Core.Abstractions.DriverCredentialPolicy.OpcUaClientUserCertificateDirectoryKey]);
 // lmxopcua#765: refresh/staleness of the cached API-key → Galaxy ack identity set (driver nodes consume it).
 builder.Services.AddGalaxyAckIdentityCache(builder.Configuration);