[Secrets] ${secret:} expansion is undone on config reload (raw placeholders reach connection strings) #32

New Issue

2026-11-09T14:39:19-05:00

dohertj2 commented

2026-11-09 14:39:19 -05:00

From the scadaproj#15 shared-library review (code-reviews/Secrets/findings.md, Secrets-003), verified at 91fb2b4. Update the finding's Resolution row when fixed.


Severity	Medium
Priority	P2
Category	Correctness & logic bugs
Status	Open
Location	`ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets/Configuration/SecretReferenceExpander.cs:115`

Description

ExpandConfigurationAsync writes the expanded value through config[key] = …. That only sets the value in the providers' in-memory Data. A file provider's Load() replaces Data on reload, so every expanded key reverts to its literal ${secret:…} placeholder. Reload happens on IConfigurationRoot.Reload(), or on any appsettings*.json change when reloadOnChange is on, which is the default for WebApplication.CreateBuilder / Host.CreateDefaultBuilder. The XML docs and the README do not mention this.

Failure scenario

The probe used a JSON file {"Cs":"pw=${secret:x}"}, expanded it to pw=hunter2, and then called cfg.Reload(). cfg["Cs"] became pw=${secret:x}. In production, an operator edits a log level in appsettings.json on a running service. From then on, IConfiguration readers and IOptionsMonitor-rebound options see the placeholder text as the credential. For example, a connection string read per use contains Password=${secret:sql/…}, and logins fail until the process restarts.

Recommendation

Expand through a dedicated configuration provider or source that wraps the others and re-expands on the change token. Alternatively, add an in-memory overlay provider last and re-run expansion on GetReloadToken(). At minimum, document that reload must be disabled for expanded keys.

Resolution

Unresolved.

From the scadaproj#15 shared-library review (`code-reviews/Secrets/findings.md`, **Secrets-003**), verified at 91fb2b4. Update the finding's Resolution row when fixed. | | | |--|--| | Severity | Medium | | Priority | P2 | | Category | Correctness & logic bugs | | Status | Open | | Location | `ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets/Configuration/SecretReferenceExpander.cs:115` | **Description** `ExpandConfigurationAsync` writes the expanded value through `config[key] = …`. That only sets the value in the providers' in-memory `Data`. A file provider's `Load()` replaces `Data` on reload, so every expanded key reverts to its literal `${secret:…}` placeholder. Reload happens on `IConfigurationRoot.Reload()`, or on any `appsettings*.json` change when `reloadOnChange` is on, which is the default for `WebApplication.CreateBuilder` / `Host.CreateDefaultBuilder`. The XML docs and the README do not mention this. **Failure scenario** The probe used a JSON file `{"Cs":"pw=${secret:x}"}`, expanded it to `pw=hunter2`, and then called `cfg.Reload()`. `cfg["Cs"]` became `pw=${secret:x}`. In production, an operator edits a log level in `appsettings.json` on a running service. From then on, `IConfiguration` readers and `IOptionsMonitor`-rebound options see the placeholder text as the credential. For example, a connection string read per use contains `Password=${secret:sql/…}`, and logins fail until the process restarts. **Recommendation** Expand through a dedicated configuration provider or source that wraps the others and re-expands on the change token. Alternatively, add an in-memory overlay provider last and re-run expansion on `GetReloadToken()`. At minimum, document that reload must be disabled for expanded keys. **Resolution** _Unresolved._

dohertj2 added the type:review-finding priority:p2 labels 2026-11-09 14:39:19 -05:00

dohertj2 commented

2026-11-09 15:01:03 -05:00

Fixed on main 25607b7 (Secrets-003 → Resolved): with a ConfigurationManager, expanded values live in an extra last-added non-reloading configuration source; any other IConfigurationRoot gets the old in-place write re-applied after each reload. All references resolve before anything is written (a missing secret leaves config untouched, not half-expanded). Behaviour: a key holding a ${secret:} reference keeps its startup value until restart. Exposed today: OtOpcUa, HistorianGateway, mxaccessgw (ConfigurationManager + reload-on-change); ScadaBridge and the ignitionoee router were not. Evidence: Secrets solution (5 test projects) all pass — Core 246 (1 DPAPI skip), SqlServer 60 (16 live-SQL skips), Akka 44, Grpc 196, Ui 20; each regression test fails with its fix removed. Ships in Secrets 0.6.6 (unpublished).

Fixed on main 25607b7 (Secrets-003 → Resolved): with a ConfigurationManager, expanded values live in an extra last-added non-reloading configuration source; any other IConfigurationRoot gets the old in-place write re-applied after each reload. All references resolve before anything is written (a missing secret leaves config untouched, not half-expanded). **Behaviour:** a key holding a ${secret:} reference keeps its startup value until restart. Exposed today: OtOpcUa, HistorianGateway, mxaccessgw (ConfigurationManager + reload-on-change); ScadaBridge and the ignitionoee router were not. Evidence: Secrets solution (5 test projects) all pass — Core 246 (1 DPAPI skip), SqlServer 60 (16 live-SQL skips), Akka 44, Grpc 196, Ui 20; each regression test fails with its fix removed. Ships in Secrets **0.6.6 (unpublished)**.

dohertj2 closed this issue

2026-11-09 15:01:03 -05:00

dohertj2 referenced this issue

2026-11-09 15:01:05 -05:00

[Secrets] CLI appsettings SqlitePath is relative (fails validation); plain IConfigurationRoot reload gap; OtOpcUa starter now redundant #49

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: dohertj2/scadaproj#32