From d68dff094609483e86f5a970cfa5acaa243fc8ea Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Wed, 11 Nov 2026 06:35:52 -0500 Subject: [PATCH] fix(security): id-form credential references match by id only; id-shaped driver names refused (#849 review) A driver named 'drv' (env form DRV) matched every sibling's id-form variable env:DRV_<12HEX>_... as 'another instance', breaking the recommended id convention fleet-wide - including for a name that pre-dates the rule. ReferenceNames is split into ReferenceNamesId / ReferenceNamesName; a reference whose instance part is id-shaped (secret: DRV-<12 hex>, env: DRV_<12 HEX>[_...]) is matched by id only. DescribeNameChangeProblemsAsync also refuses new names whose env form is DRV or starts with an id form (IsReservedInstanceName) at create, /raw rename and config-modal update. Co-Authored-By: Claude --- .../Validation/DriverInstanceIdentities.cs | 23 ++++-- .../DriverCredentialPolicy.cs | 82 +++++++++++++++++-- ...ValidatorCredentialInstanceAndPathTests.cs | 8 ++ .../CredentialInstanceScopeTests.cs | 65 ++++++++++++++- .../Uns/CredentialNameClaimSavePathTests.cs | 24 ++++++ 5 files changed, 186 insertions(+), 16 deletions(-) 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 600241ba4..cd3b0e365 100644 --- a/src/Core/ZB.MOM.WW.OtOpcUa.Configuration/Validation/DriverInstanceIdentities.cs +++ b/src/Core/ZB.MOM.WW.OtOpcUa.Configuration/Validation/DriverInstanceIdentities.cs @@ -89,7 +89,7 @@ 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 (): + /// credential reference (): /// /// /// Claim — the new name may not be named by a reference STORED on another same-type driver (its @@ -123,6 +123,19 @@ public static class DriverInstanceIdentities 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 }) @@ -154,7 +167,7 @@ public static class DriverInstanceIdentities var where = device is null ? "" : $" device '{device}'"; foreach (var reference in DriverCredentialPolicy.CollectInstanceReferences(driverType, json)) { - if (!own && DriverCredentialPolicy.ReferenceNames(reference, newName)) + if (!own && DriverCredentialPolicy.ReferenceNamesName(reference, newName)) { problems.Add( $"The name '{newName}' is the instance part of the credential reference {reference.FieldPath} on " @@ -163,9 +176,9 @@ public static class DriverInstanceIdentities + "Choose another name, or move that reference first (lmxopcua#848)."); } else if (own && currentName is not null - && DriverCredentialPolicy.ReferenceNames(reference, currentName) - && !DriverCredentialPolicy.ReferenceNames(reference, ownDriverInstanceId) - && !DriverCredentialPolicy.ReferenceNames(reference, newName)) + && 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 " 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 33a238a0e..3060b43ef 100644 --- a/src/Core/ZB.MOM.WW.OtOpcUa.Core.Abstractions/DriverCredentialPolicy.cs +++ b/src/Core/ZB.MOM.WW.OtOpcUa.Core.Abstractions/DriverCredentialPolicy.cs @@ -236,7 +236,7 @@ public static class DriverCredentialPolicy /// 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:. + /// with it followed by _ (see / ). Null for anything but an in-scope env:. /// /// The authored field value. /// The driver's scope. @@ -269,15 +269,81 @@ public static class DriverCredentialPolicy }); } + /// 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 or name - /// : a secret: segment equal to it (case-insensitive), or an env: part - /// equal to its form or starting with that form plus _. + /// 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 id or name. + /// A driver instance id. /// Whether it names it. - public static bool ReferenceNames(InstanceReference reference, string? identifier) + 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) @@ -300,7 +366,7 @@ public static class DriverCredentialPolicy CollectInstanceReferences(driverType, json).Count > 0; /// - /// lmxopcua#835/#849: inside one namespace, a reference that names (see ) a DIFFERENT + /// 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 @@ -333,7 +399,7 @@ public static class DriverCredentialPolicy .ToList(); foreach (var reference in references) { - var other = others.FirstOrDefault(o => ReferenceNames(reference, o.DriverInstanceId) || ReferenceNames(reference, o.Name)); + var other = others.FirstOrDefault(o => ReferenceNamesId(reference, o.DriverInstanceId) || ReferenceNamesName(reference, o.Name)); if (other is null) continue; problems.Add(reference.Kind == CredentialReferenceKind.Secret ? $"{reference.FieldPath} names a secret under '{reference.Part}/', which is another {rules.Scope.Label} driver " 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 e26637fd4..e7b1e4713 100644 --- a/tests/Core/ZB.MOM.WW.OtOpcUa.Configuration.Tests/DraftValidatorCredentialInstanceAndPathTests.cs +++ b/tests/Core/ZB.MOM.WW.OtOpcUa.Configuration.Tests/DraftValidatorCredentialInstanceAndPathTests.cs @@ -71,6 +71,14 @@ public sealed class DraftValidatorCredentialInstanceAndPathTests 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 6d0ab5541..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 @@ -181,8 +181,67 @@ public sealed class CredentialInstanceScopeTests 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.ReferenceNames(refs[1], "ack").ShouldBeTrue(); - DriverCredentialPolicy.ReferenceNames(refs[1], "ac").ShouldBeFalse(); - DriverCredentialPolicy.ReferenceNames(refs[0], "GW").ShouldBeTrue(); + 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/Uns/CredentialNameClaimSavePathTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Uns/CredentialNameClaimSavePathTests.cs index 53bc3bcce..07bea8100 100644 --- a/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Uns/CredentialNameClaimSavePathTests.cs +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Uns/CredentialNameClaimSavePathTests.cs @@ -140,4 +140,28 @@ public sealed class CredentialNameClaimSavePathTests 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(); + } }