From 2193d9f2e48fa49fdb1127fa5952cb7491957989 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Sun, 26 Jul 2026 10:30:04 -0400 Subject: [PATCH] fix(config): refuse to store a Sql credential in a node config override (#499) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `ClusterNode.DriverConfigOverridesJson` is the third surface a Sql driver's config is persisted on, and the only one nothing gated. It is a map keyed by `DriverInstanceId` merged onto the cluster-level `DriverConfig`, so a literal `connectionString` pasted there leaks exactly as one pasted into the driver — the leak #498 closed for `DriverInstance.DriverConfig` and `Device.DeviceConfig`. Gated at the SAVE, not at the deploy — a deliberate departure from both options the issue offered: - `DraftSnapshot` carries no `ClusterNode` rows, and adding them would widen the snapshot and every builder of one for a single rule about a value that never enters the artifact. - More to the point, a deploy gate is the wrong instrument here. Node overrides are not in the artifact, so blocking a deploy would not stop the credential being stored — it is already in the database and replicated by then. Refusing the save is the only point where "refuse to store it" is literally true, which is #498's own framing: discarding a secret on read is not the same as refusing to store it. The check moves into a shared `SqlCredentialGuard` so the two enforcement points cannot drift; `DraftValidator` now calls it instead of its own private helper. Kept deliberately narrow — the `Sql` driver's `connectionString` only, and only for instance ids that ARE Sql drivers. The issue asked whether to generalise to credential-shaped keys across every driver type; not doing that, for the same reason #498 did not: a broader sweep would start refusing configs that are legitimate today for drivers which never made an indirect-credential guarantee, which is a regression rather than defence in depth. Widen per driver, as each gains its own contract. The error names the offending driver instance(s) and NEVER the value — it reaches the AdminUI and the audit trail. 14 new cases cover both halves, including the case variants (System.Text.Json binds `ConnectionString` to `connectionString`, so a case variant is the same key, not a bypass), non-Sql ids being ignored, and every blank/malformed/non-object shape. --- .../Validation/DraftValidator.cs | 39 +---- .../Validation/SqlCredentialGuard.cs | 139 ++++++++++++++++++ .../Components/Pages/Clusters/NodeEdit.razor | 29 ++++ .../SqlCredentialGuardTests.cs | 132 +++++++++++++++++ 4 files changed, 308 insertions(+), 31 deletions(-) create mode 100644 src/Core/ZB.MOM.WW.OtOpcUa.Configuration/Validation/SqlCredentialGuard.cs create mode 100644 tests/Core/ZB.MOM.WW.OtOpcUa.Configuration.Tests/SqlCredentialGuardTests.cs diff --git a/src/Core/ZB.MOM.WW.OtOpcUa.Configuration/Validation/DraftValidator.cs b/src/Core/ZB.MOM.WW.OtOpcUa.Configuration/Validation/DraftValidator.cs index 5b9bc7c7..fde97dbd 100644 --- a/src/Core/ZB.MOM.WW.OtOpcUa.Configuration/Validation/DraftValidator.cs +++ b/src/Core/ZB.MOM.WW.OtOpcUa.Configuration/Validation/DraftValidator.cs @@ -60,6 +60,11 @@ public static class DraftValidator /// credential in the ConfigDb — persisted, replicated to every node's artifact cache, and readable by /// anyone with config access — while the runtime silently ignored it and the driver failed to connect. /// Discarding a secret on read is not the same as refusing to store it. + /// Two of the three surfaces are checked here. The third — a node's + /// — is not reachable from + /// and is gated at its save instead (#499); see + /// for why a deploy gate is the wrong instrument for a value that + /// never enters the artifact. /// Checked on both config surfaces a Sql driver reads: the instance's /// and the of every device /// beneath it, because the two are merged before the DTO sees them — a credential pasted into the @@ -74,7 +79,7 @@ public static class DraftValidator /// private static void ValidateSqlConnectionStringNotPersisted(DraftSnapshot draft, List errors) { - const string ForbiddenKey = "connectionString"; + const string ForbiddenKey = SqlCredentialGuard.ForbiddenKey; var sqlInstanceIds = draft.DriverInstances .Where(d => string.Equals(d.DriverType, Core.Abstractions.DriverTypeNames.Sql, StringComparison.Ordinal)) @@ -85,7 +90,7 @@ public static class DraftValidator foreach (var d in draft.DriverInstances) { if (!sqlInstanceIds.Contains(d.DriverInstanceId)) continue; - if (!HasTopLevelKey(d.DriverConfig, ForbiddenKey)) continue; + if (!SqlCredentialGuard.CarriesLiteralConnectionString(d.DriverConfig)) continue; errors.Add(new("SqlConnectionStringPersisted", $"Sql driver instance '{d.DriverInstanceId}' has a '{ForbiddenKey}' key in its DriverConfig. " + "A Sql driver must name its credentials indirectly via 'connectionStringRef', which resolves " + @@ -98,7 +103,7 @@ public static class DraftValidator foreach (var dev in draft.Devices) { if (!sqlInstanceIds.Contains(dev.DriverInstanceId)) continue; - if (!HasTopLevelKey(dev.DeviceConfig, ForbiddenKey)) continue; + if (!SqlCredentialGuard.CarriesLiteralConnectionString(dev.DeviceConfig)) continue; errors.Add(new("SqlConnectionStringPersisted", $"Device '{dev.DeviceId}' on Sql driver instance '{dev.DriverInstanceId}' has a " + $"'{ForbiddenKey}' key in its DeviceConfig. DeviceConfig is merged onto DriverConfig before " + @@ -107,34 +112,6 @@ public static class DraftValidator } } - /// - /// True when is a JSON object carrying at its top level, - /// matched case-insensitively. Never throws — a blank, malformed or non-object blob simply has no keys, - /// and shaping the config JSON is another rule's job. - /// - /// The config blob to inspect. - /// The property name to look for. - /// when the key is present at the top level. - private static bool HasTopLevelKey(string? json, string key) - { - if (string.IsNullOrWhiteSpace(json)) return false; - try - { - using var doc = System.Text.Json.JsonDocument.Parse(json); - if (doc.RootElement.ValueKind != System.Text.Json.JsonValueKind.Object) return false; - foreach (var property in doc.RootElement.EnumerateObject()) - { - if (string.Equals(property.Name, key, StringComparison.OrdinalIgnoreCase)) return true; - } - - return false; - } - catch (System.Text.Json.JsonException) - { - return false; - } - } - /// WP7 Calculation-driver deploy gates. For every tag bound to a Calculation driver: /// /// scriptId existence — the tag's TagConfig.scriptId must be present and resolve to diff --git a/src/Core/ZB.MOM.WW.OtOpcUa.Configuration/Validation/SqlCredentialGuard.cs b/src/Core/ZB.MOM.WW.OtOpcUa.Configuration/Validation/SqlCredentialGuard.cs new file mode 100644 index 00000000..11ed396d --- /dev/null +++ b/src/Core/ZB.MOM.WW.OtOpcUa.Configuration/Validation/SqlCredentialGuard.cs @@ -0,0 +1,139 @@ +using System.Text.Json; + +namespace ZB.MOM.WW.OtOpcUa.Configuration.Validation; + +/// +/// The one place that decides whether a persisted config blob carries a literal Sql +/// connectionString (Gitea #498 / #499). +/// +/// +/// +/// A Sql driver names its credentials indirectly — connectionStringRef resolves from +/// the environment / secret store at Initialize — so nothing persisted should ever contain a +/// database password. The typed SqlDriverConfigDto already drops a connectionString +/// key on read, but discarding a secret on read is not the same as refusing to store it: +/// config blobs are schemaless JSON columns authored through raw-JSON textareas, so the key can +/// be written even though the runtime ignores it. +/// +/// +/// There are three surfaces a Sql driver's config is persisted on, and they need different +/// enforcement points: +/// +/// DriverInstance.DriverConfig and Device.DeviceConfig — both in the +/// deployed artifact, both visible to DraftSnapshot, both gated by +/// DraftValidator. +/// ClusterNode.DriverConfigOverridesJson — the per-node override map. It is +/// not in DraftSnapshot, and deliberately does not go there: adding a +/// ClusterNode collection would widen the snapshot (and every builder of one) for a +/// single rule about a value that is not part of the artifact at all. More to the point, a +/// deploy gate is the wrong instrument here — the artifact never carries node overrides, so +/// blocking a deploy would not stop the credential being stored. It is already in the +/// database by then. This surface is therefore gated at the save, which is the only +/// point where "refuse to store it" is literally true. +/// +/// +/// +/// Deliberately narrow. This checks the Sql driver's connectionString key +/// only — not a general "credential-shaped key" sweep over every driver type. A broader rule +/// would start refusing configs that are legitimate today for drivers that never made this +/// guarantee, turning defence-in-depth into a regression. Widen it per driver, as each driver +/// gains an indirect-credential contract of its own. +/// +/// +public static class SqlCredentialGuard +{ + /// The config key a Sql driver must never persist. Matched case-insensitively. + public const string ForbiddenKey = "connectionString"; + + /// + /// Whether is a JSON object carrying at + /// its top level. + /// + /// + /// Matched case-insensitively: System.Text.Json binds ConnectionString to a + /// connectionString property by default, so a case variant is the same key, not a + /// different one. Only the top level is scanned — the DTO is flat, so a nested occurrence cannot + /// bind and is not the credential-shaped mistake this exists to catch. + /// + /// The config blob to inspect. + /// when the key is present at the top level. + public static bool CarriesLiteralConnectionString(string? configJson) + { + if (string.IsNullOrWhiteSpace(configJson)) return false; + try + { + using var doc = JsonDocument.Parse(configJson); + return doc.RootElement.ValueKind == JsonValueKind.Object + && HasTopLevelKey(doc.RootElement, ForbiddenKey); + } + catch (JsonException) + { + // A malformed blob simply has no keys. Shaping the config JSON is another rule's job, and + // throwing here would turn a formatting mistake into a credential-guard failure. + return false; + } + } + + /// + /// The DriverInstanceIds in a ClusterNode.DriverConfigOverridesJson map whose + /// override object carries a literal connection string, restricted to instances that are actually + /// Sql drivers. + /// + /// + /// The override map is shaped { "<DriverInstanceId>": { …driver config keys… } } and is + /// merged onto the cluster-level DriverConfig, so a credential pasted into one lands in + /// exactly the place already refuses on the driver + /// itself. + /// + /// The node's override map. Null / blank / malformed yields no + /// violations. + /// The ids of driver instances whose DriverType is + /// Sql. Keys outside this set are ignored — a non-Sql driver never made the indirect-credential + /// guarantee, so flagging it would be a regression, not defence in depth. + /// The offending driver-instance ids, in document order; empty when there are none. + public static IReadOnlyList FindNodeOverrideViolations( + string? overridesJson, IReadOnlyCollection sqlDriverInstanceIds) + { + if (string.IsNullOrWhiteSpace(overridesJson) || sqlDriverInstanceIds.Count == 0) + { + return []; + } + + var sqlIds = sqlDriverInstanceIds as IReadOnlySet + ?? sqlDriverInstanceIds.ToHashSet(StringComparer.Ordinal); + + try + { + using var doc = JsonDocument.Parse(overridesJson); + if (doc.RootElement.ValueKind != JsonValueKind.Object) return []; + + List? offenders = null; + foreach (var entry in doc.RootElement.EnumerateObject()) + { + if (!sqlIds.Contains(entry.Name)) continue; + if (entry.Value.ValueKind != JsonValueKind.Object) continue; + if (!HasTopLevelKey(entry.Value, ForbiddenKey)) continue; + + (offenders ??= []).Add(entry.Name); + } + + return (IReadOnlyList?)offenders ?? []; + } + catch (JsonException) + { + return []; + } + } + + /// Whether carries at its top level, + /// case-insensitively. + private static bool HasTopLevelKey(JsonElement obj, string key) + { + foreach (var property in obj.EnumerateObject()) + { + if (string.Equals(property.Name, key, StringComparison.OrdinalIgnoreCase)) return true; + } + + return false; + } +} diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Pages/Clusters/NodeEdit.razor b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Pages/Clusters/NodeEdit.razor index 2c1fef76..8f663bb5 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Pages/Clusters/NodeEdit.razor +++ b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Pages/Clusters/NodeEdit.razor @@ -9,6 +9,8 @@ @using System.ComponentModel.DataAnnotations @using ZB.MOM.WW.OtOpcUa.Configuration @using ZB.MOM.WW.OtOpcUa.Configuration.Entities +@using ZB.MOM.WW.OtOpcUa.Configuration.Validation +@using ZB.MOM.WW.OtOpcUa.Core.Abstractions @inject IDbContextFactory DbFactory @inject NavigationManager Nav @inject AuthenticationStateProvider AuthState @@ -178,6 +180,33 @@ else } await using var db = await DbFactory.CreateDbContextAsync(); + + // #499 — the third Sql-credential surface. DriverConfigOverridesJson is merged onto the + // cluster-level DriverConfig, so a literal connectionString pasted here leaks exactly as one + // pasted into the driver itself. The deploy gate cannot see it (DraftSnapshot carries no + // ClusterNode rows) and would be the wrong place anyway: node overrides never enter the + // artifact, so by the time a deploy ran the credential would already be stored. Refuse the + // save instead. The message names the driver instance but NEVER the value. + if (!string.IsNullOrWhiteSpace(_form.DriverConfigOverridesJson)) + { + var sqlDriverIds = await db.DriverInstances + .Where(d => d.ClusterId == ClusterId && d.DriverType == DriverTypeNames.Sql) + .Select(d => d.DriverInstanceId) + .ToListAsync(); + + var offenders = SqlCredentialGuard.FindNodeOverrideViolations( + _form.DriverConfigOverridesJson, sqlDriverIds); + if (offenders.Count > 0) + { + _error = + $"Driver config overrides carry a '{SqlCredentialGuard.ForbiddenKey}' for Sql driver " + + $"instance(s) {string.Join(", ", offenders)}. A Sql driver must name its credentials " + + "indirectly via 'connectionStringRef', which resolves from the environment / secret " + + "store at Initialize; a literal connection string here would be stored in the config " + + "database, and the runtime ignores it anyway. Remove the key."; + return; + } + } if (IsNew) { if (await db.ClusterNodes.AnyAsync(n => n.NodeId == _form.NodeId)) diff --git a/tests/Core/ZB.MOM.WW.OtOpcUa.Configuration.Tests/SqlCredentialGuardTests.cs b/tests/Core/ZB.MOM.WW.OtOpcUa.Configuration.Tests/SqlCredentialGuardTests.cs new file mode 100644 index 00000000..6cc1e749 --- /dev/null +++ b/tests/Core/ZB.MOM.WW.OtOpcUa.Configuration.Tests/SqlCredentialGuardTests.cs @@ -0,0 +1,132 @@ +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.Configuration.Validation; + +namespace ZB.MOM.WW.OtOpcUa.Configuration.Tests; + +/// +/// — the shared "a Sql driver never persists a literal connection +/// string" check behind both the #498 deploy gate and the #499 node-override save gate. +/// +/// +/// The node-override half is the one #499 is about: ClusterNode.DriverConfigOverridesJson is a +/// map keyed by DriverInstanceId that is merged onto the cluster-level DriverConfig, and +/// it is invisible to DraftSnapshot — so nothing gated it at all before. +/// +[Trait("Category", "Unit")] +public sealed class SqlCredentialGuardTests +{ + /// The literal an operator would paste into a raw-JSON textarea. + private const string Leaked = + """{"provider":"SqlServer","connectionString":"Server=sql,1433;Database=Mes;User ID=sa;Password=hunter2"}"""; + + private const string Clean = """{"provider":"SqlServer","connectionStringRef":"Sql:Line3"}"""; + + // ---- CarriesLiteralConnectionString -------------------------------------------------------- + + /// The key is caught at the top level of a config blob. + [Fact] + public void A_literal_connectionString_is_detected() + => SqlCredentialGuard.CarriesLiteralConnectionString(Leaked).ShouldBeTrue(); + + /// The supported indirect form is not a violation — otherwise the rule would block the fix + /// it tells operators to apply. + [Fact] + public void The_indirect_connectionStringRef_form_is_clean() + => SqlCredentialGuard.CarriesLiteralConnectionString(Clean).ShouldBeFalse(); + + /// System.Text.Json binds ConnectionString to a connectionString property by + /// default, so a case variant is the same key — not a bypass. + [Theory] + [InlineData("""{"ConnectionString":"Server=x;Password=p"}""")] + [InlineData("""{"CONNECTIONSTRING":"Server=x;Password=p"}""")] + [InlineData("""{"connectionstring":"Server=x;Password=p"}""")] + public void Case_variants_are_the_same_key(string json) + => SqlCredentialGuard.CarriesLiteralConnectionString(json).ShouldBeTrue(); + + /// Blank, malformed and non-object blobs simply have no keys. Shaping the config JSON is + /// another rule's job, and throwing here would turn a formatting mistake into a credential failure. + /// + [Theory] + [InlineData(null)] + [InlineData("")] + [InlineData(" ")] + [InlineData("{ not json")] + [InlineData("[1,2,3]")] + [InlineData("\"a string\"")] + public void Blank_malformed_and_non_object_blobs_are_not_violations(string? json) + => SqlCredentialGuard.CarriesLiteralConnectionString(json).ShouldBeFalse(); + + /// Only the top level is scanned. The DTO is flat, so a nested occurrence cannot bind and is + /// not the credential-shaped mistake this rule exists to catch. + [Fact] + public void A_nested_occurrence_is_not_flagged() + => SqlCredentialGuard + .CarriesLiteralConnectionString("""{"nested":{"connectionString":"Server=x"}}""") + .ShouldBeFalse(); + + // ---- FindNodeOverrideViolations (#499) ----------------------------------------------------- + + /// The #499 leak: a credential pasted into a node's per-driver override map. + [Fact] + public void A_credential_in_a_node_override_for_a_Sql_driver_is_a_violation() + { + var overrides = $$"""{"di-sql": {{Leaked}} }"""; + + SqlCredentialGuard.FindNodeOverrideViolations(overrides, ["di-sql"]) + .ShouldBe(["di-sql"]); + } + + /// Every offending instance is reported, not just the first — an operator fixing one at a + /// time would otherwise need as many save attempts as there are leaks. + [Fact] + public void Every_offending_instance_is_reported() + { + var overrides = $$"""{"di-a": {{Leaked}}, "di-clean": {{Clean}}, "di-b": {{Leaked}} }"""; + + SqlCredentialGuard.FindNodeOverrideViolations(overrides, ["di-a", "di-b", "di-clean"]) + .ShouldBe(["di-a", "di-b"]); + } + + /// Keys outside the Sql set are ignored. A non-Sql driver never made the + /// indirect-credential guarantee, so flagging it would break a config that is legitimate today — + /// a regression, not defence in depth. + [Fact] + public void An_override_for_a_non_Sql_driver_is_ignored() + { + var overrides = $$"""{"di-modbus": {{Leaked}} }"""; + + SqlCredentialGuard.FindNodeOverrideViolations(overrides, ["di-sql"]).ShouldBeEmpty(); + } + + /// The realistic override — a node-specific endpoint — must keep saving. + [Fact] + public void A_normal_override_is_clean() + { + var overrides = """{"di-sql": {"commandTimeoutSeconds": 45}}"""; + + SqlCredentialGuard.FindNodeOverrideViolations(overrides, ["di-sql"]).ShouldBeEmpty(); + } + + /// Nothing to check when the cluster has no Sql drivers, or the node has no overrides. + [Theory] + [InlineData(null)] + [InlineData("")] + [InlineData(" ")] + [InlineData("{ not json")] + [InlineData("[1,2,3]")] + public void Blank_and_malformed_override_maps_yield_no_violations(string? overrides) + => SqlCredentialGuard.FindNodeOverrideViolations(overrides, ["di-sql"]).ShouldBeEmpty(); + + /// An empty Sql-driver set short-circuits — there is no instance the key could belong to. + [Fact] + public void No_Sql_drivers_means_no_violations() + => SqlCredentialGuard.FindNodeOverrideViolations($$"""{"di-sql": {{Leaked}} }""", []) + .ShouldBeEmpty(); + + /// A non-object override entry cannot carry config keys, and must not throw. + [Fact] + public void A_non_object_override_entry_is_skipped() + => SqlCredentialGuard.FindNodeOverrideViolations("""{"di-sql": "connectionString"}""", ["di-sql"]) + .ShouldBeEmpty(); +}