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); + } +}