DodoTech-Public/DodoSSH
Public Access
2
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases 2 Wiki Activity

Move the keys when a membership changes, not just the flag

Browse Source 
Adding somebody to a team granted them nothing readable and removing them
rotated nothing. Both were honest — the interface said so in as many words — and
both left the actual work to a button somebody had to remember to press, on a
machine that happened to hold the key. Adding now wraps every team vault this
machine can open to the new member, and removing revokes their grants and moves
each of those vaults to a fresh key that goes to whoever is left.

The rotation is where the design had to be decided rather than written. A vault
key is per generation and an item carries the generation it was sealed under, so
advancing the vault and withdrawing the old grants would make everything already
stored unreadable to everybody, including whoever pressed the button. So earlier
grants are kept: a member holds one per generation, /me serves them as
PriorKeyWraps, and VaultKeyring holds a key per generation — the newest for
writing, the item's own for reading, chosen per item on every read path. Sharing
issues one grant per generation held, because a recipient handed only the current
key would open the vault to find most of it undecryptable; revocation takes every
generation, because leaving the history behind leaves them able to read
everything written before the rotation.

The bump itself is one server transaction. POST /vaults/{id}/rekey must name
exactly current + 1 and the vault's xmin token makes that binding, so two admins
rotating at once do not both walk away believing they succeeded — the second is
refused and told to read the vault again. The server contributes the moment and
no cryptography: it cannot generate the key, cannot tell that the one it is
handed differs from the old one, and checks that the caller held the old one the
only way it can, by requiring a live grant at the current generation.

What this does not do is re-encrypt what is already stored, and the product says
so rather than the reassuring version: everything written from the rotation
onwards is unreadable to the person who left, and nothing about the past changes.
That half is deferred and is safe to add incrementally precisely because a vault
at mixed generations stays readable. ADR 0010 records the alternatives — revoking
the old grants, chaining each key under its successor, re-sealing every item in
one request against a server that caps a push at 500 operations — and why each
was rejected.

