feat(artifacts): central ships each site's referenced secrets (ciphertext) and fails closed on a missing one

ArtifactDeploymentService now resolves each site's secret: references (via the new SiteSecretsCollector, wrapping ISecretStore) into DeployArtifactsCommand.Secrets, never plaintext. A referenced secret that is absent or tombstoned fails that site's deploy before anything is sent, with a named error, while the rest of the batch still deploys; ComputeCurrentArtifactHashesAsync instead folds a MissingSecrets marker into the hash so such a site reads Out of date. Central logs what it ships (name@revision) and errors on what's missing, never material. Also updates the two other direct ArtifactDeploymentService construction sites (PendingUpdatesEvaluatorTests, CentralUI SitesPageTests) for the new constructor parameter.
2026-09-28 18:07:17 -04:00
parent a57c36f504
commit 75b2a7244c
8 changed files with 508 additions and 22 deletions
@@ -2,6 +2,7 @@ using System.Collections.Concurrent;
 using System.Text.Json;
 using Akka.Actor;
 using Akka.TestKit.Xunit2;
+using Microsoft.Extensions.Logging;
 using Microsoft.Extensions.Logging.Abstractions;
 using Microsoft.Extensions.Options;
 using NSubstitute;
@@ -12,6 +13,7 @@ using ZB.MOM.WW.ScadaBridge.Commons.Interfaces.Services;
 using ZB.MOM.WW.ScadaBridge.Commons.Messages.Artifacts;
 using ZB.MOM.WW.ScadaBridge.Communication;
 using ZB.MOM.WW.ScadaBridge.Commons.Entities.ExternalSystems;
+using ZB.MOM.WW.Secrets.Abstractions;

 namespace ZB.MOM.WW.ScadaBridge.DeploymentManager.Tests;

@@ -27,6 +29,7 @@ public class ArtifactDeploymentServiceTests : TestKit
    private readonly INotificationRepository _notificationRepo;
    private readonly IAuditService _audit;
    private readonly ArtifactRevisionHashService _hashService = new();
+    private readonly ISecretStore _secretStore;

    public ArtifactDeploymentServiceTests()
    {
@@ -39,8 +42,28 @@ public class ArtifactDeploymentServiceTests : TestKit
        // constructor dependency of ArtifactDeploymentService).
        _notificationRepo = Substitute.For<INotificationRepository>();
        _audit = Substitute.For<IAuditService>();
+        // Unconfigured GetAsync returns null (NSubstitute default), i.e. "not in the store" —
+        // exactly the fail-closed posture a test wants unless it explicitly seeds a row.
+        _secretStore = Substitute.For<ISecretStore>();
    }

+    private static StoredSecret SecretRow(string name, long revision, string? description = null) => new()
+    {
+        Name = new SecretName(name),
+        Description = description,
+        ContentType = SecretContentType.Text,
+        Ciphertext = [1, 2, 3],
+        Nonce = [4, 5],
+        Tag = [6, 7],
+        WrappedDek = [8, 9],
+        WrapNonce = [10, 11],
+        WrapTag = [12, 13],
+        KekId = "kek-1",
+        Revision = revision,
+        CreatedUtc = DateTimeOffset.Parse("2026-01-01T00:00:00Z"),
+        UpdatedUtc = DateTimeOffset.Parse("2026-01-02T00:00:00Z"),
+    };
+
    [Fact]
    public async Task DeployToAllSitesAsync_NoSites_ReturnsFailure()
    {
@@ -479,7 +502,126 @@ public class ArtifactDeploymentServiceTests : TestKit
        await _templateRepo.DidNotReceive().GetAllSharedScriptsAsync(Arg.Any<CancellationToken>());
    }

