diff --git a/docs/plans/2026-09-28-site-secrets-artifact-deploy-design.md b/docs/plans/2026-09-28-site-secrets-artifact-deploy-design.md index b05a7560..07efb8bc 100644 --- a/docs/plans/2026-09-28-site-secrets-artifact-deploy-design.md +++ b/docs/plans/2026-09-28-site-secrets-artifact-deploy-design.md @@ -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 diff --git a/docs/requirements/Component-DeploymentManager.md b/docs/requirements/Component-DeploymentManager.md index 45157639..56627d06 100644 --- a/docs/requirements/Component-DeploymentManager.md +++ b/docs/requirements/Component-DeploymentManager.md @@ -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 `{ diff --git a/docs/requirements/Component-SiteRuntime.md b/docs/requirements/Component-SiteRuntime.md index 643108e1..24f086dd 100644 --- a/docs/requirements/Component-SiteRuntime.md +++ b/docs/requirements/Component-SiteRuntime.md @@ -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. diff --git a/src/ZB.MOM.WW.ScadaBridge.DeploymentManager/ArtifactDeploymentService.cs b/src/ZB.MOM.WW.ScadaBridge.DeploymentManager/ArtifactDeploymentService.cs index 3141e02f..b2b8b4e5 100644 --- a/src/ZB.MOM.WW.ScadaBridge.DeploymentManager/ArtifactDeploymentService.cs +++ b/src/ZB.MOM.WW.ScadaBridge.DeploymentManager/ArtifactDeploymentService.cs @@ -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); diff --git a/tests/ZB.MOM.WW.ScadaBridge.DeploymentManager.Tests/ArtifactDeploymentServiceTests.cs b/tests/ZB.MOM.WW.ScadaBridge.DeploymentManager.Tests/ArtifactDeploymentServiceTests.cs index adcc3009..5aaf80a8 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.DeploymentManager.Tests/ArtifactDeploymentServiceTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.DeploymentManager.Tests/ArtifactDeploymentServiceTests.cs @@ -726,6 +726,44 @@ public class ArtifactDeploymentServiceTests : TestKit await _siteRepo.Received(1).UpdateSiteAsync(site, Arg.Any()); } + [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()).Returns(new List { site }); + _siteRepo.GetDataConnectionsBySiteIdAsync(1, Arg.Any()) + .Returns(new List + { + 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(), "DeploySiteSecrets", Arg.Any(), Arg.Any(), + Arg.Any(), Arg.Any(), Arg.Any()); + } + [Fact] public async Task SecretDeploy_WritesOneAuditRowPerSite_WithoutMaterial() {