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 e82b8e403..1f8fa6bc3 100644 --- a/docs/Configuration.md +++ b/docs/Configuration.md @@ -552,7 +552,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 32a7d25c4..62a72e6bb 100644 --- a/docs/security.md +++ b/docs/security.md @@ -1419,7 +1419,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 @@ -1428,10 +1446,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) @@ -1443,9 +1462,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 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 {