diff --git a/src/ZB.MOM.WW.ScadaBridge.Host/Program.cs b/src/ZB.MOM.WW.ScadaBridge.Host/Program.cs index 2a415cb9..2db51c2b 100644 --- a/src/ZB.MOM.WW.ScadaBridge.Host/Program.cs +++ b/src/ZB.MOM.WW.ScadaBridge.Host/Program.cs @@ -33,7 +33,6 @@ using ZB.MOM.WW.ScadaBridge.Transport; using ZB.MOM.WW.Secrets.Abstractions; using ZB.MOM.WW.Secrets.Configuration; using ZB.MOM.WW.Secrets.DependencyInjection; -using ZB.MOM.WW.Secrets.Sqlite; using ZB.MOM.WW.Secrets.Ui; using ZB.MOM.WW.Telemetry; using Serilog; @@ -75,11 +74,35 @@ var configuration = new ConfigurationBuilder() // Expand ${secret:...} config references before any validator/binder sees them (Layer A). // Throwaway provider — disposed here, shares no singletons with the host container. // -// This always reads the node's LOCAL SQLite store: Task 11 retired the gRPC secrets hub, the only -// topology that ever put a central node's store somewhere else pre-host. (Secrets:Store:Provider -// =SqlServer, the still-supported shared-central-store feature, is a post-host DI registration — -// see SecretsRegistration — and does not change what the Layer-A expander itself reads.) +// The expander MUST read the SAME store the running node will serve — SecretsRegistration. +// UsesCentralSharedStore is the single predicate deciding that, shared with the runtime +// registration in AddScadaBridgeSecrets, so the two can never disagree. On a Central node with +// Secrets:Store:Provider=SqlServer that is the SHARED SQL-Server store (Task 5): an expander left +// on SQLite there would resolve pre-host ${secret:} references (e.g. ScadaBridge:Security: +// JwtSigningKey) from a store the node does not actually serve, so a value rotated through the +// shared store would never reach the pre-host config, and two central nodes could disagree on +// what a token resolves to. Every other case (sites, the default per-node SQLite store) keeps the +// local SQLite path exactly as it always was. A blank or ${secret:}-valued connection string on +// the shared-store path fails HERE, through the same EnsureCentralSharedStoreConnectionString +// check AddScadaBridgeSecrets runs — this is the earliest code that would otherwise hand the bad +// value to SqlConnection, whose generic "initialization string" format error would bury the +// designed message. // +// Two consequences worth knowing. This block runs BEFORE Serilog exists and outside the +// try/catch below, so a SQL Server outage at central boot exits with a bare stderr stack +// trace, no structured log — honest and container-restart-retryable, just unenriched; the +// SQLite path had the same window but its only failure mode was a local file. (That exit +// is real only because of the UnhandledException handler at the top of this file — the +// runtime's own crash path wedges as container PID 1, ScadaBridge#34.) And the role read +// stays here at the composition root rather than inside SecretsRegistration: that class +// refuses to read the node role from configuration (a config-read role could silently turn a +// site into a shared-store central — see SecretsNodeRole), while Program.cs must read it to +// branch the whole composition anyway. +var expanderRole = string.Equals( + configuration["ScadaBridge:Node:Role"], "Central", StringComparison.OrdinalIgnoreCase) + ? SecretsNodeRole.Central + : SecretsNodeRole.Site; + // Secrets 0.6.2 validates Secrets:SqlitePath as absolute AND outside the content root, finding the // content root from IHostEnvironment — which this throwaway ServiceCollection does not have, and // which does not exist yet anyway (the host is not built until further down). Both the resolution @@ -87,14 +110,17 @@ var configuration = new ConfigurationBuilder() // AddPreHostSqliteExpander for why the content-root argument is load-bearing and silent when // dropped, and PreHostSecretsContentRootTests for the regression guard. var expanderServices = new ServiceCollection(); -expanderServices.AddPreHostSqliteExpander( - configuration, SecretsRegistration.ResolveHostContentRoot()); +expanderServices.AddPreHostExpander( + configuration, expanderRole, SecretsRegistration.ResolveHostContentRoot()); #pragma warning disable ASP0000 // deliberate throwaway container await using (var secretsProvider = expanderServices.BuildServiceProvider()) #pragma warning restore ASP0000 { - await secretsProvider.GetRequiredService() + // ISecretsStoreMigrator is the store-agnostic seam both AddZbSecrets (SQLite) and + // AddZbSecretsSqlServerStore register — resolving through it means this block does not need + // to know, or re-derive, which concrete store AddPreHostExpander just composed. + await secretsProvider.GetRequiredService() .MigrateAsync(default); var resolver = secretsProvider.GetRequiredService(); @@ -359,14 +385,14 @@ try .GetValue(nameof(ZB.MOM.WW.ScadaBridge.Security.Auth.AuthDisableLoginOptions.AllowOutsideDevelopment))); builder.Services.AddSecurity(disableLogin); builder.Services.AddCentralUI(); - // Local SQLite store by default. With Secrets:Replication:Enabled true it becomes either a - // local store synced against a shared SQL-Server hub (Mode=SqlServer, and only when a hub - // connection string is present) or — the production topology, scadaproj#3 — the HOST half - // of the pull-only gRPC secrets hub (Mode=Grpc), which central serves and sites sweep. - // The Central role never registers the sweep client; passing the role explicitly is what - // makes that a compile-time property of this composition root rather than a config guess. - // See SecretsRegistration for why the SQL gate exists (eager options validation), why the - // hub credential can never itself come from the hub, and why gRPC mode fails closed. + // Local SQLite store by default. With Secrets:Store:Provider=SqlServer this central node's + // store becomes the SHARED SQL-Server database instead — no hub, no follower sweep, just + // both central nodes reading and writing the same rows, so a clustered pair can never ship + // sites a drifted secret set (Task 5). The Layer-A pre-host expander above resolves this + // same predicate (SecretsRegistration.UsesCentralSharedStore) so it always reads the store + // this node actually serves. See SecretsRegistration for why the SQL gate exists (eager + // options validation) and why the connection string can never itself be a ${secret:} + // reference. builder.Services.AddScadaBridgeSecrets(builder.Configuration, SecretsNodeRole.Central); // Secrets UI authorization: adds the named policies secrets:manage + secrets:reveal // (role-based) consumed by the /admin/secrets page. AddSecretsAuthorization only ADDS diff --git a/src/ZB.MOM.WW.ScadaBridge.Host/SecretsRegistration.cs b/src/ZB.MOM.WW.ScadaBridge.Host/SecretsRegistration.cs index 4d23057a..768ccff5 100644 --- a/src/ZB.MOM.WW.ScadaBridge.Host/SecretsRegistration.cs +++ b/src/ZB.MOM.WW.ScadaBridge.Host/SecretsRegistration.cs @@ -62,9 +62,10 @@ public enum SecretsNodeRole /// Bootstrap constraint. The shared store's own connection string can never itself come from /// that store — a node cannot read the database to learn how to reach the database. It must arrive /// from outside it: an environment variable, or a literal value in appsettings. It can never be a -/// ${secret:} reference, because on central the pre-host expander that would resolve such a -/// reference runs against the very store this string points at (see Program.cs) — the -/// reference could never resolve, so registration rejects it outright. +/// ${secret:} reference, because on central the pre-host expander (, +/// called from Program.cs — see ) resolves EVERY +/// ${secret:} token in configuration, including this one, from the very store this string +/// points at — the reference could never resolve, so registration rejects it outright. /// /// public static class SecretsRegistration @@ -87,6 +88,30 @@ public static class SecretsRegistration /// public const string StoreProviderKey = "Secrets:Store:Provider"; + /// + /// Whether this node's secret store is the shared SQL-Server database — the single predicate + /// behind BOTH 's runtime registration AND + /// 's Layer-A ${secret:} expander, so the two can never + /// disagree about which store a Central node serves. That matters more than tidiness: an + /// expander that resolved boot-time ${secret:} tokens from the per-node SQLite file + /// while the running node served the shared SQL store would let two central nodes boot with + /// different resolved values (e.g. a different JwtSigningKey) — exactly the per-node + /// drift =SqlServer exists to prevent. + /// + /// Application configuration. + /// Which half of the composition this node is. + /// + /// when is + /// and is (case-insensitively) SqlServer. + /// + internal static bool UsesCentralSharedStore(IConfiguration config, SecretsNodeRole role) + { + ArgumentNullException.ThrowIfNull(config); + + return role == SecretsNodeRole.Central + && string.Equals(config[StoreProviderKey], "SqlServer", StringComparison.OrdinalIgnoreCase); + } + /// /// Resolves the content root the HOST will use, from outside the host — the Layer-A /// ${secret:} expander in Program.cs runs before any builder exists, so @@ -146,6 +171,62 @@ public static class SecretsRegistration return services.AddZbSecrets(config, SecretsSectionPath, contentRoot); } + /// + /// Composes the Layer-A pre-host ${secret:} expander stack for — + /// the shared SQL-Server store when is true, otherwise the + /// plain local SQLite store (). + /// + /// + /// + /// This is the fix for the gap 's own remarks describe: the + /// expander MUST read the same store will register for the + /// running node, or a boot-time ${secret:} reference (e.g. in + /// ScadaBridge:Security:JwtSigningKey) resolves from a store the node does not actually + /// serve. Both this method and branch on the exact same + /// call, so they cannot drift apart the way the old + /// Grpc-mode expander branch in Program.cs and the Grpc-mode runtime branch here once + /// had to be kept in step by hand. + /// + /// + /// On the shared-store path this also runs + /// BEFORE composing anything — the same fail-closed rules apply here as at runtime: a blank + /// connection string, or one holding a ${secret:} reference (which could never resolve; + /// this very expander is what would have to resolve it), fails the boot with the designed + /// message instead of a generic SqlConnection format error. + /// + /// + /// The throwaway expander service collection. + /// Application configuration. + /// Which half of the composition this node is. + /// The host's content root, from — only used on the SQLite path. + /// The same collection, for chaining. + /// + /// is Central with the shared store selected, and + /// is blank or a ${secret:} reference. + /// + public static IServiceCollection AddPreHostExpander( + this IServiceCollection services, + IConfiguration config, + SecretsNodeRole role, + string contentRoot) + { + ArgumentNullException.ThrowIfNull(services); + ArgumentNullException.ThrowIfNull(config); + ArgumentException.ThrowIfNullOrWhiteSpace(contentRoot); + + if (UsesCentralSharedStore(config, role)) + { + EnsureCentralSharedStoreConnectionString(config, $"{StoreProviderKey}=SqlServer"); + + // Same 3-arg overload as the runtime registration in AddScadaBridgeSecrets — this is + // deliberately the SAME store, not a lookalike one composed independently. + return services.AddZbSecretsSqlServerStore( + config, SecretsSectionPath, SqlServerSectionPath); + } + + return services.AddPreHostSqliteExpander(config, contentRoot); + } + /// /// The one definition of "a Central node has a usable shared-store connection string", shared /// by every caller that puts a Central node on the shared SQL-Server store — the @@ -230,9 +311,10 @@ public static class SecretsRegistration // Secrets:Store:Provider=SqlServer on Central only. A Site never reaches this: it always // keeps its local SQLite store, straight through a WAN outage, same as every other - // topology, and never decorated — see the class remarks. - if (role == SecretsNodeRole.Central - && string.Equals(config[StoreProviderKey], "SqlServer", StringComparison.OrdinalIgnoreCase)) + // topology, and never decorated — see the class remarks. UsesCentralSharedStore is the + // SAME predicate AddPreHostExpander branches on, so the two can never disagree about + // which store a Central node serves. + if (UsesCentralSharedStore(config, role)) { // Fail-closed rules: blank, or a ${secret:} reference (bootstrap circularity — the // pre-host expander needs this same store to resolve it). diff --git a/src/ZB.MOM.WW.ScadaBridge.Host/StartupValidator.cs b/src/ZB.MOM.WW.ScadaBridge.Host/StartupValidator.cs index 9933c150..713048fa 100644 --- a/src/ZB.MOM.WW.ScadaBridge.Host/StartupValidator.cs +++ b/src/ZB.MOM.WW.ScadaBridge.Host/StartupValidator.cs @@ -31,7 +31,7 @@ public static class StartupValidator // Deploy Artifacts, and a replicated site store would publish DeployedSecretsMirror's local // deletes fleet-wide. Checked unconditionally (both roles): a Site used to be the gRPC // hub's follower half, so the flag was never Central-only. - var secretsReplicationEnabled = configuration.GetValue("Secrets:Replication:Enabled"); + var secretsReplicationEnabled = IsSecretsReplicationEnabled(configuration); // GrpcPort: default 8083 when absent; only fails the range rule when the key is // present AND invalid. The out-param assignment mirrors the original so the @@ -251,6 +251,7 @@ public static class StartupValidator var seedNodes = configuration.GetSection("ScadaBridge:Cluster:SeedNodes").Get>(); var provider = InboundApiKeyStoreOptions.ReadProvider(configuration); var secretsStoreProvider = configuration["Secrets:Store:Provider"]; + var secretsReplicationEnabled = IsSecretsReplicationEnabled(configuration); var isClusteredCentral = role == "Central" && seedNodes is { Count: >= 2 }; var warnings = new List(); @@ -283,9 +284,46 @@ public static class StartupValidator + "deploy its own secret set to sites; set Secrets:Store:Provider=SqlServer"); } + // Review round 1, Minor #4: a config that still carries Secrets:Replication or + // Secrets:GrpcHub child keys — left over from before Task 11 retired the hub — is not + // itself an error (Validate already rejects Enabled=true outright), but every key under + // either section is now silently ignored. That is worth a Warning so an operator cleans + // QA/prod appsettings rather than assuming a still-present Mode/BearerToken/Endpoint value + // does anything. Excludes the bare "Enabled" key itself: "Enabled: false" alone is the + // normal, still-valid shape of every existing deployment's config and must not warn. + var hasStaleReplicationKeys = configuration.GetSection("Secrets:Replication").GetChildren() + .Any(child => !string.Equals(child.Key, "Enabled", StringComparison.OrdinalIgnoreCase)); + var hasStaleGrpcHubKeys = configuration.GetSection("Secrets:GrpcHub").GetChildren().Any(); + + if (!secretsReplicationEnabled && (hasStaleReplicationKeys || hasStaleGrpcHubKeys)) + { + warnings.Add( + "Secrets:Replication / Secrets:GrpcHub settings are ignored — the secrets hub is " + + "retired; remove them"); + } + return warnings; } + /// + /// Whether Secrets:Replication:Enabled is "set" — read as a raw string rather than + /// IConfiguration.GetValue<bool> (review round 1, Minor #3), which throws a raw + /// binder on a non-boolean value like "yes" or + /// "1", so a malformed leftover key would surface as an unrelated parse crash instead of + /// the designed retirement message in . Any value other than absent, + /// empty, or (case-insensitively) "false" counts as "set" — deliberately stricter than + /// bool parsing, so a typo'd leftover (Enabled: 1, Enabled: yes) still gets the + /// explanatory message rather than silently passing as falsy. Shared by + /// (which rejects "set" outright) and (which, for the "not set" + /// case, separately warns about any OTHER leftover key under the same retired sections). + /// + private static bool IsSecretsReplicationEnabled(IConfiguration configuration) + { + var raw = configuration["Secrets:Replication:Enabled"]; + return !string.IsNullOrEmpty(raw) + && !string.Equals(raw, "false", StringComparison.OrdinalIgnoreCase); + } + /// /// Extracts the TCP port from an Akka seed-node address of the form /// akka.tcp://system@host:port. Returns -1 when no port can be parsed. diff --git a/src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Secrets/DeployedSecretsMirror.cs b/src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Secrets/DeployedSecretsMirror.cs index 407ce58a..c02e8cd5 100644 --- a/src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Secrets/DeployedSecretsMirror.cs +++ b/src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Secrets/DeployedSecretsMirror.cs @@ -116,13 +116,16 @@ public sealed record MirrorResult( /// can observe a stale pre-rotation plaintext for up to the cache TTL. /// /// -/// Node-local store assumption (review Minor #5). This mirror assumes the site's -/// is node-local (plain local SQLite, or the gRPC-hub follower half, -/// both of which keep writes local). It is NOT safe to run against a site node whose store is -/// decorated by SQL-Server secrets REPLICATION (Secrets:Replication:Mode=SqlServer) — its -/// DeleteAsync publishes the tombstone to the shared hub, so a site merely un-referencing -/// a secret would delete it fleet-wide. Task 11 rejects that configuration on the Site role; -/// no code change is made here. +/// Node-local store assumption (review Minor #5; Task 11 removed every replication mode). +/// This mirror assumes the site's is always plain, node-local, local +/// SQLite — the ONLY shape a Site node's store can be after Task 11 retired both the pull-only +/// gRPC secrets hub and the SQL-Server ReplicatingSecretStore mode that preceded it (see +/// SecretsRegistration). It would NOT have been safe to run this mirror against a store +/// decorated by either retired replication mode: DeleteAsync would have published the +/// tombstone to every other node sharing the decorator, so a site merely un-referencing a secret +/// would have deleted it fleet-wide. StartupValidator now rejects +/// Secrets:Replication:Enabled=true outright on either role, so that configuration can no +/// longer reach this mirror at all. /// /// /// Resolve semantics and the resolver cache. The trial-resolve verify step goes through diff --git a/tests/ZB.MOM.WW.ScadaBridge.Host.Tests/SecretsReplicationWiringTests.cs b/tests/ZB.MOM.WW.ScadaBridge.Host.Tests/SecretsReplicationWiringTests.cs index 3c933d52..01184ffa 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.Host.Tests/SecretsReplicationWiringTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.Host.Tests/SecretsReplicationWiringTests.cs @@ -36,6 +36,17 @@ public class SecretsReplicationWiringTests private const string DummyConnectionString = "Server=unused;Database=x;Integrated Security=true;"; + /// + /// A fresh directory, never the one holding 's default + /// Secrets:SqlitePath (Path.GetTempPath() itself) — the SQLite path-rules + /// validator runs eagerly on the first IOptions<SecretsOptions>.Value read (i.e. + /// on resolving ISecretStore itself, not merely on migrating), so passing + /// Path.GetTempPath() as the content root here would make every SQLite-path expander + /// test fail on "path is inside the content root" instead of testing what it means to. + /// + private static string NewContentRoot() => + Path.Combine(Path.GetTempPath(), "sb-secrets-wiring-content-root-" + Guid.NewGuid().ToString("N")); + private static IConfiguration BuildConfig(params (string Key, string Value)[] overrides) { var settings = new Dictionary @@ -214,4 +225,121 @@ public class SecretsReplicationWiringTests // the configured value, because a mistyped value here could be a pasted real credential. Assert.DoesNotContain("hub-connection-string", ex.Message, StringComparison.Ordinal); } + + // --------------------------------------------------------------------------------------- + // The Layer-A pre-host ${secret:} expander (SecretsRegistration.AddPreHostExpander, called + // from Program.cs) — review round 1, Important #1. It MUST compose against the SAME store + // AddScadaBridgeSecrets registers for the running node: an expander that resolved boot-time + // ${secret:} tokens (e.g. ScadaBridge:Security:JwtSigningKey) from the per-node SQLite file + // while the running node served the shared SQL store would let two central nodes disagree on + // a rotated value — exactly the per-node drift Store:Provider=SqlServer exists to prevent. + // UsesCentralSharedStore is the single predicate shared by both methods. + // --------------------------------------------------------------------------------------- + + [Fact] + public void UsesCentralSharedStore_TrueOnlyForCentralWithProviderSqlServer() + { + IConfiguration sqlServerConfig = BuildConfig(("Secrets:Store:Provider", "SqlServer")); + IConfiguration sqliteConfig = BuildConfig(); + + Assert.True( + SecretsRegistration.UsesCentralSharedStore(sqlServerConfig, SecretsNodeRole.Central)); + Assert.False( + SecretsRegistration.UsesCentralSharedStore(sqlServerConfig, SecretsNodeRole.Site)); + Assert.False( + SecretsRegistration.UsesCentralSharedStore(sqliteConfig, SecretsNodeRole.Central)); + } + + [Fact] + public void ExpanderCentral_StoreProviderSqlServer_ResolvesSharedSqlStore() + { + // The bug this guards against: the expander used to be wired ONLY to the retired Grpc + // mode, so a Central node on Store:Provider=SqlServer (the retirement message's own + // prescribed replacement) resolved boot-time ${secret:} tokens from the wrong store. + IConfiguration config = BuildConfig( + ("Secrets:Store:Provider", "SqlServer"), + ("Secrets:SqlServer:ConnectionString", DummyConnectionString)); + + var services = new ServiceCollection(); + services.AddLogging(); + services.AddPreHostExpander(config, SecretsNodeRole.Central, NewContentRoot()); + + using ServiceProvider provider = services.BuildServiceProvider(validateScopes: true); + + Assert.IsType(provider.GetRequiredService()); + // The store-agnostic migrator seam — resolving through it is what lets Program.cs's + // expander block stay store-agnostic instead of re-deriving which concrete migrator to + // ask for. + Assert.NotNull(provider.GetRequiredService()); + } + + [Fact] + public void ExpanderCentral_Default_ResolvesSqliteStore() + { + var services = new ServiceCollection(); + services.AddLogging(); + services.AddPreHostExpander(BuildConfig(), SecretsNodeRole.Central, NewContentRoot()); + + using ServiceProvider provider = services.BuildServiceProvider(validateScopes: true); + + Assert.IsType(provider.GetRequiredService()); + Assert.NotNull(provider.GetRequiredService()); + } + + [Fact] + public void ExpanderSite_StoreProviderSqlServer_StillResolvesSqliteStore() + { + // A Site ignores Store:Provider entirely, exactly as the runtime registration does — the + // expander must not be a second place where that rule could be got wrong. + IConfiguration config = BuildConfig(("Secrets:Store:Provider", "SqlServer")); + + var services = new ServiceCollection(); + services.AddLogging(); + services.AddPreHostExpander(config, SecretsNodeRole.Site, NewContentRoot()); + + Assert.DoesNotContain(services, IsFromSqlServerPackage); + + using ServiceProvider provider = services.BuildServiceProvider(validateScopes: true); + + Assert.IsType(provider.GetRequiredService()); + } + + [Fact] + public void ExpanderCentral_StoreProviderSqlServer_BlankConnectionString_Throws() + { + var services = new ServiceCollection(); + services.AddLogging(); + + IConfiguration config = BuildConfig(("Secrets:Store:Provider", "SqlServer")); + + var ex = Assert.Throws( + () => services.AddPreHostExpander(config, SecretsNodeRole.Central, NewContentRoot())); + + Assert.Contains( + SecretsRegistration.HubConnectionStringKey, ex.Message, StringComparison.Ordinal); + Assert.Contains( + $"{SecretsRegistration.StoreProviderKey}=SqlServer", ex.Message, StringComparison.Ordinal); + } + + [Fact] + public void ExpanderCentral_StoreProviderSqlServer_SecretReferenceConnectionString_Throws() + { + // The bootstrap-circularity rule applies here too: this expander is exactly the code that + // would otherwise have to resolve the reference in order to build the store that resolves + // it. + var services = new ServiceCollection(); + services.AddLogging(); + + IConfiguration config = BuildConfig( + ("Secrets:Store:Provider", "SqlServer"), + ("Secrets:SqlServer:ConnectionString", "${secret:hub-connection-string}")); + + var ex = Assert.Throws( + () => services.AddPreHostExpander(config, SecretsNodeRole.Central, NewContentRoot())); + + Assert.Contains( + SecretsRegistration.HubConnectionStringKey, ex.Message, StringComparison.Ordinal); + Assert.Contains("${secret:", ex.Message, StringComparison.Ordinal); + Assert.DoesNotContain("hub-connection-string", ex.Message, StringComparison.Ordinal); + } } diff --git a/tests/ZB.MOM.WW.ScadaBridge.Host.Tests/StartupValidatorTests.cs b/tests/ZB.MOM.WW.ScadaBridge.Host.Tests/StartupValidatorTests.cs index 934b63f1..5b88d844 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.Host.Tests/StartupValidatorTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.Host.Tests/StartupValidatorTests.cs @@ -784,6 +784,11 @@ public class StartupValidatorTests [Theory] [InlineData("true")] [InlineData("True")] + // Review round 1, Minor #3: these are not valid booleans. GetValue used to throw a raw + // OptionsValidationException-style binder crash on them instead of the designed message; the + // key is read as a raw string now, so any non-"false" value is "set" and rejected the same way. + [InlineData("yes")] + [InlineData("1")] public void ReplicationEnabled_IsRejected_OnCentral(string value) { var values = ValidCentralConfig(); @@ -825,6 +830,117 @@ public class StartupValidatorTests Assert.Null(ex); } + [Fact] + public void ReplicationDisabled_CaseInsensitive_PassesValidation() + { + var values = ValidCentralConfig(); + values["Secrets:Replication:Enabled"] = "FALSE"; + var config = BuildConfig(values); + + var ex = Record.Exception(() => StartupValidator.Validate(config)); + + Assert.Null(ex); + } + + /// + /// Deliberately stricter than bool parsing (review round 1, Minor #3): only an absent + /// key or exactly (case-insensitively) "false" is safe. "0" is a legal + /// bool.TryParse-adjacent falsy spelling in some conventions but is NOT exempted here — + /// a leftover key this malformed should surface the retirement message, not silently pass. + /// + [Fact] + public void ReplicationEnabled_Zero_IsRejected() + { + var values = ValidCentralConfig(); + values["Secrets:Replication:Enabled"] = "0"; + var config = BuildConfig(values); + + var ex = Assert.Throws(() => StartupValidator.Validate(config)); + + Assert.Contains( + "Secrets replication (the gRPC hub) is retired in ScadaBridge", ex.Message); + } + + /// + /// Review round 1, Minor #4: a leftover Secrets:Replication/Secrets:GrpcHub key + /// from before Task 11 is not itself a boot failure (Validate already rejects Enabled=true + /// outright), but every OTHER key under either section is now silently ignored — worth a + /// Warning so an operator cleans up QA/prod appsettings. + /// + [Fact] + public void CollectWarnings_WarnsForLeftoverReplicationModeKey() + { + var values = ValidCentralConfig(); + values["Secrets:Store:Provider"] = "SqlServer"; + values["Secrets:Replication:Mode"] = "SqlServer"; + var config = BuildConfig(values); + + var warnings = StartupValidator.CollectWarnings(config); + + Assert.Contains(warnings, w => + w.Contains("Secrets:Replication / Secrets:GrpcHub settings are ignored", StringComparison.Ordinal) + && w.Contains("the secrets hub is retired; remove them", StringComparison.Ordinal)); + } + + [Fact] + public void CollectWarnings_WarnsForLeftoverGrpcHubKey() + { + var values = ValidCentralConfig(); + values["Secrets:Store:Provider"] = "SqlServer"; + values["Secrets:GrpcHub:BearerToken"] = "stale-leftover-token"; + var config = BuildConfig(values); + + var warnings = StartupValidator.CollectWarnings(config); + + Assert.Contains(warnings, w => + w.Contains("Secrets:Replication / Secrets:GrpcHub settings are ignored", StringComparison.Ordinal)); + } + + [Fact] + public void CollectWarnings_WarnsForLeftoverKeys_OnSiteToo() + { + // The warning is not central-scoped: a Site config can carry leftover GrpcHub follower + // settings just as easily. + var values = ValidSiteConfig(); + values["Secrets:GrpcHub:Endpoint"] = "http://stale-central:8083"; + var config = BuildConfig(values); + + var warnings = StartupValidator.CollectWarnings(config); + + Assert.Contains(warnings, w => + w.Contains("Secrets:Replication / Secrets:GrpcHub settings are ignored", StringComparison.Ordinal)); + } + + [Fact] + public void CollectWarnings_SilentForBareReplicationEnabledFalse() + { + // "Enabled: false" alone is the normal, still-valid shape of every existing deployment's + // config (it is what ships in appsettings.json) — this must NOT warn just because the + // Secrets:Replication section exists. + var values = ValidCentralConfig(); + values["Secrets:Store:Provider"] = "SqlServer"; + values["Secrets:Replication:Enabled"] = "false"; + var config = BuildConfig(values); + + var warnings = StartupValidator.CollectWarnings(config); + + Assert.DoesNotContain(warnings, w => + w.Contains("Secrets:Replication / Secrets:GrpcHub settings are ignored", StringComparison.Ordinal)); + } + + [Fact] + public void CollectWarnings_SilentWhenNeitherSectionPresent() + { + var values = ValidCentralConfig(); + values["Secrets:Store:Provider"] = "SqlServer"; + var config = BuildConfig(values); + + var warnings = StartupValidator.CollectWarnings(config); + + Assert.DoesNotContain(warnings, w => + w.Contains("Secrets:Replication / Secrets:GrpcHub settings are ignored", StringComparison.Ordinal)); + } + [Fact] public void CollectWarnings_SilentForSingleSeedCentral() {