diff --git a/src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Secrets/DeployedSecretsMirror.cs b/src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Secrets/DeployedSecretsMirror.cs index 2070e48b..5e97688a 100644 --- a/src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Secrets/DeployedSecretsMirror.cs +++ b/src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Secrets/DeployedSecretsMirror.cs @@ -47,35 +47,43 @@ public sealed record MirrorResult( /// strictly newer than the local row is silently ignored, so this mirror never trusts the call /// blindly: every apply is followed by an read-back, and /// mirrored_secrets is ledgered only when the local row is confirmed LIVE and its -/// ciphertext (Ciphertext + WrappedDek) matches what was just shipped. Two -/// non-ledgered outcomes are possible, both reported as a -/// entry (never as the decrypt-failure message, which would misdiagnose either case) and never -/// overwritten a second way: +/// ciphertext (Ciphertext + WrappedDek) matches what was just shipped. A +/// non-ledgered outcome is reported as a entry (never as the +/// decrypt-failure message, which would misdiagnose either case below) and never overwritten a +/// second way. Which of the two outcomes applies is decided by ledger ownership — whether +/// mirrored_secrets already has ANY entry for the name, at any revision — not merely by +/// whether the local row happens to be a tombstone: /// /// -/// The local row is a tombstone (this node previously deleted it — e.g. the site -/// stopped referencing it, then referenced it again unchanged at central, so the shipped row's -/// UpdatedUtc is older than the tombstone's). Re-stamping is safe here: neither -/// UpdatedUtc nor Revision is bound into the AEAD associated data — confirmed -/// against ZB.MOM.WW.Secrets.Crypto.AesGcmEnvelopeCipher, whose body layer binds only +/// This node's own ledger already owns the name (a tombstone this node wrote, or a LIVE +/// row with different ciphertext than what was just shipped — the latter can happen under +/// site/central clock skew, or when an earlier tombstone-recovery's bumped stamp lands ahead of +/// a since-shipped central UpdatedUtc — fix round 2, review "clock skew" finding). +/// Re-stamping is safe either way: neither UpdatedUtc nor Revision is bound into +/// the AEAD associated data — confirmed against +/// ZB.MOM.WW.Secrets.Crypto.AesGcmEnvelopeCipher, whose body layer binds only /// SecretName.Value and whose wrap layer binds only KekId — so the mirror -/// re-applies the exact shipped ciphertext with UpdatedUtc = max(now, tombstone.UpdatedUtc + -/// 1 tick) and Revision = tombstone.Revision + 1, which beats the tombstone under LWW -/// without touching anything the cipher verifies. If that confirmed re-apply still does not +/// re-applies the exact shipped ciphertext with UpdatedUtc = max(now, local.UpdatedUtc + 1 +/// tick) and Revision = local.Revision + 1, which beats the current local row under +/// LWW without touching anything the cipher verifies. If that confirmed re-apply still does not /// read back live and matching (a concurrent conflict), the row is reported failed instead of -/// silently left broken. +/// silently left broken. A real-cipher integration test +/// (DeployedSecretsMirrorRealStackTests, fix round 2) pins that a bumped-and-recovered row +/// still decrypts to the original plaintext against the real SqliteSecretStore + +/// AesGcmEnvelopeCipher + DefaultSecretResolver stack, not only by source reading. /// /// -/// The local row is LIVE with different ciphertext — an out-of-band secret created -/// directly on this node under the same name, stamped strictly newer than what central shipped. -/// It wins LWW legitimately. This mirror does NOT overwrite it, does NOT ledger it (so a later -/// central tombstone of the name can never delete someone's out-of-band secret via this -/// mirror), and does NOT run the decrypt-verify against it (verifying would trivially "succeed" -/// against the operator's own value and mask that the deployed revision never took effect). +/// The name is NOT in this node's ledger and the local row is LIVE with different +/// ciphertext — a genuinely out-of-band secret created directly on this node under the same +/// name, stamped strictly newer than what central shipped. It wins LWW legitimately. This mirror +/// does NOT overwrite it, does NOT ledger it (so a later central tombstone of the name can never +/// delete someone's out-of-band secret via this mirror), and does NOT run the decrypt-verify +/// against it (verifying would trivially "succeed" against the operator's own value and mask +/// that the deployed revision never took effect). /// /// -/// 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. +/// 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. /// /// /// Per-row isolation (review Important #2). Each row's apply/verify body runs in its own @@ -285,12 +293,17 @@ public sealed class DeployedSecretsMirror : BackgroundService CancellationToken ct) { var name = new SecretName(row.Name); - var alreadyLedgered = - mirrored.TryGetValue(row.Name, out var mirroredRevision) && mirroredRevision == row.Secret.Revision; + // "Owned by this mirror" is ANY ledger entry for the name, at whatever revision — not + // only a match on the shipped revision. Ownership, not revision equality, is what + // decides whether a live-but-mismatched local row is OUR stale stamp (safe to bump past) + // or someone else's out-of-band value (never touched) — see ApplyAndConfirmAsync and + // the class remarks (fix round 2, review Minor "clock skew"). + var ownedByMirror = mirrored.TryGetValue(row.Name, out var mirroredRevision); + var alreadyLedgered = ownedByMirror && mirroredRevision == row.Secret.Revision; if (!alreadyLedgered) { - var outcome = await ApplyAndConfirmAsync(row, name, ct).ConfigureAwait(false); + var outcome = await ApplyAndConfirmAsync(row, name, ownedByMirror, ct).ConfigureAwait(false); switch (outcome) { @@ -318,7 +331,7 @@ public sealed class DeployedSecretsMirror : BackgroundService row.Name, $"unconfirmed:{row.Secret.Revision}", $"Secret '{row.Name}' could not be confirmed applied on this node " + - "after a tombstone re-apply attempt", + "after a re-apply attempt", failed); return; } @@ -344,7 +357,17 @@ public sealed class DeployedSecretsMirror : BackgroundService /// Calls and confirms the outcome by reading /// the row back — see the class remarks for why this mirror never trusts the call blindly. /// - private async Task ApplyAndConfirmAsync(DeployedSecretRow row, SecretName name, CancellationToken ct) + /// The shipped row to apply. + /// The row's . + /// + /// Whether this node's ledger already has ANY entry for this name (at any revision) — i.e. + /// this mirror, not an operator, is the one that put the current local row there. A + /// live-but-mismatched local row is only ever re-stamped (never reported as out-of-band) + /// when this is . + /// + /// A token to cancel the operation. + private async Task ApplyAndConfirmAsync( + DeployedSecretRow row, SecretName name, bool ownedByMirror, CancellationToken ct) { var shipped = ToStored(row.Secret); await _store.ApplyReplicatedAsync(shipped, ct).ConfigureAwait(false); @@ -356,14 +379,20 @@ public sealed class DeployedSecretsMirror : BackgroundService return ApplyOutcome.Applied; } - if (local is not null && local.IsDeleted) + if (local is not null && (local.IsDeleted || ownedByMirror)) { - // Neither UpdatedUtc nor Revision is bound into the AEAD associated data (confirmed - // against AesGcmEnvelopeCipher — only SecretName and KekId are AAD), so re-stamping - // strictly past the tombstone is safe: it changes nothing the cipher verifies. + // Either a tombstone (this node deleted it, or some earlier apply left it that way), + // or a LIVE row that is nonetheless OURS — the ledger already has an entry for this + // name, so the mismatch is this mirror's own earlier stamp (e.g. a prior tombstone + // recovery's bumped UpdatedUtc landing ahead of a central UpdatedUtc that arrives + // later, under site/central clock skew — review "clock skew" finding), not an + // operator's out-of-band value. Both are safe to re-stamp: neither UpdatedUtc nor + // Revision is bound into the AEAD associated data (confirmed against + // AesGcmEnvelopeCipher — only SecretName and KekId are AAD), so re-stamping strictly + // past the current local row changes nothing the cipher verifies. var nowUtc = _time.GetUtcNow(); - var tombstoneUtcPlusOneTick = local.UpdatedUtc.AddTicks(1); - var bumpedUpdatedUtc = nowUtc > tombstoneUtcPlusOneTick ? nowUtc : tombstoneUtcPlusOneTick; + var localUtcPlusOneTick = local.UpdatedUtc.AddTicks(1); + var bumpedUpdatedUtc = nowUtc > localUtcPlusOneTick ? nowUtc : localUtcPlusOneTick; var bumped = shipped with { UpdatedUtc = bumpedUpdatedUtc, Revision = local.Revision + 1 }; await _store.ApplyReplicatedAsync(bumped, ct).ConfigureAwait(false); @@ -374,9 +403,10 @@ public sealed class DeployedSecretsMirror : BackgroundService : ApplyOutcome.Unconfirmed; } - // local is live but its ciphertext differs from what was shipped: an out-of-band value - // with a strictly newer (UpdatedUtc, Revision) won LWW. (local is null only in a - // pathological concurrent-delete race; Unconfirmed covers that too.) + // local is live, NOT ours (never ledgered), and its ciphertext differs from what was + // shipped: a genuine out-of-band value with a strictly newer (UpdatedUtc, Revision) won + // LWW legitimately. (local is null only in a pathological concurrent-delete race; + // Unconfirmed covers that too.) return local is null ? ApplyOutcome.Unconfirmed : ApplyOutcome.LocalNewer; } diff --git a/tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Secrets/DeployedSecretsMirrorRealStackTests.cs b/tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Secrets/DeployedSecretsMirrorRealStackTests.cs new file mode 100644 index 00000000..05ac47e0 --- /dev/null +++ b/tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Secrets/DeployedSecretsMirrorRealStackTests.cs @@ -0,0 +1,159 @@ +using Microsoft.Extensions.Logging.Abstractions; +using Microsoft.Extensions.Options; +using ZB.MOM.WW.Audit; +using ZB.MOM.WW.ScadaBridge.Commons.Messages.Artifacts; +using ZB.MOM.WW.ScadaBridge.SiteRuntime.Persistence; +using ZB.MOM.WW.ScadaBridge.SiteRuntime.Secrets; +using ZB.MOM.WW.ScadaBridge.TestSupport; +using ZB.MOM.WW.Secrets; +using ZB.MOM.WW.Secrets.Abstractions; +using ZB.MOM.WW.Secrets.Crypto; +using ZB.MOM.WW.Secrets.Sqlite; + +namespace ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests.Secrets; + +/// +/// Task 9 fix round 2 (review task-9-rereview-1.md recommendation #2): runs +/// against the REAL ZB.MOM.WW.Secrets stack — +/// + + +/// over a real temp-file SQLite database — instead of the +/// in-memory fakes uses. +/// +/// +/// 's fakes never actually decrypt anything, so the +/// class remarks' claim that a tombstone-recovery re-stamp (bumping UpdatedUtc/ +/// Revision without touching ciphertext) cannot break decryption was previously proven +/// only by reading AesGcmEnvelopeCipher's source (its AAD binds only +/// and , never +/// Revision/UpdatedUtc). This test pins that property against the real cipher, so +/// a future library version that starts binding either field into the AEAD associated data +/// would fail here instead of only being caught by re-reading the source. +/// +public sealed class DeployedSecretsMirrorRealStackTests : IAsyncLifetime, IDisposable +{ + private readonly string _secretsDbPath = + Path.Combine(Path.GetTempPath(), $"zb-secrets-real-{Guid.NewGuid():N}.db"); + private readonly TestLocalDb _localDb = TestLocalDb.CreateTemp("deployed-secrets-mirror-real"); + + private SqliteSecretStore _store = null!; + private AesGcmEnvelopeCipher _cipher = null!; + private DefaultSecretResolver _resolver = null!; + private SiteStorageService _storage = null!; + + public async Task InitializeAsync() + { + var connectionFactory = new SecretsSqliteConnectionFactory(_secretsDbPath); + await new SqliteSecretsStoreMigrator(connectionFactory).MigrateAsync(CancellationToken.None); + _store = new SqliteSecretStore(connectionFactory); + + var masterKey = new FakeMasterKeyProvider("kek-central"); + _cipher = new AesGcmEnvelopeCipher(masterKey); + // DefaultSecretResolver implements BOTH ISecretResolver and ISecretCacheInvalidator (the + // real production shape — see SecretsServiceCollectionExtensions, which registers the + // same singleton instance behind both interfaces), so this test also exercises real + // cache invalidation, not a fake one. + _resolver = new DefaultSecretResolver(_store, _cipher, NoOpAuditWriter.Instance, TimeSpan.FromSeconds(30)); + + _storage = new SiteStorageService(_localDb.Db, NullLogger.Instance); + await _storage.InitializeAsync(); + } + + public Task DisposeAsync() => Task.CompletedTask; + + public void Dispose() + { + var path = _localDb.Path; + _localDb.Dispose(); + TestLocalDb.DeleteFiles(path); + foreach (var suffix in new[] { "", "-wal", "-shm" }) + { + try { File.Delete(_secretsDbPath + suffix); } catch { /* best effort */ } + } + } + + [Fact] + public async Task RealStack_EncryptApplyDeleteReAdd_TheRecoveredRowResolvesToTheOriginalPlaintext() + { + const string plaintext = "s3cr3t-value"; + + // "Central" encrypts the secret under the fleet's shared KEK (the same KekId this + // node's own master key provider reports, mirroring a real shared-KEK fleet) and ships + // it as a SecretArtifact — the exact wire shape DeployedSecretsCollector produces. + var sealedSecret = _cipher.Encrypt(new SecretName("a"), plaintext, SecretContentType.Text); + // Deliberately far in the past so DeleteAsync's real-wall-clock tombstone stamp is + // guaranteed strictly newer, regardless of when this test runs — the point being + // tested is the RECOVERY, not a race against the tombstone. + var shippedUtc = new DateTimeOffset(2000, 1, 1, 0, 0, 0, TimeSpan.Zero); + var artifact = new SecretArtifact( + Name: "a", + ContentType: SecretContentType.Text.ToString(), + Ciphertext: sealedSecret.Ciphertext, + Nonce: sealedSecret.Nonce, + Tag: sealedSecret.Tag, + WrappedDek: sealedSecret.WrappedDek, + WrapNonce: sealedSecret.WrapNonce, + WrapTag: sealedSecret.WrapTag, + KekId: sealedSecret.KekId, + Revision: 1, + CreatedUtc: shippedUtc, + UpdatedUtc: shippedUtc); + + var mirror = new DeployedSecretsMirror( + _storage, + _store, + _resolver, + _resolver, // the same singleton also serves ISecretCacheInvalidator + Options.Create(new SiteRuntimeOptions()), + NullLogger.Instance, + TimeProvider.System); + + // Apply — a fresh name, so this is a plain (non-recovery) apply. + await _storage.UpsertDeployedSecretsAsync([artifact], "d1"); + var applyPass = await mirror.ReconcileAsync(CancellationToken.None); + Assert.Equal(["a"], applyPass.Applied); + Assert.Empty(applyPass.Failed); + Assert.Equal(plaintext, await _resolver.GetAsync(new SecretName("a"), CancellationToken.None)); + + // Delete — the site stops referencing it; the REAL store tombstones (does not + // hard-delete) at DateTimeOffset.UtcNow, which is far newer than the 2000-01-01 shipped + // stamp above. + await _storage.UpsertDeployedSecretsAsync([], "d2"); + var removePass = await mirror.ReconcileAsync(CancellationToken.None); + Assert.Equal(["a"], removePass.Removed); + Assert.Null(await _resolver.GetAsync(new SecretName("a"), CancellationToken.None)); + + // Re-add the SAME (unchanged, still 2000-01-01-stamped) artifact. A naive + // ApplyReplicatedAsync call is a no-op against the newer tombstone — this forces the + // mirror's tombstone-recovery re-stamp path against the REAL store and REAL cipher. + await _storage.UpsertDeployedSecretsAsync([artifact], "d3"); + var reAddPass = await mirror.ReconcileAsync(CancellationToken.None); + Assert.Equal(["a"], reAddPass.Applied); + Assert.Empty(reAddPass.Failed); + + // The recovered row must decrypt to the ORIGINAL plaintext through the real + // AesGcmEnvelopeCipher, proving the bumped UpdatedUtc/Revision are not — and cannot + // silently become — bound into the AEAD associated data. + var resolved = await _resolver.GetAsync(new SecretName("a"), CancellationToken.None); + Assert.Equal(plaintext, resolved); + + var recovered = await _store.GetAsync(new SecretName("a"), CancellationToken.None); + Assert.NotNull(recovered); + Assert.False(recovered!.IsDeleted); + Assert.True(recovered.UpdatedUtc > shippedUtc); // genuinely re-stamped, not a no-op + } + + /// + /// A hand-written test double, mirroring the library's own + /// (test-internal, not shipped in the package) FakeMasterKeyProvider. + /// + private sealed class FakeMasterKeyProvider : IMasterKeyProvider + { + private readonly byte[] _key = System.Security.Cryptography.RandomNumberGenerator.GetBytes(32); + + public FakeMasterKeyProvider(string kekId) => KekId = kekId; + + public string KekId { get; } + + public ReadOnlyMemory GetMasterKey() => _key; + } +} 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 55692d89..3486eac8 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Secrets/DeployedSecretsMirrorTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Secrets/DeployedSecretsMirrorTests.cs @@ -141,6 +141,51 @@ public class DeployedSecretsMirrorTests : IDisposable Assert.Equal(1, mirrored.Single(m => m.Name == "a").Revision); } + [Fact] + public async Task Reconcile_ClockSkewedRecoveredRow_StillConvergesOnNextCentralRevision() + { + // Fix round 2 (review "clock skew" finding): a tombstone-recovery bump can leave a local + // row stamped AHEAD of a central UpdatedUtc that arrives afterward (site clock skew, or + // simply a central edit made before the recovery but shipped after it). The row is still + // OWNED by this mirror's ledger, so a later genuinely-new central revision must not be + // misreported as an out-of-band "newer local value" — it must converge. + var time = new FakeTimeProvider(new DateTimeOffset(2026, 9, 28, 6, 0, 0, TimeSpan.Zero)); + var (mirror, storage, store, _, invalidator) = await CreateAsync(time: time); + + await storage.UpsertDeployedSecretsAsync([Art("a", 1)], "d1"); + await mirror.ReconcileAsync(CancellationToken.None); + + // Remove then re-add — but the SITE CLOCK IS SKEWED FAR INTO THE FUTURE while the + // recovery bump happens (a bad RTC/NTP fault), so the recovered row's UpdatedUtc lands + // far ahead of anything central will ship for a long while. + store.Now = new DateTimeOffset(2026, 9, 28, 12, 0, 0, TimeSpan.Zero); + await storage.UpsertDeployedSecretsAsync([], "d2"); + await mirror.ReconcileAsync(CancellationToken.None); // tombstones "a" + + time.Now = new DateTimeOffset(2099, 1, 1, 0, 0, 0, TimeSpan.Zero); // skewed site clock + await storage.UpsertDeployedSecretsAsync([Art("a", 1)], "d3"); + var recoveredPass = await mirror.ReconcileAsync(CancellationToken.None); + Assert.Equal(["a"], recoveredPass.Applied); + Assert.True(store.Rows["a"].UpdatedUtc.Year >= 2099); // confirms the skewed stamp landed + + // Site clock recovers to normal. Central ships a genuinely NEW revision (different + // ciphertext, i.e. an actual rotation) with an ordinary timestamp — OLDER than the + // skewed local stamp, so a naive ApplyReplicatedAsync would no-op against it. + time.Now = new DateTimeOffset(2026, 9, 29, 0, 0, 0, TimeSpan.Zero); + var rotatedCiphertext = new byte[] { 7, 7, 7 }; + await storage.UpsertDeployedSecretsAsync([Art("a", 2, ciphertext: rotatedCiphertext)], "d4"); + + var convergePass = await mirror.ReconcileAsync(CancellationToken.None); + + Assert.Equal(["a"], convergePass.Applied); + Assert.Empty(convergePass.Failed); // NOT "newer local value" — this mirror owns the name + Assert.Equal(rotatedCiphertext, store.Rows["a"].Ciphertext); + Assert.Contains("a", invalidator.Invalidated); + + var mirrored = await storage.GetMirroredSecretsAsync(); + Assert.Equal(2, mirrored.Single(m => m.Name == "a").Revision); + } + [Fact] public async Task Reconcile_ReAppliesOnlyWhenRevisionChanges() { @@ -523,10 +568,10 @@ public class DeployedSecretsMirrorTests : IDisposable } private static SecretArtifact Art( - string name, long revision, string kek = "kek-1", string contentType = "Text") => new( + string name, long revision, string kek = "kek-1", string contentType = "Text", byte[]? ciphertext = null) => new( Name: name, ContentType: contentType, - Ciphertext: [1, 2, 3, 4, 5], + Ciphertext: ciphertext ?? [1, 2, 3, 4, 5], Nonce: [10, 11, 12], Tag: [20, 21, 22, 23], WrappedDek: [30, 31, 32, 33, 34, 35], diff --git a/tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests.csproj b/tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests.csproj index 581a977d..34c08189 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests.csproj +++ b/tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests.csproj @@ -15,6 +15,15 @@ + +