From d08e59eea7712290cf8d7c6d45deb9cad37d9489 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Wed, 11 Nov 2026 06:24:59 -0500 Subject: [PATCH] fix(security): a driver name may not claim another driver's credential segment, nor be renamed off its own (#848) RawTreeService.CreateDriverAsync / RenameDriverAsync / UpdateDriverAsync run DriverConfigSaveGate.ValidateNameAsync -> DriverInstanceIdentities.DescribeNameChangeProblemsAsync: - claim: the new name may not be the instance part (secret: segment or env: part) of a reference STORED on another same-type driver's DriverConfig or DeviceConfig - naming a driver 'gw' used to fail every sharer's next deploy; - pin: a driver may not be renamed away from a name its own references are named after (unless they also name its id or the new name) - the rename would free that name for every other driver's save / Test Connect / browse. Re-pointing to the id in the same config-modal save is allowed; a case-only rename is never refused. Messages name the other driver, id, device and field path, never a value. The driver + device reads are pinned on SQL Server by a [SqlServerFact]. Co-Authored-By: Claude --- .../Validation/DriverInstanceIdentities.cs | 98 +++++++++++- .../Uns/DriverConfigSaveGate.cs | 26 +++- .../Uns/RawTreeService.cs | 15 ++ .../Uns/CredentialNameClaimSavePathTests.cs | 143 ++++++++++++++++++ .../Uns/CredentialNameClaimSqlServerTests.cs | 67 ++++++++ 5 files changed, 347 insertions(+), 2 deletions(-) create mode 100644 tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Uns/CredentialNameClaimSavePathTests.cs create mode 100644 tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Uns/CredentialNameClaimSqlServerTests.cs 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..600241ba4 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,105 @@ 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 []; + + // 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.ReferenceNames(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.ReferenceNames(reference, currentName) + && !DriverCredentialPolicy.ReferenceNames(reference, ownDriverInstanceId) + && !DriverCredentialPolicy.ReferenceNames(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/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/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..53bc3bcce --- /dev/null +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Uns/CredentialNameClaimSavePathTests.cs @@ -0,0 +1,143 @@ +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); + } +} 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(); + } +}