From 1919a8e5382c6cfe7f4cfa2d7a6b97df0d6c7a65 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Sat, 25 Jul 2026 15:37:48 -0400 Subject: [PATCH] feat(config): reject a persisted Sql connectionString at the deploy gate (#498) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Sql driver's "a pasted literal connection string is never persisted" guarantee was enforced only on the READ path: SqlDriverConfigDto has no connectionString property and UnmappedMemberHandling.Skip drops the key on deserialization. Nothing stopped it being WRITTEN. Config blobs are schemaless JSON columns and the fallback authoring pattern for a driver without a typed form is a raw-JSON textarea, so an operator pasting {"connectionString":"Server=...;Password=..."} would put a live credential in the ConfigDb — persisted, replicated to every node's artifact cache, 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. DraftValidator.ValidateSqlConnectionStringNotPersisted fails the deploy (SqlConnectionStringPersisted) when a Sql-typed driver instance's DriverConfig, or the DeviceConfig of any device beneath it, carries a top-level connectionString key. - Both surfaces are checked because DeviceConfig is merged onto DriverConfig before the DTO sees it — a credential pasted into the device blob is the identical leak. - The key is matched case-insensitively: System.Text.Json binds ConnectionString to a connectionString property by default, so an ordinal match would leave the obvious bypass open. - Only the top level is scanned; the DTO is flat, so a nested occurrence cannot bind and is not the mistake this rule exists to catch. - The message never echoes the value — validation errors reach the AdminUI, the deploy log and the audit trail, and repeating the string there would leak the credential the rule is refusing to store. Pinned by a test. - Malformed/blank/non-object JSON never throws: a parse failure would take down every other rule in the same pass. 16 tests. Verified falsifiable — with the rule disabled the 6 positive assertions fail and the 10 negatives stay green, so none of them passes vacuously. Design §8.2's "if an inline connectionString is ever permitted (dev convenience only), the AdminUI redacts it" carve-out is withdrawn in the same change: redaction only hides a credential that has already been written. Not covered, and filed as a follow-up: ClusterNode.DriverConfigOverridesJson is a third persistence surface for a driver config blob, but it is not part of DraftSnapshot, so this validator cannot see it. --- .../2026-07-15-sql-poll-driver-design.md | 11 +- .../Validation/DraftValidator.cs | 91 +++++++++ .../DraftValidatorSqlSecretTests.cs | 173 ++++++++++++++++++ 3 files changed, 273 insertions(+), 2 deletions(-) create mode 100644 tests/Core/ZB.MOM.WW.OtOpcUa.Configuration.Tests/DraftValidatorSqlSecretTests.cs diff --git a/docs/plans/2026-07-15-sql-poll-driver-design.md b/docs/plans/2026-07-15-sql-poll-driver-design.md index 5e9df5ec..f125f91f 100644 --- a/docs/plans/2026-07-15-sql-poll-driver-design.md +++ b/docs/plans/2026-07-15-sql-poll-driver-design.md @@ -607,8 +607,15 @@ comment) and the env-overridable ConfigDb connection string: file. Direct env read (not `IConfiguration`) is deliberate: the factory registry materializes drivers via a static `(id, json)` closure (Modbus pattern) with no `IConfiguration` in reach, so this keeps the factory shape unchanged. -- If an inline `connectionString` is ever permitted (dev convenience only), the AdminUI **redacts** it - in display/logging and flags it dev-only. +- **An inline `connectionString` is not permitted at all** — the earlier "dev convenience, redacted in + the UI" carve-out is withdrawn (Gitea #498). Redaction only hides a credential that has *already been + written* to the config DB and replicated to every node's artifact cache. `SqlDriverConfigDto` has no + such property and `UnmappedMemberHandling.Skip` drops the key on read, but that protects only the read + path; config blobs are schemaless JSON columns, so nothing stopped the key being **written**. + `DraftValidator.ValidateSqlConnectionStringNotPersisted` now fails the deploy + (`SqlConnectionStringPersisted`) for a `connectionString` key at the top level of a Sql driver's + `DriverConfig` **or** of the `DeviceConfig` of any device beneath it (the two are merged before the DTO + sees them). The key is matched case-insensitively, and the error text never echoes the value. - **Never log the resolved connection string.** Log only provider + server host + database name. - Prefer integrated/managed auth where the estate supports it (`Integrated Security=true` / Azure AD / Kerberos) so no password transits config at all. 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 11d6e08c..5b9bc7c7 100644 --- a/src/Core/ZB.MOM.WW.OtOpcUa.Configuration/Validation/DraftValidator.cs +++ b/src/Core/ZB.MOM.WW.OtOpcUa.Configuration/Validation/DraftValidator.cs @@ -41,9 +41,100 @@ public static class DraftValidator ValidateUnsEffectiveLeafUniqueness(draft, errors); ValidateEquipReferenceResolution(draft, errors); ValidateCalculationTags(draft, errors); + ValidateSqlConnectionStringNotPersisted(draft, errors); return errors; } + /// + /// The Sql driver's "a pasted literal connection string is never persisted" guarantee, enforced + /// at the deploy gate (Gitea #498). + /// + /// + /// A Sql driver names its credentials indirectly — connectionStringRef resolves from the + /// environment / secret store at Initialize — so a deployed artifact carries no database password. + /// Until now that guarantee rested entirely on the read path: SqlDriverConfigDto has no + /// connectionString property and UnmappedMemberHandling.Skip drops the key on + /// deserialization. Nothing stopped the key being written. Config blobs are schemaless JSON + /// columns, and the fallback authoring pattern for a driver without a typed form is a raw-JSON + /// textarea, so an operator pasting {"connectionString":"Server=…;Password=…"} would put a live + /// 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. + /// 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 + /// device blob lands in exactly the same place. + /// The key is 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 rule exists to catch. + /// The message never echoes the value. Validation errors reach the AdminUI, the deploy + /// log and the audit trail; repeating the offending string there would leak the very credential the + /// rule is refusing to store. + /// + private static void ValidateSqlConnectionStringNotPersisted(DraftSnapshot draft, List errors) + { + const string ForbiddenKey = "connectionString"; + + var sqlInstanceIds = draft.DriverInstances + .Where(d => string.Equals(d.DriverType, Core.Abstractions.DriverTypeNames.Sql, StringComparison.Ordinal)) + .Select(d => d.DriverInstanceId) + .ToHashSet(StringComparer.Ordinal); + if (sqlInstanceIds.Count == 0) return; + + foreach (var d in draft.DriverInstances) + { + if (!sqlInstanceIds.Contains(d.DriverInstanceId)) continue; + if (!HasTopLevelKey(d.DriverConfig, ForbiddenKey)) 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 " + + "from the environment / secret store at Initialize; a literal connection string here would be " + + "stored in the config database and replicated to every node, and the runtime ignores it anyway. " + + "Remove the key and set 'connectionStringRef'.", + d.DriverInstanceId)); + } + + foreach (var dev in draft.Devices) + { + if (!sqlInstanceIds.Contains(dev.DriverInstanceId)) continue; + if (!HasTopLevelKey(dev.DeviceConfig, ForbiddenKey)) 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 " + + "the driver reads it, so this is the same leak: use 'connectionStringRef' on the driver instead.", + dev.DeviceId)); + } + } + + /// + /// 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/tests/Core/ZB.MOM.WW.OtOpcUa.Configuration.Tests/DraftValidatorSqlSecretTests.cs b/tests/Core/ZB.MOM.WW.OtOpcUa.Configuration.Tests/DraftValidatorSqlSecretTests.cs new file mode 100644 index 00000000..e2630790 --- /dev/null +++ b/tests/Core/ZB.MOM.WW.OtOpcUa.Configuration.Tests/DraftValidatorSqlSecretTests.cs @@ -0,0 +1,173 @@ +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.Configuration.Entities; +using ZB.MOM.WW.OtOpcUa.Configuration.Validation; + +namespace ZB.MOM.WW.OtOpcUa.Configuration.Tests; + +/// +/// The Gitea #498 deploy gate: a Sql driver's persisted config may never carry a literal +/// connectionString. The typed DTO already drops the key on read; these pin the +/// write side, which is where a credential would actually land in the config database. +/// +[Trait("Category", "Unit")] +public sealed class DraftValidatorSqlSecretTests +{ + private const string Code = "SqlConnectionStringPersisted"; + + /// A realistic leak: the literal an operator would paste into a raw-JSON config textarea. + private const string LeakedConfig = + """{"provider":"SqlServer","connectionString":"Server=sql,1433;Database=Mes;User ID=sa;Password=hunter2"}"""; + + private static DriverInstance SqlDriver(string config) => new() + { + DriverInstanceId = "di-sql", ClusterId = "c", Name = "line3-sql", + DriverType = "Sql", DriverConfig = config, + }; + + private static Device Device(string driverInstanceId, string config) => new() + { + DeviceId = "dev-1", DriverInstanceId = driverInstanceId, Name = "Device1", DeviceConfig = config, + }; + + private static DraftSnapshot Draft(DriverInstance driver, Device? device = null) => new() + { + GenerationId = 1, + ClusterId = "c", + DriverInstances = [driver], + Devices = device is null ? [] : [device], + }; + + [Fact] + public void Literal_connectionString_in_DriverConfig_is_a_deploy_error() + { + var errors = DraftValidator.Validate(Draft(SqlDriver(LeakedConfig))); + + errors.ShouldContain(e => e.Code == Code && e.Context == "di-sql"); + } + + /// + /// The error text reaches the AdminUI, the deploy log and the audit trail, so it must describe the + /// problem without repeating the credential it is refusing to store. + /// + [Fact] + public void Error_message_does_not_echo_the_credential() + { + var error = DraftValidator.Validate(Draft(SqlDriver(LeakedConfig))).First(e => e.Code == Code); + + error.Message.ShouldNotContain("hunter2"); + error.Message.ShouldNotContain("Server=sql,1433"); + error.Message.ShouldContain("connectionStringRef"); + } + + /// + /// System.Text.Json binds ConnectionString to a connectionString property by default, so + /// a case variant is the same key — matching it ordinally would leave the obvious bypass wide open. + /// + [Theory] + [InlineData("ConnectionString")] + [InlineData("CONNECTIONSTRING")] + [InlineData("connectionstring")] + public void Key_match_is_case_insensitive(string key) + { + var config = $$"""{"provider":"SqlServer","{{key}}":"Server=s;Password=p"}"""; + + DraftValidator.Validate(Draft(SqlDriver(config))) + .ShouldContain(e => e.Code == Code && e.Context == "di-sql"); + } + + /// + /// DeviceConfig is merged onto DriverConfig before the driver's DTO sees it, so a credential pasted + /// there is the identical leak and must fail the same way. + /// + [Fact] + public void Literal_connectionString_in_a_Sql_devices_DeviceConfig_is_a_deploy_error() + { + var draft = Draft(SqlDriver("""{"connectionStringRef":"DevSql"}"""), Device("di-sql", LeakedConfig)); + + DraftValidator.Validate(draft).ShouldContain(e => e.Code == Code && e.Context == "dev-1"); + } + + [Fact] + public void A_properly_authored_connectionStringRef_passes() + { + var draft = Draft( + SqlDriver("""{"provider":"SqlServer","connectionStringRef":"DevSql","nullIsBad":true}"""), + Device("di-sql", """{"pollIntervalMs":1000}""")); + + DraftValidator.Validate(draft).ShouldNotContain(e => e.Code == Code); + } + + /// + /// The rule is scoped to the Sql driver type. A non-Sql driver is not in its remit — widening the gate + /// to every driver is a separate decision, and silently failing an unrelated driver's deploy here would + /// be a regression, not defence in depth. + /// + [Fact] + public void A_non_Sql_driver_carrying_the_key_is_not_flagged_by_this_rule() + { + var modbus = new DriverInstance + { + DriverInstanceId = "di-mb", ClusterId = "c", Name = "mb", + DriverType = "Modbus", DriverConfig = LeakedConfig, + }; + + DraftValidator.Validate(Draft(modbus)).ShouldNotContain(e => e.Code == Code); + } + + /// + /// A device under a different driver must not be attributed to the Sql instance — the device + /// scan keys off DriverInstanceId, and getting that wrong would flag innocent devices. + /// + [Fact] + public void A_device_under_a_non_Sql_driver_is_not_flagged() + { + var draft = new DraftSnapshot + { + GenerationId = 1, + ClusterId = "c", + DriverInstances = + [ + SqlDriver("""{"connectionStringRef":"DevSql"}"""), + new DriverInstance + { + DriverInstanceId = "di-mb", ClusterId = "c", Name = "mb", + DriverType = "Modbus", DriverConfig = "{}", + }, + ], + Devices = [Device("di-mb", LeakedConfig)], + }; + + DraftValidator.Validate(draft).ShouldNotContain(e => e.Code == Code); + } + + /// + /// Malformed or non-object config must not throw out of the validator: shaping the JSON is another + /// rule's job, and a parse failure here would take down every other check in the same pass. + /// + [Theory] + [InlineData("")] + [InlineData(" ")] + [InlineData("not json at all")] + [InlineData("[1,2,3]")] + [InlineData("\"connectionString\"")] + [InlineData("{\"provider\":\"SqlServer\"")] + public void Malformed_config_neither_throws_nor_flags(string config) + { + Should.NotThrow(() => DraftValidator.Validate(Draft(SqlDriver(config)))) + .ShouldNotContain(e => e.Code == Code); + } + + /// + /// Only the top level is scanned. The DTO is flat, so a nested occurrence cannot bind to anything and + /// is not the credential-shaped mistake this rule exists to catch; flagging it would be a false + /// positive on, say, a tag blob that happens to describe a connection string. + /// + [Fact] + public void A_nested_connectionString_is_not_flagged() + { + var config = """{"connectionStringRef":"DevSql","notes":{"connectionString":"documented elsewhere"}}"""; + + DraftValidator.Validate(Draft(SqlDriver(config))).ShouldNotContain(e => e.Code == Code); + } +}