From 271b96a644a27b30b96fe47d60ecdd6f5d86db8d Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Wed, 11 Nov 2026 04:12:40 -0500 Subject: [PATCH 1/2] =?UTF-8?q?fix(security):=20#835/#836=20review=20?= =?UTF-8?q?=E2=80=94=20unsaved=20endpoints=20never=20borrow=20the=20owner'?= =?UTF-8?q?s=20secrets;=20audit=20credential=20use;=20path=20rule=20follow?= =?UTF-8?q?s=20AuthType?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit I-1: Test Connect / browse configs may carry unsaved edits (the device modal merges the stored DriverConfig with the typed device config). The server (DriverInstanceIdentities.EffectiveOwnerAsync, used by AdminOperationsActor and BrowserSessionService) honours the named owner only when every endpoint value (new DriverCredentialRules.EndpointFields: Galaxy Endpoint, OpcUaClient EndpointUrl/EndpointUrls/DiscoveryUrl, Heidenhain Host/Port) is one that driver stores; otherwise the owner is null and its instance-named secrets are refused. Every Test Connect / browse whose config carries an in-scope secret:/env: reference is recorded (CredentialUseAudit: IAuditWriter event category CredentialUse + a structured Information line: user, driver, endpoint hosts, outcome). L-1: the UserCertificatePath rule is skipped only in an object that explicitly sets AuthType to non-Certificate (string or ordinal); the deploy gate also checks the runtime-merged driver+device config so the switch cannot be split from the path; the form no longer persists the hidden field. L-2: a ConfigDb fault in the browse check is a refusal; RawBrowseModal catches open failures into its error message. L-4: a relative UserCertificateDirectory resolves against AppContext.BaseDirectory. Co-Authored-By: Claude --- .../CredentialReferenceDeployChecks.cs | 24 +- .../Validation/DriverInstanceIdentities.cs | 58 +++++ .../CredentialFilePaths.cs | 33 ++- .../DriverCredentialPolicy.cs | 117 +++++++++- .../OpcUaUserCertificatePaths.cs | 16 +- .../Audit/CredentialUseAudit.cs | 107 +++++++++ .../Browsing/BrowserSessionService.cs | 37 ++- .../Clients/AdminProbeService.cs | 30 ++- .../Drivers/Forms/OpcUaClientDriverForm.razor | 3 +- .../Shared/Raw/RawBrowseModal.razor | 6 + .../AdminOperations/AdminOperationsActor.cs | 2 +- ...ValidatorCredentialInstanceAndPathTests.cs | 18 ++ .../CredentialPolicyReviewTests.cs | 54 +++++ .../OpcUaClientUserCertificatePathTests.cs | 12 + .../Browsing/CredentialUseReviewTests.cs | 212 ++++++++++++++++++ ...aClientDriverPageFormSerializationTests.cs | 5 +- .../Raw/RawBrowseModalStaleOpenTests.cs | 20 ++ ...dminOperationsActorCredentialScopeTests.cs | 24 +- 18 files changed, 753 insertions(+), 25 deletions(-) create mode 100644 src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Audit/CredentialUseAudit.cs create mode 100644 tests/Core/ZB.MOM.WW.OtOpcUa.Core.Abstractions.Tests/CredentialPolicyReviewTests.cs create mode 100644 tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Browsing/CredentialUseReviewTests.cs diff --git a/src/Core/ZB.MOM.WW.OtOpcUa.Configuration/Validation/CredentialReferenceDeployChecks.cs b/src/Core/ZB.MOM.WW.OtOpcUa.Configuration/Validation/CredentialReferenceDeployChecks.cs index ad04b1103..7c3f18864 100644 --- a/src/Core/ZB.MOM.WW.OtOpcUa.Configuration/Validation/CredentialReferenceDeployChecks.cs +++ b/src/Core/ZB.MOM.WW.OtOpcUa.Configuration/Validation/CredentialReferenceDeployChecks.cs @@ -53,7 +53,7 @@ public static class CredentialReferenceDeployChecks .ToList(); foreach (var driver in draft.DriverInstances) { - if (!DriverCredentialPolicy.TryGet(driver.DriverType, out _)) continue; + if (!DriverCredentialPolicy.TryGet(driver.DriverType, out var rules)) continue; var devices = draft.Devices .Where(d => string.Equals(d.DriverInstanceId, driver.DriverInstanceId, StringComparison.Ordinal)) .ToList(); @@ -74,10 +74,32 @@ 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(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)); + } } } diff --git a/src/Core/ZB.MOM.WW.OtOpcUa.Configuration/Validation/DriverInstanceIdentities.cs b/src/Core/ZB.MOM.WW.OtOpcUa.Configuration/Validation/DriverInstanceIdentities.cs index f3fac585d..290c8c5d4 100644 --- a/src/Core/ZB.MOM.WW.OtOpcUa.Configuration/Validation/DriverInstanceIdentities.cs +++ b/src/Core/ZB.MOM.WW.OtOpcUa.Configuration/Validation/DriverInstanceIdentities.cs @@ -28,6 +28,64 @@ public static class DriverInstanceIdentities .ToList(); } + /// + /// #835 review I-1: the owner a Test Connect / browse config may claim. The caller names a driver, but the config can + /// carry UNSAVED edits (the device modal merges the stored DriverConfig with the typed device config), so a Designer + /// could type their own host as the endpoint of a victim's device and Test Connect it with the victim's own + /// secret:galaxy/<victim-id>/…. The claim is honoured only when the driver exists, has the same type, and every + /// endpoint value of the config is one it already STORES (DriverConfig or any DeviceConfig) — otherwise the owner is + /// null, i.e. the strict "new driver" treatment (no instance-named secrets at all). + /// + /// The config DB. + /// The driver type of the request. + /// The config about to be probed / browsed. + /// The driver the caller says the config belongs to. + /// Cancellation. + /// The owner to use, or null. + public static async Task EffectiveOwnerAsync( + OtOpcUaConfigDbContext db, string? driverType, string? configJson, string? claimedOwner, CancellationToken ct) + { + ArgumentNullException.ThrowIfNull(db); + if (string.IsNullOrEmpty(claimedOwner) || driverType is null) return null; + var driver = await db.DriverInstances.AsNoTracking() + .Where(d => d.DriverInstanceId == claimedOwner) + .Select(d => new { d.DriverType, d.DriverConfig }) + .FirstOrDefaultAsync(ct).ConfigureAwait(false); + if (driver is null || !string.Equals(driver.DriverType, driverType, StringComparison.OrdinalIgnoreCase)) return null; + var deviceConfigs = await db.Devices.AsNoTracking() + .Where(d => d.DriverInstanceId == claimedOwner) + .Select(d => d.DeviceConfig) + .ToListAsync(ct).ConfigureAwait(false); + return DriverCredentialPolicy.EndpointsMatchStored(driverType, configJson, [driver.DriverConfig, .. deviceConfigs]) + ? claimedOwner + : null; + } + + /// + /// The cross-instance problems of a Test Connect / browse config: first (an + /// unsaved endpoint does not get the owner's own secrets), then . + /// + /// The config DB. + /// The driver type. + /// The config. + /// The driver the caller names. + /// Cancellation. + /// The problems. + public static async Task> DescribeProbeProblemsAsync( + OtOpcUaConfigDbContext db, string? driverType, string? configJson, string? claimedOwner, CancellationToken ct) + { + if (driverType is null || !DriverCredentialPolicy.HasInstanceSegments(driverType, configJson)) return []; + var owner = await EffectiveOwnerAsync(db, driverType, configJson, claimedOwner, ct).ConfigureAwait(false); + var problems = await DescribeOtherInstanceProblemsAsync(db, driverType, configJson, owner, ct).ConfigureAwait(false); + if (problems.Count == 0 || owner is not null || string.IsNullOrEmpty(claimedOwner)) return problems; + return + [ + .. problems, + $"(The endpoint differs from what driver {claimedOwner} has saved, so its own secrets are not sent to it from an " + + "unsaved form — save the change first; lmxopcua#835.)", + ]; + } + /// /// The cross-instance problems of — reading the ConfigDb only when the config carries /// an in-scope secret: reference at all. diff --git a/src/Core/ZB.MOM.WW.OtOpcUa.Core.Abstractions/CredentialFilePaths.cs b/src/Core/ZB.MOM.WW.OtOpcUa.Core.Abstractions/CredentialFilePaths.cs index 1dbad06a6..7880be3d5 100644 --- a/src/Core/ZB.MOM.WW.OtOpcUa.Core.Abstractions/CredentialFilePaths.cs +++ b/src/Core/ZB.MOM.WW.OtOpcUa.Core.Abstractions/CredentialFilePaths.cs @@ -8,7 +8,38 @@ namespace ZB.MOM.WW.OtOpcUa.Core.Abstractions; /// /// The JSON property name (matched case-insensitively, as the driver binds it). /// How the operator configures the directory (for messages). -public sealed record CredentialPathField(string Name, string DirectoryDescription); +/// +/// Optional sibling key that switches the field on (OpcUaClient AuthType): the field is checked UNLESS the same +/// JSON object sets this key to something other than / +/// . An absent key keeps the field checked (conservative — the driver's merge may +/// supply the switch from another config), so only an EXPLICITLY inactive object is exempt. +/// +/// The string value of that activates the field. +/// The numeric (enum ordinal) value that activates the field. +public sealed record CredentialPathField( + string Name, string DirectoryDescription, string? ActiveWhenKey = null, string? ActiveWhenValue = null, int? ActiveWhenOrdinal = null) +{ + /// Whether the field is in force in (the JSON object holding it). + /// The object. + /// False only when the object explicitly switches the field off. + public bool IsActiveIn(System.Text.Json.JsonElement owner) + { + if (ActiveWhenKey is null || owner.ValueKind != System.Text.Json.JsonValueKind.Object) return true; + foreach (var p in owner.EnumerateObject()) + { + if (!string.Equals(p.Name, ActiveWhenKey, StringComparison.OrdinalIgnoreCase)) continue; + return p.Value.ValueKind switch + { + System.Text.Json.JsonValueKind.Null => true, + System.Text.Json.JsonValueKind.String => string.Equals(p.Value.GetString(), ActiveWhenValue, StringComparison.OrdinalIgnoreCase), + System.Text.Json.JsonValueKind.Number => !p.Value.TryGetInt32(out var n) || n == ActiveWhenOrdinal, + _ => true, + }; + } + + return true; + } +} /// /// The one rule for values (lmxopcua#836): authored as a RELATIVE path inside a diff --git a/src/Core/ZB.MOM.WW.OtOpcUa.Core.Abstractions/DriverCredentialPolicy.cs b/src/Core/ZB.MOM.WW.OtOpcUa.Core.Abstractions/DriverCredentialPolicy.cs index 694a1d38d..308f28bfd 100644 --- a/src/Core/ZB.MOM.WW.OtOpcUa.Core.Abstractions/DriverCredentialPolicy.cs +++ b/src/Core/ZB.MOM.WW.OtOpcUa.Core.Abstractions/DriverCredentialPolicy.cs @@ -12,11 +12,19 @@ public sealed record CredentialField(string Name, CredentialLiteralPolicy Litera /// The namespace its references are confined to. /// The credential fields its DriverConfig/DeviceConfig may carry. /// Fields naming a node-local key-material file, confined to a node directory (lmxopcua#836). +/// +/// The keys (any depth, case-insensitive) that say WHERE the driver sends its credentials — the endpoint a Test Connect +/// or browse dials (#835 review I-1: an unsaved endpoint may not borrow the stored driver's own secrets). +/// public sealed record DriverCredentialRules( - string DriverType, CredentialScope Scope, IReadOnlyList Fields, IReadOnlyList? PathFields = null) + string DriverType, CredentialScope Scope, IReadOnlyList Fields, IReadOnlyList? PathFields = null, + IReadOnlyList? EndpointFields = null) { /// The path fields (never null). public IReadOnlyList PathFieldList => PathFields ?? []; + + /// The endpoint fields (never null). + public IReadOnlyList EndpointFieldList => EndpointFields ?? []; } /// The identity of one driver instance, as the cross-instance rule (lmxopcua#835) matches it. @@ -57,7 +65,8 @@ public static class DriverCredentialPolicy /// OpcUaClient UserCertificatePath: a relative path inside the node's user-certificate directory (#836). public static readonly CredentialPathField OpcUaClientUserCertificatePathField = new( "UserCertificatePath", - OpcUaClientUserCertificateDirectoryKey + ", default /OtOpcUa/pki/user-certificates"); + OpcUaClientUserCertificateDirectoryKey + ", default /OtOpcUa/pki/user-certificates", + ActiveWhenKey: "AuthType", ActiveWhenValue: "Certificate", ActiveWhenOrdinal: 2); /// Galaxy Gateway.ApiKeySecretRef (literal accepted for back-compat, with a warning at resolve). public static readonly CredentialField GalaxyApiKeyField = new("ApiKeySecretRef", CredentialLiteralPolicy.Allowed); @@ -73,13 +82,14 @@ public static class DriverCredentialPolicy new("InspectPassword", CredentialLiteralPolicy.Refused), new("DncPassword", CredentialLiteralPolicy.Refused), new("DataPassword", CredentialLiteralPolicy.Refused), - ]), - [DriverTypeNames.Galaxy] = new(DriverTypeNames.Galaxy, GalaxyScope, [GalaxyApiKeyField, GalaxyAckPasswordField]), + ], EndpointFields: ["Host", "Port"]), + [DriverTypeNames.Galaxy] = new(DriverTypeNames.Galaxy, GalaxyScope, [GalaxyApiKeyField, GalaxyAckPasswordField], + EndpointFields: ["Endpoint"]), [DriverTypeNames.OpcUaClient] = new(DriverTypeNames.OpcUaClient, OpcUaClientScope, [ new("Password", CredentialLiteralPolicy.Allowed), new("UserCertificatePassword", CredentialLiteralPolicy.Allowed), - ], [OpcUaClientUserCertificatePathField]), + ], [OpcUaClientUserCertificatePathField], ["EndpointUrl", "EndpointUrls", "DiscoveryUrl"]), }; /// The rules for a driver type, if it resolves credential references. @@ -187,6 +197,9 @@ public static class DriverCredentialPolicy continue; } + // #836 review L-1: a path field explicitly switched off in its own object (OpcUaClient AuthType ≠ Certificate) + // is never loaded by the driver, so it is not checked; an absent switch keeps it checked. + if (pathField is not null && !pathField.IsActiveIn(element)) continue; onField(fieldPath, p.Value, field, pathField); } } @@ -290,6 +303,100 @@ public static class DriverCredentialPolicy string? driverType, string? json, string? ownDriverInstanceId, IEnumerable instances) => [.. DescribeProblems(driverType, json), .. DescribeOtherInstanceProblems(driverType, json, ownDriverInstanceId, instances)]; + /// + /// True when the config carries at least one in-scope credential REFERENCE (secret: or env:) — the + /// Test Connect / browse audit trigger (#835 review I-1). + /// + /// The driver type. + /// The config JSON. + /// Whether a reference that would be resolved is present. + public static bool HasCredentialReferences(string? driverType, string? json) + { + if (!TryGet(driverType, out var rules) || string.IsNullOrWhiteSpace(json)) return false; + var found = false; + WithDocument(json, root => Visit(root, "", rules, 0, (_, value, field, _) => + { + if (field is null || value.ValueKind != JsonValueKind.String) return; + if (CredentialReferences.Parse(value.GetString(), rules.Scope, out _) is CredentialReferenceKind.Secret or CredentialReferenceKind.Environment) + found = true; + })); + return found; + } + + /// + /// The endpoint values (see ) of a config — every string / number + /// under an endpoint key at any depth (arrays flattened), trimmed. Empty for an unreadable config or a type without + /// rules. + /// + /// The driver type. + /// The config JSON. + /// The values, in document order. + public static IReadOnlyList EndpointValues(string? driverType, string? json) + { + var values = new List(); + if (!TryGet(driverType, out var rules) || rules.EndpointFieldList.Count == 0 || string.IsNullOrWhiteSpace(json)) return values; + WithDocument(json, root => CollectEndpoints(root, rules, values, 0, false)); + return values; + } + + /// + /// #835 review I-1: whether every endpoint value of (a Test Connect / browse config, + /// possibly carrying UNSAVED edits) is one the driver already stores (its DriverConfig or any of its DeviceConfigs). + /// If not, the caller must not treat the stored driver as the config's owner: its own secrets would otherwise be sent + /// to an endpoint nobody saved. A type without endpoint fields never matches (conservative). + /// + /// The driver type. + /// The config about to be probed / browsed. + /// The owner's stored DriverConfig + DeviceConfigs. + /// Whether the submitted endpoints are all stored ones. + public static bool EndpointsMatchStored(string? driverType, string? submittedJson, IEnumerable storedJsons) + { + ArgumentNullException.ThrowIfNull(storedJsons); + if (!TryGet(driverType, out var rules) || rules.EndpointFieldList.Count == 0) return false; + var allowed = new HashSet(storedJsons.SelectMany(s => EndpointValues(driverType, s)), StringComparer.OrdinalIgnoreCase); + return EndpointValues(driverType, submittedJson).All(allowed.Contains); + } + + private static void CollectEndpoints(JsonElement element, DriverCredentialRules rules, List values, int depth, bool under) + { + if (depth > 32) return; + switch (element.ValueKind) + { + case JsonValueKind.Array: + foreach (var item in element.EnumerateArray()) CollectEndpoints(item, rules, values, depth + 1, under); + return; + case JsonValueKind.Object: + foreach (var p in element.EnumerateObject()) + { + var isEndpoint = rules.EndpointFieldList.Any(f => string.Equals(f, p.Name, StringComparison.OrdinalIgnoreCase)); + CollectEndpoints(p.Value, rules, values, depth + 1, isEndpoint); + } + + return; + case JsonValueKind.String when under: + if (element.GetString() is { } s && s.Trim().Length > 0) values.Add(s.Trim()); + return; + case JsonValueKind.Number when under: + values.Add(element.GetRawText()); + return; + } + } + + private static void WithDocument(string json, Action use) + { + JsonDocument doc; + try + { + doc = JsonDocument.Parse(json, new JsonDocumentOptions { CommentHandling = JsonCommentHandling.Skip, AllowTrailingCommas = true }); + } + catch (JsonException) + { + return; + } + + using (doc) use(doc.RootElement); + } + private static List<(string FieldPath, string Segment)> CollectSegments(string? driverType, string? json) { var found = new List<(string, string)>(); diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient.Session/OpcUaUserCertificatePaths.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient.Session/OpcUaUserCertificatePaths.cs index 2787b5a60..3e59d1580 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient.Session/OpcUaUserCertificatePaths.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient.Session/OpcUaUserCertificatePaths.cs @@ -24,10 +24,20 @@ public static class OpcUaUserCertificatePaths /// The directory in force (configured, else ). public static string CurrentDirectory => _configured ?? DefaultDirectory; - /// Sets the process-wide directory (blank ⇒ the default). Called by the host at startup. + /// + /// Sets the process-wide directory (blank ⇒ the default). Called by the host at startup. A RELATIVE value is + /// resolved against the application's install directory (), never the + /// working directory — a Windows service starts in System32 (review L-4). + /// /// The configured directory. - public static void Configure(string? directory) => - _configured = string.IsNullOrWhiteSpace(directory) ? null : Path.GetFullPath(directory); + public static void Configure(string? directory) => _configured = Normalize(directory, AppContext.BaseDirectory); + + /// The directory a configured value means: blank ⇒ null; relative ⇒ under . + /// The configured value. + /// The base a relative value is resolved against. + /// The absolute directory, or null. + public static string? Normalize(string? directory, string baseDirectory) => + string.IsNullOrWhiteSpace(directory) ? null : Path.GetFullPath(directory.Trim(), baseDirectory); /// /// Resolves an authored UserCertificatePath to the file to load, fail-closed diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Audit/CredentialUseAudit.cs b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Audit/CredentialUseAudit.cs new file mode 100644 index 000000000..a379390ae --- /dev/null +++ b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Audit/CredentialUseAudit.cs @@ -0,0 +1,107 @@ +using System.Security.Claims; +using System.Text.Json; +using Microsoft.AspNetCore.Components.Authorization; +using Microsoft.Extensions.Logging; +using ZB.MOM.WW.Audit; +using ZB.MOM.WW.OtOpcUa.Core.Abstractions; + +namespace ZB.MOM.WW.OtOpcUa.AdminUI.Audit; + +/// +/// #835 review I-1: every Test Connect / live browse whose config carries an in-scope credential reference +/// (secret: / env:) is recorded — who, which driver, which endpoint host(s), and the outcome — as an +/// (category ) AND a structured Information log line, because those are +/// the two operations that resolve a stored credential and send it to an endpoint typed into an unsaved form. Never the +/// reference value or the credential. +/// +public static class CredentialUseAudit +{ + /// The audit category. + public const string Category = "CredentialUse"; + + /// The Test Connect action. + public const string TestConnectAction = "TestConnect"; + + /// The live-browse action. + public const string BrowseAction = "Browse"; + + /// + /// The endpoint hosts of a config, print-safe: host[:port] for a URI (user-info dropped), the trimmed value + /// otherwise (capped). + /// + /// The driver type. + /// The config. + /// The hosts. + public static IReadOnlyList Hosts(string? driverType, string? configJson) => + DriverCredentialPolicy.EndpointValues(driverType, configJson) + .Select(v => Uri.TryCreate(v, UriKind.Absolute, out var u) && !string.IsNullOrEmpty(u.Host) + ? (u.IsDefaultPort || u.Port < 0 ? u.Host : $"{u.Host}:{u.Port}") + : (v.Length > 128 ? v[..128] : v)) + .Distinct(StringComparer.OrdinalIgnoreCase) + .ToList(); + + /// Builds the audit event. + /// or . + /// The user. + /// The driver type. + /// The driver the caller named, or null. + /// The endpoint hosts. + /// Whether the operation succeeded. + /// The refusal / failure message, if any. + /// The event. + public static AuditEvent Build( + string action, string actor, string driverType, string? driverInstanceId, IReadOnlyList hosts, bool ok, string? message) => + new() + { + EventId = Guid.NewGuid(), + OccurredAtUtc = DateTimeOffset.UtcNow, + Actor = actor, + Action = action, + Category = Category, + Target = driverInstanceId ?? "(unsaved driver)", + Outcome = ok ? AuditOutcome.Success : AuditOutcome.Failure, + DetailsJson = JsonSerializer.Serialize(new { driverType, endpoints = hosts, message }), + }; + + /// + /// Records one operation when the config carries an in-scope credential reference; a no-op otherwise. Never throws + /// (an audit failure is logged, not surfaced). + /// + /// The audit writer, or null (log only). + /// The circuit's authentication state, or null. + /// The logger. + /// The action. + /// The driver type. + /// The config. + /// The driver the caller named. + /// Whether it succeeded. + /// The refusal / failure message. + /// A task. + public static async Task RecordAsync( + IAuditWriter? audit, AuthenticationStateProvider? auth, ILogger logger, string action, string driverType, string configJson, + string? driverInstanceId, bool ok, string? message) + { + if (!DriverCredentialPolicy.HasCredentialReferences(driverType, configJson)) return; + try + { + var actor = await ActorAsync(auth).ConfigureAwait(false); + var hosts = Hosts(driverType, configJson); + logger.LogInformation( + "Credential use: {Action} by {User} driverType={DriverType} driver={DriverInstanceId} endpoints={Endpoints} outcome={Outcome} {Message}", + action, actor, driverType, driverInstanceId ?? "(unsaved)", string.Join(",", hosts), ok ? "Success" : "Failure", message); + if (audit is not null) + await audit.WriteAsync(Build(action, actor, driverType, driverInstanceId, hosts, ok, message)).ConfigureAwait(false); + } + catch (Exception ex) + { + logger.LogWarning(ex, "Credential-use audit for {Action} could not be written.", action); + } + } + + private static async Task ActorAsync(AuthenticationStateProvider? auth) + { + if (auth is null) return "unknown"; + var state = await auth.GetAuthenticationStateAsync().ConfigureAwait(false); + return state.User.Identity?.Name ?? state.User.FindFirst(ClaimTypes.NameIdentifier)?.Value ?? "unknown"; + } +} diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Browsing/BrowserSessionService.cs b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Browsing/BrowserSessionService.cs index 38104536c..0054d0225 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Browsing/BrowserSessionService.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Browsing/BrowserSessionService.cs @@ -2,6 +2,7 @@ using Microsoft.AspNetCore.Authorization; using Microsoft.AspNetCore.Components.Authorization; using Microsoft.EntityFrameworkCore; using Microsoft.Extensions.Logging; +using ZB.MOM.WW.OtOpcUa.AdminUI.Audit; using ZB.MOM.WW.OtOpcUa.Commons.Browsing; using ZB.MOM.WW.OtOpcUa.Configuration; using ZB.MOM.WW.OtOpcUa.Configuration.Validation; @@ -27,6 +28,7 @@ namespace ZB.MOM.WW.OtOpcUa.AdminUI.Browsing; /// The config DB, for the cross-instance credential rule (lmxopcua#835). Null (test graphs) ⇒ a config carrying an /// in-scope secret: reference cannot be checked and is refused (fail-closed). /// +/// The credential-use audit sink (#835 review I-1); null ⇒ the structured log line only. public sealed class BrowserSessionService( IEnumerable browsers, BrowseSessionRegistry registry, @@ -34,7 +36,8 @@ public sealed class BrowserSessionService( IUniversalDriverBrowser universalBrowser, IAuthorizationService authorizationService, AuthenticationStateProvider authenticationStateProvider, - IDbContextFactory? dbFactory = null) : IBrowserSessionService + IDbContextFactory? dbFactory = null, + ZB.MOM.WW.Audit.IAuditWriter? audit = null) : IBrowserSessionService { /// Upper bound on a single root/expand/attributes call. public static readonly TimeSpan PerCallTimeout = TimeSpan.FromSeconds(20); @@ -48,6 +51,15 @@ public sealed class BrowserSessionService( /// public async Task OpenAsync(string driverType, string configJson, string? driverInstanceId, CancellationToken ct) + { + var result = await OpenCheckedAsync(driverType, configJson, driverInstanceId, ct).ConfigureAwait(false); + // #835 review I-1: who browsed which endpoint with which driver's credential reference, and how it went. + await CredentialUseAudit.RecordAsync(audit, authenticationStateProvider, logger, CredentialUseAudit.BrowseAction, + driverType, configJson, driverInstanceId, result.Ok, result.Message).ConfigureAwait(false); + return result; + } + + private async Task OpenCheckedAsync(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 @@ -56,17 +68,28 @@ 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. + // lmxopcua#835: nor may an in-namespace reference name ANOTHER driver instance's secret — and the named owner's + // own secrets count only while the config's endpoint is one that driver has SAVED (review I-1). 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); + try + { + await using var db = await dbFactory.CreateDbContextAsync(ct).ConfigureAwait(false); + var instanceProblems = await DriverInstanceIdentities + .DescribeProbeProblemsAsync(db, driverType, configJson, driverInstanceId, ct).ConfigureAwait(false); + if (instanceProblems.Count > 0) + return new(false, "Browse refused: " + string.Join(" ", instanceProblems), Guid.Empty); + } + catch (Exception ex) when (ex is not OperationCanceledException || !ct.IsCancellationRequested) + { + // Review L-2: a ConfigDb fault must not become a circuit error — refuse (fail-closed) with a message. + logger.LogWarning(ex, "Browse credential check failed for driverType={DriverType}", driverType); + return new(false, "Browse refused: the driver-instance list could not be read to check this config's " + + "secret: references (lmxopcua#835, fail-closed). Try again.", Guid.Empty); + } } if (!_browsersByType.TryGetValue(driverType, out var browser)) diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Clients/AdminProbeService.cs b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Clients/AdminProbeService.cs index 93fef2ed6..1cc3b9a12 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Clients/AdminProbeService.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Clients/AdminProbeService.cs @@ -12,10 +12,26 @@ namespace ZB.MOM.WW.OtOpcUa.AdminUI.Clients; public sealed class AdminProbeService { private readonly IAdminOperationsClient _client; + private readonly ZB.MOM.WW.Audit.IAuditWriter? _audit; + private readonly Microsoft.AspNetCore.Components.Authorization.AuthenticationStateProvider? _auth; + private readonly Microsoft.Extensions.Logging.ILogger _logger; /// Initializes a new instance of the . /// The admin operations client used to dispatch probe requests. - public AdminProbeService(IAdminOperationsClient client) => _client = client; + /// Credential-use audit sink (#835 review I-1); null ⇒ the structured log line only. + /// The circuit's user (for the audit record); null ⇒ "unknown". + /// Logger; null ⇒ none. + public AdminProbeService( + IAdminOperationsClient client, + ZB.MOM.WW.Audit.IAuditWriter? audit = null, + Microsoft.AspNetCore.Components.Authorization.AuthenticationStateProvider? auth = null, + Microsoft.Extensions.Logging.ILogger? logger = null) + { + _client = client; + _audit = audit; + _auth = auth; + _logger = (Microsoft.Extensions.Logging.ILogger?)logger ?? Microsoft.Extensions.Logging.Abstractions.NullLogger.Instance; + } /// /// Dispatches a Test Connect probe for the supplied driver type and config JSON, @@ -42,17 +58,23 @@ public sealed class AdminProbeService using var outerCts = CancellationTokenSource.CreateLinkedTokenSource(ct); outerCts.CancelAfter(TimeSpan.FromSeconds(65)); + TestDriverConnectResult result; try { - return await _client.AskAsync(msg, outerCts.Token); + result = await _client.AskAsync(msg, outerCts.Token); } catch (OperationCanceledException) { - return new TestDriverConnectResult(false, "Probe request did not return within 65s.", null, correlationId); + result = new TestDriverConnectResult(false, "Probe request did not return within 65s.", null, correlationId); } catch (Exception ex) { - return new TestDriverConnectResult(false, $"Probe dispatch failed: {ex.Message}", null, correlationId); + result = new TestDriverConnectResult(false, $"Probe dispatch failed: {ex.Message}", null, correlationId); } + + // #835 review I-1: who Test-Connected which endpoint with which driver's credential reference, and how it went. + await Audit.CredentialUseAudit.RecordAsync(_audit, _auth, _logger, Audit.CredentialUseAudit.TestConnectAction, + driverType, configJson, driverInstanceId, result.Ok, result.Message).ConfigureAwait(false); + return result; } } diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Shared/Drivers/Forms/OpcUaClientDriverForm.razor b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Shared/Drivers/Forms/OpcUaClientDriverForm.razor index f7b6dae71..f52566321 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Shared/Drivers/Forms/OpcUaClientDriverForm.razor +++ b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Shared/Drivers/Forms/OpcUaClientDriverForm.razor @@ -424,7 +424,8 @@ AuthType = AuthType, Username = string.IsNullOrWhiteSpace(Username) ? null : Username, Password = string.IsNullOrWhiteSpace(Password) ? null : Password, - UserCertificatePath = string.IsNullOrWhiteSpace(UserCertificatePath) ? null : UserCertificatePath, + // #836 review L-1: the field is hidden (and never loaded) unless AuthType is Certificate — do not persist it then. + UserCertificatePath = AuthType != OpcUaAuthType.Certificate || string.IsNullOrWhiteSpace(UserCertificatePath) ? null : UserCertificatePath, UserCertificatePassword = string.IsNullOrWhiteSpace(UserCertificatePassword) ? null : UserCertificatePassword, ProbeTimeoutSeconds = ProbeTimeoutSeconds, SubscriptionPublishingInterval = SubscriptionPublishingIntervalMs is { } ms and > 0 diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Shared/Raw/RawBrowseModal.razor b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Shared/Raw/RawBrowseModal.razor index 3350553db..7cee13d41 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Shared/Raw/RawBrowseModal.razor +++ b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Shared/Raw/RawBrowseModal.razor @@ -488,6 +488,12 @@ } else { _openError = result.Message; } } + catch (Exception ex) + { + // Review L-2: a ConfigDb fault (owner lookup) or any other open failure is a message on the modal, never a + // circuit error that takes the whole page down. + if (!_disposed && !IsStale(generation)) { _openError = $"Browse could not be opened: {ex.Message}"; } + } finally { _opening = false; diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.ControlPlane/AdminOperations/AdminOperationsActor.cs b/src/Server/ZB.MOM.WW.OtOpcUa.ControlPlane/AdminOperations/AdminOperationsActor.cs index d72e40614..441810fb7 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.ControlPlane/AdminOperations/AdminOperationsActor.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.ControlPlane/AdminOperations/AdminOperationsActor.cs @@ -617,7 +617,7 @@ public sealed class AdminOperationsActor : ReceiveActor if (DriverCredentialPolicy.HasInstanceSegments(msg.DriverType, msg.ConfigJson)) { await using var db = await _dbFactory.CreateDbContextAsync(cts.Token); - var instanceProblems = await DriverInstanceIdentities.DescribeOtherInstanceProblemsAsync( + var instanceProblems = await DriverInstanceIdentities.DescribeProbeProblemsAsync( db, msg.DriverType, msg.ConfigJson, msg.DriverInstanceId, cts.Token); if (instanceProblems.Count > 0) return new TestDriverConnectResult( diff --git a/tests/Core/ZB.MOM.WW.OtOpcUa.Configuration.Tests/DraftValidatorCredentialInstanceAndPathTests.cs b/tests/Core/ZB.MOM.WW.OtOpcUa.Configuration.Tests/DraftValidatorCredentialInstanceAndPathTests.cs index db32e2045..4e32aa8f3 100644 --- a/tests/Core/ZB.MOM.WW.OtOpcUa.Configuration.Tests/DraftValidatorCredentialInstanceAndPathTests.cs +++ b/tests/Core/ZB.MOM.WW.OtOpcUa.Configuration.Tests/DraftValidatorCredentialInstanceAndPathTests.cs @@ -85,6 +85,24 @@ public sealed class DraftValidatorCredentialInstanceAndPathTests e.Message.ShouldNotContain("host.pfx"); } + /// Review L-1: a path in an object that explicitly switches certificate auth off is not checked… + [Fact] + public void A_path_under_explicit_non_certificate_auth_passes() => + Validate(Drv("DRV-o", "o", "OpcUaClient", """{"AuthType":"Anonymous","UserCertificatePath":"C:\\stale\\host.pfx"}""")).ShouldBeEmpty(); + + /// …but the driver/device split cannot smuggle one past the gate: the runtime-merged config is checked too. + [Fact] + public void A_path_split_from_its_certificate_switch_is_caught_on_the_merged_config() + { + var e = Validate( + [Drv("DRV-o", "o", "OpcUaClient", """{"AuthType":"Anonymous","UserCertificatePath":"C:\\stolen\\host.pfx"}""")], + [new Device { DeviceId = "dev-o", DriverInstanceId = "DRV-o", Name = "d", DeviceConfig = """{"AuthType":"Certificate"}""" }]) + .ShouldHaveSingleItem(); + e.Context.ShouldBe("DRV-o"); + e.Message.ShouldContain("merged with its devices"); + e.Message.ShouldContain("lmxopcua#836"); + } + [Fact] public void A_relative_user_certificate_path_passes() => Validate(Drv("DRV-o", "o", "OpcUaClient", """{"AuthType":"Certificate","UserCertificatePath":"plant1/user.pfx"}""")).ShouldBeEmpty(); diff --git a/tests/Core/ZB.MOM.WW.OtOpcUa.Core.Abstractions.Tests/CredentialPolicyReviewTests.cs b/tests/Core/ZB.MOM.WW.OtOpcUa.Core.Abstractions.Tests/CredentialPolicyReviewTests.cs new file mode 100644 index 000000000..7db403457 --- /dev/null +++ b/tests/Core/ZB.MOM.WW.OtOpcUa.Core.Abstractions.Tests/CredentialPolicyReviewTests.cs @@ -0,0 +1,54 @@ +using Shouldly; +using Xunit; + +namespace ZB.MOM.WW.OtOpcUa.Core.Abstractions.Tests; + +/// #835/#836 security review: endpoint matching (I-1), credential-reference detection (audit) and L-1. +[Trait("Category", "Unit")] +public sealed class CredentialPolicyReviewTests +{ + [Theory] + [InlineData("""{"AuthType":"Anonymous","UserCertificatePath":"C:\\x.pfx"}""", 0)] + [InlineData("""{"AuthType":"username","UserCertificatePath":"/etc/x.pfx"}""", 0)] + [InlineData("""{"AuthType":0,"UserCertificatePath":"/etc/x.pfx"}""", 0)] + [InlineData("""{"AuthType":"Certificate","UserCertificatePath":"/etc/x.pfx"}""", 1)] + [InlineData("""{"authType":2,"UserCertificatePath":"/etc/x.pfx"}""", 1)] + [InlineData("""{"UserCertificatePath":"/etc/x.pfx"}""", 1)] // no switch here — the merge may supply it + [InlineData("""{"AuthType":null,"UserCertificatePath":"/etc/x.pfx"}""", 1)] + public void L1_the_path_rule_applies_unless_the_object_explicitly_switches_certificate_auth_off(string config, int expected) => + DriverCredentialPolicy.DescribeProblems(DriverTypeNames.OpcUaClient, config).Count.ShouldBe(expected); + + [Fact] + public void Endpoint_values_are_collected_at_any_depth_and_flattened() + { + DriverCredentialPolicy.EndpointValues(DriverTypeNames.OpcUaClient, + """{"EndpointUrl":" opc.tcp://a:4840 ","EndpointUrls":["opc.tcp://b:4840"],"Devices":[{"discoveryUrl":"opc.tcp://c"}]}""") + .ShouldBe(["opc.tcp://a:4840", "opc.tcp://b:4840", "opc.tcp://c"]); + DriverCredentialPolicy.EndpointValues(DriverTypeNames.Heidenhain, """{"Devices":[{"Host":"cnc1","Port":19000}]}""") + .ShouldBe(["cnc1", "19000"]); + DriverCredentialPolicy.EndpointValues(DriverTypeNames.Modbus, """{"Host":"x"}""").ShouldBeEmpty(); + } + + [Theory] + [InlineData("""{"Gateway":{"Endpoint":"http://gw:5001"}}""", true)] + [InlineData("""{"Gateway":{"Endpoint":"HTTP://GW:5001"}}""", true)] + [InlineData("""{"Gateway":{}}""", true)] + [InlineData("""{"Gateway":{"Endpoint":"http://attacker:5001"}}""", false)] + public void An_unsaved_endpoint_does_not_match_the_stored_driver(string submitted, bool expected) => + DriverCredentialPolicy.EndpointsMatchStored(DriverTypeNames.Galaxy, submitted, + ["""{"Gateway":{"ApiKeySecretRef":"secret:galaxy/a/k"}}""", """{"Gateway":{"Endpoint":"http://gw:5001"}}"""]) + .ShouldBe(expected); + + [Fact] + public void A_type_without_endpoint_fields_never_matches() => + DriverCredentialPolicy.EndpointsMatchStored(DriverTypeNames.Modbus, "{}", ["{}"]).ShouldBeFalse(); + + [Theory] + [InlineData("""{"Gateway":{"ApiKeySecretRef":"secret:galaxy/x"}}""", true)] + [InlineData("""{"Gateway":{"ApiKeySecretRef":"env:OTOPCUA_GALAXY_X"}}""", true)] + [InlineData("""{"Gateway":{"ApiKeySecretRef":"dev:k"}}""", false)] + [InlineData("""{"Gateway":{"ApiKeySecretRef":"literal"}}""", false)] + [InlineData("""{"Gateway":{"ApiKeySecretRef":"env:ConnectionStrings__ConfigDb"}}""", false)] // out of scope: refused anyway + public void HasCredentialReferences_detects_resolvable_references(string config, bool expected) => + DriverCredentialPolicy.HasCredentialReferences(DriverTypeNames.Galaxy, config).ShouldBe(expected); +} diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient.Tests/OpcUaClientUserCertificatePathTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient.Tests/OpcUaClientUserCertificatePathTests.cs index a8a2010f6..3568672aa 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient.Tests/OpcUaClientUserCertificatePathTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient.Tests/OpcUaClientUserCertificatePathTests.cs @@ -122,6 +122,18 @@ public sealed class OpcUaClientUserCertificatePathTests : IDisposable .ShouldBeSameAs(options); } + /// Review L-4: a relative configured directory is resolved against the install directory, never the CWD. + [Fact] + public void A_relative_configured_directory_resolves_against_the_base_directory_not_the_working_directory() + { + var baseDir = Path.Combine(_outside, "install"); + OpcUaUserCertificatePaths.Normalize("pki/user-certificates", baseDir) + .ShouldBe(Path.Combine(Path.GetFullPath(baseDir), "pki", "user-certificates")); + OpcUaUserCertificatePaths.Normalize(_dir, baseDir).ShouldBe(Path.GetFullPath(_dir)); + OpcUaUserCertificatePaths.Normalize(" ", baseDir).ShouldBeNull(); + OpcUaUserCertificatePaths.Normalize(null, baseDir).ShouldBeNull(); + } + [Fact] public void LoadCertificate_refuses_an_unresolved_relative_path() { diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Browsing/CredentialUseReviewTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Browsing/CredentialUseReviewTests.cs new file mode 100644 index 000000000..f3e2a953a --- /dev/null +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Browsing/CredentialUseReviewTests.cs @@ -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; + +/// +/// 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). +/// +[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); + + /// The victim's driver stores no endpoint; its device stores (v3 layout). + private static IDbContextFactory 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? factory, IAuditWriter? audit = null) => + new([browser], new BrowseSessionRegistry(), NullLogger.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.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 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 OpenAsync(string driverType, string configJson, CancellationToken ct) => + throw new InvalidOperationException("opened"); + } + + private sealed class FixedUser(string name) : AuthenticationStateProvider + { + public override Task GetAuthenticationStateAsync() => + Task.FromResult(new AuthenticationState(new ClaimsPrincipal(new ClaimsIdentity([new Claim(ClaimTypes.Name, name)], "test")))); + } + + private sealed class RecordingAudit : IAuditWriter + { + public List Events { get; } = []; + + public Task WriteAsync(AuditEvent evt, CancellationToken ct = default) + { + Events.Add(evt); + return Task.CompletedTask; + } + } + + private sealed class ThrowingFactory : IDbContextFactory + { + public OtOpcUaConfigDbContext CreateDbContext() => throw new InvalidOperationException("ConfigDb unreachable"); + } + + private sealed class FixedReplyClient(TestDriverConnectResult reply) : IAdminOperationsClient + { + public Task StartDeploymentAsync(string createdBy, CancellationToken ct) => throw new NotSupportedException(); + + public Task AcknowledgeAlarmAsync(string alarmId, string user, string? comment, CancellationToken ct) => + throw new NotSupportedException(); + + public Task ShelveAlarmAsync( + string alarmId, string user, ShelveKind kind, DateTime? unshelveAtUtc, string? comment, CancellationToken ct) => + throw new NotSupportedException(); + + public Task AskAsync(object message, CancellationToken ct) => Task.FromResult((T)(object)reply); + } +} diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/OpcUaClientDriverPageFormSerializationTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/OpcUaClientDriverPageFormSerializationTests.cs index 9940ba49f..af598b20e 100644 --- a/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/OpcUaClientDriverPageFormSerializationTests.cs +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/OpcUaClientDriverPageFormSerializationTests.cs @@ -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)); diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Raw/RawBrowseModalStaleOpenTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Raw/RawBrowseModalStaleOpenTests.cs index eeaddab0f..12ebddc67 100644 --- a/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Raw/RawBrowseModalStaleOpenTests.cs +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Raw/RawBrowseModalStaleOpenTests.cs @@ -194,6 +194,26 @@ public sealed class RawBrowseModalStaleOpenTests : IDisposable Field(modal, "_disabledReason").ShouldBeNull(); } + /// + /// #835 review L-2: a fault inside "Connect & browse" (the browse service throwing — e.g. its ConfigDb owner check) + /// is shown on the modal, never escapes as a circuit error. + /// + [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(modal, "_openError").ShouldNotBeNull().ShouldContain("Browse could not be opened"); + Field(modal, "_opening").ShouldBeFalse(); + } + /// public void Dispose() { diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.ControlPlane.Tests/AdminOperationsActorCredentialScopeTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.ControlPlane.Tests/AdminOperationsActorCredentialScopeTests.cs index b179aa1d3..28ab21f4e 100644 --- a/tests/Server/ZB.MOM.WW.OtOpcUa.ControlPlane.Tests/AdminOperationsActorCredentialScopeTests.cs +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.ControlPlane.Tests/AdminOperationsActorCredentialScopeTests.cs @@ -95,6 +95,28 @@ public sealed class AdminOperationsActorCredentialScopeTests : ControlPlaneActor probe.Calls.ShouldBe(1); } + /// + /// #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. + /// + [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(PresenceBudget, cancellationToken: TestContext.Current.CancellationToken); + reply.Ok.ShouldBeFalse(); + reply.Message.ShouldNotBeNull().ShouldContain("save the change first"); + probe.Calls.ShouldBe(0); + } + /// lmxopcua#836: an absolute OpcUaClient user-certificate path never reaches the probe. [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 { From 90727774229d2348c3aeea4f6128d0330966e155 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Wed, 11 Nov 2026 04:12:40 -0500 Subject: [PATCH 2/2] =?UTF-8?q?docs(security):=20#835/#836=20review=20?= =?UTF-8?q?=E2=80=94=20unsaved-endpoint=20rule,=20credential-use=20audit,?= =?UTF-8?q?=20accurate=20residuals,=20AuthType-gated=20path=20rule,=20dire?= =?UTF-8?q?ctory=20resolution?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-Authored-By: Claude --- .claude/rules/drivers.md | 4 ++++ docs/Configuration.md | 2 +- docs/security.md | 38 ++++++++++++++++++++++++++++++++------ 3 files changed, 37 insertions(+), 7 deletions(-) diff --git a/.claude/rules/drivers.md b/.claude/rules/drivers.md index f5930a574..ce0f95eaa 100644 --- a/.claude/rules/drivers.md +++ b/.claude/rules/drivers.md @@ -367,6 +367,10 @@ host logger factory (wired in `DriverFactoryBootstrap`; the FOCAS and TwinCAT fa deploy (`CredentialReferenceOtherInstance`), Test Connect (`TestDriverConnect.DriverInstanceId`) and browse (`OpenAsync(…, driverInstanceId, …)`; no ConfigDb ⇒ fail closed). One rule: `DriverCredentialPolicy.DescribeOtherInstanceProblems` + `DriverInstanceIdentities`. The runtime adds no check (drivers don't know the fleet); `env:` is not instance-scoped. + ⚠️ Test Connect/browse configs can carry UNSAVED edits: the claimed owner counts only when every endpoint value + (`DriverCredentialRules.EndpointFields`) is one that driver stores (`EffectiveOwnerAsync`, server-side) — a new + credential-sending driver must declare its endpoint keys there. Credential-carrying Test Connect/browse is audited + (`CredentialUseAudit`). - **Key-material file paths (#836):** OpcUaClient `UserCertificatePath` is a RELATIVE path inside `Drivers:OpcUaClient:UserCertificateDirectory` (`DriverCredentialPolicy` path field + `CredentialFilePaths`, all seams; load-time `ResolveWithin` = lexical + link-resolved containment, before the PFX password resolves). `LoadCertificate` diff --git a/docs/Configuration.md b/docs/Configuration.md index 6de77c70f..21a635e9f 100644 --- a/docs/Configuration.md +++ b/docs/Configuration.md @@ -551,7 +551,7 @@ counterpart**: an AbLegacy driver with no key always runs libplctag (Native is o | Key | Type | Default | Meaning | |---|---|---|---| -| `UserCertificateDirectory` | string | `/OtOpcUa/pki/user-certificates` | Directory (made absolute at startup). Env form: `Drivers__OpcUaClient__UserCertificateDirectory`. ACL it to the service account + administrators. | +| `UserCertificateDirectory` | string | `/OtOpcUa/pki/user-certificates` | Directory; a relative value is resolved against the install directory (`AppContext.BaseDirectory`), never the working directory. Env form: `Drivers__OpcUaClient__UserCertificateDirectory`. ACL it to the service account + administrators. | ### `Drivers:Galaxy:AckIdentityCache` (cached API-key → Galaxy ack identity set, #765) diff --git a/docs/security.md b/docs/security.md index af7ec1b5b..947d49e27 100644 --- a/docs/security.md +++ b/docs/security.md @@ -1347,7 +1347,25 @@ through `DriverInstanceIdentities` (only when the config carries an in-scope `se 3. **Test Connect** — `AdminOperationsActor`, before the probe; `TestDriverConnect.DriverInstanceId` names the owner (the device modal passes it; omitted ⇒ treated as a new driver, i.e. stricter). 4. **Live browse** — `BrowserSessionService.OpenAsync(…, driverInstanceId, …)`, before any browser; `/raw` browse - resolves the owner from the device. Without a ConfigDb it fails closed. + resolves the owner from the device. Without a ConfigDb, or when the ConfigDb read faults, it is refused (a message on + the modal, never a circuit error). + +**An unsaved endpoint never gets the named owner's own secrets (review I-1).** Test Connect and browse take a config +that may carry UNSAVED edits — the device modal merges the stored DriverConfig with the device config being typed — so +naming the owner would otherwise let a Designer type their own host into a victim's device and Test Connect it with the +victim's `secret:galaxy//…`. The server (`DriverInstanceIdentities.EffectiveOwnerAsync`, called by the actor +and by `BrowserSessionService` — the client's claim is not trusted) honours the named owner only when it exists, has the +same type, and every **endpoint value** of the config (`DriverCredentialRules.EndpointFields`: Galaxy `Endpoint`, +OpcUaClient `EndpointUrl`/`EndpointUrls`/`DiscoveryUrl`, Heidenhain `Host`/`Port`) is one that driver already stores +(its DriverConfig or any DeviceConfig). Otherwise the owner is treated as null — no instance-named secret at all — and +the refusal says to save the change first. Saving is the deliberate, persisted act; a saved endpoint change then +behaves like any other saved config. + +**Every credential-carrying Test Connect / browse is audited.** When the config carries an in-scope `secret:` or `env:` +reference, `CredentialUseAudit` records an audit event (category `CredentialUse`, action `TestConnect` / `Browse`, +actor = the signed-in user, target = the named driver or `(unsaved driver)`, outcome, and the endpoint host(s) — URI +user-info dropped, never the reference or a value) through the shared `IAuditWriter`, plus a structured Information +log line (`Credential use: …`). Already-running drivers: a deployed driver resolves its reference exactly as before (the driver itself does not know the other instances, so the runtime adds no check) — the same deprecated window as #815: the deploy gate refuses the @@ -1356,10 +1374,11 @@ the driver's own value under `galaxy//…` (or a shared na at it. Residuals (accepted): `env:` references are not instance-scoped (a variable is node-wide, and there is no naming -convention to match — a deployment that needs per-instance isolation must use the secret store); and the rule cannot -stop a Designer from editing an **existing** driver's endpoint and Test-Connecting with that driver's own reference — -that is the general "anyone who may author config may redirect it" property described above (the lever is the page -policy). A gateway-side complement — mxaccessgw binding each API key to the client identity / Galaxy it was issued for +convention to match — a deployment that needs per-instance isolation must use the secret store); a reference to a +**shared** name (one no instance is called) is not tied to any owner, so an unsaved endpoint can still be Test-Connected +with it (audited); and a Designer who **saves** a new endpoint on an existing driver can then Test Connect / browse / +deploy it with that driver's own reference — the general "anyone who may author config may redirect it" property +described above (the save is persisted, the use is audited; the lever is the page policy). A gateway-side complement — mxaccessgw binding each API key to the client identity / Galaxy it was issued for — would make a captured key useless elsewhere; that is cross-repo (`mxaccessgw`) and not implemented here. #### Key-material file paths are confined to a node directory (#836) @@ -1371,9 +1390,16 @@ service account can read (another product's client certificate, a host certifica - The field is a **relative path** (`plant1/user.pfx`) inside the node's user-certificate directory, **`Drivers:OpcUaClient:UserCertificateDirectory`** (env `Drivers__OpcUaClient__UserCertificateDirectory`; default `/OtOpcUa/pki/user-certificates`, beside the driver's PKI stores — for a service running as LocalSystem - that is under `C:\Windows\System32\config\systemprofile\AppData\Local`). Segments are `[A-Za-z0-9._-]`; an absolute, + that is under `C:\Windows\System32\config\systemprofile\AppData\Local`). A **relative** directory setting is resolved + against the install directory (`AppContext.BaseDirectory`), never the working directory (a Windows service starts in + `System32`). Segments are `[A-Za-z0-9._-]`; an absolute, rooted, drive-qualified or UNC path, `.`/`..`/empty segments and anything else are refused (`CredentialFilePaths`, rule row `DriverCredentialPolicy.OpcUaClientUserCertificatePathField`). +- The rule applies unless the **same JSON object** explicitly sets `AuthType` to something other than `Certificate` + (string or enum ordinal) — the driver never loads the file then, and the AdminUI form no longer persists the hidden + field. An object without `AuthType` stays checked, and the deploy gate also checks the driver's config **as the + runtime merges it with its devices**, so splitting `AuthType: Certificate` (device) from the path (driver) cannot slip + a path past the gate. - **Refused at the same five seams as #815** (save, deploy gate `CredentialReferenceOutOfScope`, Test Connect, browse — messages never carry the path) and **at load time**: `OpcUaUserIdentityFactory.ResolveSecretRefsAsync` (called by the running driver and the AdminUI browse before the identity is built) joins it to the directory, canonicalises it, and