fix(site-secrets): ship an empty Secrets list, not null, when a site references no secrets

The site applies DeployArtifactsCommand.Secrets as the full set (rows absent from it are
soft-removed and DeployedSecretsMirror deletes them on both nodes), while null means
'central shipped no secrets' and touches nothing. The command builder shipped null for a
site with no references, so dropping the LAST secret: reference never removed the secret
from either site node. Found by the Task 13 live gate (check 5: site-a-a and site-a-b
both still held sb-gate-mxgw r1, deployed_secrets.is_removed=0 after the removal deploy).

Central now always ships a list. Old sites ignore proto field 9; the artifact hash is
unchanged (empty and null hash identically); SecretsDeployed stays null and no
DeploySiteSecrets audit row is written when nothing shipped. Regression test
NoSecretReferences_ShipsEmptySecretsList_SoTheSiteRemovesPreviouslyDeployedOnes (red
before, green after). Spec notes in Component-DeploymentManager/SiteRuntime + the design
doc's as-built section.
This commit is contained in:
Joseph Doherty
2026-09-28 20:43:36 -04:00
parent 1444f85919
commit ad19d4b158
5 changed files with 63 additions and 2 deletions
@@ -220,6 +220,12 @@ accurate as the spec:
side effect that needs explicit user consent, and it would be a dead pin here regardless —
hub retirement (above) removes ScadaBridge's only dependency on the hub client entirely. It
remains available for the library's other hub consumers once published.
- **Empty, not null, when a site references no secrets (Task 13 live-gate fix).** The command
builder first shipped `Secrets = null` for a site with no references ("null-when-empty", borrowed
from the hash convention). But the site treats `null` as "central shipped no secrets" and touches
nothing, so dropping a site's LAST `secret:` reference never soft-removed the row and the secret
stayed live on both nodes. Central now always ships a list (empty when there are no references);
the hash is unchanged because an empty and a null list hash identically.
- **Rig posture.** `docker/docker-compose.yml`'s central nodes now carry
`Secrets__Store__Provider=SqlServer` plus a shared KEK; site-a shares that KEK (it is the
secrets-via-artifact-deploy site); site-b/site-c deliberately carry a **mismatched** KEK so the
@@ -375,6 +375,15 @@ out below.
null-when-empty convention for every artifact class), so a site with no
secret references keeps its pre-existing hash across the upgrade that added
this feature.
- **Always a list on the wire.** `DeployArtifactsCommand.Secrets` is never
`null` from a secrets-aware central — a site with no secret references gets an
**empty** list. The site applies it as the full set (every `deployed_secrets`
row absent from it is soft-removed and the mirror deletes it on both nodes),
whereas `null` is the pre-feature "central shipped no secrets" contract that
touches nothing; shipping `null` once the last reference was dropped left the
secret live on both site nodes indefinitely (caught by the Task 13 live gate,
check 5). An old site ignores proto field 9, so the empty list costs it
nothing, and the hash is unaffected (empty == null above).
- **Audit.** One best-effort `DeploySiteSecrets` / `SiteSecrets` audit row is
written per site deploy that carried secrets or a missing-secret failure
(never for a site with no secret references at all) — after-state is `{
+1 -1
View File
@@ -163,7 +163,7 @@ flowchart TD
- **Reconciles deletions**: each non-null artifact list is the complete set for its class, so after upserting, any stored row absent from the set is deleted (the artifact was deleted centrally — there is no per-artifact delete command). A removed shared script is also unregistered from the compiled script library, and a removed data connection's live DCL connection actor is stopped. A `null` list means "field not shipped" and reconciles nothing.
### Deployed Secrets (2026-09-28)
- `HandleDeployArtifacts` additionally accepts the command's `Secrets` list (`SecretArtifact`, ciphertext only) and writes the full shipped set to the `deployed_secrets` table via `SiteStorageService.UpsertDeployedSecretsAsync` — upsert every shipped name, soft-remove (`is_removed = 1`) any stored name absent from the set. A `null` `Secrets` field (pre-feature central) touches nothing and answers with no `SecretsApplied`. Storage detail: *Deployed Secrets (Site SQLite / LocalDb)* below.
- `HandleDeployArtifacts` additionally accepts the command's `Secrets` list (`SecretArtifact`, ciphertext only) and writes the full shipped set to the `deployed_secrets` table via `SiteStorageService.UpsertDeployedSecretsAsync` — upsert every shipped name, soft-remove (`is_removed = 1`) any stored name absent from the set. A `null` `Secrets` field (pre-feature central) touches nothing and answers with no `SecretsApplied`. A current central always ships a list — **empty** when the site references no secrets — so dropping a site's last `secret:` reference removes the secret from both nodes. Storage detail: *Deployed Secrets (Site SQLite / LocalDb)* below.
- After writing the table, the active node runs `DeployedSecretsMirror.ReconcileAsync` **synchronously**, before replying — so the deploy response's `SecretsApplied` count and any apply/verify error reflect this deploy's own outcome, not a value left over from the mirror's background schedule. `SecretsApplied` counts only the names *this deploy shipped* that applied and verified cleanly; a mirror failure for an unrelated, already-broken name (a different deploy's leftover, or a stale removal) is logged at Warning and excluded — it neither fails this deploy nor reduces its `SecretsApplied`.
- No `DeployedSecretsMirror` registered (secrets deployment not composed on this node) with a non-empty `Secrets` list fails the deploy with `Secret deployment is not enabled on this site node`; with an empty `Secrets` list it still succeeds (`SecretsApplied = 0`).
- See *Deployed Secrets (Site SQLite / LocalDb)* below for the mirror itself.
@@ -182,7 +182,15 @@ public class ArtifactDeploymentService
// SMTP config (incl. credentials) is central-only — never shipped to sites.
SmtpConfigurations: null,
DateTimeOffset.UtcNow,
Secrets: collected.Secrets.Count > 0 ? collected.Secrets : null,
// ALWAYS a list — never null — even when the site references no secrets. The site
// applies Secrets as the FULL set (every deployed_secrets row absent from it is
// marked removed and the mirror deletes it), and a null Secrets means "central did
// not ship secrets at all" (the pre-feature/old-central contract), which touches
// nothing. Shipping null here when the LAST reference is dropped therefore left the
// secret live on both site nodes forever (Task 13 live gate, check 5). An empty
// list costs an old site nothing (it ignores proto field 9), and the hash is
// unaffected: the hash service folds an empty and a null list identically.
Secrets: collected.Secrets,
MissingSecrets: collected.Missing.Count > 0 ? collected.Missing.Select(m => m.Name).ToList() : null);
return (command, collected.Missing);
@@ -726,6 +726,44 @@ public class ArtifactDeploymentServiceTests : TestKit
await _siteRepo.Received(1).UpdateSiteAsync(site, Arg.Any<CancellationToken>());
}
[Fact]
public async Task NoSecretReferences_ShipsEmptySecretsList_SoTheSiteRemovesPreviouslyDeployedOnes()
{
// The site applies DeployArtifactsCommand.Secrets as the FULL set: rows absent from it
// are marked removed and the mirror deletes them, while a NULL Secrets means "central
// shipped no secrets" and touches nothing. Dropping a site's LAST secret reference must
// therefore ship an empty list, not null — otherwise the previously deployed secret stays
// live on both site nodes forever (Task 13 live gate, check 5).
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":"a-literal-key"}""" }
});
var recorder = new ArtifactProbeRecorder();
var probe = Sys.ActorOf(Props.Create(() => new SecretsAckProbeActor(recorder, _ => 0)));
var service = CreateServiceWithCommActor(probe);
var result = await service.DeployToAllSitesAsync("admin");
Assert.True(result.IsSuccess);
var cmd = Assert.Single(recorder.Received);
Assert.NotNull(cmd.Secrets);
Assert.Empty(cmd.Secrets!);
Assert.Null(cmd.MissingSecrets);
var siteResult = Assert.Single(result.Value.SiteResults);
Assert.True(siteResult.Success);
// Nothing shipped => nothing to confirm; SecretsDeployed stays null, and no
// DeploySiteSecrets audit row is written for a deploy that carried no secrets.
Assert.Null(siteResult.SecretsDeployed);
Assert.NotNull(site.ArtifactRevisionHash);
await _audit.DidNotReceive().LogAsync(
Arg.Any<string>(), "DeploySiteSecrets", Arg.Any<string>(), Arg.Any<string>(),
Arg.Any<string>(), Arg.Any<object?>(), Arg.Any<CancellationToken>());
}
[Fact]
public async Task SecretDeploy_WritesOneAuditRowPerSite_WithoutMaterial()
{