diff --git a/ZB.MOM.WW.Secrets/CHANGELOG.md b/ZB.MOM.WW.Secrets/CHANGELOG.md index a0bcf6d..c4996e6 100644 --- a/ZB.MOM.WW.Secrets/CHANGELOG.md +++ b/ZB.MOM.WW.Secrets/CHANGELOG.md @@ -26,6 +26,15 @@ Fixes six scadaproj#15 code-review findings (`code-reviews/Secrets/findings.md`) mode no registration order applied it. New trailing optional parameter on `AddZbSecretsSqlServerStore`, `AddZbSecretsSqlServerReplication` and `AddZbSecretsAkkaReplication` (source-compatible; callers recompile). Pass it from every pre-host composition. +- **`${secret:}` expansion survives configuration reload** (scadaproj#32, Secrets-003). + `ExpandConfigurationAsync` used to write through `config[key] = …`, which a file provider's reload + replaces, so editing `appsettings.json` on a running service put the `${secret:…}` placeholder back + into every reader. Given a `ConfigurationManager` (`builder.Configuration`), it now pins the expanded + values in an in-memory overlay source appended last that never reloads; given any other + `IConfigurationRoot` it writes in place as before and re-applies the values from the root's reload + token. Behaviour changes: a referencing key keeps its startup value until restart (edits to it are + ignored on reload); every reference is resolved before anything is written, so a fail-closed miss + leaves the configuration untouched instead of half expanded. ## 0.6.5 — unreleased (NOT published to the feed) diff --git a/ZB.MOM.WW.Secrets/README.md b/ZB.MOM.WW.Secrets/README.md index d4a023e..5aaa6b6 100644 --- a/ZB.MOM.WW.Secrets/README.md +++ b/ZB.MOM.WW.Secrets/README.md @@ -91,6 +91,13 @@ await expander.ExpandConfigurationAsync((IConfigurationRoot)builder.Configuratio } ``` +**Reload.** Expansion is a startup snapshot that survives configuration reload (since `0.6.6`): pass +`builder.Configuration` (a `ConfigurationManager`) and the expanded values are pinned in an overlay +source that an `appsettings.json` reload cannot revert. A key that held a `${secret:}` reference +keeps its startup value until restart — editing it, or rotating the secret, takes effect on the next +start. A plain `ConfigurationBuilder().Build()` root gets the values re-applied after each reload, +which is weaker; build such a root without `reloadOnChange`. + The master key is 32 bytes, provided base64 in `ZB_SECRETS_MASTER_KEY` (Environment source). A missing/invalid key fails closed at startup (`MasterKeyUnavailableException`). diff --git a/ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets/Configuration/SecretExpansionConfigurationSource.cs b/ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets/Configuration/SecretExpansionConfigurationSource.cs new file mode 100644 index 0000000..5927065 --- /dev/null +++ b/ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets/Configuration/SecretExpansionConfigurationSource.cs @@ -0,0 +1,51 @@ +using Microsoft.Extensions.Configuration; + +namespace ZB.MOM.WW.Secrets.Configuration; + +/// +/// The in-memory overlay appends to a +/// to hold expanded ${secret:} values (scadaproj#32). +/// +/// +/// always returns the SAME provider: rebuilds +/// every provider from its sources when its source list changes, and a fresh, empty provider would +/// silently drop the pins and expose the placeholders again. +/// +internal sealed class SecretExpansionConfigurationSource : IConfigurationSource +{ + /// The one provider this source ever builds. + public SecretExpansionConfigurationProvider Provider { get; } = new(); + + /// + public IConfigurationProvider Build(IConfigurationBuilder builder) => Provider; +} + +/// +/// Holds the pinned expansions. is deliberately a no-op, so a configuration +/// reload — which calls Load on every provider — leaves the pins in place while the file +/// providers beneath it reload their placeholder text. +/// +internal sealed class SecretExpansionConfigurationProvider : ConfigurationProvider +{ + /// + public override void Load() + { + // Intentionally empty: the pinned values are a startup snapshot and never reload. + } + + /// Pins , optionally raising this provider's reload token. + /// Configuration key to expanded value. + /// Whether to signal listeners that the overlay changed. + public void Pin(IReadOnlyDictionary values, bool notify) + { + foreach (KeyValuePair entry in values) + { + Data[entry.Key] = entry.Value; + } + + if (notify) + { + OnReload(); + } + } +} diff --git a/ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets/Configuration/SecretReferenceExpander.cs b/ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets/Configuration/SecretReferenceExpander.cs index acecf37..4a2f9f0 100644 --- a/ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets/Configuration/SecretReferenceExpander.cs +++ b/ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets/Configuration/SecretReferenceExpander.cs @@ -1,5 +1,6 @@ using System.Text.RegularExpressions; using Microsoft.Extensions.Configuration; +using Microsoft.Extensions.Primitives; using ZB.MOM.WW.Secrets.Abstractions; namespace ZB.MOM.WW.Secrets.Configuration; @@ -73,10 +74,9 @@ public sealed class SecretReferenceExpander } /// - /// Walks every entry in and rewrites, in place, any value that - /// contains a ${secret:...} token to its expanded plaintext. Writes go through the - /// indexer so subsequent reads — and any options binding done - /// afterward — observe the expanded values. + /// Walks every entry in and expands, to its plaintext, any value that + /// contains a ${secret:...} token, so subsequent reads — and any options binding done + /// afterward — observe the expanded values, including after a configuration reload. /// /// /// This MUST run after configuration is loaded and before options validation @@ -84,6 +84,34 @@ public sealed class SecretReferenceExpander /// Fail-closed: a missing referenced secret propagates out /// of startup. /// + /// Reload (scadaproj#32). Expansion is a startup snapshot: each key that held a reference + /// is pinned to the value it expanded to, for the life of the process, and a reload of the + /// underlying files never reverts it to the placeholder text. How the pin is held depends on + /// : + /// + /// + /// + /// A (what WebApplication.CreateBuilder and + /// Host.CreateApplicationBuilder expose as builder.Configuration, whose + /// appsettings*.json sources reload on change by default) gets a dedicated in-memory + /// overlay source appended LAST. It never reloads, so it wins over every provider beneath it + /// through any number of file reloads, and every IOptionsMonitor rebind reads the pinned + /// value. This is the robust form — pass builder.Configuration here. + /// + /// + /// Any other (e.g. from ConfigurationBuilder.Build()) + /// cannot take a new provider, so the values are written into its providers as before and + /// re-applied from the root's reload token. Direct readers see the + /// pinned value after a reload; an IOptionsMonitor bound to that root may rebind from the + /// placeholder first, because change callbacks run in an order this library does not control. + /// Build such a root without reloadOnChange, or pass a . + /// + /// + /// + /// Consequently, editing a referencing key (or rotating the secret it names) takes effect on the + /// next restart, never on reload. Keys that did not hold a reference at startup reload normally. + /// + /// /// Documentation/comment keys are skipped: any entry whose leaf key segment (the part after the /// last :) begins with an underscore — the widely-used "_comment" JSON convention — /// is left untouched. This lets an appsettings file document the ${secret:...} syntax in a @@ -91,7 +119,7 @@ public sealed class SecretReferenceExpander /// a real reference to resolve. Functional keys are never named with a leading underscore. /// /// - /// The loaded configuration root to mutate. + /// The loaded configuration root to expand. /// A token to cancel the operation. /// A referenced secret does not exist (fail-closed). public async Task ExpandConfigurationAsync(IConfigurationRoot config, CancellationToken ct) @@ -110,9 +138,64 @@ public sealed class SecretReferenceExpander } } + // Resolve EVERYTHING before writing anything, so a fail-closed miss part-way through leaves + // the configuration untouched rather than half expanded. + var expanded = new Dictionary(StringComparer.OrdinalIgnoreCase); foreach (KeyValuePair entry in pending) { - config[entry.Key] = await ExpandAsync(entry.Value, ct).ConfigureAwait(false); + expanded[entry.Key] = await ExpandAsync(entry.Value, ct).ConfigureAwait(false); + } + + if (expanded.Count == 0) + { + return; + } + + if (config is IConfigurationBuilder builder) + { + PinInOverlay(builder, expanded); + } + else + { + PinInPlace(config, expanded); + } + } + + // ConfigurationManager: append (or reuse) the overlay source. It is never reloaded, so it keeps + // winning over the file providers beneath it whatever they reload to. + private static void PinInOverlay(IConfigurationBuilder builder, Dictionary expanded) + { + SecretExpansionConfigurationSource? overlay = + builder.Sources.OfType().FirstOrDefault(); + if (overlay is null) + { + overlay = new SecretExpansionConfigurationSource(); + overlay.Provider.Pin(expanded, notify: false); + builder.Add(overlay); + } + else + { + overlay.Provider.Pin(expanded, notify: true); + } + } + + // A plain IConfigurationRoot has a fixed provider list: write through the indexer (every + // provider) and re-apply after each reload, which is the best a closed root allows. + private static void PinInPlace(IConfigurationRoot config, Dictionary expanded) + { + var pinned = new Dictionary(expanded, StringComparer.OrdinalIgnoreCase); + Apply(config, pinned); + ChangeToken.OnChange(config.GetReloadToken, () => Apply(config, pinned)); + + static void Apply(IConfigurationRoot root, Dictionary values) + { + foreach (KeyValuePair entry in values) + { + if (!string.Equals(root[entry.Key], entry.Value, StringComparison.Ordinal)) + { + root[entry.Key] = entry.Value; + } + } } } diff --git a/ZB.MOM.WW.Secrets/tests/ZB.MOM.WW.Secrets.Tests/Configuration/SecretExpansionReloadTests.cs b/ZB.MOM.WW.Secrets/tests/ZB.MOM.WW.Secrets.Tests/Configuration/SecretExpansionReloadTests.cs new file mode 100644 index 0000000..6ec0963 --- /dev/null +++ b/ZB.MOM.WW.Secrets/tests/ZB.MOM.WW.Secrets.Tests/Configuration/SecretExpansionReloadTests.cs @@ -0,0 +1,113 @@ +using Microsoft.Extensions.Configuration; +using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Options; +using ZB.MOM.WW.Secrets.Abstractions; +using ZB.MOM.WW.Secrets.Configuration; + +namespace ZB.MOM.WW.Secrets.Tests.Configuration; + +/// +/// scadaproj#32 (Secrets-003): a configuration reload must not revert ${secret:} expansion +/// to the placeholder text. Before the fix the expander wrote into the providers' in-memory data, and +/// a file provider's Load() replaces that data, so the next appsettings.json reload put +/// Password=${secret:…} back into every reader's hands. +/// +public sealed class SecretExpansionReloadTests : IDisposable +{ + private readonly string _dir = Path.Combine(Path.GetTempPath(), $"zb-reload-{Guid.NewGuid():N}"); + + public SecretExpansionReloadTests() => Directory.CreateDirectory(_dir); + + private sealed class FixedResolver(Dictionary values) : ISecretResolver + { + public Task GetAsync(SecretName name, CancellationToken ct) => + Task.FromResult(values.TryGetValue(name.Value, out string? v) ? v : null); + } + + private static SecretReferenceExpander Expander() => + new(new FixedResolver(new Dictionary { ["db/pw"] = "hunter2" })); + + private string WriteSettings(string level) + { + string path = Path.Combine(_dir, "appsettings.json"); + File.WriteAllText(path, $$""" + { "Cs": "Server=s;Password=${secret:db/pw}", "Opts": { "Cs": "pw=${secret:db/pw}", "Level": "{{level}}" } } + """); + return path; + } + + private sealed class Opts + { + public string? Cs { get; set; } + + public string? Level { get; set; } + } + + [Fact] + public async Task ConfigurationManager_keeps_the_expansion_through_a_reload() + { + // The shape every family host uses: builder.Configuration is a ConfigurationManager. + var config = new ConfigurationManager(); + config.AddJsonFile(WriteSettings("Info"), optional: false, reloadOnChange: false); + + await Expander().ExpandConfigurationAsync(config, CancellationToken.None); + Assert.Equal("Server=s;Password=hunter2", config["Cs"]); + + ((IConfigurationRoot)config).Reload(); + + Assert.Equal("Server=s;Password=hunter2", config["Cs"]); + } + + [Fact] + public async Task An_options_monitor_rebinds_to_the_expanded_value_and_unrelated_keys_still_reload() + { + var config = new ConfigurationManager(); + string path = WriteSettings("Info"); + config.AddJsonFile(path, optional: false, reloadOnChange: false); + await Expander().ExpandConfigurationAsync(config, CancellationToken.None); + + var services = new ServiceCollection(); + services.Configure(config.GetSection("Opts")); + using ServiceProvider provider = services.BuildServiceProvider(); + IOptionsMonitor monitor = provider.GetRequiredService>(); + Assert.Equal("pw=hunter2", monitor.CurrentValue.Cs); + + // The operator edits a log level on a running service. + WriteSettings("Debug"); + ((IConfigurationRoot)config).Reload(); + + Assert.Equal("Debug", monitor.CurrentValue.Level); // the edit took effect... + Assert.Equal("pw=hunter2", monitor.CurrentValue.Cs); // ...and the credential survived it + } + + [Fact] + public async Task A_plain_root_reapplies_the_expansion_after_a_reload() + { + IConfigurationRoot config = new ConfigurationBuilder() + .AddJsonFile(WriteSettings("Info"), optional: false, reloadOnChange: false) + .Build(); + + await Expander().ExpandConfigurationAsync(config, CancellationToken.None); + ((IConfigurationRoot)config).Reload(); + + Assert.Equal("Server=s;Password=hunter2", config["Cs"]); + } + + [Fact] + public async Task A_missing_secret_fails_closed_without_expanding_anything() + { + var config = new ConfigurationManager(); + config.AddInMemoryCollection(new Dictionary + { + ["A"] = "${secret:db/pw}", + ["B"] = "${secret:db/absent}", + }); + + await Assert.ThrowsAsync( + () => Expander().ExpandConfigurationAsync(config, CancellationToken.None)); + + Assert.Equal("${secret:db/pw}", config["A"]); + } + + public void Dispose() => Directory.Delete(_dir, recursive: true); +} diff --git a/code-reviews/README.md b/code-reviews/README.md index 2d30401..c669915 100644 --- a/code-reviews/README.md +++ b/code-reviews/README.md @@ -40,7 +40,7 @@ code-reviews/ ## Summary -10 of 10 libraries reviewed. 28 pending findings across all libraries. +10 of 10 libraries reviewed. 27 pending findings across all libraries. Coverage: Secrets, LocalDb, GalaxyRepository and Overview first reviewed, and Auth, Health, Theme, Audit, Configuration and Telemetry re-reviewed, on 2026-11-09 (#15). Each finding also @@ -50,9 +50,9 @@ carries a P1/P2/P3 priority (P1 = Critical/High, P2 = Medium, P3 = Low). |----------|---------------| | Critical | 0 | | High | 0 | -| Medium | 9 | +| Medium | 8 | | Low | 19 | -| **Total** | **28** | +| **Total** | **27** | ## Library Status @@ -65,7 +65,7 @@ carries a P1/P2/P3 priority (P1 = Critical/High, P2 = Medium, P3 = Low). | [Health](Health/findings.md) | 2026-11-09 | `91fb2b4` | 0/0/0/0 | 0 | 7 | | [LocalDb](LocalDb/findings.md) | 2026-11-09 | `91fb2b4` | 0/0/4/3 | 7 | 7 | | [Overview](Overview/findings.md) | 2026-11-09 | `91fb2b4` | 0/0/0/0 | 0 | 5 | -| [Secrets](Secrets/findings.md) | 2026-11-09 | `91fb2b4` | 0/0/4/13 | 17 | 19 | +| [Secrets](Secrets/findings.md) | 2026-11-09 | `91fb2b4` | 0/0/3/13 | 16 | 19 | | [Telemetry](Telemetry/findings.md) | 2026-11-09 | `91fb2b4` | 0/0/0/0 | 0 | 11 | | [Theme](Theme/findings.md) | 2026-11-09 | `91fb2b4` | 0/0/0/0 | 0 | 8 | @@ -84,7 +84,7 @@ _None open._ _None open._ -### Medium (9) +### Medium (8) | ID | Library | Title | |----|---------|-------| @@ -93,7 +93,6 @@ _None open._ | LocalDb-002 | [LocalDb](LocalDb/findings.md) | Snapshot streams tables by name and rows by `pk_json` text, so FK children land before parents and are dead-lettered for good | | LocalDb-003 | [LocalDb](LocalDb/findings.md) | `INSERT OR REPLACE` apply silently deletes a different row on a secondary `UNIQUE` conflict, so the pair diverges | | LocalDb-004 | [LocalDb](LocalDb/findings.md) | `FailClosedOnDrift` is bypassed on the snapshot path, so a future-stamped snapshot row moves the local HLC forward permanently | -| Secrets-003 | [Secrets](Secrets/findings.md) | `${secret:}` expansion is undone by configuration reload | | Secrets-010 | [Secrets](Secrets/findings.md) | Akka replication actor is created only when `ISecretStore` is first resolved; a node that never resolves it never replicates | | Secrets-013 | [Secrets](Secrets/findings.md) | UI add/rotate/delete: if the audit write throws after the store write succeeded, the change is reported and audited as a failure and the resolver cache is not evicted | | Secrets-017 | [Secrets](Secrets/findings.md) | `secret set`/`rotate` take the plaintext on argv; the host builder re-parses argv as configuration and can crash with the value in the error | diff --git a/code-reviews/Secrets/findings.md b/code-reviews/Secrets/findings.md index 42096c1..f057613 100644 --- a/code-reviews/Secrets/findings.md +++ b/code-reviews/Secrets/findings.md @@ -10,7 +10,7 @@ | Last reviewed | 2026-11-09 | | Reviewer | Claude (scadaproj#15) | | Commit reviewed | `91fb2b4` | -| Open findings | 17 | +| Open findings | 16 | ## Summary @@ -128,7 +128,7 @@ Resolved 2026-11-09 in the commit tagged `(scadaproj#31)` (Secrets `0.6.6`, unpu | Severity | Medium | | Priority | P2 | | Category | Correctness & logic bugs | -| Status | Open | +| Status | Resolved | | Location | `ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets/Configuration/SecretReferenceExpander.cs:115` | **Description** @@ -145,7 +145,7 @@ Expand through a dedicated configuration provider or source that wraps the other **Resolution** -_Unresolved._ +Resolved 2026-11-09 in the commit tagged `(scadaproj#32)` (Secrets `0.6.6`, unpublished). `ExpandConfigurationAsync` now resolves every reference first, then pins the results. Given a `ConfigurationManager` (every family host passes `builder.Configuration`), it appends an internal `SecretExpansionConfigurationSource` LAST whose provider's `Load()` is a no-op and whose `Build` always returns the same provider, so a reload of the file providers beneath it cannot revert the values and `IOptionsMonitor` rebinds read them. Given any other `IConfigurationRoot` (which cannot take a provider), it writes in place as before and re-applies from `ChangeToken.OnChange(config.GetReloadToken, …)`. Expansion is documented as a startup snapshot: a referencing key keeps its value until restart. Regression tests `SecretExpansionReloadTests` — `ConfigurationManager_keeps_the_expansion_through_a_reload`, `An_options_monitor_rebinds_to_the_expanded_value_and_unrelated_keys_still_reload` and `A_plain_root_reapplies_the_expansion_after_a_reload` (real JSON file + `Reload()`) all fail without the fix. Note that ScadaBridge and the ignitionoee router expand a plain root built with `reloadOnChange: false` and chain it into the host's configuration, so they were not exposed; OtOpcUa, HistorianGateway and mxaccessgw pass their `ConfigurationManager` and were exposed whenever reload-on-change is on (the `WebApplication.CreateBuilder` default). ### Secrets-004 — Corrupt or wrong-length crypto fields escape `Decrypt` as `ArgumentException`: not audited, and breaks the cipher contract