From 67a3950f74b8f0ee390189e485001316606f866b Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Mon, 28 Sep 2026 21:08:34 -0400 Subject: [PATCH] fix(site-secrets): one shared secret: reference parser for scanner and MxGateway adapter Final review M3: SecretReferenceScanner.TryParseSecretReference (trimmed, case-insensitive prefix, trimmed name, empty = not a reference, ${secret:} never matches) is used by the scanner and by MxGatewayDataConnection's ApiKey resolution, so a padded or case-variant reference resolves the name central ships. A bare 'secret:' ApiKey now fails closed. M7: unit test pins stamped hash == ComputeCurrentArtifactHashesAsync with a non-empty Secrets list. --- .../Types/Secrets/SecretReferenceScanner.cs | 29 ++++++++++++-- .../Adapters/MxGatewayDataConnection.cs | 14 +++++-- .../Secrets/SecretReferenceScannerTests.cs | 36 +++++++++++++++++ .../MxGatewayDataConnectionSecretsTests.cs | 37 +++++++++++++++++ .../ArtifactDeploymentServiceTests.cs | 40 +++++++++++++++++++ 5 files changed, 149 insertions(+), 7 deletions(-) diff --git a/src/ZB.MOM.WW.ScadaBridge.Commons/Types/Secrets/SecretReferenceScanner.cs b/src/ZB.MOM.WW.ScadaBridge.Commons/Types/Secrets/SecretReferenceScanner.cs index fff25a39..3cfe4d22 100644 --- a/src/ZB.MOM.WW.ScadaBridge.Commons/Types/Secrets/SecretReferenceScanner.cs +++ b/src/ZB.MOM.WW.ScadaBridge.Commons/Types/Secrets/SecretReferenceScanner.cs @@ -17,6 +17,29 @@ public static class SecretReferenceScanner public const string Prefix = "secret:"; private static readonly string[] BootstrapPrefixes = ["SB-GRPC-PSK-"]; + /// + /// The single parser for a secret:<name> reference value, shared by this scanner + /// (what central ships) and every adapter that resolves the reference at connect time (what + /// the site looks up), so both sides always agree on the name (final review M3). The value is + /// trimmed, the secret: prefix matches ordinal-ignore-case, and the remainder is + /// trimmed; an empty remainder is not a reference. The config-expander form + /// ${secret:…} is a different syntax and never matches. + /// + /// A configuration string value. + /// The referenced secret name when this returns ; otherwise empty. + /// when is a secret:<name> reference. + public static bool TryParseSecretReference(string? value, out string name) + { + name = string.Empty; + if (value is null) return false; + var trimmed = value.Trim(); + if (!trimmed.StartsWith(Prefix, StringComparison.OrdinalIgnoreCase)) return false; + var candidate = trimmed[Prefix.Length..].Trim(); + if (candidate.Length == 0) return false; + name = candidate; + return true; + } + /// /// Scans every connection's primary and backup configuration JSON for secret:<name> /// string values and returns the distinct names found, ordinal-sorted, each with the ordinal-sorted, @@ -51,10 +74,8 @@ public static class SecretReferenceScanner case JsonValueKind.Object: foreach (var p in e.EnumerateObject()) stack.Push(p.Value); break; case JsonValueKind.Array: foreach (var i in e.EnumerateArray()) stack.Push(i); break; case JsonValueKind.String: - var s = e.GetString()!.Trim(); - if (!s.StartsWith(Prefix, StringComparison.OrdinalIgnoreCase)) break; - var name = s[Prefix.Length..].Trim(); - if (name.Length == 0 || BootstrapPrefixes.Any(b => name.StartsWith(b, StringComparison.OrdinalIgnoreCase))) break; + if (!TryParseSecretReference(e.GetString(), out var name)) break; + if (BootstrapPrefixes.Any(b => name.StartsWith(b, StringComparison.OrdinalIgnoreCase))) break; yield return name; break; } diff --git a/src/ZB.MOM.WW.ScadaBridge.DataConnectionLayer/Adapters/MxGatewayDataConnection.cs b/src/ZB.MOM.WW.ScadaBridge.DataConnectionLayer/Adapters/MxGatewayDataConnection.cs index 9b57f60d..f54e03f5 100644 --- a/src/ZB.MOM.WW.ScadaBridge.DataConnectionLayer/Adapters/MxGatewayDataConnection.cs +++ b/src/ZB.MOM.WW.ScadaBridge.DataConnectionLayer/Adapters/MxGatewayDataConnection.cs @@ -3,6 +3,7 @@ using Microsoft.Extensions.Logging; using ZB.MOM.WW.ScadaBridge.Commons.Interfaces.Protocol; using ZB.MOM.WW.ScadaBridge.Commons.Serialization; using ZB.MOM.WW.ScadaBridge.Commons.Types.Enums; +using ZB.MOM.WW.ScadaBridge.Commons.Types.Secrets; using ZB.MOM.WW.Secrets.Abstractions; namespace ZB.MOM.WW.ScadaBridge.DataConnectionLayer.Adapters; @@ -98,20 +99,27 @@ public class MxGatewayDataConnection // so a fail-closed throw here leaves adapter state untouched (no half-open client, // previous connection — if any — still intact for a later retry). A literal key // (no `secret:` prefix) passes through unchanged for back-compat. + // The reference is parsed by the SAME parser central's SecretReferenceScanner uses to + // decide which secret to ship (trimmed, case-insensitive prefix), so a padded or + // case-variant reference resolves exactly the name that was deployed (final review M3). var apiKey = cfg.ApiKey; - if (!string.IsNullOrEmpty(apiKey) && - apiKey.StartsWith("secret:", StringComparison.OrdinalIgnoreCase)) + if (SecretReferenceScanner.TryParseSecretReference(apiKey, out var secretName)) { if (_secretResolver is null) { throw new InvalidOperationException( "MxGateway ApiKey uses a 'secret:' reference but no ISecretResolver is available to resolve it."); } - var secretName = apiKey["secret:".Length..]; apiKey = await _secretResolver.GetAsync(new SecretName(secretName), cancellationToken) ?? throw new InvalidOperationException( $"MxGateway ApiKey secret '{secretName}' could not be resolved (not found or tombstoned)."); } + else if (!string.IsNullOrEmpty(apiKey) && + apiKey.TrimStart().StartsWith(SecretReferenceScanner.Prefix, StringComparison.OrdinalIgnoreCase)) + { + // `secret:` with no name: fail closed rather than sending the literal text as a key. + throw new InvalidOperationException("MxGateway ApiKey is a 'secret:' reference with no secret name."); + } // Reconnect reuses this adapter: cancel the previous event loop and dispose // the previous client so a flapping gateway cannot accumulate orphaned diff --git a/tests/ZB.MOM.WW.ScadaBridge.Commons.Tests/Types/Secrets/SecretReferenceScannerTests.cs b/tests/ZB.MOM.WW.ScadaBridge.Commons.Tests/Types/Secrets/SecretReferenceScannerTests.cs index c1372806..bbb0c287 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.Commons.Tests/Types/Secrets/SecretReferenceScannerTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.Commons.Tests/Types/Secrets/SecretReferenceScannerTests.cs @@ -46,4 +46,40 @@ public class SecretReferenceScannerTests Assert.Equal(["mxgw-key"], refs.Select(r => r.Name)); Assert.Equal(["GW1"], refs.Single(r => r.Name == "mxgw-key").ReferencedBy); } + + // ── Shared parser (final review M3) ── + + [Theory] + [InlineData("secret:mxgw-key", "mxgw-key")] + [InlineData("SECRET:mxgw-key", "mxgw-key")] + [InlineData(" Secret: mxgw-key ", "mxgw-key")] + [InlineData("secret:a/b/c", "a/b/c")] + public void TryParseSecretReference_ParsesTrimmedCaseInsensitive(string value, string expected) + { + Assert.True(SecretReferenceScanner.TryParseSecretReference(value, out var name)); + Assert.Equal(expected, name); + } + + [Theory] + [InlineData(null)] + [InlineData("")] + [InlineData("secret:")] + [InlineData("secret: ")] + [InlineData("${secret:node-local}")] + [InlineData("literal-api-key")] + [InlineData("see secret:x in docs")] + public void TryParseSecretReference_RejectsNonReferences(string? value) + { + Assert.False(SecretReferenceScanner.TryParseSecretReference(value, out var name)); + Assert.Equal(string.Empty, name); + } + + [Fact] + public void Scan_AndParser_AgreeOnThePaddedCaseVariantName() + { + const string value = " SECRET: mxgw-key "; + var shipped = Assert.Single(SecretReferenceScanner.Scan([Dc("GW", $$"""{"ApiKey":"{{value}}"}""")])).Name; + Assert.True(SecretReferenceScanner.TryParseSecretReference(value, out var parsed)); + Assert.Equal(shipped, parsed); + } } diff --git a/tests/ZB.MOM.WW.ScadaBridge.DataConnectionLayer.Tests/Adapters/MxGatewayDataConnectionSecretsTests.cs b/tests/ZB.MOM.WW.ScadaBridge.DataConnectionLayer.Tests/Adapters/MxGatewayDataConnectionSecretsTests.cs index 27cca9c3..b3ce8664 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.DataConnectionLayer.Tests/Adapters/MxGatewayDataConnectionSecretsTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.DataConnectionLayer.Tests/Adapters/MxGatewayDataConnectionSecretsTests.cs @@ -52,6 +52,43 @@ public class MxGatewayDataConnectionSecretsTests Assert.Contains("mxgateway/testconn/api-key", resolver.Requested); } + [Theory] + [InlineData("secret:mxgw-key ")] + [InlineData(" SECRET: mxgw-key")] + [InlineData("Secret:mxgw-key")] + public async Task ConnectAsync_resolves_padded_or_case_variant_ref_to_the_name_the_scanner_ships(string apiKey) + { + // Final review M3: the adapter and central's SecretReferenceScanner share one parser, + // so the name looked up is exactly the name that was deployed. + var shipped = Assert.Single(ZB.MOM.WW.ScadaBridge.Commons.Types.Secrets.SecretReferenceScanner.Scan([ + new ZB.MOM.WW.ScadaBridge.Commons.Messages.Artifacts.DataConnectionArtifact( + "GW", "MxGateway", System.Text.Json.JsonSerializer.Serialize(new { ApiKey = apiKey }), null, 3), + ])).Name; + + var fake = new FakeMxGatewayClient(); + var resolver = new FakeSecretResolver("resolved-plaintext-key"); + var adapter = new MxGatewayDataConnection(fake, NullLogger.Instance, resolver); + + await adapter.ConnectAsync(Details(apiKey)); + + Assert.Equal("mxgw-key", shipped); + Assert.Equal([shipped], resolver.Requested); + Assert.Equal("resolved-plaintext-key", fake.ConnectedWith!.ApiKey); + } + + [Fact] + public async Task ConnectAsync_fails_closed_on_a_secret_ref_with_no_name() + { + var fake = new FakeMxGatewayClient(); + var resolver = new FakeSecretResolver("SHOULD-NOT-BE-USED"); + var adapter = new MxGatewayDataConnection(fake, NullLogger.Instance, resolver); + + await Assert.ThrowsAsync(() => adapter.ConnectAsync(Details("secret: "))); + + Assert.Null(fake.ConnectedWith); + Assert.Empty(resolver.Requested); + } + [Fact] public async Task ConnectAsync_passes_literal_apikey_unchanged_and_does_not_call_resolver() { diff --git a/tests/ZB.MOM.WW.ScadaBridge.DeploymentManager.Tests/ArtifactDeploymentServiceTests.cs b/tests/ZB.MOM.WW.ScadaBridge.DeploymentManager.Tests/ArtifactDeploymentServiceTests.cs index 5aaf80a8..cfcc14be 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.DeploymentManager.Tests/ArtifactDeploymentServiceTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.DeploymentManager.Tests/ArtifactDeploymentServiceTests.cs @@ -470,6 +470,46 @@ public class ArtifactDeploymentServiceTests : TestKit } } + [Fact] + public async Task ComputeCurrentArtifactHashesAsync_MatchesTheHashADeployStamps_WithSecrets() + { + // Final review M7: the parity above runs with no data connections. Pin it for a site + // whose connections reference secrets, so the stamped hash (built from a command that + // carries a non-empty Secrets list) equals the "what would it be now" hash — a + // successful secrets deploy must read Current, not Out of date. + var site = new Site("Site One", "site-1") { Id = 1 }; + var sites = new List { site }; + _siteRepo.GetAllSitesAsync(Arg.Any()).Returns(sites); + _siteRepo.GetDataConnectionsBySiteIdAsync(1, Arg.Any()) + .Returns(new List + { + new("GW1", "MxGateway", 1) + { + PrimaryConfiguration = """{"ApiKey":"secret:key1","Password":"secret:key2"}""" + } + }); + _secretStore.GetAsync(Arg.Is(n => n.Value == "key1"), Arg.Any()) + .Returns(Task.FromResult(SecretRow("key1", 3))); + _secretStore.GetAsync(Arg.Is(n => n.Value == "key2"), Arg.Any()) + .Returns(Task.FromResult(SecretRow("key2", 7))); + + var recorder = new ArtifactProbeRecorder(); + var probe = Sys.ActorOf(Props.Create(() => new SecretsAckProbeActor( + recorder, cmd => cmd.Secrets == null ? (int?)null : cmd.Secrets.Count))); + var service = CreateServiceWithCommActor(probe); + + var deployResult = await service.DeployToAllSitesAsync("admin"); + Assert.True(deployResult.IsSuccess); + Assert.Equal(1, deployResult.Value.SuccessCount); + var shipped = Assert.Single(recorder.Received); + Assert.Equal(2, shipped.Secrets!.Count); + Assert.NotNull(site.ArtifactRevisionHash); + + var current = await service.ComputeCurrentArtifactHashesAsync(sites); + + Assert.Equal(site.ArtifactRevisionHash, current[site.Id]); + } + [Fact] public async Task ComputeCurrentArtifactHashesAsync_HoistsTheGlobalQueriesOnce() {