fix(config): refuse to store a Sql credential in a node config override (#499)
`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.
This commit is contained in:
Joseph Doherty
@@ -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.</para>
|
||||
/// <para><b>Two of the three surfaces are checked here.</b> The third — a node's
|
||||
/// <see cref="ClusterNode.DriverConfigOverridesJson"/> — is not reachable from
|
||||
/// <see cref="DraftSnapshot"/> and is gated at its save instead (#499); see
|
||||
/// <see cref="SqlCredentialGuard"/> for why a deploy gate is the wrong instrument for a value that
|
||||
/// never enters the artifact.</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
|
||||
@@ -74,7 +79,7 @@ public static class DraftValidator
|
||||
/// </remarks>
|
||||
private static void ValidateSqlConnectionStringNotPersisted(DraftSnapshot draft, List<ValidationError> 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
|
||||
}
|
||||
}
|
||||
|
||||
/// <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
|
||||
|
||||
@@ -0,0 +1,139 @@
|
||||
using System.Text.Json;
|
||||
|
||||
namespace ZB.MOM.WW.OtOpcUa.Configuration.Validation;
|
||||
|
||||
/// <summary>
|
||||
/// The one place that decides whether a persisted config blob carries a literal Sql
|
||||
/// <c>connectionString</c> (Gitea #498 / #499).
|
||||
/// </summary>
|
||||
/// <remarks>
|
||||
/// <para>
|
||||
/// A <c>Sql</c> driver names its credentials indirectly — <c>connectionStringRef</c> resolves from
|
||||
/// the environment / secret store at Initialize — so nothing persisted should ever contain a
|
||||
/// database password. The typed <c>SqlDriverConfigDto</c> already drops a <c>connectionString</c>
|
||||
/// key on <b>read</b>, 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.
|
||||
/// </para>
|
||||
/// <para>
|
||||
/// There are <b>three</b> surfaces a Sql driver's config is persisted on, and they need different
|
||||
/// enforcement points:
|
||||
/// <list type="bullet">
|
||||
/// <item><c>DriverInstance.DriverConfig</c> and <c>Device.DeviceConfig</c> — both in the
|
||||
/// deployed artifact, both visible to <c>DraftSnapshot</c>, both gated by
|
||||
/// <c>DraftValidator</c>.</item>
|
||||
/// <item><c>ClusterNode.DriverConfigOverridesJson</c> — the per-node override map. It is
|
||||
/// <b>not</b> in <c>DraftSnapshot</c>, and deliberately does not go there: adding a
|
||||
/// <c>ClusterNode</c> 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 <b>save</b>, which is the only
|
||||
/// point where "refuse to store it" is literally true.</item>
|
||||
/// </list>
|
||||
/// </para>
|
||||
/// <para>
|
||||
/// <b>Deliberately narrow.</b> This checks the <c>Sql</c> driver's <c>connectionString</c> 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.
|
||||
/// </para>
|
||||
/// </remarks>
|
||||
public static class SqlCredentialGuard
|
||||
{
|
||||
/// <summary>The config key a Sql driver must never persist. Matched case-insensitively.</summary>
|
||||
public const string ForbiddenKey = "connectionString";
|
||||
|
||||
/// <summary>
|
||||
/// Whether <paramref name="configJson"/> is a JSON object carrying <see cref="ForbiddenKey"/> at
|
||||
/// its top level.
|
||||
/// </summary>
|
||||
/// <remarks>
|
||||
/// 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 exists to catch.
|
||||
/// </remarks>
|
||||
/// <param name="configJson">The config blob to inspect.</param>
|
||||
/// <returns><see langword="true"/> when the key is present at the top level.</returns>
|
||||
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;
|
||||
}
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// The <c>DriverInstanceId</c>s in a <c>ClusterNode.DriverConfigOverridesJson</c> map whose
|
||||
/// override object carries a literal connection string, restricted to instances that are actually
|
||||
/// Sql drivers.
|
||||
/// </summary>
|
||||
/// <remarks>
|
||||
/// The override map is shaped <c>{ "<DriverInstanceId>": { …driver config keys… } }</c> and is
|
||||
/// merged onto the cluster-level <c>DriverConfig</c>, so a credential pasted into one lands in
|
||||
/// exactly the place <see cref="CarriesLiteralConnectionString"/> already refuses on the driver
|
||||
/// itself.
|
||||
/// </remarks>
|
||||
/// <param name="overridesJson">The node's override map. Null / blank / malformed yields no
|
||||
/// violations.</param>
|
||||
/// <param name="sqlDriverInstanceIds">The ids of driver instances whose <c>DriverType</c> is
|
||||
/// <c>Sql</c>. 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.</param>
|
||||
/// <returns>The offending driver-instance ids, in document order; empty when there are none.</returns>
|
||||
public static IReadOnlyList<string> FindNodeOverrideViolations(
|
||||
string? overridesJson, IReadOnlyCollection<string> sqlDriverInstanceIds)
|
||||
{
|
||||
if (string.IsNullOrWhiteSpace(overridesJson) || sqlDriverInstanceIds.Count == 0)
|
||||
{
|
||||
return [];
|
||||
}
|
||||
|
||||
var sqlIds = sqlDriverInstanceIds as IReadOnlySet<string>
|
||||
?? sqlDriverInstanceIds.ToHashSet(StringComparer.Ordinal);
|
||||
|
||||
try
|
||||
{
|
||||
using var doc = JsonDocument.Parse(overridesJson);
|
||||
if (doc.RootElement.ValueKind != JsonValueKind.Object) return [];
|
||||
|
||||
List<string>? 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<string>?)offenders ?? [];
|
||||
}
|
||||
catch (JsonException)
|
||||
{
|
||||
return [];
|
||||
}
|
||||
}
|
||||
|
||||
/// <summary>Whether <paramref name="obj"/> carries <paramref name="key"/> at its top level,
|
||||
/// case-insensitively.</summary>
|
||||
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;
|
||||
}
|
||||
}
|
||||
@@ -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<OtOpcUaConfigDbContext> 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))
|
||||
|
||||
Reference in New Issue
Block a user