feat(secrets): fail-closed store-path validation; default moves outside the content root

Secrets:SqlitePath is now validated at startup by SecretsOptionsValidator using two rules in the new public SecretsStorePathRules. Both come from production incidents and neither implies the other. Must be absolute: a relative value resolves against the process working directory, which for a Windows service is C:\Windows\System32, so the store lands outside the installation; in a dev loop it lands in the source tree (a gateway was observed writing its secrets database into src/, tripping that repo's own tree-hygiene test). Must not be inside the content root: an absolute path can still point into the directory every deployment procedure in this family renames on upgrade. On 2026-08-09 a gateway's credential store sat inside its Server\ directory; the deploy renamed Server to Server.bak.*, the database went with it, and the process created a fresh empty one at the same path. Every consumer lost its credential, silently, for two days. The existing "must be absolute" convention did not help — the path was absolute. The rules reject rather than relocate. Silently moving a credential store presents later as an authentication failure pointing nowhere near the configuration that caused it; a boot error is seen immediately by whoever caused it. Validation is deliberately not exempted in Development: a guard that never runs in a dev loop or in CI rots, and the configuration behind the outage looked entirely reasonable. BREAKING for configuration, not API. SecretsOptions.SqlitePath now defaults to an absolute per-user path under LocalApplicationData; it was "secrets.db", which the new rules reject. An application configuring a relative path will fail to boot — remove the key or set an absolute one outside the content root. All five deployed instances of this family were checked first and already set an absolute path outside their content root, so the migration edge (a host relying on the OLD default now finding nothing and creating an empty store — this bug, re-introduced by its own fix) does not apply to any of them. Per-user rather than machine-wide is deliberate: CommonApplicationData is /usr/share on Linux and macOS, which a normal user cannot write, so a machine-wide library default would fail every consuming application's boot on a developer machine. An application may compute its own default; the configured value always wins. Validator registration is idempotent. AddZbSecrets is legitimately called more than once in real compositions, and TryAddEnumerable cannot de-duplicate a factory-built descriptor — it de-duplicates on implementation type, which a factory does not carry, so it throws on the second call rather than skipping. The existing suite caught this. Tests cover both rules including the segment-boundary case (/srv/app-data must not read as inside /srv/app — a false positive on a fail-closed startup rule is an application that will not boot), that the shipped default satisfies the rules it ships with, that the default is not machine-wide, idempotent registration, and — most importantly — that a bad path actually throws when IOptions is resolved, since a correctly implemented validator that is never invoked is the same "guard that does not fire" failure this change exists to prevent. Family bumps to 0.6.0. Adds a dependency on ZB.MOM.WW.Configuration 0.1.0 for the shared OptionsValidatorBase/ValidationBuilder vocabulary, which every consumer already references.
2026-08-11 08:18:37 -04:00
parent f088cb81ba
commit 944595ad32
9 changed files with 556 additions and 4 deletions
@@ -0,0 +1,62 @@
+using Microsoft.Extensions.Hosting;
+using ZB.MOM.WW.Configuration;
+
+namespace ZB.MOM.WW.Secrets.Configuration;
+
+/// <summary>
+/// Fail-closed startup validation for <see cref="SecretsOptions"/>: the secret store must live at
+/// an absolute path outside the application's content root. See <see cref="SecretsStorePathRules"/>
+/// for the two incidents that motivate each rule.
+/// </summary>
+/// <remarks>
+/// Registered by <see cref="DependencyInjection.SecretsServiceCollectionExtensions.AddZbSecrets"/>
+/// with <c>ValidateOnStart</c>, so a bad path fails the boot rather than surfacing later as a
+/// missing secret. This is deliberately not conditional on the hosting environment: a guard that is
+/// exempted in Development never runs in a dev loop or in CI, so it rots — and the configuration
+/// that caused the 2026-08-09 outage looked entirely reasonable, which is exactly the case an
+/// unexercised guard fails to catch.
+/// </remarks>
+public sealed class SecretsOptionsValidator : OptionsValidatorBase<SecretsOptions>
+{
+    private readonly string? _contentRootPath;
+
+    /// <summary>
+    /// Initializes a new instance of the <see cref="SecretsOptionsValidator"/> class.
+    /// </summary>
+    /// <param name="hostEnvironment">
+    /// The host environment supplying the content root, or <see langword="null"/> when the secrets
+    /// subsystem is composed outside a host (the CLI does this). With no content root the
+    /// under-content-root rule is skipped; the absolute-path rule still applies.
+    /// </param>
+    public SecretsOptionsValidator(IHostEnvironment? hostEnvironment) =>
+        _contentRootPath = hostEnvironment?.ContentRootPath;
+
+    /// <inheritdoc />
+    protected override void Validate(ValidationBuilder builder, SecretsOptions options)
+    {
+        ArgumentNullException.ThrowIfNull(builder);
+        ArgumentNullException.ThrowIfNull(options);
+
+        builder.Required(options.SqlitePath, "Secrets:SqlitePath");
+
+        SecretsStorePathRules.AddIfNotRooted(
+            options.SqlitePath,
+            "Secrets:SqlitePath must be an absolute path. A relative value resolves against the "
+                + "process working directory — for a Windows service that is C:\\Windows\\System32, "
+                + "so the store lands outside the installation; in a dev loop it lands in the source "
+                + "tree. Set an absolute path, or remove the key entirely to accept the "
+                + "per-user default.",
+            builder);
+
+        SecretsStorePathRules.AddIfUnderContentRoot(
+            options.SqlitePath,
+            _contentRootPath,
+            $"Secrets:SqlitePath ('{options.SqlitePath}') is inside the application content root "
+                + $"('{_contentRootPath}'). Deployment procedures in this family replace or rename "
+                + "that directory on upgrade, which takes the secret store with it and leaves the "
+                + "process to create a fresh empty one at the same path — an outage that presents as "
+                + "an authentication failure, not as a deployment error. Put the store in a sibling "
+                + "directory that the upgrade does not touch.",
+            builder);
+    }
+}
@@ -0,0 +1,120 @@
+using ZB.MOM.WW.Configuration;
+
+namespace ZB.MOM.WW.Secrets.Configuration;
+
+/// <summary>
+/// Filesystem-path validation primitives for the secret store's location.
+/// </summary>
+/// <remarks>
+/// <para>
+/// Both rules exist because of production incidents, and neither implies the other:
+/// </para>
+/// <list type="bullet">
+///   <item>
+///     <description>
+///     <b>Not rooted.</b> A relative path resolves against the process working directory. For a
+///     Windows service that is <c>C:\Windows\System32</c>, so the store silently lands outside the
+///     installation entirely; in a dev loop it lands in the source tree (observed: a gateway wrote
+///     its secrets DB into <c>src/</c> and tripped the repo's own tree-hygiene test).
+///     </description>
+///   </item>
+///   <item>
+///     <description>
+///     <b>Under the content root.</b> An <em>absolute</em> path can still point inside the
+///     application directory, which every deployment procedure in this family replaces or renames
+///     on upgrade. On 2026-08-09 a gateway's API-key store sat at
+///     <c>…\MxGateway\Server\gateway-auth.db</c>; the deploy renamed <c>Server</c> to
+///     <c>Server.bak.*</c>, the database left with it, and the process created a fresh empty one at
+///     the same path. Every consumer lost its credential, silently, for two days. Rooted did not
+///     save it — the path was absolute and still doomed.
+///     </description>
+///   </item>
+/// </list>
+/// <para>
+/// Both reject rather than relocate. Silently moving a credential store is worse than a boot error:
+/// a boot error is seen immediately by the person who caused it, whereas a relocated store presents
+/// later as an authentication failure pointing nowhere near the configuration that caused it.
+/// </para>
+/// </remarks>
+public static class SecretsStorePathRules
+{
+    /// <summary>
+    /// Records a failure when <paramref name="value"/> is non-blank and not an absolute (rooted)
+    /// path on the current OS. Blank is the caller's required-field concern and passes here.
+    /// </summary>
+    /// <param name="value">The configured path value.</param>
+    /// <param name="message">The failure message to record.</param>
+    /// <param name="builder">The validation builder accumulating failures.</param>
+    public static void AddIfNotRooted(string? value, string message, ValidationBuilder builder)
+    {
+        ArgumentNullException.ThrowIfNull(builder);
+
+        if (string.IsNullOrWhiteSpace(value))
+        {
+            return;
+        }
+
+        if (!Path.IsPathRooted(value))
+        {
+            builder.Add(message);
+        }
+    }
+
+    /// <summary>
+    /// Records a failure when <paramref name="value"/> resolves to a location inside
+    /// <paramref name="contentRoot"/>. A blank value or a blank content root passes: there is
+    /// nothing to compare, and a host that cannot report its content root must not be turned into
+    /// a boot failure by a rule about a directory it does not know it has.
+    /// </summary>
+    /// <param name="value">The configured path value.</param>
+    /// <param name="contentRoot">The application content root, or <see langword="null"/> when unknown.</param>
+    /// <param name="message">The failure message to record.</param>
+    /// <param name="builder">The validation builder accumulating failures.</param>
+    public static void AddIfUnderContentRoot(
+        string? value,
+        string? contentRoot,
+        string message,
+        ValidationBuilder builder)
+    {
+        ArgumentNullException.ThrowIfNull(builder);
+
+        if (string.IsNullOrWhiteSpace(value) || string.IsNullOrWhiteSpace(contentRoot))
+        {
+            return;
+        }
+
+        string fullValue;
+        string fullRoot;
+        try
+        {
+            fullValue = Path.GetFullPath(value);
+            fullRoot = Path.GetFullPath(contentRoot);
+        }
+        catch (Exception ex) when (ex is ArgumentException or NotSupportedException
+                                      or PathTooLongException or IOException)
+        {
+            // A syntactically invalid path is a different failure with its own message; refusing
+            // to guess here keeps one rule reporting one thing.
+            return;
+        }
+
+        // The separator is load-bearing, not cosmetic. Without it a bare prefix test treats
+        // "/srv/app-data" as inside "/srv/app" and refuses a legitimate sibling directory — and
+        // because this rule is fail-closed at startup, that false positive is an application that
+        // will not boot.
+        string rootWithSeparator = fullRoot.TrimEnd(
+            Path.DirectorySeparatorChar, Path.AltDirectorySeparatorChar) + Path.DirectorySeparatorChar;
+
+        // Path case sensitivity follows the OS, not a convention. Comparing case-insensitively
+        // everywhere would reject "/srv/App/store.db" as living under "/srv/app" on Linux, where
+        // those are genuinely different directories.
+        StringComparison comparison = OperatingSystem.IsWindows()
+            ? StringComparison.OrdinalIgnoreCase
+            : StringComparison.Ordinal;
+
+        if (fullValue.StartsWith(rootWithSeparator, comparison))
+        {
+            builder.Add(message);
+        }
+    }
+}
@@ -1,9 +1,12 @@
+using System.Linq;
 using Microsoft.Extensions.Configuration;
 using Microsoft.Extensions.DependencyInjection;
 using Microsoft.Extensions.DependencyInjection.Extensions;
+using Microsoft.Extensions.Hosting;
 using Microsoft.Extensions.Options;
 using ZB.MOM.WW.Audit;
 using ZB.MOM.WW.Secrets.Abstractions;
+using ZB.MOM.WW.Secrets.Configuration;
 using ZB.MOM.WW.Secrets.Crypto;
 using ZB.MOM.WW.Secrets.MasterKey;
 using ZB.MOM.WW.Secrets.Sqlite;
@@ -36,7 +39,28 @@ public static class SecretsServiceCollectionExtensions
        ArgumentNullException.ThrowIfNull(config);
        ArgumentException.ThrowIfNullOrWhiteSpace(sectionPath);

-        services.Configure<SecretsOptions>(config.GetSection(sectionPath));
+        // Bound AND validated, with ValidateOnStart, so a store path that would be destroyed by the
+        // next deployment fails the boot instead of surfacing later as a missing secret.
+        //
+        // The validator must be built by FACTORY, because it takes IHostEnvironment *optionally* —
+        // constructor injection would make the container throw wherever the secrets subsystem is
+        // composed without a host, which the CLI legitimately does.
+        //
+        // But AddZbSecrets is called more than once in real compositions (a replication package
+        // calls it internally after the application already has), so registration has to be
+        // idempotent — and TryAddEnumerable cannot do it here: it de-duplicates on implementation
+        // TYPE, which a factory descriptor does not carry, so it rejects the second call outright
+        // rather than skipping it. Guarding on the concrete registration gives both properties.
+        if (services.All(d => d.ServiceType != typeof(SecretsOptionsValidator)))
+        {
+            services.AddSingleton(sp => new SecretsOptionsValidator(sp.GetService<IHostEnvironment>()));
+            services.AddSingleton<IValidateOptions<SecretsOptions>>(
+                sp => sp.GetRequiredService<SecretsOptionsValidator>());
+        }
+
+        services.AddOptions<SecretsOptions>()
+            .Bind(config.GetSection(sectionPath))
+            .ValidateOnStart();

        // One connection factory targets the configured SQLite path. Singleton: it is stateless
        // aside from the path and opens a fresh connection per operation.
@@ -10,8 +10,48 @@ namespace ZB.MOM.WW.Secrets;
 /// </summary>
 public sealed class SecretsOptions
 {
-    /// <summary>The path to the SQLite database backing the secret store. Defaults to <c>secrets.db</c>.</summary>
-    public string SqlitePath { get; set; } = "secrets.db";
+    /// <summary>
+    /// The path to the SQLite database backing the secret store. Must be an absolute path outside
+    /// the application content root — see <see cref="Configuration.SecretsOptionsValidator"/>, which
+    /// enforces both at startup. Defaults to <see cref="DefaultSqlitePath"/>.
+    /// </summary>
+    public string SqlitePath { get; set; } = DefaultSqlitePath;
+
+    /// <summary>
+    /// The default store location: <c>ZB.MOM.WW/secrets.db</c> under the per-user local application
+    /// data folder (<c>~/.local/share</c> on Linux and macOS, <c>%LOCALAPPDATA%</c> on Windows).
+    /// </summary>
+    /// <remarks>
+    /// <para>
+    /// Per-user rather than machine-wide on purpose. <c>CommonApplicationData</c> is
+    /// <c>/usr/share</c> on Linux and macOS, which a normal user cannot write — as a library default
+    /// that would fail the boot of every consuming application on a developer machine until each
+    /// one set an absolute path. A guard whose first observable effect is breaking every local dev
+    /// loop is a guard that gets reverted, and then the rule protects nothing.
+    /// </para>
+    /// <para>
+    /// A consuming application is free to compute a different default (a machine-wide Windows
+    /// service reasonably prefers <c>CommonApplicationData</c> alongside its other stores); the
+    /// configured value always wins over this one. Production deployments set an explicit absolute
+    /// path regardless, so this default governs dev loops and unconfigured installs only.
+    /// </para>
+    /// <para>
+    /// When the platform reports no local-application-data folder, this stays relative on purpose.
+    /// The startup validator then rejects it with a message naming the key, which is a better
+    /// outcome than inventing a location for a credential store.
+    /// </para>
+    /// </remarks>
+    public static string DefaultSqlitePath { get; } = ComputeDefaultSqlitePath();
+
+    private static string ComputeDefaultSqlitePath()
+    {
+        string baseDirectory = Environment.GetFolderPath(
+            Environment.SpecialFolder.LocalApplicationData);
+
+        return string.IsNullOrEmpty(baseDirectory)
+            ? "secrets.db"
+            : Path.Combine(baseDirectory, "ZB.MOM.WW", "secrets.db");
+    }

    /// <summary>The master-key (KEK) resolution configuration. Defaults to a fresh <see cref="MasterKeyOptions"/>.</summary>
    public MasterKeyOptions MasterKey { get; set; } = new();
@@ -15,6 +15,7 @@
    <PackageReference Include="Microsoft.Extensions.Hosting.Abstractions" />
    <PackageReference Include="Microsoft.Extensions.Logging.Abstractions" />
    <PackageReference Include="ZB.MOM.WW.Audit" />
+    <PackageReference Include="ZB.MOM.WW.Configuration" />
  </ItemGroup>

  <PropertyGroup>