dohertj2/ScadaBridge
1
0
Fork 0
Code Issues 2 Pull Requests Actions Packages Projects Releases Wiki Activity

fix(secrets): review round 1 — pre-host expander reads the same store the runtime serves, raw-string Replication:Enabled parse, leftover-key warning

Browse Source 
- SecretsRegistration: UsesCentralSharedStore predicate shared by AddScadaBridgeSecrets and the
  new AddPreHostExpander, so a Central node's Layer-A ${secret:} expander composes against the
  shared SQL store whenever Secrets:Store:Provider=SqlServer does — closing the gap where a
  central migrated off the retired Grpc topology would boot-time-resolve config from per-node
  SQLite while serving the shared store.
- Program.cs: expander block now calls AddPreHostExpander + resolves the store-agnostic
  ISecretsStoreMigrator seam; stale Grpc-hub/replication comment rewritten.
- DeployedSecretsMirror.cs: stale "gRPC-hub follower half" doc comment rewritten to state the
  now-permanent local-SQLite-only invariant.
- StartupValidator: Secrets:Replication:Enabled read as a raw string (any non-empty, non-"false"
  value rejected) instead of GetValue<bool>, which threw a raw binder crash on "yes"/"1"; added a
  CollectWarnings entry for leftover Secrets:Replication/Secrets:GrpcHub child keys once Enabled
  is false/unset.
- Tests: +15 (AddPreHostExpander/UsesCentralSharedStore wiring, Replication:Enabled edge values,
  leftover-key warnings). Full solution build and full Host.Tests green (593/593).
This commit is contained in:
Joseph Doherty
2026-09-28 20:10:17 -04:00
parent 2bac13170c
commit bc29dc412b
6 changed files with 423 additions and 30 deletions
Download Patch File Download Diff File Expand all files Collapse all files
tests/ZB.MOM.WW.ScadaBridge.Host.Tests/SecretsReplicationWiringTests.cs
+128
View File
@@ -36,6 +36,17 @@ public class SecretsReplicationWiringTests
private const string DummyConnectionString =
"Server=unused;Database=x;Integrated Security=true;";
/// <summary>
/// A fresh directory, never the one holding <see cref="BuildConfig"/>'s default
/// <c>Secrets:SqlitePath</c> (<c>Path.GetTempPath()</c> itself) — the SQLite path-rules
/// validator runs eagerly on the first <c>IOptions&lt;SecretsOptions&gt;.Value</c> read (i.e.
/// on resolving <c>ISecretStore</c> itself, not merely on migrating), so passing
/// <c>Path.GetTempPath()</c> 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.
/// </summary>
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<string, string?>
@@ -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<SqlServerSecretStore>(provider.GetRequiredService<ISecretStore>());
// 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<ISecretsStoreMigrator>());
}
[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<SqliteSecretStore>(provider.GetRequiredService<ISecretStore>());
Assert.NotNull(provider.GetRequiredService<ISecretsStoreMigrator>());
}
[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<SqliteSecretStore>(provider.GetRequiredService<ISecretStore>());
}
[Fact]
public void ExpanderCentral_StoreProviderSqlServer_BlankConnectionString_Throws()
{
var services = new ServiceCollection();
services.AddLogging();
IConfiguration config = BuildConfig(("Secrets:Store:Provider", "SqlServer"));
var ex = Assert.Throws<InvalidOperationException>(
() => 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<InvalidOperationException>(
() => 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);
}
}
tests/ZB.MOM.WW.ScadaBridge.Host.Tests/StartupValidatorTests.cs
+116
View File
@@ -784,6 +784,11 @@ public class StartupValidatorTests
[Theory]
[InlineData("true")]
[InlineData("True")]
// Review round 1, Minor #3: these are not valid booleans. GetValue<bool> 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);
}
/// <summary>
/// Deliberately stricter than <c>bool</c> parsing (review round 1, Minor #3): only an absent
/// key or exactly (case-insensitively) <c>"false"</c> is safe. <c>"0"</c> is a legal
/// <c>bool.TryParse</c>-adjacent falsy spelling in some conventions but is NOT exempted here —
/// a leftover key this malformed should surface the retirement message, not silently pass.
/// </summary>
[Fact]
public void ReplicationEnabled_Zero_IsRejected()
{
var values = ValidCentralConfig();
values["Secrets:Replication:Enabled"] = "0";
var config = BuildConfig(values);
var ex = Assert.Throws<InvalidOperationException>(() => StartupValidator.Validate(config));
Assert.Contains(
"Secrets replication (the gRPC hub) is retired in ScadaBridge", ex.Message);
}
/// <summary>
/// Review round 1, Minor #4: a leftover <c>Secrets:Replication</c>/<c>Secrets:GrpcHub</c> 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.
/// </summary>
[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()
{