From a57c36f5043009202162c28798675e3e57844965 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Mon, 28 Sep 2026 17:56:30 -0400 Subject: [PATCH] =?UTF-8?q?fix(secrets):=20review=20round=201=20=E2=80=94?= =?UTF-8?q?=20gate=20the=20clustered-central=20secrets=20warning=20on=20Re?= =?UTF-8?q?plication:Enabled,=20name=20the=20real=20trigger=20in=20the=20s?= =?UTF-8?q?hared-store=20connection-string=20message,=20pin=20Grpc-vs-Stor?= =?UTF-8?q?e:Provider=20precedence?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- src/ZB.MOM.WW.ScadaBridge.Host/Program.cs | 3 +- .../SecretsRegistration.cs | 36 ++++++++++++------- .../StartupValidator.cs | 10 ++++++ .../SecretsReplicationWiringTests.cs | 34 +++++++++++++++++- .../StartupValidatorTests.cs | 19 ++++++++++ 5 files changed, 88 insertions(+), 14 deletions(-) diff --git a/src/ZB.MOM.WW.ScadaBridge.Host/Program.cs b/src/ZB.MOM.WW.ScadaBridge.Host/Program.cs index 1104eb0f..921b5069 100644 --- a/src/ZB.MOM.WW.ScadaBridge.Host/Program.cs +++ b/src/ZB.MOM.WW.ScadaBridge.Host/Program.cs @@ -111,7 +111,8 @@ var expanderUsesSharedSqlStore = var expanderServices = new ServiceCollection(); if (expanderUsesSharedSqlStore) { - SecretsRegistration.EnsureCentralSharedStoreConnectionString(configuration); + SecretsRegistration.EnsureCentralSharedStoreConnectionString( + configuration, $"{SecretsRegistration.ReplicationModeKey}=Grpc"); expanderServices.AddZbSecretsSqlServerStore(configuration, "Secrets"); } else diff --git a/src/ZB.MOM.WW.ScadaBridge.Host/SecretsRegistration.cs b/src/ZB.MOM.WW.ScadaBridge.Host/SecretsRegistration.cs index 821138c0..5a144b39 100644 --- a/src/ZB.MOM.WW.ScadaBridge.Host/SecretsRegistration.cs +++ b/src/ZB.MOM.WW.ScadaBridge.Host/SecretsRegistration.cs @@ -257,34 +257,45 @@ public static class SecretsRegistration } /// - /// The one definition of "a Central node in Grpc mode has a usable shared-store connection - /// string", shared by and the Layer-A expander in - /// Program.cs so the two can never diverge. The expander runs FIRST on a real boot — - /// without this sharing, a bad value would die inside SqlConnection with a generic - /// "initialization string" format error before the designed message ever fired. + /// 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 Grpc-hub + /// branch of , the =SqlServer + /// branch, and the Layer-A expander in Program.cs — so none of them can diverge on what + /// "usable" means. The expander runs FIRST on a real boot — without this sharing, a bad value + /// would die inside SqlConnection with a generic "initialization string" format error + /// before the designed message ever fired. /// /// Application configuration. + /// + /// Names the setting that put this Central node on the shared store, e.g. + /// "Secrets:Replication:Mode=Grpc" or "Secrets:Store:Provider=SqlServer" — folded + /// into the blank-connection-string message so a boot failure on either path names its own + /// real trigger instead of misdescribing the other one (a Store:Provider=SqlServer failure + /// must not claim Grpc mode is in effect, and vice versa). + /// /// The validated connection string. /// /// is blank or contains a ${secret:} reference. /// - internal static string EnsureCentralSharedStoreConnectionString(IConfiguration config) + internal static string EnsureCentralSharedStoreConnectionString( + IConfiguration config, string triggerContext) { ArgumentNullException.ThrowIfNull(config); + ArgumentException.ThrowIfNullOrWhiteSpace(triggerContext); var hubConnectionString = config[HubConnectionStringKey]; if (string.IsNullOrWhiteSpace(hubConnectionString)) { - // Fail closed, matching the mode's philosophy: central in Grpc mode REQUIRES the + // Fail closed, matching the mode's philosophy: this configuration REQUIRES the // shared SQL store. An independent local store per central node is exactly the // divergence scadaproj#4 recorded — one central hub answered authenticated followers // with an EMPTY manifest, so a hub failover would "succeed" against nothing and stop // convergence silently. throw new InvalidOperationException( $"{HubConnectionStringKey} is empty, but this node is Central with " - + $"{ReplicationModeKey}=Grpc. Central in Grpc mode requires the " - + "SHARED SQL-Server secret store — an independent local store per " + + $"{triggerContext}. That configuration requires the SHARED " + + "SQL-Server secret store — an independent local store per " + "central node is the divergence scadaproj#4 recorded (a hub " + "failover would 'succeed' against an empty manifest). Supply the " + "connection string via appsettings or the environment " @@ -341,8 +352,9 @@ public static class SecretsRegistration { // Same fail-closed rules as the Grpc-mode central branch below: blank, or a // ${secret:} reference (bootstrap circularity — the pre-host expander needs this - // same store to resolve it). - EnsureCentralSharedStoreConnectionString(config); + // same store to resolve it). Names ITS OWN trigger (Store:Provider=SqlServer, not + // Replication:Mode=Grpc) — this branch runs only when replication is off. + EnsureCentralSharedStoreConnectionString(config, $"{StoreProviderKey}=SqlServer"); // Registers the shared SQL-Server store as ISecretStore directly — no hub, no // follower sweep. Both central nodes just point at the same database, so they serve @@ -364,7 +376,7 @@ public static class SecretsRegistration // instead of surfacing later as a package validation message or a // malformed-connection-string fault on first use. On a real boot the Layer-A // expander in Program.cs has already run the same check even earlier. - EnsureCentralSharedStoreConnectionString(config); + EnsureCentralSharedStoreConnectionString(config, $"{ReplicationModeKey}=Grpc"); // Central's store is the SHARED SQL-Server store: registers // SqlServerSecretStore as ISecretStore BEFORE calling AddZbSecrets internally, diff --git a/src/ZB.MOM.WW.ScadaBridge.Host/StartupValidator.cs b/src/ZB.MOM.WW.ScadaBridge.Host/StartupValidator.cs index a353e2c6..ef65431e 100644 --- a/src/ZB.MOM.WW.ScadaBridge.Host/StartupValidator.cs +++ b/src/ZB.MOM.WW.ScadaBridge.Host/StartupValidator.cs @@ -238,6 +238,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 = configuration.GetValue("Secrets:Replication:Enabled"); var isClusteredCentral = role == "Central" && seedNodes is { Count: >= 2 }; var warnings = new List(); @@ -255,7 +256,16 @@ public static class StartupValidator // explicitly "Sqlite". A fleet that ships secrets to sites on every artifact deploy needs // both central nodes to deploy the SAME secret set; a per-node store means each node has // silently drifted the moment anyone edits a secret on only one of them. + // + // Gated off when Secrets:Replication:Enabled is true (review round 1, finding #1): that + // flag already puts this central node on a topology that shares state across the pair — + // the pre-existing gRPC hub (whose OWN central-side store is the shared SQL-Server store, + // not per-node SQLite — see SecretsRegistration's Grpc branch) or SQL-Server replication + // (ReplicatingSecretStore). Firing this warning there would be a false positive against + // the design's own named production topology (scadaproj#3), telling an operator to set a + // key that is already functionally moot. if (isClusteredCentral + && !secretsReplicationEnabled && !string.Equals(secretsStoreProvider, "SqlServer", StringComparison.OrdinalIgnoreCase)) { warnings.Add( diff --git a/tests/ZB.MOM.WW.ScadaBridge.Host.Tests/SecretsReplicationWiringTests.cs b/tests/ZB.MOM.WW.ScadaBridge.Host.Tests/SecretsReplicationWiringTests.cs index 3a1b2213..03d4e601 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.Host.Tests/SecretsReplicationWiringTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.Host.Tests/SecretsReplicationWiringTests.cs @@ -230,9 +230,15 @@ public class SecretsReplicationWiringTests var ex = Assert.Throws( () => services.AddScadaBridgeSecrets(config, SecretsNodeRole.Central)); - // Same helper, same message style as the Grpc-mode central check. + // Same helper, same message style as the Grpc-mode central check — but this path's + // message must name ITS OWN trigger (review round 1, finding #2), never claim + // Secrets:Replication:Mode=Grpc, which is not in effect here. Assert.Contains( SecretsRegistration.HubConnectionStringKey, ex.Message, StringComparison.Ordinal); + Assert.Contains( + $"{SecretsRegistration.StoreProviderKey}=SqlServer", ex.Message, StringComparison.Ordinal); + Assert.DoesNotContain( + $"{SecretsRegistration.ReplicationModeKey}=Grpc", ex.Message, StringComparison.Ordinal); } [Fact] @@ -267,6 +273,32 @@ public class SecretsReplicationWiringTests Assert.IsType(provider.GetRequiredService()); } + /// + /// Precedence pin (review round 1, finding #3): with BOTH triggers configured, the + /// pre-existing Secrets:Replication:Enabled=true Grpc-hub topology wins, and the new + /// Store:Provider=SqlServer branch defers to it rather than skipping hub registration. The + /// Store:Provider branch's own guard (!ReplicationEnabledKey) is what makes this so — + /// this test locks the ordering in against a future refactor of + /// that reorders the two branches. + /// + [Fact] + public void Central_ReplicationEnabledGrpcMode_TakesPrecedenceOverStoreProvider() + { + IConfiguration config = BuildConfig( + [ + .. GrpcCentralConfig(), + ("Secrets:Store:Provider", "SqlServer"), + ]); + + using ServiceProvider provider = BuildProvider(config, SecretsNodeRole.Central); + + // Same shared SQL-Server store either way, but the Grpc hub is ALSO wired here — the + // Store:Provider-only path never registers the interceptor or the follower sweep. + Assert.IsType(provider.GetRequiredService()); + Assert.NotNull(provider.GetService()); + Assert.True(SecretsRegistration.UsesGrpcHub(config)); + } + // --------------------------------------------------------------------------------------- // SQL-Server mode — regression pins. Behaviour must be byte-for-byte what it was before the // mode key existed, both when the mode is left unset and when it is named explicitly. diff --git a/tests/ZB.MOM.WW.ScadaBridge.Host.Tests/StartupValidatorTests.cs b/tests/ZB.MOM.WW.ScadaBridge.Host.Tests/StartupValidatorTests.cs index 3d5c3527..628fe73f 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.Host.Tests/StartupValidatorTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.Host.Tests/StartupValidatorTests.cs @@ -771,6 +771,25 @@ public class StartupValidatorTests Assert.Empty(warnings); } + [Fact] + public void ClusteredCentral_ReplicationEnabled_NoSecretsStoreWarning() + { + // Review round 1, finding #1: Secrets:Replication:Enabled=true already puts this central + // node on a topology that shares state across the pair — the pre-existing gRPC hub + // (whose central-side store is the shared SQL-Server store, not per-node SQLite) or + // SQL-Server replication. No Secrets:Store:Provider is set at all here — the exact case + // that used to warn before this fix — and it must stay silent because the node is not + // actually on a per-node SQLite store in this configuration. + var values = ValidCentralConfig(); + values["ScadaBridge:InboundApi:ApiKeyStore:Provider"] = "SqlServer"; + values["Secrets:Replication:Enabled"] = "true"; + var config = BuildConfig(values); + + var warnings = StartupValidator.CollectWarnings(config); + + Assert.Empty(warnings); + } + [Fact] public void CollectWarnings_SilentForSingleSeedCentral() {