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

feat(artifacts): never stamp a site current unless it applied every secret; DeploySiteSecrets audit row

Browse Source
This commit is contained in:
Joseph Doherty
2026-09-28 18:20:54 -04:00
parent 75b2a7244c
commit 00c89aff12
2 changed files with 334 additions and 9 deletions
Download Patch File Download Diff File Expand all files Collapse all files
src/ZB.MOM.WW.ScadaBridge.DeploymentManager/ArtifactDeploymentService.cs
+128 -8
View File
@@ -188,6 +188,86 @@ public class ArtifactDeploymentService
return (command, collected.Missing);
}
/// <summary>
/// Task 7 (design §3.4): reconciles a site's raw <see cref="ArtifactDeploymentResponse"/>
/// against what the deployed <see cref="DeployArtifactsCommand"/> 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 <c>SecretsApplied = null</c> 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
/// <see cref="SiteArtifactResult.Success"/>.
///
/// Only fires when the command actually shipped secrets — a site with nothing to apply
/// truthfully has nothing to confirm, so its <c>SecretsApplied</c> stays uninterpreted
/// (old-site behaviour is unchanged when no secrets are in play, per the acceptance
/// criteria).
/// </summary>
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);
}
/// <summary>
/// Best-effort <c>DeploySiteSecrets</c> audit row (task 7): one row per site whose command
/// carried, or would have carried, at least one secret — a resolved
/// <see cref="DeployArtifactsCommand.Secrets"/> entry, an unresolved reference in
/// <paramref name="missing"/> (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 <see cref="SiteArtifactResult.Success"/> beforehand — change
/// whether the site gets stamped.
///
/// The <c>afterState</c> payload is built ONLY from names/revisions/missing names/success/
/// error — never <see cref="SecretArtifact.Ciphertext"/>, <c>Nonce</c>, <c>Tag</c>,
/// <c>WrappedDek</c>, or any other material (global constraint: never plaintext or material
/// in logs or audit rows).
/// </summary>
private Task LogSecretsAuditAsync(
string user,
DeployArtifactsCommand command,
IReadOnlyList<SecretReference> 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);
}
/// <summary>
/// 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<SiteArtifactResult>.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<SiteArtifactResult>.Success(result)
: Result<SiteArtifactResult>.Failure(response.ErrorMessage ?? "Retry failed.");
: Result<SiteArtifactResult>.Failure(result.ErrorMessage ?? "Retry failed.");
}
catch (Exception ex)
{
@@ -673,11 +784,20 @@ public class ArtifactDeploymentService
/// <summary>
/// Per-site result for artifact deployment.
/// </summary>
/// <param name="SecretsDeployed">
/// How many secrets the site confirmed applying, when the deployed command shipped at
/// least one (task 7, design §3.4). <c>null</c> when the site shipped no secrets at all
/// — deliberately the same value an old site's unset <see cref="ArtifactDeploymentResponse.SecretsApplied"/>
/// 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.
/// </param>
public record SiteArtifactResult(
string SiteId,
string SiteName,
bool Success,
string? ErrorMessage);
string? ErrorMessage,
int? SecretsDeployed = null);
/// <summary>
/// Summary of system-wide artifact deployment with per-site results.
tests/ZB.MOM.WW.ScadaBridge.DeploymentManager.Tests/ArtifactDeploymentServiceTests.cs
+206 -1
View File
@@ -600,7 +600,11 @@ public class ArtifactDeploymentServiceTests : TestKit
.Returns(Task.FromResult<StoredSecret?>(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<ArtifactDeploymentService>();
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<CancellationToken>()).Returns(new List<Site> { site });
_siteRepo.GetDataConnectionsBySiteIdAsync(1, Arg.Any<CancellationToken>())
.Returns(new List<DataConnection>
{
new("GW1", "MxGateway", 1) { PrimaryConfiguration = """{"ApiKey":"secret:the-key"}""" }
});
_secretStore.GetAsync(Arg.Is<SecretName>(n => n.Value == "the-key"), Arg.Any<CancellationToken>())
.Returns(Task.FromResult<StoredSecret?>(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<CancellationToken>());
}
[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<CancellationToken>()).Returns(new List<Site> { site });
_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", 1)));
_secretStore.GetAsync(Arg.Is<SecretName>(n => n.Value == "key2"), Arg.Any<CancellationToken>())
.Returns(Task.FromResult<StoredSecret?>(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<CancellationToken>());
}
[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<CancellationToken>()).Returns(new List<Site> { site });
_siteRepo.GetDataConnectionsBySiteIdAsync(1, Arg.Any<CancellationToken>())
.Returns(new List<DataConnection>());
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<CancellationToken>());
}
[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<CancellationToken>()).Returns(new List<Site> { secretSite, plainSite });
_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>());
_secretStore.GetAsync(Arg.Is<SecretName>(n => n.Value == "present-key"), Arg.Any<CancellationToken>())
.Returns(Task.FromResult<StoredSecret?>(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<string>(), "DeploySiteSecrets", Arg.Any<string>(), Arg.Any<string>(), Arg.Any<string>(),
Arg.Any<object>(), Arg.Any<CancellationToken>()))
.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<ArtifactDeploymentService>? logger = null)
{
var comms = new CommunicationService(
@@ -726,4 +906,29 @@ public class ArtifactDeploymentServiceTests : TestKit
});
}
}
/// <summary>
/// Task 7: stand-in CentralCommunicationActor that always reports the deploy itself as
/// successful but lets a test control exactly what <see cref="ArtifactDeploymentResponse.SecretsApplied"/>
/// comes back for a given command — an old site (<c>null</c>), 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 <see cref="ArtifactProbeActor"/> models.
/// </summary>
private class SecretsAckProbeActor : ReceiveActor
{
public SecretsAckProbeActor(ArtifactProbeRecorder recorder, Func<DeployArtifactsCommand, int?> secretsApplied)
{
Receive<SiteEnvelope>(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)));
}
});
}
}
}