Two things fell out of the change rather than being asked for. The grant listing
would have shown a member once per generation, so it now returns one row per
holder carrying the best key they hold, which is what makes a row below the
vault's generation mean "still owed the new key". And MarkUnreadable gives up the
write target as well as reporting: a client whose vault was rotated elsewhere
would otherwise have gone on sealing items under its superseded key — readable to
its author, unreadable to everybody else, with nothing to show for it.
This commit is contained in:
jaap-jan
2026-08-03 23:05:40 +02:00
parent e82a25c912
commit d5b1a73182
35 changed files with 2838 additions and 173 deletions
Download Patch File Download Diff File Expand all files Collapse all files
tests/DodoSSH.Client.App.Tests/FakeVaultServer.Teams.cs
+136 -16
View File
@@ -26,7 +26,14 @@ internal sealed partial class FakeVaultServer : ITeamApi, IDirectoryApi, IVaultG
private readonly List<TeamSummary> teams = [];
private readonly Dictionary<Guid, List<TeamMemberSummary>> members = [];
private readonly Dictionary<Guid, VaultSummary> teamVaults = [];
private readonly Dictionary<(Guid VaultId, Guid UserId), IssueVaultGrantRequest> grants = [];
/// <remarks>
/// Keyed by generation as well as by recipient, because the real table is: a rotation leaves a
/// member holding one grant per generation, and a fake that kept one per person would quietly model
/// sharing the history as overwriting it — which is the bug this half of the feature exists to
/// avoid.
/// </remarks>
private readonly Dictionary<(Guid VaultId, Guid UserId, uint KeyGeneration), IssueVaultGrantRequest>
grants = [];
private readonly List<KeyLogRecord> keyLog = [];
private readonly List<DirectoryEntry> directory = [];
private readonly Dictionary<Guid, List<TeamInvitationSummary>> invitations = [];
@@ -51,8 +58,29 @@ internal sealed partial class FakeVaultServer : ITeamApi, IDirectoryApi, IVaultG
/// <inheritdoc />
public IVaultGrantApi Grants => this;
/// <summary>Grants this fake has been asked to record, for a test to assert on.</summary>
internal IReadOnlyDictionary<(Guid VaultId, Guid UserId), IssueVaultGrantRequest> IssuedGrants => grants;
/// <summary>
/// Grants this fake has been asked to record, newest generation per recipient.
/// </summary>
/// <remarks>
/// Flattened to one entry per recipient because that is the question most tests are asking — can
/// this person open the vault as it stands. <see cref="GenerationsGranted"/> is for the ones asking
/// whether they were also given its history.
/// </remarks>
internal IReadOnlyDictionary<(Guid VaultId, Guid UserId), IssueVaultGrantRequest> IssuedGrants =>
grants
.GroupBy(entry => (entry.Key.VaultId, entry.Key.UserId))
.ToDictionary(
group => group.Key,
group => group.OrderByDescending(entry => entry.Key.KeyGeneration).First().Value);
/// <summary>Which generations of one vault's key a recipient has been wrapped, oldest first.</summary>
internal IReadOnlyList<uint> GenerationsGranted(Guid vaultId, Guid userId) =>
[
.. grants.Keys
.Where(key => key.VaultId == vaultId && key.UserId == userId)
.Select(key => key.KeyGeneration)
.Order(),
];
/// <summary>
/// When true, the log served omits its last entry's link, so its chain no longer verifies.
@@ -451,11 +479,17 @@ internal sealed partial class FakeVaultServer : ITeamApi, IDirectoryApi, IVaultG
// Every grant they held from this team goes with them, as the real service revokes them in the
// same transaction. A fake that removed the membership and left the grants would let a test
// "prove" a revocation that had not happened.
foreach (var vaultId in teamVaults.Values
.Where(vault => vault.TeamId == teamId)
.Select(vault => vault.VaultId))
var theirs = grants.Keys
.Where(key => key.UserId == userId
&& teamVaults.TryGetValue(key.VaultId, out var vault)
&& vault.TeamId == teamId)
.ToList();
// Every generation, not only the newest. A revocation that left the history behind would let
// them go on reading everything written before the rotation that follows.
foreach (var key in theirs)
{
grants.Remove((vaultId, userId));
grants.Remove(key);
}
Recount(teamId);
@@ -491,6 +525,17 @@ internal sealed partial class FakeVaultServer : ITeamApi, IDirectoryApi, IVaultG
teamVaults[vault.VaultId] = vault;
// The creator's own grant, as the real create records it in the same transaction. Without it a
// rotation here would report no earlier wraps and the vault's first generation would vanish.
grants[(vault.VaultId, UserId, 1)] = new IssueVaultGrantRequest(
UserId,
RecipientKeyFingerprint: new byte[32],
KeyGeneration: 1,
request.WrappedVaultKey,
KeyLogHead: new byte[32],
request.GrantSignature,
request.GrantedAt);
Recount(teamId);
return Task.FromResult(vault);
@@ -539,16 +584,20 @@ internal sealed partial class FakeVaultServer : ITeamApi, IDirectoryApi, IVaultG
CancellationToken cancellationToken) =>
Task.FromResult(new VaultGrantsResponse(
vaultId,
KeyGeneration: 1,
KeyGeneration: Generation(vaultId),
RekeyRequired: false,
Grants:
[
.. grants.Where(entry => entry.Key.VaultId == vaultId).Select(entry =>
new VaultGrantSummary(
entry.Key.UserId,
directory.Find(candidate => candidate.UserId == entry.Key.UserId)?.Email,
// One row per holder rather than per grant, as the real listing shows a member once
// and lets the generation say whether their key is current.
.. grants
.Where(entry => entry.Key.VaultId == vaultId)
.GroupBy(entry => entry.Key.UserId)
.Select(group => new VaultGrantSummary(
group.Key,
directory.Find(candidate => candidate.UserId == group.Key)?.Email,
null,
KeyGeneration: 1,
KeyGeneration: group.Max(entry => entry.Key.KeyGeneration),
VaultGrantState.Active,
UserId,
DateTimeOffset.UnixEpoch,
@@ -561,17 +610,88 @@ internal sealed partial class FakeVaultServer : ITeamApi, IDirectoryApi, IVaultG
IssueVaultGrantRequest request,
CancellationToken cancellationToken)
{
grants[(vaultId, request.RecipientUserId)] = request;
grants[(vaultId, request.RecipientUserId, request.KeyGeneration)] = request;
return Task.CompletedTask;
}
/// <inheritdoc />
/// <remarks>
/// Models the one part of a rotation that is the server's: the generation advances, the caller's own
/// grant for it is recorded, and everything older is left standing so the vault's stored items go on
/// opening. What comes back is what the real endpoint returns — the vault at its new generation,
/// with the caller's earlier wraps attached.
/// </remarks>
public Task<VaultSummary> RekeyVaultAsync(
Guid vaultId,
RekeyVaultRequest request,
CancellationToken cancellationToken)
{
if (!teamVaults.TryGetValue(vaultId, out var vault))
{
throw new DodoSshApiException(
System.Net.HttpStatusCode.NotFound, code: null, "No such vault.");
}
if (request.KeyGeneration != vault.KeyGeneration + 1)
{
throw new DodoSshApiException(
System.Net.HttpStatusCode.BadRequest,
ProblemCodes.InvalidVaultGrant,
$"This vault is at key generation {vault.KeyGeneration}.");
}
grants[(vaultId, UserId, request.KeyGeneration)] = new IssueVaultGrantRequest(
UserId,
RecipientKeyFingerprint: new byte[32],
request.KeyGeneration,
request.WrappedVaultKey,
KeyLogHead: new byte[32],
request.GrantSignature,
request.GrantedAt);
var prior = grants
.Where(entry => entry.Key.VaultId == vaultId
&& entry.Key.UserId == UserId
&& entry.Key.KeyGeneration < request.KeyGeneration)
.OrderBy(entry => entry.Key.KeyGeneration)
.Select(entry => new VaultKeyWrap(entry.Key.KeyGeneration, entry.Value.WrappedVaultKey))
.ToList();
var rotated = vault with
{
KeyGeneration = request.KeyGeneration,
WrappedVaultKey = request.WrappedVaultKey,
RekeyRequired = false,
PriorKeyWraps = prior,
};
teamVaults[vaultId] = rotated;
return Task.FromResult(rotated);
}
/// <inheritdoc />
public Task<bool> RevokeVaultGrantAsync(
Guid vaultId,
Guid userId,
CancellationToken cancellationToken) =>
Task.FromResult(grants.Remove((vaultId, userId)));
CancellationToken cancellationToken)
{
var theirs = grants.Keys
.Where(key => key.VaultId == vaultId && key.UserId == userId)
.ToList();
foreach (var key in theirs)
{
grants.Remove(key);
}
return Task.FromResult(theirs.Count > 0);
}
/// <summary>The generation a vault currently stands at.</summary>
private uint Generation(Guid vaultId) =>
teamVaults.TryGetValue(vaultId, out var vault) ? vault.KeyGeneration : 1;
/// <summary>Publishes the enrolling account's own key, in the directory and the key log.</summary>
private void RegisterSelf(KeyStatement statement, byte[] statementSignature)
tests/DodoSSH.Client.App.Tests/TeamSharingTests.cs
+132 -10
View File
@@ -98,12 +98,12 @@ public sealed class TeamSharingTests : IAsyncLifetime
}
/// <remarks>
/// The whole point of a team, in one test. Note what the status line says after the add and before
/// the share: adding somebody grants them nothing readable, and the interface has to say so rather
/// than let a user believe the credential is already with their colleague.
/// The whole point of a team, in one test. Adding somebody wraps every team vault this machine can
/// open to them, so the status line names what they were given rather than what is still owed —
/// and the grant is on the server before the add has finished reporting.
/// </remarks>
[Fact]
public async Task CreatingATeamAndSharingItsVault_WrapsTheKeyToTheOtherMember()
public async Task AddingAMember_WrapsEveryTeamVaultThisMachineHoldsToThem()
{
await UnlockedAsync();
@@ -115,11 +115,121 @@ public sealed class TeamSharingTests : IAsyncLifetime
await CreateVaultAsync(teams, "Platform secrets");
teams.Vaults.Count.ShouldBe(1, teams.Status);
var vaultId = teams.Vaults[0].VaultId;
teams.InviteEmail = "bob@example.com";
await teams.AddMemberCommand.ExecuteAsync(null);
teams.Members.Count.ShouldBe(2, teams.Status);
teams.Status.ShouldContain("cannot read anything yet");
server.IssuedGrants.ShouldContainKey(
(vaultId, colleague),
"adding somebody to a team is what shares its vaults with them");
teams.Status.ShouldContain("Platform secrets");
}
/// <remarks>
/// <para>
/// The other half of the same idea. Removing somebody withdraws their grants — which only blocks
/// future reads — so the vault is rotated in the same breath and the new key goes to the people who
/// are left. From that moment nothing written is readable to the person who went.
/// </para>
/// <para>
/// The remaining member is given the earlier generation as well as the new one, which is what keeps
/// the vault's existing items readable to them: a rotation re-keys the vault, not its contents.
/// </para>
/// </remarks>
[Fact]
public async Task RemovingAMember_RotatesTheVaultAndHandsTheNewKeyToWhoIsLeft()
{
await UnlockedAsync();
var teams = shell.Teams;
var leaving = server.AddAccount("bob@example.com", "Bob Example");
var staying = server.AddAccount("carol@example.com", "Carol Example");
await CreateTeamAsync(teams, "Platform", "platform");
await CreateVaultAsync(teams, "Platform secrets");
var vaultId = teams.Vaults[0].VaultId;
foreach (var address in (string[])["bob@example.com", "carol@example.com"])
{
teams.InviteEmail = address;
await teams.AddMemberCommand.ExecuteAsync(null);
}
teams.Members.Count.ShouldBe(3, teams.Status);
teams.SelectedMember = teams.Members.Single(member => member.UserId == leaving);
await teams.RemoveMemberCommand.ExecuteAsync(null);
teams.Status.ShouldContain("Rotated", customMessage: teams.Status);
teams.Status.ShouldContain("Platform secrets");
// 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();
// And the member who stayed holds both: the new key for what comes next, the old one for what
// is already stored under it.
server.GenerationsGranted(vaultId, staying).ShouldBe([1u, 2u]);
}
/// <remarks>
/// Somebody added after a rotation is given every generation the sharing machine holds, not only the
/// newest. A vault shared as one key would open to a list of items that will not decrypt, which
/// reads as corruption rather than as the missing grant it is.
/// </remarks>
[Fact]
public async Task AddingAMemberToARotatedVault_HandsThemItsHistoryAsWell()
{
await UnlockedAsync();
var teams = shell.Teams;
var first = server.AddAccount("bob@example.com", "Bob Example");
var second = server.AddAccount("carol@example.com", "Carol Example");
await CreateTeamAsync(teams, "Platform", "platform");
await CreateVaultAsync(teams, "Platform secrets");
var vaultId = teams.Vaults[0].VaultId;
teams.InviteEmail = "bob@example.com";
await teams.AddMemberCommand.ExecuteAsync(null);
// Removing them is what rotates the vault, so the next person to be added arrives at a vault
// with a history rather than one that has only ever had a single key.
teams.SelectedMember = teams.Members.Single(member => member.UserId == first);
await teams.RemoveMemberCommand.ExecuteAsync(null);
teams.InviteEmail = "carol@example.com";
await teams.AddMemberCommand.ExecuteAsync(null);
server.GenerationsGranted(vaultId, second).ShouldBe([1u, 2u], teams.Status);
}
/// <remarks>
/// The manual path still works and is still worth having: a vault whose key this machine did not
/// hold when somebody was added is shared by pressing the button once it does. Re-wrapping to
/// somebody who already holds the key is the same call, and the server replaces the row rather than
/// adding a second one.
/// </remarks>
[Fact]
public async Task SharingAVaultByHand_WrapsTheKeyAndSaysWhatItCannotPromise()
{
await UnlockedAsync();
var teams = shell.Teams;
var colleague = server.AddAccount("bob@example.com", "Bob Example");
await CreateTeamAsync(teams, "Platform", "platform");
await CreateVaultAsync(teams, "Platform secrets");
teams.InviteEmail = "bob@example.com";
await teams.AddMemberCommand.ExecuteAsync(null);
teams.SelectedMember = teams.Members.Single(member => member.UserId == colleague);
teams.SelectedVault = teams.Vaults[0];
@@ -158,17 +268,25 @@ public sealed class TeamSharingTests : IAsyncLifetime
await CreateTeamAsync(teams, "Platform", "platform");
await CreateVaultAsync(teams, "Platform secrets");
// Before the add, because the add now shares. Both routes to a wrap have to refuse, and a test
// that corrupted the log afterwards would be asserting about the second one only.
server.CorruptKeyLog = true;
teams.InviteEmail = "mallory@example.com";
await teams.AddMemberCommand.ExecuteAsync(null);
var vaultId = teams.Vaults[0].VaultId;
server.IssuedGrants.ShouldNotContainKey((vaultId, colleague));
teams.Status.ShouldContain("Could not share");
teams.Status.ShouldContain("key log");
teams.SelectedMember = teams.Members.Single(member => member.UserId == colleague);
teams.SelectedVault = teams.Vaults[0];
server.CorruptKeyLog = true;
await teams.ShareVaultCommand.ExecuteAsync(null);
server.IssuedGrants.ShouldBeEmpty();
server.IssuedGrants.ShouldNotContainKey((vaultId, colleague));
teams.Status.ShouldContain("Did not share");
teams.Status.ShouldContain("key log");
}
@@ -303,9 +421,13 @@ public sealed class TeamSharingTests : IAsyncLifetime
teams.SelectedVault = null;
teams.SelectedVault = teams.Vaults[0];
var holder = teams.Grants.ShouldHaveSingleItem();
// Two, and the second one matters: the creator's own grant is recorded when the vault is made,
// so a list that showed only the people it was shared with would be describing a vault its
// owner cannot open.
teams.Grants.Count.ShouldBe(2, teams.Status);
var holder = teams.Grants.Single(row => row.UserId == colleague);
holder.UserId.ShouldBe(colleague);
holder.IsLive.ShouldBeTrue(teams.Status);
holder.State.ShouldBe("holds a key");
}