dohertj2/lmxopcua
1
0
Fork 0
Code Issues 4 Pull Requests Actions Packages Projects Releases Wiki Activity

feat(config): reject a persisted Sql connectionString at the deploy gate (#498)

Browse Source 
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.
This commit is contained in:
Joseph Doherty
2026-07-25 15:37:48 -04:00
parent b95b99ba8e
commit 1919a8e538
3 changed files with 273 additions and 2 deletions
Download Patch File Download Diff File Expand all files Collapse all files
src/Core/ZB.MOM.WW.OtOpcUa.Configuration/Validation/DraftValidator.cs
+91
View File
@@ -41,9 +41,100 @@ public static class DraftValidator
ValidateUnsEffectiveLeafUniqueness(draft, errors);
ValidateEquipReferenceResolution(draft, errors);
ValidateCalculationTags(draft, errors);
ValidateSqlConnectionStringNotPersisted(draft, errors);
return errors;
}
/// <summary>
/// The <c>Sql</c> driver's "a pasted literal connection string is never persisted" guarantee, enforced
/// at the deploy gate (Gitea #498).
/// </summary>
/// <remarks>
/// <para>A Sql driver names its credentials indirectly — <c>connectionStringRef</c> resolves from the
/// environment / secret store at Initialize — so a deployed artifact carries no database password.
/// Until now that guarantee rested entirely on the <b>read</b> path: <c>SqlDriverConfigDto</c> has no
/// <c>connectionString</c> property and <c>UnmappedMemberHandling.Skip</c> drops the key on
/// deserialization. Nothing stopped the key being <b>written</b>. 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 <c>{"connectionString":"Server=…;Password=…"}</c> 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.</para>
/// <para>Checked on <b>both</b> config surfaces a Sql driver reads: the instance's
/// <see cref="DriverInstance.DriverConfig"/> and the <see cref="Device.DeviceConfig"/> 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.</para>
/// <para>The key is matched <b>case-insensitively</b>: <c>System.Text.Json</c> binds
/// <c>ConnectionString</c> to a <c>connectionString</c> 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.</para>
/// <para><b>The message never echoes the value.</b> 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.</para>
/// </remarks>
private static void ValidateSqlConnectionStringNotPersisted(DraftSnapshot draft, List<ValidationError> 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));
}
}
/// <summary>
/// True when <paramref name="json"/> is a JSON object carrying <paramref name="key"/> 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.
/// </summary>
/// <param name="json">The config blob to inspect.</param>
/// <param name="key">The property name to look for.</param>
/// <returns><see langword="true"/> when the key is present at the top level.</returns>
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;
}
}
/// <summary>WP7 Calculation-driver deploy gates. For every tag bound to a <c>Calculation</c> driver:
/// <list type="number">
/// <item><b>scriptId existence</b> — the tag's <c>TagConfig.scriptId</c> must be present and resolve to