DraftValidator cannot see ClusterNode.DriverConfigOverridesJson — the third driver-config persistence surface #499

New Issue

2026-07-25T16:08:59-04:00

dohertj2 commented

2026-07-25 16:08:59 -04:00

Split out of #498.

That issue is fixed for the two config surfaces DraftSnapshot exposes: DriverInstance.DriverConfig and Device.DeviceConfig. There is a third place a driver config blob is persisted — ClusterNode.DriverConfigOverridesJson, the per-node override JSON keyed by DriverInstanceId that is merged onto the cluster-level DriverConfig.

DraftSnapshot carries no ClusterNode collection, so DraftValidator cannot see it. A connectionString pasted into a node override would therefore be persisted into the ConfigDb and replicated, exactly the leak #498 closes elsewhere — just via a surface the validator is structurally blind to.

Two ways to close it, both needing a decision:

Add ClusterNode rows to DraftSnapshot and extend the existing rule. Widens the snapshot (and everything that builds one) for a single rule.
Validate the override JSON where it is authored/saved instead, next to whatever writes DriverConfigOverridesJson.

Also worth deciding while here: whether the rule should generalise beyond Sql + connectionString to a credential-shaped key check across every driver type (password, secret, …). #498 was deliberately kept to the Sql driver as reported, since silently failing an unrelated driver’s deploy would be a regression rather than defence in depth.

Not urgent: node overrides are an uncommon authoring path and the runtime still ignores the key. This is about refusing to store the secret.

Split out of #498. That issue is fixed for the two config surfaces `DraftSnapshot` exposes: `DriverInstance.DriverConfig` and `Device.DeviceConfig`. There is a **third** place a driver config blob is persisted — `ClusterNode.DriverConfigOverridesJson`, the per-node override JSON keyed by `DriverInstanceId` that is merged onto the cluster-level `DriverConfig`. `DraftSnapshot` carries no `ClusterNode` collection, so `DraftValidator` cannot see it. A `connectionString` pasted into a node override would therefore be persisted into the ConfigDb and replicated, exactly the leak #498 closes elsewhere — just via a surface the validator is structurally blind to. Two ways to close it, both needing a decision: 1. Add `ClusterNode` rows to `DraftSnapshot` and extend the existing rule. Widens the snapshot (and everything that builds one) for a single rule. 2. Validate the override JSON where it is authored/saved instead, next to whatever writes `DriverConfigOverridesJson`. Also worth deciding while here: whether the rule should generalise beyond `Sql` + `connectionString` to a credential-shaped key check across every driver type (`password`, `secret`, …). #498 was deliberately kept to the Sql driver as reported, since silently failing an unrelated driver’s deploy would be a regression rather than defence in depth. Not urgent: node overrides are an uncommon authoring path and the runtime still ignores the key. This is about refusing to *store* the secret.

dohertj2 referenced this issue

2026-07-25 16:09:10 -04:00

Deploy-time guard: reject a Sql DriverConfig blob carrying a connectionString key (FF-3) #498

dohertj2 referenced this issue from a commit

2026-07-25 22:44:40 -04:00

merge: Sql driver follow-ups #496/#497/#498 + the Runtime.Tests flake #500 (fix/sql-driver-followups)

dohertj2 commented

2026-07-26 10:43:02 -04:00

Fixed on fix/issues-503-501-493-499-488, commit 2193d9f2, live-verified.

Neither of the two options as stated — gated at the SAVE. Reasoning:

Option 1 (ClusterNode into DraftSnapshot) widens the snapshot and every builder of one for a single rule about a value that never enters the artifact.
More decisively, 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 ConfigDb 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 moved into a shared SqlCredentialGuard so the two enforcement points cannot drift; DraftValidator now calls it instead of its own private helper.

On generalising to credential-shaped keys across all drivers: not doing it, for the reason #498 gave. A broader sweep would start refusing configs that are legitimate today for drivers which never made an indirect-credential guarantee — a regression, not defence in depth. Widen per driver as each gains its own contract.

The error names the offending instance(s) and never the value.

Live gate (docker-dev, real Sql driver DRV-73f6a1061cfe in MAIN):

Pasted {"DRV-73f6a1061cfe":{"connectionString":"Server=sql,1433;…;Password=hunter2"}} into the node's override box → save refused, banner: "Driver config overrides carry a 'connectionString' for Sql driver instance(s) DRV-73f6a1061cfe… Remove the key." No credential text in the message. ClusterNode.DriverConfigOverridesJson still NULL in SQL.
Replaced with {"DRV-73f6a1061cfe":{"connectionStringRef":"Sql:Line3","commandTimeoutSeconds":45}} → saved, verified in SQL. Rig restored afterwards.

⚠️ The live gate found the editor page was completely dead, both defects pre-existing on master and unrelated to this change — fixed in 4cc0215b, see the note I am filing separately. Without them this guard was unreachable by any operator.

14 new SqlCredentialGuardTests cover both surfaces, 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. Configuration.Tests 131/131.

Fixed on `fix/issues-503-501-493-499-488`, commit `2193d9f2`, **live-verified**. **Neither of the two options as stated — gated at the SAVE.** Reasoning: - Option 1 (`ClusterNode` into `DraftSnapshot`) widens the snapshot and every builder of one for a single rule about a value that never enters the artifact. - More decisively, 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 ConfigDb 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 moved into a shared **`SqlCredentialGuard`** so the two enforcement points cannot drift; `DraftValidator` now calls it instead of its own private helper. **On generalising to credential-shaped keys across all drivers: not doing it**, for the reason #498 gave. A broader sweep would start refusing configs that are legitimate today for drivers which never made an indirect-credential guarantee — a regression, not defence in depth. Widen per driver as each gains its own contract. The error names the offending instance(s) and **never** the value. **Live gate (docker-dev, real Sql driver `DRV-73f6a1061cfe` in MAIN):** 1. Pasted `{"DRV-73f6a1061cfe":{"connectionString":"Server=sql,1433;…;Password=hunter2"}}` into the node's override box → **save refused**, banner: *"Driver config overrides carry a 'connectionString' for Sql driver instance(s) DRV-73f6a1061cfe… Remove the key."* No credential text in the message. `ClusterNode.DriverConfigOverridesJson` still `NULL` in SQL. 2. Replaced with `{"DRV-73f6a1061cfe":{"connectionStringRef":"Sql:Line3","commandTimeoutSeconds":45}}` → **saved**, verified in SQL. Rig restored afterwards. ⚠️ **The live gate found the editor page was completely dead**, both defects pre-existing on master and unrelated to this change — fixed in `4cc0215b`, see the note I am filing separately. Without them this guard was unreachable by any operator. 14 new `SqlCredentialGuardTests` cover both surfaces, 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. Configuration.Tests 131/131.

dohertj2 closed this issue

2026-07-26 10:43:02 -04:00

dohertj2 referenced this issue

2026-07-26 10:43:27 -04:00

AdminUI cluster-node editor was dead: HTTP 500 on load (InputNumber<byte>) + Save a silent no-op (NodeId regex forbids ':') #505

dohertj2 referenced this issue from a commit

2026-07-26 10:53:53 -04:00

fix(config): refuse to store a Sql credential in a node config override (#499)

dohertj2 referenced this issue from a commit

2026-07-26 10:53:53 -04:00

fix(adminui): the cluster-node editor was dead — 500 on load, silent no-op on save

dohertj2 referenced this issue from a commit