diff --git a/src/ZB.MOM.WW.ScadaBridge.DeploymentManager/ArtifactDeploymentService.cs b/src/ZB.MOM.WW.ScadaBridge.DeploymentManager/ArtifactDeploymentService.cs index 859ce505..3141e02f 100644 --- a/src/ZB.MOM.WW.ScadaBridge.DeploymentManager/ArtifactDeploymentService.cs +++ b/src/ZB.MOM.WW.ScadaBridge.DeploymentManager/ArtifactDeploymentService.cs @@ -188,6 +188,86 @@ public class ArtifactDeploymentService return (command, collected.Missing); } + /// + /// Task 7 (design §3.4): reconciles a site's raw + /// against what the deployed actually shipped, so + /// central never stamps a site *Current* on the strength of a reply that only proves the + /// artifact set was RECEIVED, not that every shipped secret was APPLIED. + /// + /// A site that predates the secrets feature answers with SecretsApplied = null even + /// on an otherwise-successful apply — silently ignoring the new proto field rather than + /// failing loudly — so that case must be caught here rather than trusted as success. A site + /// that applied some but not all shipped secrets (a partial local failure) reports a count + /// that undershoots what was sent. Both are downgraded to a failure; the site's previous + /// stamp (or lack of one) is left untouched because stamping already keys off + /// . + /// + /// Only fires when the command actually shipped secrets — a site with nothing to apply + /// truthfully has nothing to confirm, so its SecretsApplied stays uninterpreted + /// (old-site behaviour is unchanged when no secrets are in play, per the acceptance + /// criteria). + /// + private static SiteArtifactResult ReconcileSecretsOutcome( + DeployArtifactsCommand cmd, ArtifactDeploymentResponse r, string siteIdentifier, string siteName) + { + var shipped = cmd.Secrets?.Count ?? 0; + if (r.Success && shipped > 0 && r.SecretsApplied is null) + return new(siteIdentifier, siteName, false, + "Site build predates secret deployment — upgrade the site before deploying secrets", null); + if (r.Success && shipped > 0 && r.SecretsApplied != shipped) + return new(siteIdentifier, siteName, false, + $"Site applied {r.SecretsApplied} of {shipped} secrets", r.SecretsApplied); + return new(siteIdentifier, siteName, r.Success, r.ErrorMessage, shipped > 0 ? r.SecretsApplied : null); + } + + /// + /// Best-effort DeploySiteSecrets audit row (task 7): one row per site whose command + /// carried, or would have carried, at least one secret — a resolved + /// entry, an unresolved reference in + /// (the fail-closed case, where nothing was sent), or both. A + /// site with no secret references at all gets no row. + /// + /// Per the audit-is-best-effort rule (an audit failure never changes the deploy outcome) + /// this is fire-and-forget-shaped only in intent, not mechanism — like every other audit + /// call on this path it is still awaited (never on a script path; this is a management + /// operation), but a thrown/faulted write here must not — and structurally cannot, since + /// the caller already decided beforehand — change + /// whether the site gets stamped. + /// + /// The afterState payload is built ONLY from names/revisions/missing names/success/ + /// error — never , Nonce, Tag, + /// WrappedDek, or any other material (global constraint: never plaintext or material + /// in logs or audit rows). + /// + private Task LogSecretsAuditAsync( + string user, + DeployArtifactsCommand command, + IReadOnlyList missing, + string siteIdentifier, + string siteName, + bool success, + string? error, + CancellationToken cancellationToken) + { + var hasSecretRefs = (command.Secrets?.Count ?? 0) > 0 || missing.Count > 0; + if (!hasSecretRefs) + return Task.CompletedTask; + + return _auditService.LogAsync( + user, "DeploySiteSecrets", "SiteSecrets", siteIdentifier, siteName, + new + { + deploymentId = command.DeploymentId, + secrets = (command.Secrets ?? []) + .Select(s => new { name = s.Name, revision = s.Revision }) + .ToList(), + missing = missing.Select(m => m.Name).ToList(), + success, + error, + }, + cancellationToken); + } + /// /// Formats the fail-closed message for one or more secret references a site's data /// connections make that could not be resolved at central (design §3.3). Never includes @@ -399,8 +479,10 @@ public class ArtifactDeploymentService var response = await _communicationService.DeployArtifactsAsync( site.SiteIdentifier, command, cts.Token); - return new SiteArtifactResult( - site.SiteIdentifier, site.Name, response.Success, response.ErrorMessage); + // Task 7 (design §3.4): a reply that only proves receipt is not enough to + // stamp *Current* — reconcile it against what was actually shipped before + // this result feeds the stamp decision below. + return ReconcileSecretsOutcome(command, response, site.SiteIdentifier, site.Name); } catch (Exception ex) when (ex is TimeoutException or OperationCanceledException or TaskCanceledException) { @@ -429,6 +511,22 @@ public class ArtifactDeploymentService perSiteResults[result.SiteId] = result; } + // Task 7 (design §3.4): one best-effort DeploySiteSecrets audit row per site whose + // command carried or would have carried secrets — covers the reconciled + // success/failure path AND the fail-closed (missing secret, nothing sent) path, since + // both already flow into perSiteResults above by the time this loop runs. Driven off + // siteCommands/perSiteResults rather than inlined into the per-site task above so this + // is a single serial pass (no concurrent audit writes) that runs — per the ordering + // rule — BEFORE the best-effort stamp below. + foreach (var site in sites) + { + var (command, missing) = siteCommands[site.Id]; + var result = perSiteResults[site.SiteIdentifier]; + await LogSecretsAuditAsync( + user, command, missing, site.SiteIdentifier, site.Name, + result.Success, result.ErrorMessage, cancellationToken); + } + // Persist the system artifact deployment record. // SystemArtifactDeploymentRecord has no dedicated // DeploymentId column (adding one is a Commons/ConfigurationDatabase @@ -514,6 +612,11 @@ public class ArtifactDeploymentService var missingMessage = BuildMissingSecretsMessage(missing); _logger.LogError( "Cannot deploy artifacts to site {SiteId}: {Error}", siteIdentifier, missingMessage); + // Task 7: the fail-closed case still gets its DeploySiteSecrets row — nothing + // was sent, but the command carried (or would have carried) secrets. + await LogSecretsAuditAsync( + user, command, missing, siteIdentifier, siteIdentifier, false, missingMessage, + cancellationToken); return Result.Failure(missingMessage); } @@ -527,10 +630,18 @@ public class ArtifactDeploymentService var response = await _communicationService.DeployArtifactsAsync(siteIdentifier, command, cts.Token); - var result = new SiteArtifactResult(siteIdentifier, siteIdentifier, response.Success, response.ErrorMessage); + // Task 7 (design §3.4): reconcile the raw wire response against what was + // actually shipped before it feeds the audit row, the stamp decision, or the + // returned Result — an old site's/partial site's reply must not read as success. + var result = ReconcileSecretsOutcome(command, response, siteIdentifier, siteIdentifier); await _auditService.LogAsync(user, "RetryArtifactDeployment", "SystemArtifact", - command.DeploymentId, siteIdentifier, new { response.Success }, cancellationToken); + command.DeploymentId, siteIdentifier, new { result.Success }, cancellationToken); + + // Ordering rule: audit before the best-effort stamp. + await LogSecretsAuditAsync( + user, command, missing, siteIdentifier, siteIdentifier, + result.Success, result.ErrorMessage, cancellationToken); // Same stamp-on-success rule as the sweep path: a retry that lands is a // successful artifact deploy and must move the site's revision stamp; a @@ -541,14 +652,14 @@ public class ArtifactDeploymentService // an artifact deploy that actually succeeded, so the stamp must (and does) keep // its own try/catch inside StampArtifactRevisionAsync rather than lean on this // one. - if (response.Success) + if (result.Success) { await StampRetriedSiteAsync(siteDbId, command, cancellationToken); } - return response.Success + return result.Success ? Result.Success(result) - : Result.Failure(response.ErrorMessage ?? "Retry failed."); + : Result.Failure(result.ErrorMessage ?? "Retry failed."); } catch (Exception ex) { @@ -673,11 +784,20 @@ public class ArtifactDeploymentService /// /// Per-site result for artifact deployment. /// +/// +/// How many secrets the site confirmed applying, when the deployed command shipped at +/// least one (task 7, design §3.4). null when the site shipped no secrets at all +/// — deliberately the same value an old site's unset +/// produces, so the two "no secrets involved" cases (nothing shipped vs. an old site that +/// doesn't know about the field) are indistinguishable, which is correct: neither one is a +/// secrets-gating concern. +/// public record SiteArtifactResult( string SiteId, string SiteName, bool Success, - string? ErrorMessage); + string? ErrorMessage, + int? SecretsDeployed = null); /// /// Summary of system-wide artifact deployment with per-site results. diff --git a/tests/ZB.MOM.WW.ScadaBridge.DeploymentManager.Tests/ArtifactDeploymentServiceTests.cs b/tests/ZB.MOM.WW.ScadaBridge.DeploymentManager.Tests/ArtifactDeploymentServiceTests.cs index ba37f60b..859a0492 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.DeploymentManager.Tests/ArtifactDeploymentServiceTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.DeploymentManager.Tests/ArtifactDeploymentServiceTests.cs @@ -600,7 +600,11 @@ public class ArtifactDeploymentServiceTests : TestKit .Returns(Task.FromResult(resolvedRow)); var recorder = new ArtifactProbeRecorder(); - var probe = Sys.ActorOf(Props.Create(() => new ArtifactProbeActor(recorder))); + // Task 7: ok-site's command ships one secret, so the probe must ack it (SecretsApplied + // = shipped count) for the site to reconcile as a success — an unset SecretsApplied + // now reads as an old site (see the dedicated reconciliation tests below). + var probe = Sys.ActorOf(Props.Create(() => + new SecretsAckProbeActor(recorder, cmd => cmd.Secrets == null ? (int?)null : cmd.Secrets.Count))); var capturingLogger = new CapturingLogger(); var service = CreateServiceWithCommActor(probe, capturingLogger); @@ -621,6 +625,182 @@ public class ArtifactDeploymentServiceTests : TestKit Assert.DoesNotContain(capturingLogger.Messages, m => m.Contains(ciphertextHex, StringComparison.OrdinalIgnoreCase)); } + // ── Task 7: stamp gating on secret application + DeploySiteSecrets audit row ── + + [Fact] + public async Task OldSite_NullSecretsApplied_WithSecrets_FailsAndDoesNotStamp() + { + // The site's build predates the secrets feature: it acks the artifact deploy as a + // whole (Success=true) but never populates SecretsApplied, silently ignoring the new + // proto field. That must not read as "the secret was applied". + var site = new Site("Site One", "site-1") { Id = 1 }; + _siteRepo.GetAllSitesAsync(Arg.Any()).Returns(new List { site }); + _siteRepo.GetDataConnectionsBySiteIdAsync(1, Arg.Any()) + .Returns(new List + { + new("GW1", "MxGateway", 1) { PrimaryConfiguration = """{"ApiKey":"secret:the-key"}""" } + }); + _secretStore.GetAsync(Arg.Is(n => n.Value == "the-key"), Arg.Any()) + .Returns(Task.FromResult(SecretRow("the-key", 1))); + + var recorder = new ArtifactProbeRecorder(); + var probe = Sys.ActorOf(Props.Create(() => new SecretsAckProbeActor(recorder, _ => null))); + var service = CreateServiceWithCommActor(probe); + + var result = await service.DeployToAllSitesAsync("admin"); + + Assert.True(result.IsSuccess); + Assert.Equal(0, result.Value.SuccessCount); + Assert.Equal(1, result.Value.FailureCount); + var siteResult = Assert.Single(result.Value.SiteResults); + Assert.False(siteResult.Success); + Assert.Equal( + "Site build predates secret deployment — upgrade the site before deploying secrets", + siteResult.ErrorMessage); + Assert.Null(siteResult.SecretsDeployed); + Assert.Null(site.ArtifactRevisionHash); + Assert.Null(site.ArtifactsDeployedAt); + await _siteRepo.DidNotReceive().UpdateSiteAsync(site, Arg.Any()); + } + + [Fact] + public async Task PartialApply_FailsAndDoesNotStamp() + { + // The site applied 1 of the 2 shipped secrets (a partial local failure). Success=true + // on the wire response is not enough — the count must match exactly. + var site = new Site("Site One", "site-1") { Id = 1 }; + _siteRepo.GetAllSitesAsync(Arg.Any()).Returns(new List { site }); + _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", 1))); + _secretStore.GetAsync(Arg.Is(n => n.Value == "key2"), Arg.Any()) + .Returns(Task.FromResult(SecretRow("key2", 1))); + + var recorder = new ArtifactProbeRecorder(); + var probe = Sys.ActorOf(Props.Create(() => new SecretsAckProbeActor(recorder, _ => 1))); + var service = CreateServiceWithCommActor(probe); + + var result = await service.DeployToAllSitesAsync("admin"); + + Assert.True(result.IsSuccess); + Assert.Equal(0, result.Value.SuccessCount); + Assert.Equal(1, result.Value.FailureCount); + var siteResult = Assert.Single(result.Value.SiteResults); + Assert.False(siteResult.Success); + Assert.Equal("Site applied 1 of 2 secrets", siteResult.ErrorMessage); + Assert.Equal(1, siteResult.SecretsDeployed); + Assert.Null(site.ArtifactRevisionHash); + await _siteRepo.DidNotReceive().UpdateSiteAsync(site, Arg.Any()); + } + + [Fact] + public async Task OldSite_NoSecrets_StillStamps() + { + // No data connection references a secret at all, so the command ships zero secrets. + // An old site's SecretsApplied=null reply must NOT gate the stamp in this case — + // unchanged behaviour. + var site = new Site("Site One", "site-1") { Id = 1 }; + _siteRepo.GetAllSitesAsync(Arg.Any()).Returns(new List { site }); + _siteRepo.GetDataConnectionsBySiteIdAsync(1, Arg.Any()) + .Returns(new List()); + + 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); + var siteResult = Assert.Single(result.Value.SiteResults); + Assert.True(siteResult.Success); + Assert.Null(siteResult.SecretsDeployed); + Assert.NotNull(site.ArtifactRevisionHash); + await _siteRepo.Received(1).UpdateSiteAsync(site, Arg.Any()); + } + + [Fact] + public async Task SecretDeploy_WritesOneAuditRowPerSite_WithoutMaterial() + { + var ciphertext = new byte[] { 0xDE, 0xAD, 0xBE, 0xEF, 0x13, 0x37 }; + var resolvedRow = SecretRow("present-key", 3) with { Ciphertext = ciphertext }; + var ciphertextBase64 = Convert.ToBase64String(ciphertext); + var ciphertextHex = Convert.ToHexString(ciphertext); + + var secretSite = new Site("Secret Site", "sec-site") { Id = 1 }; + var plainSite = new Site("Plain Site", "plain-site") { Id = 2 }; + _siteRepo.GetAllSitesAsync(Arg.Any()).Returns(new List { secretSite, plainSite }); + _siteRepo.GetDataConnectionsBySiteIdAsync(1, Arg.Any()) + .Returns(new List + { + new("GW1", "MxGateway", 1) { PrimaryConfiguration = """{"ApiKey":"secret:present-key"}""" } + }); + _siteRepo.GetDataConnectionsBySiteIdAsync(2, Arg.Any()) + .Returns(new List()); + _secretStore.GetAsync(Arg.Is(n => n.Value == "present-key"), Arg.Any()) + .Returns(Task.FromResult(resolvedRow)); + + var recorder = new ArtifactProbeRecorder(); + // Correctly acks whatever it received, so this test exercises only the audit-row + // requirement, not the reconciliation-failure paths covered above. + var probe = Sys.ActorOf(Props.Create(() => + new SecretsAckProbeActor(recorder, cmd => cmd.Secrets == null ? (int?)null : cmd.Secrets.Count))); + var service = CreateServiceWithCommActor(probe); + + object? captured = null; + var callCount = 0; + _audit.When(x => x.LogAsync( + Arg.Any(), "DeploySiteSecrets", Arg.Any(), Arg.Any(), Arg.Any(), + Arg.Any(), Arg.Any())) + .Do(ci => + { + callCount++; + if ((string)ci[3] == "sec-site") + { + Assert.Equal("admin", ci[0]); + Assert.Equal("SiteSecrets", ci[2]); + Assert.Equal("Secret Site", ci[4]); + captured = ci[5]; + } + else + { + Assert.Fail($"Unexpected DeploySiteSecrets row for site '{ci[3]}' — only sec-site has secret refs."); + } + }); + + var result = await service.DeployToAllSitesAsync("admin"); + + Assert.True(result.IsSuccess); + Assert.Equal(2, result.Value.SuccessCount); + + // Exactly one DeploySiteSecrets row — for sec-site only; plain-site has no secret + // references at all and gets none. + Assert.Equal(1, callCount); + Assert.NotNull(captured); + var json = JsonSerializer.Serialize(captured); + Assert.Contains("present-key", json); + Assert.Contains("\"revision\":3", json); + Assert.Contains("\"success\":true", json); + + // ... but never any secret material, in any rendering. + Assert.DoesNotContain(ciphertextBase64, json); + Assert.DoesNotContain(ciphertextHex, json); + Assert.DoesNotContain(ciphertextHex, json, StringComparison.OrdinalIgnoreCase); + Assert.DoesNotContain("Ciphertext", json, StringComparison.OrdinalIgnoreCase); + Assert.DoesNotContain("Nonce", json, StringComparison.OrdinalIgnoreCase); + Assert.DoesNotContain("WrappedDek", json, StringComparison.OrdinalIgnoreCase); + Assert.DoesNotContain("WrapNonce", json, StringComparison.OrdinalIgnoreCase); + Assert.DoesNotContain("WrapTag", json, StringComparison.OrdinalIgnoreCase); + Assert.DoesNotContain("\"tag\"", json, StringComparison.OrdinalIgnoreCase); + } + private ArtifactDeploymentService CreateService(ILogger? logger = null) { var comms = new CommunicationService( @@ -726,4 +906,29 @@ public class ArtifactDeploymentServiceTests : TestKit }); } } + + /// + /// Task 7: stand-in CentralCommunicationActor that always reports the deploy itself as + /// successful but lets a test control exactly what + /// comes back for a given command — an old site (null), a partial apply (a count + /// short of what was shipped), or a correct ack (the shipped count) — so the secrets- + /// reconciliation gate can be exercised independently of the wire-level success/failure + /// the shared models. + /// + private class SecretsAckProbeActor : ReceiveActor + { + public SecretsAckProbeActor(ArtifactProbeRecorder recorder, Func secretsApplied) + { + Receive(env => + { + if (env.Message is DeployArtifactsCommand cmd) + { + recorder.Received.Add(cmd); + Sender.Tell(new ArtifactDeploymentResponse( + cmd.DeploymentId, env.SiteId, true, null, DateTimeOffset.UtcNow, + secretsApplied(cmd))); + } + }); + } + } }