fix(secrets): ${secret:} expansion survives configuration reload (scadaproj#32)

ExpandConfigurationAsync wrote through config[key] = ..., which only sets the providers' in-memory Data; a file provider's reload replaces it, so editing appsettings.json on a running service reverted every expanded key to its ${secret:...} placeholder. - ConfigurationManager: expanded values are pinned in an internal overlay source appended last; its provider's Load() is a no-op and Build always returns the same provider, so reloads (and source rebuilds) keep the pins - any other IConfigurationRoot: in-place write as before, re-applied from the root's reload token - all references resolve before anything is written (a fail-closed miss no longer leaves the config half expanded) - tests: SecretExpansionReloadTests (real JSON file + Reload(), options monitor, plain root); three fail without the fix - docs: README reload note, CHANGELOG - code-reviews: Secrets-003 Resolved; README regenerated Claude-Session: https://claude.ai/code/session_01GT67AEHd2Acv1YPwGfXEoV
2026-11-09 14:52:45 -05:00
parent c3b07d6766
commit f60f641069
7 changed files with 277 additions and 15 deletions
@@ -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)

@@ -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`).

@@ -0,0 +1,51 @@
+using Microsoft.Extensions.Configuration;
+
+namespace ZB.MOM.WW.Secrets.Configuration;
+
+/// <summary>
+/// The in-memory overlay <see cref="SecretReferenceExpander"/> appends to a
+/// <see cref="ConfigurationManager"/> to hold expanded <c>${secret:}</c> values (scadaproj#32).
+/// </summary>
+/// <remarks>
+/// <see cref="Build"/> always returns the SAME provider: <see cref="ConfigurationManager"/> 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.
+/// </remarks>
+internal sealed class SecretExpansionConfigurationSource : IConfigurationSource
+{
+    /// <summary>The one provider this source ever builds.</summary>
+    public SecretExpansionConfigurationProvider Provider { get; } = new();
+
+    /// <inheritdoc />
+    public IConfigurationProvider Build(IConfigurationBuilder builder) => Provider;
+}
+
+/// <summary>
+/// Holds the pinned expansions. <see cref="Load"/> is deliberately a no-op, so a configuration
+/// reload — which calls <c>Load</c> on every provider — leaves the pins in place while the file
+/// providers beneath it reload their placeholder text.
+/// </summary>
+internal sealed class SecretExpansionConfigurationProvider : ConfigurationProvider
+{
+    /// <inheritdoc />
+    public override void Load()
+    {
+        // Intentionally empty: the pinned values are a startup snapshot and never reload.
+    }
+
+    /// <summary>Pins <paramref name="values"/>, optionally raising this provider's reload token.</summary>
+    /// <param name="values">Configuration key to expanded value.</param>
+    /// <param name="notify">Whether to signal listeners that the overlay changed.</param>
+    public void Pin(IReadOnlyDictionary<string, string> values, bool notify)
+    {
+        foreach (KeyValuePair<string, string> entry in values)
+        {
+            Data[entry.Key] = entry.Value;
+        }
+
+        if (notify)
+        {
+            OnReload();
+        }
+    }
+}
@@ -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
    }

    /// <summary>
-    /// Walks every entry in <paramref name="config"/> and rewrites, in place, any value that
-    /// contains a <c>${secret:...}</c> token to its expanded plaintext. Writes go through the
-    /// <see cref="IConfigurationRoot"/> indexer so subsequent reads — and any options binding done
-    /// afterward — observe the expanded values.
+    /// Walks every entry in <paramref name="config"/> and expands, to its plaintext, any value that
+    /// contains a <c>${secret:...}</c> token, so subsequent reads — and any options binding done
+    /// afterward — observe the expanded values, <b>including after a configuration reload</b>.
    /// </summary>
    /// <remarks>
    /// This MUST run <em>after</em> configuration is loaded and <em>before</em> options validation
@@ -84,6 +84,34 @@ public sealed class SecretReferenceExpander
    /// Fail-closed: a missing referenced secret propagates <see cref="SecretNotFoundException"/> out
    /// of startup.
    /// <para>
+    /// <b>Reload (scadaproj#32).</b> 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
+    /// <paramref name="config"/>:
+    /// </para>
+    /// <list type="bullet">
+    /// <item><description>
+    /// A <see cref="ConfigurationManager"/> (what <c>WebApplication.CreateBuilder</c> and
+    /// <c>Host.CreateApplicationBuilder</c> expose as <c>builder.Configuration</c>, whose
+    /// <c>appsettings*.json</c> 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 <c>IOptionsMonitor</c> rebind reads the pinned
+    /// value. This is the robust form — pass <c>builder.Configuration</c> here.
+    /// </description></item>
+    /// <item><description>
+    /// Any other <see cref="IConfigurationRoot"/> (e.g. from <c>ConfigurationBuilder.Build()</c>)
+    /// 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 <see cref="IConfiguration"/> readers see the
+    /// pinned value after a reload; an <c>IOptionsMonitor</c> 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 <c>reloadOnChange</c>, or pass a <see cref="ConfigurationManager"/>.
+    /// </description></item>
+    /// </list>
+    /// <para>
+    /// 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.
+    /// </para>
+    /// <para>
    /// Documentation/comment keys are skipped: any entry whose leaf key segment (the part after the
    /// last <c>:</c>) begins with an underscore — the widely-used <c>"_comment"</c> JSON convention —
    /// is left untouched. This lets an appsettings file document the <c>${secret:...}</c> 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.
    /// </para>
    /// </remarks>
-    /// <param name="config">The loaded configuration root to mutate.</param>
+    /// <param name="config">The loaded configuration root to expand.</param>
    /// <param name="ct">A token to cancel the operation.</param>
    /// <exception cref="SecretNotFoundException">A referenced secret does not exist (fail-closed).</exception>
    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<string, string>(StringComparer.OrdinalIgnoreCase);
        foreach (KeyValuePair<string, string> 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<string, string> expanded)
+    {
+        SecretExpansionConfigurationSource? overlay =
+            builder.Sources.OfType<SecretExpansionConfigurationSource>().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<string, string> expanded)
+    {
+        var pinned = new Dictionary<string, string>(expanded, StringComparer.OrdinalIgnoreCase);
+        Apply(config, pinned);
+        ChangeToken.OnChange(config.GetReloadToken, () => Apply(config, pinned));
+
+        static void Apply(IConfigurationRoot root, Dictionary<string, string> values)
+        {
+            foreach (KeyValuePair<string, string> entry in values)
+            {
+                if (!string.Equals(root[entry.Key], entry.Value, StringComparison.Ordinal))
+                {
+                    root[entry.Key] = entry.Value;
+                }
+            }
        }
    }

@@ -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;
+
+/// <summary>
+/// scadaproj#32 (Secrets-003): a configuration reload must not revert <c>${secret:}</c> expansion
+/// to the placeholder text. Before the fix the expander wrote into the providers' in-memory data, and
+/// a file provider's <c>Load()</c> replaces that data, so the next <c>appsettings.json</c> reload put
+/// <c>Password=${secret:…}</c> back into every reader's hands.
+/// </summary>
+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<string, string> values) : ISecretResolver
+    {
+        public Task<string?> 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<string, string> { ["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<Opts>(config.GetSection("Opts"));
+        using ServiceProvider provider = services.BuildServiceProvider();
+        IOptionsMonitor<Opts> monitor = provider.GetRequiredService<IOptionsMonitor<Opts>>();
+        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<string, string?>
+        {
+            ["A"] = "${secret:db/pw}",
+            ["B"] = "${secret:db/absent}",
+        });
+
+        await Assert.ThrowsAsync<SecretNotFoundException>(
+            () => Expander().ExpandConfigurationAsync(config, CancellationToken.None));
+
+        Assert.Equal("${secret:db/pw}", config["A"]);
+    }
+
+    public void Dispose() => Directory.Delete(_dir, recursive: true);
+}
@@ -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 |
@@ -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