-    private ArtifactDeploymentService CreateService()
+    // ── Secrets ride the artifact deploy (design §3.1/§3.3) ──
+
+    [Fact]
+    public async Task DeployToAllSites_SiteWithMissingSecret_FailsThatSiteOnly_AndSendsNothingToIt()
+    {
+        var sites = new List<Site>
+        {
+            new("Site One", "ok-site") { Id = 1 },
+            new("Site Two", "bad-site") { Id = 2 }
+        };
+        _siteRepo.GetAllSitesAsync(Arg.Any<CancellationToken>()).Returns(sites);
+        _siteRepo.GetDataConnectionsBySiteIdAsync(1, Arg.Any<CancellationToken>())
+            .Returns(new List<DataConnection>());
+        _siteRepo.GetDataConnectionsBySiteIdAsync(2, Arg.Any<CancellationToken>())
+            .Returns(new List<DataConnection>
+            {
+                new("GW1", "MxGateway", 2) { PrimaryConfiguration = """{"ApiKey":"secret:missing-key"}""" }
+            });
+        // _secretStore is unconfigured for "missing-key" — GetAsync returns null (absent).
+
+        var recorder = new ArtifactProbeRecorder();
+        var probe = Sys.ActorOf(Props.Create(() => new ArtifactProbeActor(recorder)));
+        var service = CreateServiceWithCommActor(probe);
+
+        var result = await service.DeployToAllSitesAsync("admin");
+
+        Assert.True(result.IsSuccess);
+        Assert.Equal(1, result.Value.SuccessCount);
+        Assert.Equal(1, result.Value.FailureCount);
+        Assert.Contains(result.Value.SiteResults, r => r.SiteId == "ok-site" && r.Success);
+        var badResult = Assert.Single(result.Value.SiteResults, r => r.SiteId == "bad-site");
+        Assert.False(badResult.Success);
+        Assert.Equal(
+            "Secret 'missing-key' referenced by data connection(s) GW1 is not in the central secret store",
+            badResult.ErrorMessage);
+
+        // Nothing was sent to the bad site: the probe only ever saw ok-site's command.
+        var sentCommand = Assert.Single(recorder.Received);
+        Assert.DoesNotContain(
+            sentCommand.DataConnections ?? [],
+            dc => dc.Name == "GW1");
+
+        // The bad site keeps no stamp — it still reads "out of date" / "never deployed".
+        var badSite = sites.Single(s => s.SiteIdentifier == "bad-site");
+        Assert.Null(badSite.ArtifactRevisionHash);
+    }
+
+    [Fact]
+    public async Task ComputeCurrentHashes_MissingSecret_UsesMissingMarker()
+    {
+        var site = new Site("Site One", "site-1") { Id = 1 };
+        _siteRepo.GetDataConnectionsBySiteIdAsync(1, Arg.Any<CancellationToken>())
+            .Returns(new List<DataConnection>
+            {
+                new("GW1", "MxGateway", 1) { PrimaryConfiguration = """{"ApiKey":"secret:the-key"}""" }
+            });
+        var service = CreateService();
+
+        // The reference does not resolve yet: ComputeCurrentArtifactHashesAsync must not
+        // fail the call — it folds a Missing marker into the hash instead.
+        var beforeCreated = await service.ComputeCurrentArtifactHashesAsync([site]);
+
+        // The secret now exists in the store.
+        _secretStore.GetAsync(Arg.Is<SecretName>(n => n.Value == "the-key"), Arg.Any<CancellationToken>())
+            .Returns(Task.FromResult<StoredSecret?>(SecretRow("the-key", 1)));
+        var afterCreated = await service.ComputeCurrentArtifactHashesAsync([site]);
+
+        Assert.NotEqual(beforeCreated[1], afterCreated[1]);
+    }
+
+    [Fact]
+    public async Task DeployLogs_NeverContainSecretMaterial()
+    {
+        var ciphertext = new byte[] { 0xDE, 0xAD, 0xBE, 0xEF, 0x13, 0x37 };
+        var resolvedRow = SecretRow("present-key", 5) with { Ciphertext = ciphertext };
+        var ciphertextBase64 = Convert.ToBase64String(ciphertext);
+        var ciphertextHex = Convert.ToHexString(ciphertext);
+
+        var sites = new List<Site>
+        {
+            new("Site One", "ok-site") { Id = 1 },
+            new("Site Two", "bad-site") { Id = 2 }
+        };
+        _siteRepo.GetAllSitesAsync(Arg.Any<CancellationToken>()).Returns(sites);
+        _siteRepo.GetDataConnectionsBySiteIdAsync(1, Arg.Any<CancellationToken>())
+            .Returns(new List<DataConnection>
+            {
+                new("GW1", "MxGateway", 1) { PrimaryConfiguration = """{"ApiKey":"secret:present-key"}""" }
+            });
+        _siteRepo.GetDataConnectionsBySiteIdAsync(2, Arg.Any<CancellationToken>())
+            .Returns(new List<DataConnection>
+            {
+                new("GW2", "MxGateway", 2) { PrimaryConfiguration = """{"ApiKey":"secret:missing-key"}""" }
+            });
+        _secretStore.GetAsync(Arg.Is<SecretName>(n => n.Value == "present-key"), Arg.Any<CancellationToken>())
+            .Returns(Task.FromResult<StoredSecret?>(resolvedRow));
+
+        var recorder = new ArtifactProbeRecorder();
+        var probe = Sys.ActorOf(Props.Create(() => new ArtifactProbeActor(recorder)));
+        var capturingLogger = new CapturingLogger<ArtifactDeploymentService>();
+        var service = CreateServiceWithCommActor(probe, capturingLogger);
+
+        var result = await service.DeployToAllSitesAsync("admin");
+
+        Assert.True(result.IsSuccess);
+        Assert.Equal(1, result.Value.SuccessCount);
+        Assert.Equal(1, result.Value.FailureCount);
+
+        // The Information/Error lines still name what shipped and what didn't ...
+        Assert.Contains(capturingLogger.Messages, m => m.Contains("present-key@r5"));
+        Assert.Contains(capturingLogger.Messages, m => m.Contains("missing-key"));
+        Assert.Contains(capturingLogger.Messages, m => m.Contains("not in the central secret store"));
+
+        // ... but never the ciphertext, in any rendering, in any log message or exception text.
+        Assert.DoesNotContain(capturingLogger.Messages, m => m.Contains(ciphertextBase64));
+        Assert.DoesNotContain(capturingLogger.Messages, m => m.Contains(ciphertextHex));
+        Assert.DoesNotContain(capturingLogger.Messages, m => m.Contains(ciphertextHex, StringComparison.OrdinalIgnoreCase));
+    }
+
+    private ArtifactDeploymentService CreateService(ILogger<ArtifactDeploymentService>? logger = null)
    {
        var comms = new CommunicationService(
            Options.Create(new CommunicationOptions()),
@@ -489,11 +631,13 @@ public class ArtifactDeploymentServiceTests : TestKit
            _siteRepo, _deploymentRepo, _templateRepo, _externalSystemRepo, _notificationRepo,
            comms, _audit,
            Options.Create(new DeploymentManagerOptions()),
-            NullLogger<ArtifactDeploymentService>.Instance,
-            _hashService);
+            logger ?? NullLogger<ArtifactDeploymentService>.Instance,
+            _hashService,
+            new SiteSecretsCollector(_secretStore));
    }

