diff --git a/.claude/rules/drivers.md b/.claude/rules/drivers.md index 4e2ddef7b..abe6ffff9 100644 --- a/.claude/rules/drivers.md +++ b/.claude/rules/drivers.md @@ -372,7 +372,11 @@ host logger factory (wired in `DriverFactoryBootstrap`; the FOCAS and TwinCAT fa (case-insensitive; another-instance match beats an own-name match) is refused at save (`DriverConfigSaveGate.ValidateAsync`), 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. + + `DriverInstanceIdentities`. The runtime adds no check (drivers don't know the fleet). Since #849 `env:_…` + is instance-scoped the same way (INSTANCE = id/name upper-cased, other chars `_`; `_`-bounded prefix match); an id-shaped + part (`DRV_<12 HEX>`, `secret:…/DRV-<12 hex>/`) matches by id only, and id-shaped names (`drv`, `DRV-<12 hex>…`) are + refused. #848: create/rename refuse a name another driver's stored reference uses, and a rename away from a name the + driver's own references use (`DescribeNameChangeProblemsAsync`). ⚠️ 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 diff --git a/docs/security.md b/docs/security.md index f82f9bd0b..3529816f8 100644 --- a/docs/security.md +++ b/docs/security.md @@ -1415,7 +1415,7 @@ driver, device, field, allowed form, never the value. The same list, plus runnin warn-only resolution (**Running**, from driver-health diagnostics, fleet-wide), is shown as a banner on `/raw`, with a Re-check button. -#### Per-instance credential references (#835) +#### Per-instance credential references (#835, #848, #849) The namespace confines a reference to values provisioned for that driver *type*, but not to one *instance*: before #835 a Designer could create a Galaxy driver pointing at their own endpoint with @@ -1430,7 +1430,31 @@ that instance's **id or name**, case-insensitively. | `secret:galaxy/DRV-a/apikey`, `secret:galaxy/PlantA/apikey` | allowed — its own | | `secret:galaxy/gw/apikey`, `secret:galaxy/apikey` (no instance is called `gw` / `apikey`) | allowed — a shared key | | `secret:galaxy/DRV-b/apikey`, `secret:galaxy/PlantB/apikey`, `secret:galaxy/plantb` | **refused** — another instance's | -| `env:OTOPCUA_GALAXY_…` | not checked — environment variables carry no instance convention (see residuals) | +| `env:OTOPCUA_GALAXY_DRV_A_KEY`, `env:OTOPCUA_GALAXY_PLANTA_KEY` | allowed — its own (#849, see below) | +| `env:GALAXY_MXGW_API_KEY`, `env:OTOPCUA_GALAXY_API_KEY` (no instance is called `API`) | allowed — a shared variable | +| `env:OTOPCUA_GALAXY_DRV_B_KEY`, `env:OTOPCUA_GALAXY_PLANTB_KEY`, `env:GALAXY_MXGW_PLANTB` | **refused** — another instance's (#849) | + +**`env:` references have the same rule (#849).** The convention is `env:_…` for every prefix of the +driver's scope (`OTOPCUA_GALAXY_`, `GALAXY_MXGW_`, `OTOPCUA_OPCUACLIENT_`, `OTOPCUA_HEIDENHAIN_`), where `INSTANCE` is +a driver id or name in environment form — ASCII letters upper-cased, digits kept, every other character `_` +(`DRV-1a2b` ⇒ `DRV_1A2B`, `Line 1` ⇒ `LINE_1`). The part after the prefix **names** an instance when it equals that +form or starts with it followed by `_` (so `PLANTB` names `env:OTOPCUA_GALAXY_PLANTB_KEY` but not +`env:OTOPCUA_GALAXY_PLANTBX_KEY`). It is applied at every seam below with the same code +(`CredentialReferenceOtherInstance`, message tagged `lmxopcua#849`). Because `_` both separates words and stands in for +`-`/space, a variable can name more than one instance (`OTOPCUA_GALAXY_LINE_1_KEY` names both `Line` and `Line 1`); as +for secrets, any match against another instance wins — use the id form (`env:OTOPCUA_GALAXY_DRV_1A2B_KEY`) for +per-instance variables. Shared variables that start with no instance's id or name (`GALAXY_MXGW_API_KEY`) keep working. + +- **Id-form references are matched by id only.** A reference whose instance part has the shape of a system-generated + id — `secret:…/DRV-<12 hex>/…` or `env:DRV_<12 HEX>` alone or followed by `_` — is never matched against + driver **names**, so a driver named `drv` (env form `DRV`), or one named like another's id, cannot make every + sibling's id-form variable "another instance's", whether that name was created before or after this rule. New names + of that form are refused outright at create / rename / modal update (`DriverCredentialPolicy.IsReservedInstanceName`: + env form `DRV` or starting with `DRV_<12 HEX>`). +- **Name-form variables are only reliable for ASCII letter/digit names.** Normalisation turns every other character — + including non-ASCII letters — into `_`, so `Linie Süd` and `Linie S_d` share `LINIE_S_D`, and a name with no ASCII + letter or digit at all normalises to empty and gets **no** name protection (only its id does). Use the id form for + per-instance variables. Rules worth knowing: @@ -1439,11 +1463,24 @@ Rules worth knowing: `secret:galaxy//…`. Consequence: two same-type drivers that share a *name* cannot use name-scoped secrets — scope them by **driver id** (`secret:galaxy//…`, ids are unique) instead. - A driver **being created** owns nothing yet, so any reference naming an existing instance is refused. -- Creating or renaming a driver whose name equals a segment other drivers already use as a *shared* name makes those - drivers' next deploy fail (`CredentialReferenceOtherInstance`) — rename the shared secret or the driver. +- **A driver's name is checked against the references other drivers store (#848).** Creating a driver — or renaming + one, through the `/raw` rename or the driver config modal — whose new name is the instance part of a reference + **stored on another same-type driver** (its DriverConfig or any DeviceConfig; `secret:` segment or `env:` part) is + refused at save (`DriverConfigSaveGate.ValidateNameAsync` → `DriverInstanceIdentities.DescribeNameChangeProblemsAsync`): + otherwise naming a driver `gw` (or `API`, for `env:GALAXY_MXGW_API_KEY`) would turn every other driver's shared + reference into "another instance's" and fail their next deploy. The message names the other driver, its id, the device + and the field path — never the reference or a value. +- **A driver cannot be renamed away from a name its own references are named after (#848).** If the driver's stored + references (or the config being saved with the rename) use its *current name* — `secret:galaxy/PlantA/…`, + `env:OTOPCUA_GALAXY_PLANTA_…` — and do not also name its id or the new name, the rename is refused: it would free + the old name, and that reference would then look like an unclaimed shared name to every other driver's save, Test + Connect and browse (the issue's "renaming the victim frees its name until the deploy gate runs" window). Move the + reference to the id form first (the config modal can do both in one save), then rename. A case-only rename changes + nothing the rule matches and is allowed. Id-scoped references are immune — ids never change. One rule — `DriverCredentialPolicy.DescribeOtherInstanceProblems` — at the same seams as #815, reading the instance list -through `DriverInstanceIdentities` (only when the config carries an in-scope `secret:` reference at all): +through `DriverInstanceIdentities` (only when the config carries an in-scope `secret:` or `env:` reference at all — so +since #849 a browse of an `env:`-only config also needs the ConfigDb and fails closed without one): 1. **AdminUI save** — `DriverConfigSaveGate.ValidateAsync` (every `RawTreeService` driver/device create + update; the owner is the driver itself, or the device's driver). @@ -1477,8 +1514,24 @@ config at its next deploy, and the upgrade preflight / `/raw` banner lists it (D the driver's own value under `galaxy//…` (or a shared name no instance uses) and point the field 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); a reference to a +Upgrade note (#848/#849): the `env:` rule is new, so a stored config whose variable starts with **another same-type +driver's id or name** (e.g. a Galaxy driver named `API` beside `env:GALAXY_MXGW_API_KEY` users) is refused at its next +deploy — the upgrade preflight / `/raw` banner lists it. Rename that driver (the #848 rename check allows it, since the +references naming it belong to other drivers — unless its own references use the name too, in which case move those to +its id form first) or move the other drivers' variables to their id form. Existing driver names are not +re-checked against stored references except when they change. + +Residuals (accepted): a **deleted** driver's id and name stop protecting its secrets and variables — after the delete +any driver may reference `secret:galaxy//…` — so remove (or rotate) a deleted driver's secrets with it. The +#848 rename pin does not stop a Designer who may edit the victim driver: a **single** config-modal save can move (or +drop) its name-scoped reference and rename it in the same commit, after which the old name is free for any driver. That +is the same "may author the victim's config ⇒ may redirect it" property as a saved endpoint change below (the save is a +persisted, visible draft change, and the later credential use is audited) — use id-scoped references where this +matters. For the same reason a low-severity race is accepted: a rename and a **concurrent** device save that adds a +reference to the old name are not tied together (the driver's `RowVersion` does not cover its device rows), so both can +commit and leave a device reference named after a name the driver no longer has — again needs a Designer who may author that driver's +config, and audited at use. A variable is node-wide, so the `env:` rule +constrains which driver may *name* it, not who can read the process environment. 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 @@ -1503,7 +1556,9 @@ service account can read (another product's client certificate, a host certifica (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. + a path past the gate. The upgrade preflight and the `/raw` banner run the same merged check + (`CredentialReferenceDeployChecks.MergedOnlyProblems`, shared with the gate) and list such a split as a Draft finding + "(merged with its devices)", so it shows before a deploy does (#850). - **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 7c3f18864..9346a7343 100644 --- a/src/Core/ZB.MOM.WW.OtOpcUa.Configuration/Validation/CredentialReferenceDeployChecks.cs +++ b/src/Core/ZB.MOM.WW.OtOpcUa.Configuration/Validation/CredentialReferenceDeployChecks.cs @@ -74,35 +74,44 @@ public static class CredentialReferenceDeployChecks foreach (var problem in DriverCredentialPolicy.DescribeProblems(driver.DriverType, driver.DriverConfig)) errors.Add(new(Code, $"{driver.DriverType} driver '{driver.Name}': {problem}{MigrationHint}", driver.DriverInstanceId)); - var reported = new HashSet(StringComparer.Ordinal); - foreach (var problem in DriverCredentialPolicy.DescribeProblems(driver.DriverType, driver.DriverConfig)) - reported.Add(problem); foreach (var device in devices) { foreach (var problem in DriverCredentialPolicy.DescribeProblems(driver.DriverType, device.DeviceConfig)) - { - reported.Add(problem); errors.Add(new(Code, $"{driver.DriverType} device '{device.Name}': {problem}{MigrationHint}", device.DeviceId)); - } } - // #836 review L-1: a path field is exempt only in an object that explicitly switches it off (OpcUaClient - // AuthType ≠ Certificate), and the switch and the path may sit in different configs (driver vs device). Check - // the config exactly as the runtime merges it, so a split can never deploy a path the driver will load. - if (rules.PathFieldList.Count > 0) - { - var merged = DriverDeviceConfigMerger.Merge( - driver.DriverConfig, - devices.Select(d => new DriverDeviceConfigMerger.DeviceRow(d.Name, d.DeviceConfig)).ToList(), - []); - foreach (var problem in DriverCredentialPolicy.DescribeProblems(driver.DriverType, merged)) - { - if (reported.Add(problem)) - errors.Add(new(Code, $"{driver.DriverType} driver '{driver.Name}' (merged with its devices): {problem}{MigrationHint}", driver.DriverInstanceId)); - } - } + foreach (var problem in MergedOnlyProblems( + driver.DriverType, driver.DriverConfig, devices.Select(d => (d.Name, (string?)d.DeviceConfig)).ToList())) + errors.Add(new(Code, $"{driver.DriverType} driver '{driver.Name}' (merged with its devices): {problem}{MigrationHint}", driver.DriverInstanceId)); } return errors; } + + /// + /// #836 review L-1 / lmxopcua#850: 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). This checks the config exactly as the runtime merges it () and + /// returns the problems the merge reveals that the driver and device configs, checked one by one, do not — so a + /// split can never hide a path the driver will load. The ONE implementation the deploy gate and the upgrade + /// preflight / /raw banner share. Empty for a type without path fields. + /// + /// The driver type. + /// The stored DriverConfig. + /// Its devices (name, DeviceConfig). + /// The merged-only problems. + public static IReadOnlyList MergedOnlyProblems( + string? driverType, string? driverConfig, IReadOnlyList<(string Name, string? DeviceConfig)> devices) + { + ArgumentNullException.ThrowIfNull(devices); + if (!DriverCredentialPolicy.TryGet(driverType, out var rules) || rules.PathFieldList.Count == 0) return []; + var reported = new HashSet(DriverCredentialPolicy.DescribeProblems(driverType, driverConfig), StringComparer.Ordinal); + foreach (var (_, deviceConfig) in devices) + reported.UnionWith(DriverCredentialPolicy.DescribeProblems(driverType, deviceConfig)); + var merged = DriverDeviceConfigMerger.Merge( + driverConfig ?? "{}", + devices.Select(d => new DriverDeviceConfigMerger.DeviceRow(d.Name, d.DeviceConfig ?? "{}")).ToList(), + []); + return DriverCredentialPolicy.DescribeProblems(driverType, merged).Where(reported.Add).ToList(); + } } 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 290c8c5d4..cd3b0e365 100644 --- a/src/Core/ZB.MOM.WW.OtOpcUa.Configuration/Validation/DriverInstanceIdentities.cs +++ b/src/Core/ZB.MOM.WW.OtOpcUa.Configuration/Validation/DriverInstanceIdentities.cs @@ -86,9 +86,118 @@ public static class DriverInstanceIdentities ]; } + /// + /// lmxopcua#848: the problems of giving a driver the name (create, or a rename via the + /// /raw rename or the driver config modal). Two rules, both about a name being the instance part of a stored + /// credential reference (): + /// + /// + /// Claim — the new name may not be named by a reference STORED on another same-type driver (its + /// DriverConfig or any DeviceConfig). Otherwise that reference would start naming "another instance" and the + /// other driver's next deploy fails (naming a driver gw when others share secret:galaxy/gw/…). + /// + /// + /// Pin — a driver may not be renamed AWAY from a name its own references are named after (unless they + /// also name its immutable id or the new name): renaming would free the old name, and a reference to it would + /// then look like an unclaimed shared name to every other driver's Test Connect, browse and save. + /// + /// + /// A case-only rename changes nothing the rule matches, so it is never refused. Messages name drivers, devices and + /// field paths — never a credential value. + /// + /// The config DB. + /// The driver's type. + /// The driver being renamed; null when it is being created. + /// Its stored name; null when it is being created. + /// The name about to be stored. + /// + /// The DriverConfig being saved with the rename (the config modal saves both at once); null ⇒ the stored one. + /// + /// Cancellation. + /// The problems; empty when the name may be stored. + public static async Task> DescribeNameChangeProblemsAsync( + OtOpcUaConfigDbContext db, string? driverType, string? ownDriverInstanceId, string? currentName, string newName, + string? ownDriverConfig, CancellationToken ct) + { + ArgumentNullException.ThrowIfNull(db); + if (!DriverCredentialPolicy.TryGet(driverType, out var rules)) return []; + if (currentName is not null && string.Equals(currentName, newName, StringComparison.OrdinalIgnoreCase)) return []; + + // #849 review: a name in the system-generated id form (env form 'DRV' or 'DRV_<12 HEX>…') would shadow the id + // convention every driver's own references are recommended to use. The matcher already ignores such a name for + // id-shaped references (so a pre-existing one breaks nothing); this keeps new ones from being created at all. + if (DriverCredentialPolicy.IsReservedInstanceName(newName)) + { + return + [ + $"The name '{newName}' has the form of a system-generated driver id ('{DriverCredentialPolicy.DriverIdPrefix}' or " + + $"'{DriverCredentialPolicy.DriverIdPrefix}-<12 hex>…'), which {rules.Scope.Label} credential references use to name a " + + "driver by its id — choose another name (lmxopcua#848/#849).", + ]; + } + + // Same shape as LoadAsync: the type filter stays client-side (case-insensitive), the device read is a plain IN. + var drivers = (await db.DriverInstances.AsNoTracking() + .Select(d => new { d.DriverInstanceId, d.Name, d.DriverType, d.DriverConfig }) + .ToListAsync(ct).ConfigureAwait(false)) + .Where(d => string.Equals(d.DriverType, rules.DriverType, StringComparison.OrdinalIgnoreCase)) + .ToList(); + var ids = drivers.Select(d => d.DriverInstanceId).ToList(); + var devices = ids.Count == 0 + ? [] + : await db.Devices.AsNoTracking() + .Where(d => ids.Contains(d.DriverInstanceId)) + .Select(d => new { d.DriverInstanceId, d.Name, d.DeviceConfig }) + .ToListAsync(ct).ConfigureAwait(false); + + var problems = new List(); + foreach (var driver in drivers) + { + var own = string.Equals(driver.DriverInstanceId, ownDriverInstanceId, StringComparison.Ordinal); + var configs = new List<(string? Device, string? Json)> + { + (null, own && ownDriverConfig is not null ? ownDriverConfig : driver.DriverConfig), + }; + configs.AddRange(devices + .Where(d => string.Equals(d.DriverInstanceId, driver.DriverInstanceId, StringComparison.Ordinal)) + .Select(d => ((string?)d.Name, (string?)d.DeviceConfig))); + + foreach (var (device, json) in configs) + { + var where = device is null ? "" : $" device '{device}'"; + foreach (var reference in DriverCredentialPolicy.CollectInstanceReferences(driverType, json)) + { + if (!own && DriverCredentialPolicy.ReferenceNamesName(reference, newName)) + { + problems.Add( + $"The name '{newName}' is the instance part of the credential reference {reference.FieldPath} on " + + $"{rules.Scope.Label} driver '{driver.Name}' ({driver.DriverInstanceId}){where} — naming this driver " + + "so would make that reference name another driver instance and fail that driver's next deploy. " + + "Choose another name, or move that reference first (lmxopcua#848)."); + } + else if (own && currentName is not null + && DriverCredentialPolicy.ReferenceNamesName(reference, currentName) + && !DriverCredentialPolicy.ReferenceNamesId(reference, ownDriverInstanceId) + && !DriverCredentialPolicy.ReferenceNamesName(reference, newName)) + { + problems.Add( + $"This driver's credential reference {reference.FieldPath}{where} is named after its current name " + + $"'{currentName}' — renaming would free that name, and any other {rules.Scope.Label} driver could then " + + "reference the same credential. Move the reference to this driver's id first " + + $"(secret:{rules.Scope.SecretNamespaces[0]}{ownDriverInstanceId}/… or " + + $"env:{rules.Scope.EnvVariablePrefixes[0]}{DriverCredentialPolicy.NormalizeEnvironmentIdentifier(ownDriverInstanceId)}_…), " + + "then rename (lmxopcua#848)."); + } + } + } + } + + return problems; + } + /// /// The cross-instance problems of — reading the ConfigDb only when the config carries - /// an in-scope secret: reference at all. + /// an in-scope secret: or env: reference at all. /// /// The config DB. /// The owning driver's type. 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 308f28bfd..3060b43ef 100644 --- a/src/Core/ZB.MOM.WW.OtOpcUa.Core.Abstractions/DriverCredentialPolicy.cs +++ b/src/Core/ZB.MOM.WW.OtOpcUa.Core.Abstractions/DriverCredentialPolicy.cs @@ -211,8 +211,8 @@ public static class DriverCredentialPolicy /// /// The instance-identifying segment of an in-scope secret: reference: the first path segment after the /// matched namespace (secret:galaxy/<X>/apikey ⇒ X; secret:otopcua/galaxy/<X> ⇒ - /// X). Null for anything else — env: references carry no instance convention, and literals and - /// out-of-scope references are the #815 rule's business. + /// X). Null for anything else — env: references use , and literals + /// and out-of-scope references are the #815 rule's business. /// /// The authored field value. /// The driver's scope. @@ -232,24 +232,147 @@ public static class DriverCredentialPolicy } /// - /// True when the config carries at least one in-scope secret: reference — i.e. the cross-instance rule - /// () has something to check, so a seam must load the instance list. - /// Lets seams skip the ConfigDb read otherwise. + /// lmxopcua#849: the part of an in-scope env: reference after the matched (longest) prefix — + /// env:OTOPCUA_GALAXY_<REST> ⇒ REST. The per-instance convention is + /// env:<PREFIX><INSTANCE>_…, INSTANCE being a driver id or name in + /// form; the part NAMES an instance when it equals that form or starts + /// with it followed by _ (see / ). Null for anything but an in-scope env:. + /// + /// The authored field value. + /// The driver's scope. + /// The remainder, or null. + public static string? EnvironmentInstancePart(string? value, CredentialScope scope) + { + ArgumentNullException.ThrowIfNull(scope); + if (CredentialReferences.Parse(value, scope, out var name) != CredentialReferenceKind.Environment) return null; + var prefix = scope.EnvVariablePrefixes + .Where(p => name.StartsWith(p, StringComparison.Ordinal)) + .OrderByDescending(p => p.Length) + .FirstOrDefault(); + return prefix is null ? null : name[prefix.Length..]; + } + + /// + /// lmxopcua#849: a driver id or name in environment-variable form — ASCII letters upper-cased, digits kept, every + /// other character _ (DRV-1a2b ⇒ DRV_1A2B, Line 1 ⇒ LINE_1). Empty when the + /// identifier has no letter or digit (such an identifier names no variable). + /// + /// The id or name. + /// The normalised form. + public static string NormalizeEnvironmentIdentifier(string? identifier) + { + if (string.IsNullOrEmpty(identifier) || !identifier.Any(char.IsAsciiLetterOrDigit)) return ""; + return string.Create(identifier.Length, identifier, static (span, id) => + { + for (var i = 0; i < id.Length; i++) + span[i] = char.IsAsciiLetterOrDigit(id[i]) ? char.ToUpperInvariant(id[i]) : '_'; + }); + } + + /// The prefix of every system-generated driver instance id (DRV- + 12 hex, RawTreeService.NewId). + public const string DriverIdPrefix = "DRV"; + + private const int DriverIdHexLength = 12; + + /// + /// Whether an instance reference () names the driver id + /// : a secret: segment equal to it (case-insensitive), or an env: + /// part equal to its form or starting with that form plus _. + /// + /// The reference. + /// A driver instance id. + /// Whether it names it. + public static bool ReferenceNamesId(InstanceReference reference, string? driverInstanceId) => + Matches(reference, driverInstanceId); + + /// + /// Whether an instance reference names the driver NAME — as , + /// except that a reference whose instance part has the shape of a system-generated id + /// (: secret:…/DRV-<12 hex>/…, env:<PREFIX>DRV_<12 HEX>[_…]) is + /// matched by id only. Otherwise a driver NAMED drv (env form DRV) would "own" the id-form variable of + /// every other driver of its type, and the recommended id convention would break fleet-wide — including for a + /// name that pre-dates the rule (lmxopcua#849 review). + /// + /// The reference. + /// A driver name. + /// Whether it names it. + public static bool ReferenceNamesName(InstanceReference reference, string? name) => + !IsIdShaped(reference) && Matches(reference, name); + + /// + /// Whether the reference's instance part has the shape of a system-generated driver id: a secret: segment + /// DRV-<12 hex> (case-insensitive), or an env: part DRV_<12 HEX> alone or followed by _. + /// + /// The reference. + /// Whether it is id-shaped. + public static bool IsIdShaped(InstanceReference reference) => + reference.Kind == CredentialReferenceKind.Secret + ? reference.Part.Length == DriverIdPrefix.Length + 1 + DriverIdHexLength && StartsWithIdForm(reference.Part, '-', ignoreCase: true) + : StartsWithIdForm(reference.Part, '_', ignoreCase: false) + && (reference.Part.Length == DriverIdPrefix.Length + 1 + DriverIdHexLength + || reference.Part[DriverIdPrefix.Length + 1 + DriverIdHexLength] == '_'); + + /// + /// lmxopcua#849 review: a driver NAME that may not be given to a credential-bearing driver — one whose environment + /// form () is DRV or starts with an id form DRV_<12 HEX>. + /// Such a name would collide with the system-generated id convention every driver's own references are + /// recommended to use. + /// + /// The proposed name. + /// Whether the name is reserved. + public static bool IsReservedInstanceName(string? name) + { + var env = NormalizeEnvironmentIdentifier(name); + return env == DriverIdPrefix + || IsIdShaped(new InstanceReference("", CredentialReferenceKind.Environment, env)); + } + + private static bool StartsWithIdForm(string part, char separator, bool ignoreCase) + { + var length = DriverIdPrefix.Length + 1 + DriverIdHexLength; + if (part.Length < length + || !part.StartsWith(DriverIdPrefix, ignoreCase ? StringComparison.OrdinalIgnoreCase : StringComparison.Ordinal) + || part[DriverIdPrefix.Length] != separator) + return false; + for (var i = DriverIdPrefix.Length + 1; i < length; i++) + { + var c = part[i]; + if (!(char.IsAsciiDigit(c) || c is >= 'A' and <= 'F' || (ignoreCase && c is >= 'a' and <= 'f'))) return false; + } + + return true; + } + + private static bool Matches(InstanceReference reference, string? identifier) + { + if (string.IsNullOrEmpty(identifier)) return false; + if (reference.Kind == CredentialReferenceKind.Secret) + return string.Equals(reference.Part, identifier, StringComparison.OrdinalIgnoreCase); + var normalized = NormalizeEnvironmentIdentifier(identifier); + return normalized.Length > 0 + && (string.Equals(reference.Part, normalized, StringComparison.Ordinal) + || reference.Part.StartsWith(normalized + "_", StringComparison.Ordinal)); + } + + /// + /// True when the config carries at least one in-scope secret: or env: reference — i.e. the + /// cross-instance rule () has something to check, so a seam must load + /// the instance list. Lets seams skip the ConfigDb read otherwise. /// /// The owning driver's type. /// The config JSON. - /// Whether any instance segment is present. + /// Whether any instance reference is present. public static bool HasInstanceSegments(string? driverType, string? json) => - CollectSegments(driverType, json).Count > 0; + CollectInstanceReferences(driverType, json).Count > 0; /// - /// lmxopcua#835: inside one namespace, a reference whose instance-identifying segment (see - /// ) names a DIFFERENT existing instance of the same driver type — by its - /// or its , - /// case-insensitively — is refused: otherwise a Designer could create (or Test Connect / browse) a driver that - /// points at their own endpoint with secret:galaxy/<other-instance>/apikey and capture that - /// instance's key. Shared names that are no other instance's id or name (secret:galaxy/gw/apikey, - /// secret:galaxy/apikey) stay valid, as does a reference naming the instance itself. + /// lmxopcua#835/#849: inside one namespace, a reference that names (see / ) a DIFFERENT + /// existing instance of the same driver type — by its or its + /// — is refused: otherwise a Designer could create (or Test Connect / + /// browse) a driver that points at their own endpoint with secret:galaxy/<other-instance>/apikey (or + /// env:OTOPCUA_GALAXY_<OTHER_INSTANCE>_KEY) and capture that instance's key. Shared names that name no + /// other instance (secret:galaxy/gw/apikey, env:GALAXY_MXGW_API_KEY) stay valid, as does a reference + /// naming the instance itself. /// /// /// A match against ANOTHER instance wins over a match against the owner — names are not unique, and renaming @@ -267,24 +390,26 @@ public static class DriverCredentialPolicy ArgumentNullException.ThrowIfNull(instances); var problems = new List(); if (!TryGet(driverType, out var rules)) return problems; - var segments = CollectSegments(driverType, json); - if (segments.Count == 0) return problems; + var references = CollectInstanceReferences(driverType, json); + if (references.Count == 0) return problems; var others = instances .Where(i => string.Equals(i.DriverType, rules.DriverType, StringComparison.OrdinalIgnoreCase) && !string.Equals(i.DriverInstanceId, ownDriverInstanceId, StringComparison.Ordinal)) .ToList(); - foreach (var (fieldPath, segment) in segments) + foreach (var reference in references) { - var other = others.FirstOrDefault(o => - string.Equals(o.DriverInstanceId, segment, StringComparison.OrdinalIgnoreCase) - || string.Equals(o.Name, segment, StringComparison.OrdinalIgnoreCase)); + var other = others.FirstOrDefault(o => ReferenceNamesId(reference, o.DriverInstanceId) || ReferenceNamesName(reference, o.Name)); if (other is null) continue; - problems.Add( - $"{fieldPath} names a secret under '{segment}/', which is another {rules.Scope.Label} driver instance " - + $"('{other.Name}', {other.DriverInstanceId}) — a driver may reference only its own secrets " - + $"(secret:{rules.Scope.SecretNamespaces[0]}/…) or a shared name that is no other " - + "instance's id or name (lmxopcua#835)."); + problems.Add(reference.Kind == CredentialReferenceKind.Secret + ? $"{reference.FieldPath} names a secret under '{reference.Part}/', which is another {rules.Scope.Label} driver " + + $"instance ('{other.Name}', {other.DriverInstanceId}) — a driver may reference only its own secrets " + + $"(secret:{rules.Scope.SecretNamespaces[0]}/…) or a shared name that is no other " + + "instance's id or name (lmxopcua#835)." + : $"{reference.FieldPath} names an environment variable of another {rules.Scope.Label} driver instance " + + $"('{other.Name}', {other.DriverInstanceId}) — a driver may reference only its own variables " + + $"(env:{rules.Scope.EnvVariablePrefixes[0]}_…, the id upper-cased with every other " + + "character as '_') or a shared name that starts with no other instance's id or name (lmxopcua#849)."); } return problems; @@ -397,29 +522,36 @@ public static class DriverCredentialPolicy using (doc) use(doc.RootElement); } - private static List<(string FieldPath, string Segment)> CollectSegments(string? driverType, string? json) + /// + /// Every in-scope instance reference of a config (lmxopcua#835 secret: segments + #849 env: parts), at + /// any depth. Empty for an unreadable config or a type without rules. + /// + /// The owning driver's type. + /// The config JSON. + /// The references, in document order. + public static IReadOnlyList CollectInstanceReferences(string? driverType, string? json) { - var found = new List<(string, string)>(); + var found = new List(); if (!TryGet(driverType, out var rules) || string.IsNullOrWhiteSpace(json)) return found; - JsonDocument doc; - try + // Unreadable JSON is reported by DescribeProblems. + WithDocument(json, root => Visit(root, "", rules, 0, (fieldPath, value, field, _) => { - doc = JsonDocument.Parse(json, new JsonDocumentOptions { CommentHandling = JsonCommentHandling.Skip, AllowTrailingCommas = true }); - } - catch (JsonException) - { - return found; // DescribeProblems reports unreadable JSON. - } - - using (doc) - { - Visit(doc.RootElement, "", rules, 0, (fieldPath, value, field, _) => - { - if (field is null || value.ValueKind != JsonValueKind.String) return; - if (InstanceSegment(value.GetString(), rules.Scope) is { Length: > 0 } segment) found.Add((fieldPath, segment)); - }); - } - + if (field is null || value.ValueKind != JsonValueKind.String) return; + var text = value.GetString(); + if (InstanceSegment(text, rules.Scope) is { Length: > 0 } segment) + found.Add(new(fieldPath, CredentialReferenceKind.Secret, segment)); + else if (EnvironmentInstancePart(text, rules.Scope) is { Length: > 0 } part) + found.Add(new(fieldPath, CredentialReferenceKind.Environment, part)); + })); return found; } } + +/// The instance-identifying part of one in-scope credential reference (lmxopcua#835 / #849). +/// Where it sits in the config (never the value). +/// or . +/// +/// The secret: instance segment () or the env: remainder +/// after the prefix (). +/// +public readonly record struct InstanceReference(string FieldPath, CredentialReferenceKind Kind, string Part); 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 0054d0225..30b84f638 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Browsing/BrowserSessionService.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Browsing/BrowserSessionService.cs @@ -26,7 +26,7 @@ namespace ZB.MOM.WW.OtOpcUa.AdminUI.Browsing; /// Supplies the calling circuit's user. /// /// 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). +/// in-scope secret:/env: 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( @@ -73,7 +73,7 @@ public sealed class BrowserSessionService( 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: " + return new(false, "Browse refused: the driver-instance list needed to check this config's secret:/env: " + "references is unavailable (lmxopcua#835, fail-closed).", Guid.Empty); try { @@ -88,7 +88,7 @@ public sealed class BrowserSessionService( // 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); + + "secret:/env: references (lmxopcua#835/#849, fail-closed). Try again.", Guid.Empty); } } diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Deployments/CredentialReferencePreflight.cs b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Deployments/CredentialReferencePreflight.cs index 45d896c15..ac9cdd2ff 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Deployments/CredentialReferencePreflight.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Deployments/CredentialReferencePreflight.cs @@ -5,6 +5,7 @@ using Microsoft.Extensions.Logging; using ZB.MOM.WW.OtOpcUa.Configuration; using ZB.MOM.WW.OtOpcUa.Configuration.Entities; using ZB.MOM.WW.OtOpcUa.Configuration.Enums; +using ZB.MOM.WW.OtOpcUa.Configuration.Validation; using ZB.MOM.WW.OtOpcUa.Core.Abstractions; using ZB.MOM.WW.OtOpcUa.Runtime.Drivers; @@ -85,13 +86,28 @@ public sealed class CredentialReferencePreflight(IDbContextFactory string.Equals(dev.DriverInstanceId, d.DriverInstanceId, StringComparison.Ordinal)) + .OrderBy(dev => dev.DeviceId, StringComparer.Ordinal) + .Select(dev => (dev.Name, (string?)dev.DeviceConfig)) + .ToList(); + foreach (var p in CredentialReferenceDeployChecks.MergedOnlyProblems(d.DriverType, d.DriverConfig, own)) + findings.Add(new(CredentialFindingSource.Draft, d.DriverType, d.Name, d.DriverInstanceId, null, $"(merged with its devices) {p}")); + } + var deployedList = deployed.ToList(); var deployedInstances = deployedList.Select(s => new DriverInstanceIdentity(s.DriverInstanceId, s.Name, s.DriverType)).ToList(); foreach (var s in deployedList) diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/DriverConfigSaveGate.cs b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/DriverConfigSaveGate.cs index 6b69c6a97..63bc8a697 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/DriverConfigSaveGate.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/DriverConfigSaveGate.cs @@ -34,7 +34,7 @@ public static class DriverConfigSaveGate /// plus the cross-instance rule (lmxopcua#835 — a secret:galaxy/<X>/… reference may not name ANOTHER /// existing instance of the same driver type), which needs the stored instance list. /// - /// The config DB (read only when the config carries an in-scope secret: reference). + /// The config DB (read only when the config carries an in-scope secret:/env: reference). /// The owning driver's type. /// The DriverConfig or DeviceConfig JSON about to be stored. /// The owning driver; null when it is being created. @@ -48,4 +48,28 @@ public static class DriverConfigSaveGate .DescribeOtherInstanceProblemsAsync(db, driverType, configJson, ownDriverInstanceId, ct).ConfigureAwait(false); return problems.Count == 0 ? null : string.Join(" ", problems); } + + /// + /// The authority check runs on every driver create and rename (lmxopcua#848, + /// ): the new name may not be the instance part + /// of another same-type driver's stored credential reference, and a rename may not free a name the driver's own + /// references are named after. + /// + /// The config DB. + /// The driver's type. + /// The driver being renamed; null on create. + /// Its stored name; null on create. + /// The name about to be stored. + /// The DriverConfig saved with it; null ⇒ the stored one. + /// Cancellation. + /// The refusal (never containing a credential value), or null. + public static async Task ValidateNameAsync( + OtOpcUaConfigDbContext db, string? driverType, string? ownDriverInstanceId, string? currentName, string newName, + string? ownDriverConfig, CancellationToken ct) + { + var problems = await DriverInstanceIdentities + .DescribeNameChangeProblemsAsync(db, driverType, ownDriverInstanceId, currentName, newName, ownDriverConfig, ct) + .ConfigureAwait(false); + return problems.Count == 0 ? null : string.Join(" ", problems); + } } diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/RawTreeService.cs b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/RawTreeService.cs index e1fa723b8..6e0a8e5c6 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/RawTreeService.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/RawTreeService.cs @@ -492,6 +492,10 @@ public sealed class RawTreeService(IDbContextFactory dbF if (await DriverConfigSaveGate.ValidateAsync(db, driverType, driverConfigJson, null, ct) is { } instanceRefusal) return new UnsMutationResult(false, instanceRefusal); + // lmxopcua#848: nor may its NAME be the instance part of another driver's stored reference. + if (await DriverConfigSaveGate.ValidateNameAsync(db, driverType, null, null, name, driverConfigJson, ct) is { } nameRefusal) + return new UnsMutationResult(false, nameRefusal); + if (!await db.ServerClusters.AnyAsync(c => c.ClusterId == clusterId, ct)) return new UnsMutationResult(false, $"Cluster '{clusterId}' not found."); @@ -549,6 +553,12 @@ public sealed class RawTreeService(IDbContextFactory dbF && d.Name == newName && d.DriverInstanceId != driverInstanceId, ct)) return RawRenameResult.Failure($"A driver named '{newName}' already exists here."); + // lmxopcua#848: the new name may not be another driver's credential segment, nor may this rename free a name + // the driver's own credential references are named after. + if (await DriverConfigSaveGate.ValidateNameAsync( + db, entity.DriverType, entity.DriverInstanceId, entity.Name, newName, null, ct) is { } nameRefusal) + return RawRenameResult.Failure(nameRefusal); + var warnings = await BuildRenameWarningsAsync(db, await CollectTagsBeneathDriverAsync(db, driverInstanceId, ct), ct); db.Entry(entity).Property(e => e.RowVersion).OriginalValue = rowVersion; @@ -1129,6 +1139,11 @@ public sealed class RawTreeService(IDbContextFactory dbF && d.Name == name && d.DriverInstanceId != driverInstanceId, ct)) return new UnsMutationResult(false, $"A driver named '{name}' already exists here."); + // lmxopcua#848 — as RenameDriverAsync, judged against the config being saved with the rename. + if (await DriverConfigSaveGate.ValidateNameAsync( + db, entity.DriverType, entity.DriverInstanceId, entity.Name, name, driverConfigJson, ct) is { } nameRefusal) + return new UnsMutationResult(false, nameRefusal); + db.Entry(entity).Property(e => e.RowVersion).OriginalValue = rowVersion; entity.Name = name; entity.DriverConfig = string.IsNullOrWhiteSpace(driverConfigJson) ? "{}" : driverConfigJson; 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 441810fb7..12e015bc6 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.ControlPlane/AdminOperations/AdminOperationsActor.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.ControlPlane/AdminOperations/AdminOperationsActor.cs @@ -611,9 +611,9 @@ public sealed class AdminOperationsActor : ReceiveActor var sw = System.Diagnostics.Stopwatch.StartNew(); try { - // lmxopcua#835: a reference naming ANOTHER driver instance's secret (secret:galaxy//…) is refused + // lmxopcua#835/#849: a reference naming ANOTHER driver instance's secret (secret:galaxy//…, env:OTOPCUA_GALAXY__…) is refused // before the probe resolves it — off the mailbox, since it reads the ConfigDb (only when the config - // carries an in-scope secret: reference at all). + // carries an in-scope secret:/env: reference at all). if (DriverCredentialPolicy.HasInstanceSegments(msg.DriverType, msg.ConfigJson)) { await using var db = await _dbFactory.CreateDbContextAsync(cts.Token); 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 4e32aa8f3..e7b1e4713 100644 --- a/tests/Core/ZB.MOM.WW.OtOpcUa.Configuration.Tests/DraftValidatorCredentialInstanceAndPathTests.cs +++ b/tests/Core/ZB.MOM.WW.OtOpcUa.Configuration.Tests/DraftValidatorCredentialInstanceAndPathTests.cs @@ -50,6 +50,35 @@ public sealed class DraftValidatorCredentialInstanceAndPathTests [new Device { DeviceId = "dev-b", DriverInstanceId = "DRV-b", Name = "d", DeviceConfig = """{"Password":"secret:opcuaclient/victim/pw"}""" }]) .ShouldHaveSingleItem().Context.ShouldBe("dev-b"); + /// lmxopcua#849: the env: form carries the same per-instance rule, under the same code. + [Fact] + public void A_galaxy_env_reference_naming_another_instance_is_refused() + { + var e = Validate( + Drv("DRV-victim", "Victim", "GalaxyMxGateway", """{"Gateway":{"ApiKeySecretRef":"env:OTOPCUA_GALAXY_VICTIM_KEY"}}"""), + Drv("DRV-evil", "Evil", "GalaxyMxGateway", """{"Gateway":{"Endpoint":"http://evil:5001","ApiKeySecretRef":"env:GALAXY_MXGW_DRV_VICTIM_KEY"}}""")) + .ShouldHaveSingleItem(); + e.Code.ShouldBe(CredentialReferenceDeployChecks.OtherInstanceCode); + e.Context.ShouldBe("DRV-evil"); + e.Message.ShouldContain("lmxopcua#849"); + } + + [Fact] + public void Shared_and_own_env_variables_pass() => + Validate( + Drv("DRV-a", "A", "GalaxyMxGateway", """{"Gateway":{"ApiKeySecretRef":"env:OTOPCUA_GALAXY_DRV_A_KEY"}}"""), + Drv("DRV-b", "B", "GalaxyMxGateway", """{"Gateway":{"ApiKeySecretRef":"env:GALAXY_MXGW_API_KEY"}}"""), + Drv("DRV-c", "C", "GalaxyMxGateway", """{"Gateway":{"ApiKeySecretRef":"env:GALAXY_MXGW_API_KEY"}}""")) + .ShouldBeEmpty(); + + /// #849 review: a pre-existing driver named 'drv' must not fail the id-form variables of its siblings. + [Fact] + public void A_driver_named_drv_does_not_break_id_form_env_variables() => + Validate( + Drv("DRV-0123456789ab", "A", "GalaxyMxGateway", """{"Gateway":{"ApiKeySecretRef":"env:OTOPCUA_GALAXY_DRV_0123456789AB_KEY"}}"""), + Drv("DRV-fedcba987654", "drv", "GalaxyMxGateway", """{"Gateway":{"ApiKeySecretRef":"env:OTOPCUA_GALAXY_DRV_FEDCBA987654_KEY"}}""")) + .ShouldBeEmpty(); + [Fact] public void Shared_and_own_keys_pass() => Validate( diff --git a/tests/Core/ZB.MOM.WW.OtOpcUa.Core.Abstractions.Tests/CredentialInstanceScopeTests.cs b/tests/Core/ZB.MOM.WW.OtOpcUa.Core.Abstractions.Tests/CredentialInstanceScopeTests.cs index 22e032bca..87c832fd3 100644 --- a/tests/Core/ZB.MOM.WW.OtOpcUa.Core.Abstractions.Tests/CredentialInstanceScopeTests.cs +++ b/tests/Core/ZB.MOM.WW.OtOpcUa.Core.Abstractions.Tests/CredentialInstanceScopeTests.cs @@ -44,7 +44,10 @@ public sealed class CredentialInstanceScopeTests [InlineData("secret:galaxy/gw/apikey")] // a shared name no instance uses [InlineData("secret:galaxy/apikey")] [InlineData("secret:galaxy/UpstreamC/apikey")] // another TYPE's instance — not this namespace's owner - [InlineData("env:OTOPCUA_GALAXY_PLANTB_KEY")] // env carries no instance convention + [InlineData("env:OTOPCUA_GALAXY_PLANTA_KEY")] // its own name in env form (lmxopcua#849) + [InlineData("env:OTOPCUA_GALAXY_DRV_AAA_KEY")] // its own id in env form + [InlineData("env:GALAXY_MXGW_API_KEY")] // a shared variable that names no instance + [InlineData("env:OTOPCUA_GALAXY_PLANTBX_KEY")] // 'PLANTB' only at a '_' boundary [InlineData("dev:k")] [InlineData("")] public void Own_shared_and_non_secret_references_pass(string reference) => @@ -98,7 +101,7 @@ public sealed class CredentialInstanceScopeTests public void HasInstanceSegments_gates_the_ConfigDb_read() { DriverCredentialPolicy.HasInstanceSegments(DriverTypeNames.Galaxy, Galaxy("secret:galaxy/x/apikey")).ShouldBeTrue(); - DriverCredentialPolicy.HasInstanceSegments(DriverTypeNames.Galaxy, Galaxy("env:OTOPCUA_GALAXY_X")).ShouldBeFalse(); + DriverCredentialPolicy.HasInstanceSegments(DriverTypeNames.Galaxy, Galaxy("env:OTOPCUA_GALAXY_X")).ShouldBeTrue(); // #849 DriverCredentialPolicy.HasInstanceSegments(DriverTypeNames.Galaxy, Galaxy("env:ConnectionStrings__ConfigDb")).ShouldBeFalse(); DriverCredentialPolicy.HasInstanceSegments(DriverTypeNames.Modbus, """{"Password":"secret:galaxy/x"}""").ShouldBeFalse(); DriverCredentialPolicy.HasInstanceSegments(DriverTypeNames.Galaxy, "not json").ShouldBeFalse(); @@ -113,4 +116,132 @@ public sealed class CredentialInstanceScopeTests problems.ShouldContain(p => p.Contains("lmxopcua#815")); problems.ShouldContain(p => p.Contains("lmxopcua#835")); } + + // --------------------------------------------------------------------------------------------------------------- + // lmxopcua#849 — env: references follow the same per-instance rule (env:_…) + // --------------------------------------------------------------------------------------------------------------- + + [Theory] + [InlineData("env:OTOPCUA_GALAXY_PLANTB_KEY")] // another instance's name, upper-cased + [InlineData("env:OTOPCUA_GALAXY_PLANTB")] // the part alone + [InlineData("env:OTOPCUA_GALAXY_DRV_BBB_KEY")] // another instance's id, '-' as '_' + [InlineData("env:GALAXY_MXGW_PLANTB_API_KEY")] // the legacy prefix too + public void An_env_reference_naming_another_instance_is_refused(string reference) + { + var problem = DriverCredentialPolicy + .DescribeOtherInstanceProblems(DriverTypeNames.Galaxy, Galaxy(reference), "DRV-aaa", Instances) + .ShouldHaveSingleItem(); + problem.ShouldContain("lmxopcua#849"); + problem.ShouldContain("DRV-bbb"); + problem.ShouldContain("Gateway.ApiKeySecretRef"); + } + + [Fact] + public void An_env_match_against_another_instance_wins_over_the_owners() + { + // Owner "Line 1" (LINE_1) and another instance "Line" (LINE): OTOPCUA_GALAXY_LINE_1_KEY names both. + IEnumerable lines = + [new("DRV-1", "Line 1", DriverTypeNames.Galaxy), new("DRV-2", "Line", DriverTypeNames.Galaxy)]; + DriverCredentialPolicy.DescribeOtherInstanceProblems(DriverTypeNames.Galaxy, Galaxy("env:OTOPCUA_GALAXY_LINE_1_KEY"), "DRV-1", lines) + .ShouldHaveSingleItem(); + // …while the owner's id-scoped variable names only the owner. + DriverCredentialPolicy.DescribeOtherInstanceProblems(DriverTypeNames.Galaxy, Galaxy("env:OTOPCUA_GALAXY_DRV_1_KEY"), "DRV-1", lines) + .ShouldBeEmpty(); + } + + [Fact] + public void OpcUaClient_and_Heidenhain_env_forms_have_the_rule() + { + DriverCredentialPolicy.DescribeOtherInstanceProblems( + DriverTypeNames.OpcUaClient, """{"Password":"env:OTOPCUA_OPCUACLIENT_UPSTREAMD_PW"}""", "DRV-ccc", Instances) + .ShouldHaveSingleItem().ShouldContain("lmxopcua#849"); + IEnumerable hh = [new("H1", "line1", DriverTypeNames.Heidenhain), new("H2", "line2", DriverTypeNames.Heidenhain)]; + DriverCredentialPolicy.DescribeOtherInstanceProblems( + DriverTypeNames.Heidenhain, """{"Access":{"DncPassword":"env:OTOPCUA_HEIDENHAIN_LINE2_DNC"}}""", "H1", hh) + .ShouldHaveSingleItem(); + DriverCredentialPolicy.DescribeOtherInstanceProblems( + DriverTypeNames.Heidenhain, """{"Access":{"DncPassword":"env:OTOPCUA_HEIDENHAIN_DNC_PASSWORD"}}""", "H1", hh) + .ShouldBeEmpty(); + } + + [Theory] + [InlineData("DRV-1a2b", "DRV_1A2B")] + [InlineData("Line 1", "LINE_1")] + [InlineData("plant.b", "PLANT_B")] + [InlineData("---", "")] + [InlineData("", "")] + public void Identifiers_normalise_to_environment_form(string identifier, string expected) => + DriverCredentialPolicy.NormalizeEnvironmentIdentifier(identifier).ShouldBe(expected); + + [Fact] + public void Instance_references_are_collected_for_both_kinds() + { + var refs = DriverCredentialPolicy.CollectInstanceReferences(DriverTypeNames.Galaxy, + """{"Gateway":{"ApiKeySecretRef":"secret:galaxy/gw/k"},"AlarmAck":{"FallbackPasswordSecretRef":"env:OTOPCUA_GALAXY_ACK_PW"}}"""); + refs.Count.ShouldBe(2); + refs[0].ShouldBe(new InstanceReference("Gateway.ApiKeySecretRef", CredentialReferenceKind.Secret, "gw")); + refs[1].ShouldBe(new InstanceReference("AlarmAck.FallbackPasswordSecretRef", CredentialReferenceKind.Environment, "ACK_PW")); + DriverCredentialPolicy.ReferenceNamesName(refs[1], "ack").ShouldBeTrue(); + DriverCredentialPolicy.ReferenceNamesName(refs[1], "ac").ShouldBeFalse(); + DriverCredentialPolicy.ReferenceNamesName(refs[0], "GW").ShouldBeTrue(); + } + + // --------------------------------------------------------------------------------------------------------------- + // #849 review — a driver NAMED 'drv' (or id-shaped) must not shadow the id convention + // --------------------------------------------------------------------------------------------------------------- + + private const string RealId = "DRV-0123456789ab"; // the RawTreeService.NewId shape + + private static readonly DriverInstanceIdentity[] WithDrvNamed = + [ + new(RealId, "PlantA", DriverTypeNames.Galaxy), + new("DRV-fedcba987654", "drv", DriverTypeNames.Galaxy), // a pre-existing driver named 'drv' + new("DRV-111111111111", "DRV-0123456789AB", DriverTypeNames.Galaxy), // …or named like another's id + ]; + + [Theory] + [InlineData("env:OTOPCUA_GALAXY_DRV_0123456789AB_KEY")] + [InlineData("env:OTOPCUA_GALAXY_DRV_0123456789AB")] + [InlineData("env:GALAXY_MXGW_DRV_0123456789AB_API_KEY")] + [InlineData("secret:galaxy/DRV-0123456789ab/apikey")] + public void An_id_form_reference_is_matched_by_id_only_so_a_pre_existing_drv_name_does_not_break_it(string reference) => + DriverCredentialPolicy.DescribeOtherInstanceProblems(DriverTypeNames.Galaxy, Galaxy(reference), RealId, WithDrvNamed) + .ShouldBeEmpty(); + + [Fact] + public void An_id_form_reference_still_names_the_other_instance_by_its_id() => + DriverCredentialPolicy.DescribeOtherInstanceProblems( + DriverTypeNames.Galaxy, Galaxy("env:OTOPCUA_GALAXY_DRV_0123456789AB_KEY"), "DRV-fedcba987654", WithDrvNamed) + .ShouldHaveSingleItem().ShouldContain(RealId); + + [Fact] + public void A_drv_name_still_matches_a_reference_that_is_not_id_shaped() => + // 'DRV_FOO' is not an id form, so the name 'drv' does claim it (the create/rename check refuses that name anyway). + DriverCredentialPolicy.DescribeOtherInstanceProblems(DriverTypeNames.Galaxy, Galaxy("env:OTOPCUA_GALAXY_DRV_FOO_KEY"), RealId, WithDrvNamed) + .ShouldHaveSingleItem(); + + [Theory] + [InlineData("env:OTOPCUA_GALAXY_DRV_0123456789AB_KEY", true)] + [InlineData("env:OTOPCUA_GALAXY_DRV_0123456789AB", true)] + [InlineData("env:OTOPCUA_GALAXY_DRV_0123456789ABC_KEY", false)] // 13 hex: not an id form + [InlineData("env:OTOPCUA_GALAXY_DRV_0123456789AG_KEY", false)] // not hex + [InlineData("env:OTOPCUA_GALAXY_DRV_KEY", false)] + [InlineData("secret:galaxy/DRV-0123456789ab/k", true)] + [InlineData("secret:galaxy/drv-0123456789AB/k", true)] + [InlineData("secret:galaxy/DRV-0123456789ab-x/k", false)] + public void Id_shape_detection(string reference, bool expected) => + DriverCredentialPolicy.IsIdShaped(DriverCredentialPolicy.CollectInstanceReferences(DriverTypeNames.Galaxy, Galaxy(reference)).Single()) + .ShouldBe(expected); + + [Theory] + [InlineData("drv", true)] + [InlineData("DRV", true)] + [InlineData("drv-", false)] // env form 'DRV_' — neither 'DRV' nor an id form + [InlineData("DRV-0123456789ab", true)] + [InlineData("drv 0123456789AB extra", true)] + [InlineData("DRV-0123456789", false)] + [InlineData("Driver", false)] + [InlineData("PlantA", false)] + public void Reserved_instance_names(string name, bool expected) => + DriverCredentialPolicy.IsReservedInstanceName(name).ShouldBe(expected); } diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Browsing/BrowserSessionCredentialScopeTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Browsing/BrowserSessionCredentialScopeTests.cs index a5459573e..916ed9105 100644 --- a/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Browsing/BrowserSessionCredentialScopeTests.cs +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Browsing/BrowserSessionCredentialScopeTests.cs @@ -72,6 +72,41 @@ public sealed class BrowserSessionCredentialScopeTests universal.Opens.ShouldBe(0); } + /// lmxopcua#849: an env: reference named after ANOTHER instance never opens a browser either. + [Theory] + [InlineData("env:OTOPCUA_GALAXY_VICTIM_KEY")] + [InlineData("env:GALAXY_MXGW_DRV_VICTIM_KEY")] + public async Task An_env_reference_naming_another_instance_never_opens_a_browser(string reference) + { + var bespoke = new RecordingBrowser(DriverTypeNames.Galaxy); + var svc = new BrowserSessionService( + [bespoke], new BrowseSessionRegistry(), NullLogger.Instance, new RecordingUniversal(), null!, null!, + SeededFactory()); + + var result = await svc.OpenAsync(DriverTypeNames.Galaxy, + """{"Gateway":{"Endpoint":"http://evil:5001","ApiKeySecretRef":"REF"}}""".Replace("REF", reference), "DRV-evil", + TestContext.Current.CancellationToken); + + result.Ok.ShouldBeFalse(); + result.Message.ShouldNotBeNull().ShouldContain("lmxopcua#849"); + bespoke.Opens.ShouldBe(0); + } + + /// Positive control: a shared env variable that names no instance reaches the browser. + [Fact] + public async Task A_shared_env_variable_reaches_the_browser() + { + var bespoke = new RecordingBrowser(DriverTypeNames.Galaxy); + var svc = new BrowserSessionService( + [bespoke], new BrowseSessionRegistry(), NullLogger.Instance, new RecordingUniversal(), null!, null!, + SeededFactory()); + + await svc.OpenAsync(DriverTypeNames.Galaxy, """{"Gateway":{"ApiKeySecretRef":"env:GALAXY_MXGW_API_KEY"}}""", + TestContext.Current.CancellationToken); + + bespoke.Opens.ShouldBe(1); + } + /// Positive control: the owning driver's own id-scoped secret reaches the browser. [Fact] public async Task The_owners_own_secret_reaches_the_browser() diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Deployments/CredentialReferencePreflightInstanceAndPathTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Deployments/CredentialReferencePreflightInstanceAndPathTests.cs index d47573f2c..452648a9e 100644 --- a/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Deployments/CredentialReferencePreflightInstanceAndPathTests.cs +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Deployments/CredentialReferencePreflightInstanceAndPathTests.cs @@ -52,4 +52,46 @@ public sealed class CredentialReferencePreflightInstanceAndPathTests f.Problem.ShouldContain("lmxopcua#836"); f.Problem.ShouldNotContain("plant.pfx"); } + + /// + /// lmxopcua#850: the AuthType switch in one config and the path in the other — each config passes alone, the + /// runtime-merged config does not. The deploy gate catches it; the banner must show it before a deploy does. + /// + [Fact] + public void A_path_split_from_its_certificate_switch_is_listed_from_the_merged_config() + { + var findings = CredentialReferencePreflight.Scan( + [Drv("o", "Upstream", DriverTypeNames.OpcUaClient, """{"AuthType":"Anonymous","UserCertificatePath":"C:\\stolen\\host.pfx"}""")], + [new Device { DeviceId = "dev-o", DriverInstanceId = "o", Name = "d", DeviceConfig = """{"AuthType":"Certificate"}""" }], + []); + + var f = findings.ShouldHaveSingleItem(); + f.Source.ShouldBe(CredentialFindingSource.Draft); + f.DriverInstanceId.ShouldBe("o"); + f.DeviceName.ShouldBeNull(); + f.Problem.ShouldContain("merged with its devices"); + f.Problem.ShouldContain("lmxopcua#836"); + f.Problem.ShouldNotContain("host.pfx"); + } + + /// Control: a problem already reported on one config is not listed a second time from the merge. + [Fact] + public void A_path_already_reported_on_its_own_config_is_not_duplicated_by_the_merge() => + CredentialReferencePreflight.Scan( + [Drv("o", "Upstream", DriverTypeNames.OpcUaClient, """{"AuthType":"Certificate","UserCertificatePath":"C:\\certs\\plant.pfx"}""")], + [new Device { DeviceId = "dev-o", DriverInstanceId = "o", Name = "d", DeviceConfig = "{}" }], + []) + .ShouldHaveSingleItem().Problem.ShouldNotContain("merged"); + + /// lmxopcua#849: an env: reference naming another instance is listed like a secret: one. + [Fact] + public void A_cross_instance_env_reference_is_listed() => + CredentialReferencePreflight.Scan( + [ + Drv("DRV-a", "A", DriverTypeNames.Galaxy, "{}"), + Drv("DRV-b", "B", DriverTypeNames.Galaxy, """{"Gateway":{"ApiKeySecretRef":"env:OTOPCUA_GALAXY_DRV_A_KEY"}}"""), + ], + [], + []) + .ShouldHaveSingleItem().Problem.ShouldContain("lmxopcua#849"); } diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Uns/CredentialNameClaimSavePathTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Uns/CredentialNameClaimSavePathTests.cs new file mode 100644 index 000000000..07bea8100 --- /dev/null +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Uns/CredentialNameClaimSavePathTests.cs @@ -0,0 +1,167 @@ +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.AdminUI.Uns; +using ZB.MOM.WW.OtOpcUa.Core.Abstractions; + +namespace ZB.MOM.WW.OtOpcUa.AdminUI.Tests.Uns; + +/// +/// lmxopcua#848: a driver's NAME is checked against the credential references stored on other drivers. Naming a driver +/// after another same-type driver's (shared) secret segment or env part would make that reference name "another +/// instance" and fail its next deploy; renaming a driver away from a name its own references are named after would +/// free that name for every other driver's Test Connect / browse / save. Both are refused at create, at the /raw +/// rename and at the config modal's update. +/// +[Trait("Category", "Unit")] +public sealed class CredentialNameClaimSavePathTests +{ + private static CancellationToken Ct => TestContext.Current.CancellationToken; + + private static string Galaxy(string reference) => + """{"Gateway":{"Endpoint":"http://gw:5001","ApiKeySecretRef":"REF"}}""".Replace("REF", reference); + + private static async Task<(RawTreeService Svc, string Db, string SharerId)> WithSharerAsync(string reference) + { + var name = RawTreeTestDb.SeedFresh(); + var svc = new RawTreeService(RawTreeTestDb.Factory(name)); + var sharer = await svc.CreateDriverAsync(RawTreeTestDb.ClusterId, null, "Sharer", DriverTypeNames.Galaxy, Galaxy(reference), Ct); + sharer.Ok.ShouldBeTrue(sharer.Error); + return (svc, name, sharer.CreatedId!); + } + + private static byte[] RowVersion(string db, string driverId) + { + using var ctx = RawTreeTestDb.CreateNamed(db); + return ctx.DriverInstances.Single(d => d.DriverInstanceId == driverId).RowVersion; + } + + [Theory] + [InlineData("secret:galaxy/gw/very-private-name", "gw")] + [InlineData("secret:galaxy/gw/very-private-name", "GW")] // case-insensitive, as the rule matches + [InlineData("env:GALAXY_MXGW_API_KEY_VERY_PRIVATE", "api")] // #849 env form: API is a '_'-bounded prefix + [InlineData("env:OTOPCUA_GALAXY_LINE_1_VERY_PRIVATE", "line-1")] // normalised LINE_1 + public async Task Creating_a_driver_named_after_another_drivers_reference_is_refused(string reference, string newName) + { + var (svc, db, sharerId) = await WithSharerAsync(reference); + + var result = await svc.CreateDriverAsync(RawTreeTestDb.ClusterId, null, newName, DriverTypeNames.Galaxy, "{}", Ct); + + result.Ok.ShouldBeFalse(); + result.Error!.ShouldContain("lmxopcua#848"); + result.Error!.ShouldContain("Sharer"); + result.Error!.ShouldContain(sharerId); + result.Error!.ShouldContain("Gateway.ApiKeySecretRef"); + result.Error!.ShouldNotContain("PRIVATE", Case.Insensitive); // never the rest of the reference + using var check = RawTreeTestDb.CreateNamed(db); + check.DriverInstances.ShouldNotContain(d => d.Name == newName); + } + + [Fact] + public async Task A_device_level_reference_claims_the_name_too() + { + var (svc, db, sharerId) = await WithSharerAsync("secret:galaxy/sharer-own/k"); + using (var ctx = RawTreeTestDb.CreateNamed(db)) + { + var device = ctx.Devices.Single(d => d.DriverInstanceId == sharerId); + device.DeviceConfig = Galaxy("secret:galaxy/plant7/k"); + ctx.SaveChanges(); + } + + var result = await svc.CreateDriverAsync(RawTreeTestDb.ClusterId, null, "Plant7", DriverTypeNames.Galaxy, "{}", Ct); + + result.Ok.ShouldBeFalse(); + result.Error!.ShouldContain("device 'Device1'"); + } + + [Fact] + public async Task Another_driver_type_and_unrelated_names_are_not_claimed() + { + var (svc, _, _) = await WithSharerAsync("secret:galaxy/gw/k"); + + (await svc.CreateDriverAsync(RawTreeTestDb.ClusterId, null, "gw", DriverTypeNames.OpcUaClient, "{}", Ct)).Ok.ShouldBeTrue(); + (await svc.CreateDriverAsync(RawTreeTestDb.ClusterId, null, "gw2", DriverTypeNames.Galaxy, "{}", Ct)).Ok.ShouldBeTrue(); + } + + [Fact] + public async Task Renaming_onto_a_claimed_name_is_refused_on_both_rename_paths() + { + var (svc, db, _) = await WithSharerAsync("secret:galaxy/gw/k"); + var other = await svc.CreateDriverAsync(RawTreeTestDb.ClusterId, null, "Other", DriverTypeNames.Galaxy, "{}", Ct); + other.Ok.ShouldBeTrue(other.Error); + + var renamed = await svc.RenameDriverAsync(other.CreatedId!, "gw", RowVersion(db, other.CreatedId!), Ct); + renamed.Ok.ShouldBeFalse(); + renamed.Error!.ShouldContain("lmxopcua#848"); + + var updated = await svc.UpdateDriverAsync(other.CreatedId!, "gw", "{}", null, RowVersion(db, other.CreatedId!), Ct); + updated.Ok.ShouldBeFalse(); + updated.Error!.ShouldContain("lmxopcua#848"); + + using var check = RawTreeTestDb.CreateNamed(db); + check.DriverInstances.Single(d => d.DriverInstanceId == other.CreatedId).Name.ShouldBe("Other"); + } + + /// Issue (b): renaming the victim away from the name its own secret lives under would free that name. + [Theory] + [InlineData("secret:galaxy/Sharer/apikey")] + [InlineData("env:OTOPCUA_GALAXY_SHARER_KEY")] + public async Task A_driver_whose_references_are_named_after_it_cannot_be_renamed_away(string reference) + { + var (svc, db, sharerId) = await WithSharerAsync(reference); + + var renamed = await svc.RenameDriverAsync(sharerId, "Sharer-old", RowVersion(db, sharerId), Ct); + renamed.Ok.ShouldBeFalse(); + renamed.Error!.ShouldContain("lmxopcua#848"); + renamed.Error!.ShouldContain(sharerId); // points at the id-scoped form + + (await svc.UpdateDriverAsync(sharerId, "Sharer-old", Galaxy(reference), null, RowVersion(db, sharerId), Ct)).Ok.ShouldBeFalse(); + } + + [Fact] + public async Task Re_pointing_to_the_id_in_the_same_save_allows_the_rename() + { + var (svc, db, sharerId) = await WithSharerAsync("secret:galaxy/Sharer/apikey"); + + var updated = await svc.UpdateDriverAsync( + sharerId, "Sharer-new", Galaxy($"secret:galaxy/{sharerId}/apikey"), null, RowVersion(db, sharerId), Ct); + + updated.Ok.ShouldBeTrue(updated.Error); + } + + [Theory] + [InlineData("sharer")] // case-only: the rule matches case-insensitively, so nothing is freed or claimed + [InlineData("Renamed")] + public async Task Renames_that_free_and_claim_nothing_pass(string newName) + { + var reference = newName == "sharer" ? "secret:galaxy/Sharer/apikey" : "secret:galaxy/shared-gw/apikey"; + var (svc, db, sharerId) = await WithSharerAsync(reference); + + var renamed = await svc.RenameDriverAsync(sharerId, newName, RowVersion(db, sharerId), Ct); + + renamed.Ok.ShouldBeTrue(renamed.Error); + } + + /// + /// #849 review: a name in the system-generated id form ('drv', 'DRV-<12 hex>…') would shadow every driver's + /// id-form references — refused at create, the /raw rename and the config modal, even with no reference stored. + /// + [Theory] + [InlineData("drv")] + [InlineData("DRV")] + [InlineData("DRV-0123456789ab")] + [InlineData("drv-0123456789AB-line")] + public async Task An_id_shaped_name_is_refused_on_every_name_path(string newName) + { + var (svc, db, sharerId) = await WithSharerAsync("env:OTOPCUA_GALAXY_DRV_0123456789AB_KEY"); + + var create = await svc.CreateDriverAsync(RawTreeTestDb.ClusterId, null, newName, DriverTypeNames.Galaxy, "{}", Ct); + create.Ok.ShouldBeFalse(); + create.Error!.ShouldContain("system-generated driver id"); + + (await svc.RenameDriverAsync(sharerId, newName, RowVersion(db, sharerId), Ct)).Ok.ShouldBeFalse(); + (await svc.UpdateDriverAsync(sharerId, newName, "{}", null, RowVersion(db, sharerId), Ct)).Ok.ShouldBeFalse(); + + // A type without credential rules is not affected. + (await svc.CreateDriverAsync(RawTreeTestDb.ClusterId, null, newName, DriverTypeNames.Modbus, "{}", Ct)).Ok.ShouldBeTrue(); + } +} diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Uns/CredentialNameClaimSqlServerTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Uns/CredentialNameClaimSqlServerTests.cs new file mode 100644 index 000000000..22d1236f5 --- /dev/null +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Uns/CredentialNameClaimSqlServerTests.cs @@ -0,0 +1,67 @@ +using Microsoft.EntityFrameworkCore; +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.AdminUI.Uns; +using ZB.MOM.WW.OtOpcUa.Configuration.Entities; +using ZB.MOM.WW.OtOpcUa.Configuration.Enums; +using ZB.MOM.WW.OtOpcUa.Configuration.Validation; +using ZB.MOM.WW.OtOpcUa.Core.Abstractions; + +namespace ZB.MOM.WW.OtOpcUa.AdminUI.Tests.Uns; + +/// +/// The SQL-Server-backed half of (lmxopcua#848): the driver + device +/// reads of must translate on the real provider, +/// not only run as LINQ-to-Objects under EF InMemory (the #815 live gate's crash-looping boot came from exactly that gap). +/// +[Collection(nameof(AdminUiSqlServerCollection))] +public sealed class CredentialNameClaimSqlServerTests(AdminUiSqlServerFixture sql) +{ + private static CancellationToken Ct => TestContext.Current.CancellationToken; + + [SqlServerFact] + public async Task Create_and_rename_are_checked_against_stored_driver_and_device_references() + { + // 8 hex chars: unique per run in the shared database, short enough for the id columns. + var token = Guid.NewGuid().ToString("N")[..8]; + var clusterId = $"C-{token}"; + var sharerId = $"DRV-s{token}"; + await using (var db = await sql.Factory.CreateDbContextAsync(Ct)) + { + db.ServerClusters.Add(new ServerCluster + { + ClusterId = clusterId, Name = $"Cluster {token}", Enterprise = "zb", Site = "warsaw-west", + NodeCount = 1, RedundancyMode = RedundancyMode.None, CreatedBy = "test", + }); + db.DriverInstances.Add(new DriverInstance + { + DriverInstanceId = sharerId, ClusterId = clusterId, Name = $"Sharer{token}", DriverType = DriverTypeNames.Galaxy, + DriverConfig = "{\"Gateway\":{\"ApiKeySecretRef\":\"secret:galaxy/gw" + token + "/k\"}}", + }); + db.Devices.Add(new Device + { + DeviceId = $"DEV-s{token}", DriverInstanceId = sharerId, Name = "Device1", + DeviceConfig = "{\"AlarmAck\":{\"FallbackPasswordSecretRef\":\"env:OTOPCUA_GALAXY_SHARER" + token.ToUpperInvariant() + "_ACK\"}}", + }); + await db.SaveChangesAsync(Ct); + } + + var svc = new RawTreeService(sql.Factory); + + // Claim (driver-level secret: reference). + var create = await svc.CreateDriverAsync(clusterId, null, $"gw{token}", DriverTypeNames.Galaxy, "{}", Ct); + create.Ok.ShouldBeFalse(); + create.Error!.ShouldContain("lmxopcua#848"); + + // Pin (device-level env: reference named after the sharer's own name). + byte[] rv; + await using (var db = await sql.Factory.CreateDbContextAsync(Ct)) + rv = await db.DriverInstances.AsNoTracking().Where(d => d.DriverInstanceId == sharerId).Select(d => d.RowVersion).SingleAsync(Ct); + var rename = await svc.RenameDriverAsync(sharerId, $"Moved{token}", rv, Ct); + rename.Ok.ShouldBeFalse(); + rename.Error!.ShouldContain("device 'Device1'"); + + // Positive control: an unclaimed name creates. + (await svc.CreateDriverAsync(clusterId, null, $"free{token}", DriverTypeNames.Galaxy, "{}", Ct)).Ok.ShouldBeTrue(); + } +} 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 28ab21f4e..45157e005 100644 --- a/tests/Server/ZB.MOM.WW.OtOpcUa.ControlPlane.Tests/AdminOperationsActorCredentialScopeTests.cs +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.ControlPlane.Tests/AdminOperationsActorCredentialScopeTests.cs @@ -76,6 +76,44 @@ public sealed class AdminOperationsActorCredentialScopeTests : ControlPlaneActor probe.Calls.ShouldBe(0); } + /// lmxopcua#849: an env: reference named after ANOTHER instance never reaches the probe either. + [Theory] + [InlineData("env:OTOPCUA_GALAXY_VICTIM_API_KEY")] + [InlineData("env:GALAXY_MXGW_DRV_VICTIM_KEY")] + public void An_env_reference_naming_another_instance_never_reaches_the_probe(string reference) + { + 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":"REF"}}""".Replace("REF", reference), + 5, Guid.NewGuid(), "DRV-evil")); + + var reply = ExpectMsg(PresenceBudget, cancellationToken: TestContext.Current.CancellationToken); + reply.Ok.ShouldBeFalse(); + reply.Message.ShouldNotBeNull().ShouldContain("lmxopcua#849"); + probe.Calls.ShouldBe(0); + } + + /// Positive control: a shared env variable naming no instance reaches the probe. + [Fact] + public void A_shared_env_variable_reaches_the_probe() + { + 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":"env:GALAXY_MXGW_API_KEY"}}""", + 5, Guid.NewGuid())); + + ExpectMsg(PresenceBudget, cancellationToken: TestContext.Current.CancellationToken).Ok.ShouldBeTrue(); + probe.Calls.ShouldBe(1); + } + /// Positive control: the owner's own secret (and a shared one) does reach the probe. [Theory] [InlineData("secret:galaxy/DRV-victim/apikey")]