From 6f1787abe193d32707a3fdd76ab30fe701ebac3f Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Mon, 28 Sep 2026 21:08:34 -0400 Subject: [PATCH] fix(site-secrets): mirror heals same-revision local drift and isolates unreadable rows Final review I1: a name already ledgered at the shipped revision is now read back every pass; a missing, tombstoned or edited local row is healed through the mirror-owned re-stamp path (bump, apply, read back, invalidate) before verify, instead of a local delete being misreported as a KEK mismatch that blocked every later deploy. Drift logs Warning once per occurrence. M4: row_json that cannot be parsed becomes a Failed entry for that name via the new SiteStorageService.GetDeployedSecretsTolerantAsync; an unreadable removed row is still honoured by name. M10: invalidator ctor doc corrected. --- .../Persistence/SiteStorageService.cs | 66 +++++++-- .../Secrets/DeployedSecretsMirror.cs | 110 ++++++++++++--- .../Secrets/DeployedSecretsMirrorTests.cs | 128 ++++++++++++++++++ 3 files changed, 277 insertions(+), 27 deletions(-) diff --git a/src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Persistence/SiteStorageService.cs b/src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Persistence/SiteStorageService.cs index 499f7217..880c0cbf 100644 --- a/src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Persistence/SiteStorageService.cs +++ b/src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Persistence/SiteStorageService.cs @@ -780,9 +780,32 @@ public class SiteStorageService /// /// Returns every row in deployed_secrets, including soft-deleted (is_removed) /// ones, with each row's round-tripped from row_json. + /// Strict: a row whose row_json cannot be parsed throws. The per-node mirror reads + /// through instead, so one unreadable row is + /// reported against its own name rather than aborting the whole reconcile pass. /// /// A task that resolves to every stored deployed-secret row. public async Task> GetDeployedSecretsAsync() + { + var (rows, unreadable) = await GetDeployedSecretsTolerantAsync(); + if (unreadable.Count > 0) + { + throw new InvalidOperationException( + $"deployed_secrets row '{unreadable[0].Name}' has invalid row_json ({unreadable[0].Error})"); + } + + return rows; + } + + /// + /// Returns every row in deployed_secrets like , + /// but never throws for a single bad row: a row whose row_json does not deserialize + /// is returned in Unreadable (name, removal flag and the parse error — never the + /// row's content) instead of failing the whole read (final review M4, per-row isolation). + /// + /// The parsed rows and the rows that could not be parsed. + public async Task<(IReadOnlyList Rows, IReadOnlyList Unreadable)> + GetDeployedSecretsTolerantAsync() { await using var connection = OpenConnection(); @@ -790,22 +813,39 @@ public class SiteStorageService command.CommandText = "SELECT name, row_json, is_removed, deployment_id FROM deployed_secrets"; var results = new List(); + var unreadable = new List(); await using var reader = await command.ExecuteReaderAsync(); while (await reader.ReadAsync()) { var name = reader.GetString(0); - var rowJson = reader.GetString(1); - var secret = JsonSerializer.Deserialize(rowJson) - ?? throw new InvalidOperationException($"deployed_secrets row '{name}' has invalid row_json"); + var isRemoved = reader.GetInt64(2) != 0; - results.Add(new DeployedSecretRow( - name, - secret, - reader.GetInt64(2) != 0, - reader.GetString(3))); + SecretArtifact? secret; + string? error = null; + try + { + secret = JsonSerializer.Deserialize(reader.GetString(1)); + if (secret is null) + { + error = "row_json deserialized to null"; + } + } + catch (Exception ex) when (ex is JsonException or NotSupportedException or InvalidCastException) + { + secret = null; + error = $"{ex.GetType().Name}: {ex.Message}"; + } + + if (secret is null) + { + unreadable.Add(new UnreadableDeployedSecretRow(name, isRemoved, error!)); + continue; + } + + results.Add(new DeployedSecretRow(name, secret, isRemoved, reader.GetString(3))); } - return results; + return (results, unreadable); } // ── Mirrored Secrets ledger (node-local only — deliberately NOT LocalDb-replicated) ── @@ -1331,6 +1371,14 @@ public record NativeAlarmRow( /// public record DeployedSecretRow(string Name, SecretArtifact Secret, bool IsRemoved, string DeploymentId); +/// +/// A deployed_secrets row whose row_json could not be parsed — returned by +/// so the mirror can report it +/// against its own name instead of aborting the pass. is the parse error +/// (exception type and message), never the row's content. +/// +public record UnreadableDeployedSecretRow(string Name, bool IsRemoved, string Error); + /// /// A single row from the site-local, node-local (NOT LocalDb-replicated) mirrored_secrets /// ledger — the names this node has mirrored into its own secret store, and at what revision. diff --git a/src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Secrets/DeployedSecretsMirror.cs b/src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Secrets/DeployedSecretsMirror.cs index c02e8cd5..993d1565 100644 --- a/src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Secrets/DeployedSecretsMirror.cs +++ b/src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Secrets/DeployedSecretsMirror.cs @@ -94,6 +94,12 @@ public sealed record MirrorResult( /// that the deployed revision never took effect). /// /// +/// Drift at the same revision (final review I1). A name already ledgered at the shipped +/// revision is still read back every pass: if an operator deleted it locally, or edited its value, +/// the local row no longer matches the shipped ciphertext and is healed exactly like the owned +/// re-stamp case above (bumped stamp, apply, read-back, cache invalidation) before the verify — +/// so a local delete is repaired rather than misreported as a master-key mismatch, and a local +/// edit is reverted to the deployed value. /// Ledgering always uses the SHIPPED artifact's own revision (not any bumped local revision) so /// a later pass's "is this ledgered at the shipped revision" check stays correct. /// @@ -111,8 +117,8 @@ public sealed record MirrorResult( /// ; every ZB.MOM.WW.Secrets write path invalidates after a /// write for exactly this reason. This mirror does the same: after every successful apply /// (including a tombstone re-materialization) or delete, and before the verify step, it calls -/// (injected optionally — null if the -/// host never registered one) so neither the verify nor a script running immediately afterward +/// (a null invalidator is tolerated for +/// direct construction; under DI AddZbSecrets always registers one) so neither the verify nor a script running immediately afterward /// can observe a stale pre-rotation plaintext for up to the cache TTL. /// /// @@ -172,14 +178,23 @@ public sealed class DeployedSecretsMirror : BackgroundService /// private readonly Dictionary _lastLoggedFailure = new(StringComparer.Ordinal); + /// + /// Names whose same-revision local drift (final review I1) has already been logged at + /// Warning and not yet healed — a heal that keeps failing logs its drift at Debug on repeats, + /// matching the de-spam rule. Cleared on a successful apply. + /// + private readonly HashSet _driftLogged = new(StringComparer.Ordinal); + /// Initializes a new instance of . /// Site-local storage exposing deployed_secrets and the mirrored_secrets ledger. /// This node's own secret store. /// This node's own secret resolver, used to trial-decrypt each deployed secret. /// - /// Optional cache invalidator for (the concrete - /// DefaultSecretResolver implements this itself; null if the host never - /// registered one, in which case no invalidation is attempted). + /// Cache invalidator for (the concrete + /// DefaultSecretResolver implements this itself). The parameter has no default, so + /// under MS DI activation fails unless an is registered + /// — AddZbSecrets always registers one. A caller constructing the mirror directly may + /// pass , in which case no invalidation is attempted. /// /// Supplies . /// Logger instance. Never logs secret values — names, revisions, and KEK ids only. @@ -217,7 +232,10 @@ public sealed class DeployedSecretsMirror : BackgroundService await _gate.WaitAsync(ct).ConfigureAwait(false); try { - var rows = await _storage.GetDeployedSecretsAsync().ConfigureAwait(false); + // Tolerant read (final review M4): a row whose row_json cannot be parsed is handed + // back separately instead of throwing out of the whole pass, so it is reported + // against its own name like any other per-row failure. + var (rows, unreadable) = await _storage.GetDeployedSecretsTolerantAsync().ConfigureAwait(false); var mirrored = (await _storage.GetMirroredSecretsAsync().ConfigureAwait(false)) .ToDictionary(m => m.Name, m => m.Revision, StringComparer.Ordinal); @@ -232,7 +250,7 @@ public sealed class DeployedSecretsMirror : BackgroundService { if (row.IsRemoved) { - await ProcessRemovedRowAsync(row, mirrored, removed, ct).ConfigureAwait(false); + await ProcessRemovedRowAsync(row.Name, mirrored, removed, ct).ConfigureAwait(false); } else { @@ -250,6 +268,37 @@ public sealed class DeployedSecretsMirror : BackgroundService } } + foreach (var bad in unreadable) + { + try + { + if (bad.IsRemoved) + { + // A removal needs only the name, so an unreadable tombstone is still + // honoured for a name this node previously mirrored. + await ProcessRemovedRowAsync(bad.Name, mirrored, removed, ct).ConfigureAwait(false); + } + else + { + ReportFailure( + bad.Name, + $"unreadable:{bad.Error}", + $"Secret '{bad.Name}' could not be read from deployed_secrets on this node ({bad.Error})", + failed, + failedNames); + } + } + catch (Exception ex) when (!ct.IsCancellationRequested) + { + ReportFailure( + bad.Name, + $"error:unreadable:{ex.GetType().Name}:{ex.Message}", + $"Secret '{bad.Name}' failed to process on this node ({ex.GetType().Name}: {ex.Message})", + failed, + failedNames); + } + } + var result = new MirrorResult(applied, removed, failed, failedNames); if (result.Changed) @@ -262,7 +311,7 @@ public sealed class DeployedSecretsMirror : BackgroundService { _logger.LogDebug( "Deployed secrets mirror in sync ({Count} secret(s), {FailedCount} failing)", - rows.Count(r => !r.IsRemoved), failed.Count); + rows.Count(r => !r.IsRemoved) + unreadable.Count(u => !u.IsRemoved), failed.Count); } return result; @@ -279,26 +328,28 @@ public sealed class DeployedSecretsMirror : BackgroundService /// local secret this mirror never applied is left alone. /// private async Task ProcessRemovedRowAsync( - DeployedSecretRow row, Dictionary mirrored, List removed, CancellationToken ct) + string rowName, Dictionary mirrored, List removed, CancellationToken ct) { - if (!mirrored.ContainsKey(row.Name)) + if (!mirrored.ContainsKey(rowName)) { return; } - await _store.DeleteAsync(new SecretName(row.Name), DeleteActor, ct).ConfigureAwait(false); - _invalidator?.Invalidate(new SecretName(row.Name)); - await _storage.RemoveMirroredSecretAsync(row.Name).ConfigureAwait(false); - removed.Add(row.Name); - _lastLoggedFailure.Remove(row.Name); + await _store.DeleteAsync(new SecretName(rowName), DeleteActor, ct).ConfigureAwait(false); + _invalidator?.Invalidate(new SecretName(rowName)); + await _storage.RemoveMirroredSecretAsync(rowName).ConfigureAwait(false); + removed.Add(rowName); + _lastLoggedFailure.Remove(rowName); _logger.LogInformation( "Removed deployed secret {SecretName} (no longer referenced by this site)", - row.Name); + rowName); } /// /// Handles a live deployed_secrets row: applies it (confirmed by read-back) when not - /// already ledgered at the shipped revision, then verifies it decrypts — unless the apply + /// already ledgered at the shipped revision, or when it is ledgered but the local row has + /// since gone missing, been tombstoned or drifted to different ciphertext (healed via the + /// mirror-owned re-stamp path), then verifies it decrypts — unless the apply /// outcome makes a verify meaningless (an out-of-band local secret won LWW), in which case a /// single, distinct failure is reported instead. /// @@ -319,7 +370,29 @@ public sealed class DeployedSecretsMirror : BackgroundService var ownedByMirror = mirrored.TryGetValue(row.Name, out var mirroredRevision); var alreadyLedgered = ownedByMirror && mirroredRevision == row.Secret.Revision; - if (!alreadyLedgered) + // Ledgered at the shipped revision is NOT proof the local row still holds it (final + // review I1): an operator may have deleted or edited this mirror-owned name on the node + // since. Read it back; only a live row whose ciphertext matches the shipped artifact goes + // straight to verify. A missing, tombstoned or drifted row is healed through the same + // mirror-owned re-stamp path as any other owned mismatch — central is authoritative for + // every name it deploys — instead of being misreported as a master-key mismatch. + var needsApply = !alreadyLedgered; + if (alreadyLedgered) + { + var local = await _store.GetAsync(name, ct).ConfigureAwait(false); + if (local is null || local.IsDeleted || !MatchesShippedCiphertext(local, ToStored(row.Secret))) + { + needsApply = true; + _logger.Log( + _driftLogged.Add(row.Name) ? LogLevel.Warning : LogLevel.Debug, + "Deployed secret {SecretName} r{Revision} was {Drift} on this node; re-applying the deployed revision", + row.Name, + row.Secret.Revision, + local is null ? "missing" : local.IsDeleted ? "deleted" : "changed"); + } + } + + if (needsApply) { var outcome = await ApplyAndConfirmAsync(row, name, ownedByMirror, ct).ConfigureAwait(false); @@ -330,6 +403,7 @@ public sealed class DeployedSecretsMirror : BackgroundService _invalidator?.Invalidate(name); applied.Add(row.Name); _lastLoggedFailure.Remove(row.Name); + _driftLogged.Remove(row.Name); _logger.LogInformation( "Applied deployed secret {SecretName} r{Revision} (kek {KekId})", row.Name, row.Secret.Revision, row.Secret.KekId); diff --git a/tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Secrets/DeployedSecretsMirrorTests.cs b/tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Secrets/DeployedSecretsMirrorTests.cs index 0209d552..f7f728a5 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Secrets/DeployedSecretsMirrorTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Secrets/DeployedSecretsMirrorTests.cs @@ -205,6 +205,126 @@ public class DeployedSecretsMirrorTests : IDisposable Assert.Equal(2, store.Rows["a"].Revision); } + // ── Same-revision drift (final review I1) ── + + [Fact] + public async Task Reconcile_OwnedNameDeletedLocally_IsHealedAndVerified_NotMisreportedAsKekMismatch() + { + var time = new FakeTimeProvider(new DateTimeOffset(2026, 9, 28, 18, 0, 0, TimeSpan.Zero)); + var (mirror, storage, store, resolver, invalidator) = await CreateAsync(time: time); + resolver.BackingStore = store; // resolve returns null once the local row is gone + await storage.UpsertDeployedSecretsAsync([Art("a", 1)], "d1"); + Assert.Equal(["a"], (await mirror.ReconcileAsync(CancellationToken.None)).Applied); + + // An operator deletes the mirror-owned name on this node (library CLI/UI) — the + // ledger still says r1 is mirrored, and central still ships r1. + await store.DeleteAsync(new SecretName("a"), "operator", CancellationToken.None); + Assert.True(store.IsTombstoned("a")); + invalidator.Invalidated.Clear(); + + var r = await mirror.ReconcileAsync(CancellationToken.None); + + Assert.Empty(r.Failed); + Assert.Equal(["a"], r.Applied); + Assert.True(store.Contains("a")); + Assert.Equal(new byte[] { 1, 2, 3, 4, 5 }, store.Rows["a"].Ciphertext); + Assert.Contains("a", invalidator.Invalidated); + + // Healed state is stable: the next pass is a no-op that still verifies clean. + var next = await mirror.ReconcileAsync(CancellationToken.None); + Assert.False(next.Changed); + Assert.Empty(next.Failed); + Assert.Equal(1, (await storage.GetMirroredSecretsAsync()).Single(m => m.Name == "a").Revision); + } + + [Fact] + public async Task Reconcile_OwnedNameEditedLocally_IsRevertedToTheDeployedCiphertext() + { + var time = new FakeTimeProvider(new DateTimeOffset(2026, 9, 28, 18, 0, 0, TimeSpan.Zero)); + var (mirror, storage, store, _, invalidator) = await CreateAsync(time: time); + await storage.UpsertDeployedSecretsAsync([Art("a", 1)], "d1"); + Assert.Equal(["a"], (await mirror.ReconcileAsync(CancellationToken.None)).Applied); + + // An operator overwrites the mirror-owned name locally with a NEWER stamp. + store.Seed("a", + updatedUtc: new DateTimeOffset(2026, 9, 28, 12, 0, 0, TimeSpan.Zero), + revision: 7, + ciphertext: [200, 201, 202]); + invalidator.Invalidated.Clear(); + + var r = await mirror.ReconcileAsync(CancellationToken.None); + + Assert.Empty(r.Failed); + Assert.Equal(["a"], r.Applied); + Assert.Equal(new byte[] { 1, 2, 3, 4, 5 }, store.Rows["a"].Ciphertext); + Assert.Equal(new byte[] { 30, 31, 32, 33, 34, 35 }, store.Rows["a"].WrappedDek); + Assert.True(store.Rows["a"].Revision > 7); // re-stamped past the operator's row + Assert.Contains("a", invalidator.Invalidated); + } + + [Fact] + public async Task Reconcile_OwnedNameDrift_LogsOncePerDriftNotEveryPass() + { + var logger = new CapturingLogger(); + var time = new FakeTimeProvider(new DateTimeOffset(2026, 9, 28, 18, 0, 0, TimeSpan.Zero)); + var (mirror, storage, store, _, _) = await CreateAsync(logger: logger, time: time); + await storage.UpsertDeployedSecretsAsync([Art("a", 1)], "d1"); + await mirror.ReconcileAsync(CancellationToken.None); + await store.DeleteAsync(new SecretName("a"), "operator", CancellationToken.None); + + await mirror.ReconcileAsync(CancellationToken.None); + await mirror.ReconcileAsync(CancellationToken.None); + await mirror.ReconcileAsync(CancellationToken.None); + + Assert.Single(logger.Entries, e => e.Level == LogLevel.Warning && e.Message.Contains("re-applying")); + Assert.DoesNotContain(logger.Entries, e => e.Level == LogLevel.Error); + } + + // ── Unreadable row_json (final review M4) ── + + [Fact] + public async Task Reconcile_UnparseableRowJson_IsAFailedEntryForThatName_OthersStillApply() + { + var (mirror, storage, store, _, _) = await CreateAsync(); + await storage.UpsertDeployedSecretsAsync([Art("bad", 1), Art("good", 1)], "d1"); + await CorruptRowJsonAsync(storage, "bad"); + + var r = await mirror.ReconcileAsync(CancellationToken.None); + + Assert.Equal(["good"], r.Applied); + Assert.Equal(["bad"], r.FailedNames); + var message = Assert.Single(r.Failed); + Assert.Contains("'bad'", message); + Assert.Contains("could not be read", message); + Assert.True(store.Contains("good")); + Assert.False(store.Contains("bad")); + } + + [Fact] + public async Task Reconcile_UnparseableRemovedRow_StillRemovesAMirroredName() + { + var (mirror, storage, store, _, _) = await CreateAsync(); + await storage.UpsertDeployedSecretsAsync([Art("a", 1)], "d1"); + await mirror.ReconcileAsync(CancellationToken.None); + await storage.UpsertDeployedSecretsAsync([], "d2"); // soft-removes "a" + await CorruptRowJsonAsync(storage, "a"); + + var r = await mirror.ReconcileAsync(CancellationToken.None); + + Assert.Equal(["a"], r.Removed); + Assert.Empty(r.Failed); + Assert.True(store.IsTombstoned("a")); + } + + private static async Task CorruptRowJsonAsync(SiteStorageService storage, string name) + { + await using var connection = storage.CreateConnection(); + await using var command = connection.CreateCommand(); + command.CommandText = "UPDATE deployed_secrets SET row_json = '{not json' WHERE name = @name"; + command.Parameters.AddWithValue("@name", name); + Assert.Equal(1, await command.ExecuteNonQueryAsync()); + } + [Fact] public async Task Reconcile_AppliesThroughApplyReplicatedAsync_MappedFromSecretArtifact() { @@ -740,6 +860,9 @@ public class DeployedSecretsMirrorTests : IDisposable public int CallCount; + /// When set, a name that is absent or tombstoned in this store resolves to null, like the real resolver. + public FakeSecretStore? BackingStore { get; set; } + /// Row absent/tombstoned path — DefaultSecretResolver.GetAsync returns null. public void ReturnNullFor(string name) => _nullReturning.Add(name); @@ -760,6 +883,11 @@ public class DeployedSecretsMirrorTests : IDisposable throw new InvalidOperationException("simulated decrypt failure"); } + if (BackingStore is not null && !BackingStore.Contains(name.Value)) + { + return Task.FromResult(null); + } + return Task.FromResult(_nullReturning.Contains(name.Value) ? null : "decrypted-value"); } }