-    private ArtifactDeploymentService CreateServiceWithCommActor(IActorRef commActor)
+    private ArtifactDeploymentService CreateServiceWithCommActor(
+        IActorRef commActor, ILogger<ArtifactDeploymentService>? logger = null)
    {
        var comms = new CommunicationService(
            Options.Create(new CommunicationOptions
@@ -510,8 +654,40 @@ public class ArtifactDeploymentServiceTests : TestKit
            {
                ArtifactDeploymentTimeoutPerSite = TimeSpan.FromSeconds(5)
            }),
-            NullLogger<ArtifactDeploymentService>.Instance,
-            _hashService);
+            logger ?? NullLogger<ArtifactDeploymentService>.Instance,
+            _hashService,
+            new SiteSecretsCollector(_secretStore));
+    }
+
+    /// <summary>
+    /// Captures every formatted log message (and, for entries logged with an exception, the
+    /// exception's <see cref="Exception.ToString"/>) so a test can assert on log CONTENT —
+    /// specifically, that secret material never appears in it.
+    /// </summary>
+    private sealed class CapturingLogger<T> : ILogger<T>
+    {
+        public List<string> Messages { get; } = [];
+
+        public IDisposable BeginScope<TState>(TState state) where TState : notnull => NullScope.Instance;
+
+        public bool IsEnabled(LogLevel logLevel) => true;
+
+        public void Log<TState>(
+            LogLevel logLevel, EventId eventId, TState state, Exception? exception,
+            Func<TState, Exception?, string> formatter)
+        {
+            Messages.Add(formatter(state, exception));
+            if (exception is not null)
+            {
+                Messages.Add(exception.ToString());
+            }
+        }
+
+        private sealed class NullScope : IDisposable
+        {
+            public static readonly NullScope Instance = new();
+            public void Dispose() { }
+        }
    }

    /// <summary>