Public Access
Take a rotated vault's contents onto the new key as well
Rotating a vault re-keyed the vault and not its contents, which was the deal struck last time: everything already stored stayed sealed under the generation it was written with, every remaining member kept the older keys, and the guarantee was narrowed to "nothing written from now on". That left one gap worth closing — somebody who walked off with the old key could still open old ciphertext they later got hold of — and the reason it was safe to defer is the reason it was cheap to add. A vault at mixed generations reads perfectly well, so the pass that moves items across can stop half way and be run again. VaultResealer walks the vault and rewrites each item as an ordinary upsert against the version the server holds. It never decodes the plaintext: an item is opened and the same bytes are sealed again under a fresh data key, so an item written by a newer client crosses a rotation untouched rather than being re-encoded through this build's codec and quietly losing the fields this build has no concept of. It also means nothing in the pass knows what an item is, which is why one loop covers every type including the ones added after it. A conflict is counted and skipped rather than merged — there is nothing to merge, since no content changes — and the next pass picks the item up at the version the other client left. The half that a pass over stored items cannot see is a change queued before the rotation and pushed after it, which would put a brand-new item into the vault under the key the person who just left still holds. So the push path re-seals a stale payload as it dispatches it, writing the revision back to the outbox first so that a retry sends the same bytes rather than a fresh envelope. Between the two, nothing reaches the server under a superseded generation at all. Queued items are therefore deliberately left alone by the pass: rewriting one there would overwrite the user's unpushed work with the version the server holds, which is the one thing a re-keying pass must never do. Removal runs it last, after a sync — a mirror that is behind produces a batch of conflicts instead of a re-sealed vault — and the status line distinguishes the two guarantees, because they are not the same: a vault fully re-sealed is closed to the person who left, and one with items outstanding is closed only to what happens next. Six tests, and three mutations run against them: making the re-seal return the payload unchanged fails five of the six, making the push path skip re-sealing fails the queued-edit test and only that one, and counting conflicts as applied fails the write-elsewhere test. One of the six was wrong before it was right — it modelled a third-party write by re-pushing an existing payload at a bumped version, which no real client would do, and it took reading the AAD to see that the test was lying rather than the code.
This commit is contained in:
@@ -169,6 +169,11 @@ public sealed class TeamSharingTests : IAsyncLifetime
|
||||
teams.Status.ShouldContain("Rotated", customMessage: teams.Status);
|
||||
teams.Status.ShouldContain("Platform secrets");
|
||||
|
||||
// The last act of a rotation is moving what is already stored onto the new key. Proven by the
|
||||
// bytes in DodoSSH.Client.Sync.Tests; what this asserts is that the shell asks for it at all,
|
||||
// and says which of the two guarantees the user has ended up with.
|
||||
teams.Status.ShouldContain("re-sealed under the new key", customMessage: teams.Status);
|
||||
|
||||
// Gone entirely, at every generation. A revocation that left the history behind would leave them
|
||||
// able to read everything written before they went, from a copy of the ciphertext.
|
||||
server.GenerationsGranted(vaultId, leaving).ShouldBeEmpty();
|
||||
|
||||
@@ -1,3 +1,4 @@
|
||||
using System.Security.Cryptography;
|
||||
using DodoSSH.Client.Domain;
|
||||
using DodoSSH.Client.Storage;
|
||||
using DodoSSH.Crypto;
|
||||
@@ -16,6 +17,9 @@ internal sealed class SyncDevice : IDisposable
|
||||
private readonly ClientCacheFactory factory;
|
||||
private readonly LocalCacheProtector protector;
|
||||
|
||||
private readonly FakeVaultServer server;
|
||||
private readonly SyncOptions options;
|
||||
|
||||
private SyncDevice(
|
||||
string name,
|
||||
ClientCacheFactory factory,
|
||||
@@ -27,6 +31,8 @@ internal sealed class SyncDevice : IDisposable
|
||||
Name = name;
|
||||
this.factory = factory;
|
||||
this.protector = protector;
|
||||
this.server = server;
|
||||
this.options = options;
|
||||
Keyring = keyring;
|
||||
|
||||
Items = new ItemStore(factory, protector);
|
||||
@@ -96,6 +102,15 @@ internal sealed class SyncDevice : IDisposable
|
||||
internal Task<SyncReport> SyncAsync() =>
|
||||
Engine.SyncAsync(SyncHarness.VaultId, TestContext.Current.CancellationToken);
|
||||
|
||||
/// <summary>Moves everything this machine can see onto the vault's current key.</summary>
|
||||
/// <remarks>
|
||||
/// Built per call rather than held, as the engine is: it carries no state between passes, and one
|
||||
/// per call is what the session does.
|
||||
/// </remarks>
|
||||
internal Task<ResealReport> ResealAsync() =>
|
||||
new VaultResealer(server, Items, Outbox, Keyring, TimeProvider.System, options)
|
||||
.ResealAsync(SyncHarness.VaultId, TestContext.Current.CancellationToken);
|
||||
|
||||
internal Task<ItemListing<HostSecret>> ListAsync() =>
|
||||
Hosts.ListAsync(SyncHarness.VaultId, TestContext.Current.CancellationToken);
|
||||
|
||||
@@ -274,6 +289,41 @@ internal sealed class SyncHarness : IDisposable
|
||||
}
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// Rotates the vault: a new key, taken by both machines, and a server that says so.
|
||||
/// </summary>
|
||||
/// <returns>The key the vault has just moved off, so a test can prove it no longer opens anything.</returns>
|
||||
/// <remarks>
|
||||
/// <para>
|
||||
/// Stands in for the server call the real rotation makes. What matters here is the state it leaves —
|
||||
/// a vault whose current generation is one past everything stored in it — and the grant round trip
|
||||
/// that produces that state is <c>DodoSSH.Api.Tests</c>'s subject, not this suite's.
|
||||
/// </para>
|
||||
/// <para>
|
||||
/// Each keyring gets its own copy of the bytes, because a keyring owns what it is handed and zeroes
|
||||
/// it on disposal; sharing one array would leave the second machine holding a zeroed key at the end
|
||||
/// of a test and produce failures that look like a decryption bug.
|
||||
/// </para>
|
||||
/// </remarks>
|
||||
internal byte[] Rotate()
|
||||
{
|
||||
var generation = Server.KeyGeneration + 1;
|
||||
|
||||
First.Keyring.TryGetAt(VaultId, Server.KeyGeneration, out var previous).ShouldBeTrue();
|
||||
|
||||
var superseded = previous.ToArray();
|
||||
var key = VaultKeys.Create();
|
||||
|
||||
First.Keyring.Adopt(VaultId, [.. key], generation);
|
||||
Second.Keyring.Adopt(VaultId, [.. key], generation);
|
||||
|
||||
CryptographicOperations.ZeroMemory(key);
|
||||
|
||||
Server.KeyGeneration = generation;
|
||||
|
||||
return superseded;
|
||||
}
|
||||
|
||||
/// <summary>Brings both devices up to date, twice, so the result is a settled state.</summary>
|
||||
/// <remarks>
|
||||
/// Twice because one pass per device is not enough for a change made on one to be merged on the
|
||||
|
||||
@@ -0,0 +1,224 @@
|
||||
namespace DodoSSH.Client.Sync.Tests;
|
||||
|
||||
/// <summary>
|
||||
/// Moving a rotated vault's stored items onto its current key.
|
||||
/// </summary>
|
||||
/// <remarks>
|
||||
/// <para>
|
||||
/// A rotation re-keys the vault and not its contents, which is what makes it cheap and safe (ADR 0010)
|
||||
/// and what leaves this pass to be run. The claim it has to earn is narrow and testable: after it, the
|
||||
/// key the vault has moved off opens nothing. Every test here that says "resealed" also checks that,
|
||||
/// because a pass that re-wrapped everything under the same key would report exactly the same numbers.
|
||||
/// </para>
|
||||
/// <para>
|
||||
/// The other half is the push path. A change queued before a rotation is sealed under the old key, and
|
||||
/// sending it as it stands would put a brand-new item into the vault under the key the person who was
|
||||
/// just removed still holds — the one hole a pass over stored items cannot see.
|
||||
/// </para>
|
||||
/// </remarks>
|
||||
public sealed class VaultResealTests
|
||||
{
|
||||
[Fact]
|
||||
public async Task ARotatedVault_MovesItsStoredItemsOntoTheNewKey()
|
||||
{
|
||||
using var harness = await SyncHarness.CreateAsync();
|
||||
|
||||
var web = await harness.First.CreateAsync(SyncHarness.Host("web-01"));
|
||||
var db = await harness.First.CreateAsync(SyncHarness.Host("db-01"));
|
||||
|
||||
await harness.SettleAsync();
|
||||
|
||||
var superseded = harness.Rotate();
|
||||
|
||||
var report = await harness.First.ResealAsync();
|
||||
|
||||
report.Resealed.ShouldBe(2);
|
||||
report.Complete.ShouldBeTrue();
|
||||
report.KeyGeneration.ShouldBe(2u);
|
||||
|
||||
foreach (var entityId in (Guid[])[web, db])
|
||||
{
|
||||
var row = harness.Server.Find(entityId).ShouldNotBeNull();
|
||||
|
||||
row.Payload.KeyGeneration.ShouldBe(2u);
|
||||
|
||||
// The point of the whole pass: the key somebody left with opens nothing here any more.
|
||||
HostCipher.TryOpen(row.Payload, superseded, entityId, row.Version).ShouldBeNull();
|
||||
}
|
||||
|
||||
// And the vault still reads as itself — the plaintext was carried across, not re-encoded.
|
||||
var hosts = await harness.First.HostsSortedAsync();
|
||||
|
||||
hosts.Select(host => host.Label).ShouldBe(["db-01", "web-01"]);
|
||||
}
|
||||
|
||||
/// <remarks>
|
||||
/// The pass is run after every rotation and can be run again at any time, so "nothing left to do"
|
||||
/// has to be cheap and silent rather than a second round of writes. A pass that re-sealed on every
|
||||
/// call would churn the vault's version numbers and hand every other client a pull per item.
|
||||
/// </remarks>
|
||||
[Fact]
|
||||
public async Task ASecondPass_FindsNothingLeftToDo()
|
||||
{
|
||||
using var harness = await SyncHarness.CreateAsync();
|
||||
|
||||
await harness.First.CreateAsync(SyncHarness.Host("web-01"));
|
||||
await harness.SettleAsync();
|
||||
|
||||
harness.Rotate();
|
||||
|
||||
(await harness.First.ResealAsync()).Resealed.ShouldBe(1);
|
||||
|
||||
var again = await harness.First.ResealAsync();
|
||||
|
||||
again.Resealed.ShouldBe(0);
|
||||
again.Complete.ShouldBeTrue();
|
||||
harness.Server.PushCount.ShouldBe(2, "an empty pass must not send a batch at all");
|
||||
}
|
||||
|
||||
/// <remarks>
|
||||
/// An item with an edit waiting to go is left alone by the pass and re-sealed by the push instead.
|
||||
/// Doing it here as well would overwrite the user's queued work with the version the server holds,
|
||||
/// which is the one thing a re-keying pass must never do.
|
||||
/// </remarks>
|
||||
[Fact]
|
||||
public async Task AQueuedEdit_IsLeftToThePushPathAndStillLandsUnderTheNewKey()
|
||||
{
|
||||
using var harness = await SyncHarness.CreateAsync();
|
||||
|
||||
var entityId = await harness.First.CreateAsync(SyncHarness.Host("web-01"));
|
||||
|
||||
await harness.SettleAsync();
|
||||
|
||||
// Queued while the old key was current, and not yet pushed.
|
||||
await harness.First.UpdateAsync(entityId, SyncHarness.Host("web-01", notes: "moved rack"));
|
||||
|
||||
var superseded = harness.Rotate();
|
||||
|
||||
var report = await harness.First.ResealAsync();
|
||||
|
||||
report.Deferred.ShouldBe(1);
|
||||
report.Resealed.ShouldBe(0);
|
||||
report.Complete.ShouldBeTrue("a queued change is not something this pass has left undone");
|
||||
|
||||
await harness.First.SyncAsync();
|
||||
|
||||
var row = harness.Server.Find(entityId).ShouldNotBeNull();
|
||||
|
||||
row.Payload.KeyGeneration.ShouldBe(2u);
|
||||
HostCipher.TryOpen(row.Payload, superseded, entityId, row.Version).ShouldBeNull();
|
||||
|
||||
// The edit itself survived the re-sealing, which is the half that would be easy to lose.
|
||||
var seen = await harness.Second.SyncAsync();
|
||||
|
||||
seen.Pulled.ShouldBeGreaterThan(0);
|
||||
(await harness.Second.FindAsync(entityId)).Secret.Notes.ShouldBe("moved rack");
|
||||
}
|
||||
|
||||
/// <remarks>
|
||||
/// Somebody else writing an item mid-pass is not a failure and not a merge — there is nothing to
|
||||
/// merge, since this pass changes no content. It is counted, left where it is, and picked up by the
|
||||
/// next pass against the version they left behind. That is the whole of the resumability claim.
|
||||
/// </remarks>
|
||||
[Fact]
|
||||
public async Task AnItemWrittenElsewhereMeanwhile_IsCountedAndPickedUpNextTime()
|
||||
{
|
||||
using var harness = await SyncHarness.CreateAsync();
|
||||
|
||||
var entityId = await harness.First.CreateAsync(SyncHarness.Host("web-01"));
|
||||
|
||||
await harness.SettleAsync();
|
||||
|
||||
var superseded = harness.Rotate();
|
||||
|
||||
// A third machine that has not heard about the rotation yet: it writes version 2 under the key
|
||||
// it still believes is current. That is the item this pass has to find and move, and sealing it
|
||||
// by hand is the only way to produce one — every client in this harness now holds the new key.
|
||||
var held = harness.Server.Find(entityId).ShouldNotBeNull();
|
||||
|
||||
harness.Server.ExternalUpsert(
|
||||
entityId,
|
||||
HostCipher.Seal(
|
||||
SyncHarness.Host("web-01", notes: "renamed elsewhere"),
|
||||
superseded,
|
||||
entityId,
|
||||
keyGeneration: 1,
|
||||
itemVersion: held.Version + 1),
|
||||
held.Fields);
|
||||
|
||||
var contested = await harness.First.ResealAsync();
|
||||
|
||||
contested.Contested.ShouldBe(1);
|
||||
contested.Resealed.ShouldBe(0);
|
||||
contested.Complete.ShouldBeFalse();
|
||||
|
||||
// Read what they wrote, then run the pass again: nothing to recover, nothing to decide.
|
||||
await harness.First.SyncAsync();
|
||||
|
||||
var second = await harness.First.ResealAsync();
|
||||
|
||||
second.Resealed.ShouldBe(1);
|
||||
second.Complete.ShouldBeTrue();
|
||||
|
||||
var row = harness.Server.Find(entityId).ShouldNotBeNull();
|
||||
|
||||
row.Payload.KeyGeneration.ShouldBe(2u);
|
||||
HostCipher.TryOpen(row.Payload, superseded, entityId, row.Version).ShouldBeNull();
|
||||
}
|
||||
|
||||
/// <remarks>
|
||||
/// Every synced type, not the one the tests happen to use most. The pass is written over the item
|
||||
/// store rather than over the repositories precisely so that a type added later is covered without
|
||||
/// anybody remembering to add it — and this is the test that would notice if it stopped being true.
|
||||
/// </remarks>
|
||||
[Fact]
|
||||
public async Task EveryKindOfItem_MovesOntoTheNewKey()
|
||||
{
|
||||
using var harness = await SyncHarness.CreateAsync();
|
||||
|
||||
await harness.First.CreateAsync(SyncHarness.Host("web-01"));
|
||||
await harness.First.CreateKeyAsync(SyncHarness.Key("deploy"));
|
||||
await harness.First.CreateCredentialAsync(SyncHarness.Credential("registry"));
|
||||
await harness.First.CreateKnownHostAsync(SyncHarness.KnownHost("db.internal"));
|
||||
|
||||
await harness.SettleAsync();
|
||||
|
||||
harness.Rotate();
|
||||
|
||||
var report = await harness.First.ResealAsync();
|
||||
|
||||
report.Resealed.ShouldBe(4);
|
||||
report.Complete.ShouldBeTrue();
|
||||
|
||||
// Read back through the repositories, so this asserts the items are usable and not merely
|
||||
// rewritten: a pass that produced ciphertext nobody could open would pass every count above.
|
||||
(await harness.First.ListAsync()).Unreadable.ShouldBe(0);
|
||||
(await harness.First.ListKeysAsync()).Unreadable.ShouldBe(0);
|
||||
(await harness.First.ListCredentialsAsync()).Unreadable.ShouldBe(0);
|
||||
(await harness.First.ListKnownHostsAsync()).Unreadable.ShouldBe(0);
|
||||
}
|
||||
|
||||
/// <remarks>
|
||||
/// A member who has been rotated past and not yet re-wrapped holds the history and no current key.
|
||||
/// They must not attempt this: there is nothing to seal under, and the honest answer is a report of
|
||||
/// zero rather than an exception on a background pass nobody asked for.
|
||||
/// </remarks>
|
||||
[Fact]
|
||||
public async Task AMachineWithNoCurrentKey_DoesNothingRatherThanFailing()
|
||||
{
|
||||
using var harness = await SyncHarness.CreateAsync();
|
||||
|
||||
await harness.First.CreateAsync(SyncHarness.Host("web-01"));
|
||||
await harness.SettleAsync();
|
||||
|
||||
// What RefreshVaultsAsync does when the server reports a generation this machine has no grant
|
||||
// for: the vault is marked unreadable and the write target goes with it.
|
||||
harness.First.Keyring.MarkUnreadable(SyncHarness.VaultId);
|
||||
|
||||
var report = await harness.First.ResealAsync();
|
||||
|
||||
report.KeyGeneration.ShouldBe(0u);
|
||||
report.Resealed.ShouldBe(0);
|
||||
harness.Server.PushCount.ShouldBe(1, "nothing was sent");
|
||||
}
|
||||
}
|
||||
Reference in New Issue
Block a user