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.
This commit is contained in:
@@ -17,6 +17,29 @@ public static class SecretReferenceScanner
|
||||
public const string Prefix = "secret:";
|
||||
private static readonly string[] BootstrapPrefixes = ["SB-GRPC-PSK-"];
|
||||
|
||||
/// <summary>
|
||||
/// The single parser for a <c>secret:<name></c> 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 <c>secret:</c> prefix matches ordinal-ignore-case, and the remainder is
|
||||
/// trimmed; an empty remainder is not a reference. The config-expander form
|
||||
/// <c>${secret:…}</c> is a different syntax and never matches.
|
||||
/// </summary>
|
||||
/// <param name="value">A configuration string value.</param>
|
||||
/// <param name="name">The referenced secret name when this returns <see langword="true"/>; otherwise empty.</param>
|
||||
/// <returns><see langword="true"/> when <paramref name="value"/> is a <c>secret:<name></c> reference.</returns>
|
||||
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;
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// Scans every connection's primary and backup configuration JSON for <c>secret:<name></c>
|
||||
/// 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;
|
||||
}
|
||||
|
||||
@@ -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
|
||||
|
||||
+36
@@ -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);
|
||||
}
|
||||
}
|
||||
|
||||
+37
@@ -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<MxGatewayDataConnection>.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<MxGatewayDataConnection>.Instance, resolver);
|
||||
|
||||
await Assert.ThrowsAsync<InvalidOperationException>(() => 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()
|
||||
{
|
||||
|
||||
@@ -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> { site };
|
||||
_siteRepo.GetAllSitesAsync(Arg.Any<CancellationToken>()).Returns(sites);
|
||||
_siteRepo.GetDataConnectionsBySiteIdAsync(1, Arg.Any<CancellationToken>())
|
||||
.Returns(new List<DataConnection>
|
||||
{
|
||||
new("GW1", "MxGateway", 1)
|
||||
{
|
||||
PrimaryConfiguration = """{"ApiKey":"secret:key1","Password":"secret:key2"}"""
|
||||
}
|
||||
});
|
||||
_secretStore.GetAsync(Arg.Is<SecretName>(n => n.Value == "key1"), Arg.Any<CancellationToken>())
|
||||
.Returns(Task.FromResult<StoredSecret?>(SecretRow("key1", 3)));
|
||||
_secretStore.GetAsync(Arg.Is<SecretName>(n => n.Value == "key2"), Arg.Any<CancellationToken>())
|
||||
.Returns(Task.FromResult<StoredSecret?>(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()
|
||||
{
|
||||
|
||||
Reference in New Issue
Block a user