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

fix(site): DeployedSecretsMirror — ledger-owned rows recover past clock skew; real-cipher recovery test

Browse Source 
Fix round 2 (review task-9-rereview-1.md), both Minors:

1. Clock skew. ApplyAndConfirmAsync now decides the tombstone-vs-out-of-band split by LEDGER
   OWNERSHIP (any mirrored_secrets entry for the name, at any revision), not merely by whether
   the local row happens to be a tombstone. A live local row this node already owns but whose
   ciphertext no longer matches the newly shipped revision (e.g. an earlier tombstone-recovery
   re-stamp landed ahead of a since-shipped central UpdatedUtc, under site/central clock skew)
   is now re-stamped and re-applied exactly like a tombstone, instead of being misreported as
   someone else's "newer local value" and left permanently un-converged. The out-of-band Failed
   message is now only ever produced for a name that is NOT in the ledger. New test:
   Reconcile_ClockSkewedRecoveredRow_StillConvergesOnNextCentralRevision (a skewed future
   re-stamp followed by a genuinely new central revision converges and re-invalidates the
   cache).
2. Real-cipher integration test. Added DeployedSecretsMirrorRealStackTests, running the mirror
   against the REAL ZB.MOM.WW.Secrets stack (SqliteSecretStore + AesGcmEnvelopeCipher +
   DefaultSecretResolver over a real temp-file SQLite database, plus a hand-written
   IMasterKeyProvider test double mirroring the library's own) instead of in-memory fakes:
   encrypt -> SecretArtifact -> apply -> delete (tombstone) -> re-add the same artifact -> the
   recovered row resolves to the original plaintext. Pins the "UpdatedUtc/Revision are not
   AEAD-bound" property against the real cipher rather than by source inspection alone.
   Test-only package reference: ZB.MOM.WW.Secrets (SiteRuntime itself still depends only on
   ZB.MOM.WW.Secrets.Abstractions).

Verify: dotnet test tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests --filter FullyQualifiedName~DeployedSecretsMirrorTests (25/25); full SiteRuntime.Tests suite 878/878.
This commit is contained in:
Joseph Doherty
2026-09-28 19:19:22 -04:00
parent 4d7d6456e3
commit 838fb4177b
4 changed files with 280 additions and 37 deletions
Download Patch File Download Diff File Expand all files Collapse all files
src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Secrets/DeployedSecretsMirror.cs
+65 -35
View File
@@ -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 <see cref="ISecretStore.GetAsync"/> read-back, and
/// <c>mirrored_secrets</c> is ledgered only when the local row is confirmed LIVE and its
/// ciphertext (<c>Ciphertext</c> + <c>WrappedDek</c>) matches what was just shipped. Two
/// non-ledgered outcomes are possible, both reported as a <see cref="MirrorResult.Failed"/>
/// entry (never as the decrypt-failure message, which would misdiagnose either case) and never
/// overwritten a second way:
/// ciphertext (<c>Ciphertext</c> + <c>WrappedDek</c>) matches what was just shipped. A
/// non-ledgered outcome is reported as a <see cref="MirrorResult.Failed"/> 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 <b>ledger ownership</b> — whether
/// <c>mirrored_secrets</c> already has ANY entry for the name, at any revision — not merely by
/// whether the local row happens to be a tombstone:
/// <list type="bullet">
/// <item>
/// <b>The local row is a tombstone</b> (this node previously deleted it — e.g. the site
/// stopped referencing it, then referenced it again unchanged at central, so the shipped row's
/// <c>UpdatedUtc</c> is older than the tombstone's). Re-stamping is safe here: neither
/// <c>UpdatedUtc</c> nor <c>Revision</c> is bound into the AEAD associated data — confirmed
/// against <c>ZB.MOM.WW.Secrets.Crypto.AesGcmEnvelopeCipher</c>, whose body layer binds only
/// <b>This node's own ledger already owns the name</b> (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 <c>UpdatedUtc</c> — fix round 2, review "clock skew" finding).
/// Re-stamping is safe either way: neither <c>UpdatedUtc</c> nor <c>Revision</c> is bound into
/// the AEAD associated data — confirmed against
/// <c>ZB.MOM.WW.Secrets.Crypto.AesGcmEnvelopeCipher</c>, whose body layer binds only
/// <c>SecretName.Value</c> and whose wrap layer binds only <c>KekId</c> — so the mirror
/// re-applies the exact shipped ciphertext with <c>UpdatedUtc = max(now, tombstone.UpdatedUtc +
/// 1 tick)</c> and <c>Revision = tombstone.Revision + 1</c>, 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 <c>UpdatedUtc = max(now, local.UpdatedUtc + 1
/// tick)</c> and <c>Revision = local.Revision + 1</c>, 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
/// (<c>DeployedSecretsMirrorRealStackTests</c>, fix round 2) pins that a bumped-and-recovered row
/// still decrypts to the original plaintext against the real <c>SqliteSecretStore</c> +
/// <c>AesGcmEnvelopeCipher</c> + <c>DefaultSecretResolver</c> stack, not only by source reading.
/// </item>
/// <item>
/// <b>The local row is LIVE with different ciphertext</b> — 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).
/// <b>The name is NOT in this node's ledger</b> 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).
/// </item>
/// </list>
/// 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.
/// </para>
/// <para>
/// <b>Per-row isolation (review Important #2).</b> 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 <see cref="ISecretStore.ApplyReplicatedAsync"/> and confirms the outcome by reading
/// the row back — see the class remarks for why this mirror never trusts the call blindly.
/// </summary>
private async Task<ApplyOutcome> ApplyAndConfirmAsync(DeployedSecretRow row, SecretName name, CancellationToken ct)
/// <param name="row">The shipped row to apply.</param>
/// <param name="name">The row's <see cref="SecretName"/>.</param>
/// <param name="ownedByMirror">
/// 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 <see langword="true"/>.
/// </param>
/// <param name="ct">A token to cancel the operation.</param>
private async Task<ApplyOutcome> 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;
}