Public Access
Sync SSH keys as a vault item type, over a shared write path
The private key now lives in the vault as ciphertext, syncs between a user's machines, and is stored on the server so it can later be shared — sharing itself needs M3's signed grants; this is the storage that makes it possible. More was already reserved than expected: SyncEntityType.SshKey, CryptoSpec.AadResourceType.SshKey, ChangeEntityType.SshKey, SyncPlaintextFields.PublicKeyFingerprint, and SshPrivateKeyCredential wired through PrivateKeyFile over a MemoryStream so a key never touches disk. The frozen contract and crypto spec needed no change at all. What was missing was the server. Rather than copy the push path per item type — version check, change-log append, exactly-once receipt, advisory lock — it is now written once over IVaultItem, with everything type-specific behind IItemKind: which table, which plaintext columns, and what those columns must satisfy. Ten copies of that logic by M5, with a fix applied to nine, is the outcome this avoids. The refactor landed first with no behaviour change, so all 66 existing Host tests were the regression net, and they stayed green. An interface rather than a base class, deliberately: EF Core maps an inheritance hierarchy when it can see one, so a mapped base would quietly become a table-per-hierarchy discriminator across item types — the very arrangement per-type tables exist to avoid. ssh_key mirrors host and pointedly has no relay trio. That is the argument for separate tables rather than one wide item table: the columns a host needs are columns a key must never have, and a shared table could only make them nullable and trust the code. A key carrying a relay target is refused with a reason rather than silently dropped. A key hydrates PlaintextFields as null, not an empty instance — the difference is visible on the wire, because an all-defaults instance still serialises "relayEnabled": false and invites a reader to believe the setting exists and is off. It has none. Two things now defended by tests rather than by comments. Each kind states its own ChangeEntityType instead of casting: the two enums agree numerically but do not even share member names (Host against SshHost), and filing key changes under the host type is silent sync corruption — sabotaging it fails three tests. And EntityTypeAlignmentTests asserts the two enums stay aligned in both directions and in count, which nothing did before. The client half is next: SshKeySecret, its codec and merge, the cipher, a repository, and the UI. Note for that work — SyncEntityType.SshKey is 3 while AadResourceType.SshKey is 6, so a cast between them would seal key ciphertext as a vault and nothing would fail.
This commit is contained in:
@@ -0,0 +1,78 @@
|
||||
using DodoSSH.Contracts;
|
||||
using DodoSSH.Domain;
|
||||
|
||||
namespace DodoSSH.Api.Tests;
|
||||
|
||||
/// <summary>
|
||||
/// The two entity-type enums have to agree, and nothing but this makes them.
|
||||
/// </summary>
|
||||
/// <remarks>
|
||||
/// <para>
|
||||
/// <c>SyncService</c> converts between the wire's <see cref="SyncEntityType"/> and the change log's
|
||||
/// <see cref="ChangeEntityType"/> with a raw <c>(ChangeEntityType)(int)</c> cast in both directions. That
|
||||
/// works only because two independently maintained enums in two assemblies happen to number their members
|
||||
/// identically — and they do not even name them identically: <c>Host = 1</c> against <c>SshHost = 1</c>.
|
||||
/// </para>
|
||||
/// <para>
|
||||
/// If they ever drift, nothing fails loudly. Changes get filed in the log under a different item type's
|
||||
/// name, and the next delta pull loads rows of the wrong type — or none — for entities the client asked
|
||||
/// about. That is silent data loss on the sync path, which is precisely the failure this project has
|
||||
/// designed everything else to avoid, so the alignment gets a test rather than a comment.
|
||||
/// </para>
|
||||
/// <para>
|
||||
/// Numeric alignment only. The two lists are deliberately <em>not</em> aligned with
|
||||
/// <c>CryptoSpec.AadResourceType</c>, which also carries None/User/Device/Vault and therefore numbers the
|
||||
/// same item types differently — asserting three-way equality would be asserting something false.
|
||||
/// </para>
|
||||
/// </remarks>
|
||||
public sealed class EntityTypeAlignmentTests
|
||||
{
|
||||
[Fact]
|
||||
public void EveryWireEntityType_HasAChangeLogTypeWithTheSameValue()
|
||||
{
|
||||
var changeValues = Enum.GetValues<ChangeEntityType>().Select(value => (int)value).ToHashSet();
|
||||
|
||||
foreach (var wire in Enum.GetValues<SyncEntityType>())
|
||||
{
|
||||
changeValues.ShouldContain(
|
||||
(int)wire,
|
||||
$"SyncEntityType.{wire} = {(int)wire} has no ChangeEntityType with that value, so "
|
||||
+ "SyncService's cast would produce an undefined enum value.");
|
||||
}
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public void EveryChangeLogType_HasAWireTypeWithTheSameValue()
|
||||
{
|
||||
// The other direction matters just as much: a pull converts stored changes back to wire types, so a
|
||||
// change-log type with no wire counterpart would be served to clients as an undefined enum.
|
||||
var wireValues = Enum.GetValues<SyncEntityType>().Select(value => (int)value).ToHashSet();
|
||||
|
||||
foreach (var change in Enum.GetValues<ChangeEntityType>())
|
||||
{
|
||||
wireValues.ShouldContain(
|
||||
(int)change,
|
||||
$"ChangeEntityType.{change} = {(int)change} has no SyncEntityType with that value.");
|
||||
}
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public void TheTwoEnums_HaveTheSameNumberOfMembers()
|
||||
{
|
||||
// Catches a member added to one list only, which the two checks above would miss if it reused a
|
||||
// value already present in the other.
|
||||
Enum.GetValues<SyncEntityType>().Length.ShouldBe(Enum.GetValues<ChangeEntityType>().Length);
|
||||
}
|
||||
|
||||
/// <remarks>
|
||||
/// Spot-checked by name as well, because the pairing that is easiest to get wrong is the one whose names
|
||||
/// differ. Someone adding an item type may reasonably assume the lists are name-matched, notice
|
||||
/// <c>Host</c> has no <c>Host</c> on the other side, and renumber to "fix" it.
|
||||
/// </remarks>
|
||||
[Fact]
|
||||
public void TheDifferentlyNamedPair_IsTheOneThatMatches()
|
||||
{
|
||||
((int)SyncEntityType.Host).ShouldBe((int)ChangeEntityType.SshHost);
|
||||
((int)SyncEntityType.SshKey).ShouldBe((int)ChangeEntityType.SshKey);
|
||||
}
|
||||
}
|
||||
@@ -608,6 +608,145 @@ public sealed class SyncEndpointTests(ApiFixture fixture)
|
||||
// Just-in-time provisioning is covered by IdentityEndpointTests, against /me — the endpoint a
|
||||
// client actually calls first, and the only one reachable before enrollment.
|
||||
|
||||
// ---- SSH keys ----
|
||||
|
||||
[Fact]
|
||||
public async Task AnSshKey_RoundTripsWithNoPlaintextFields()
|
||||
{
|
||||
var (subject, vaultId) = await SeedUserWithVaultAsync();
|
||||
var client = fixture.CreateClientFor(subject);
|
||||
|
||||
var keyId = Guid.CreateVersion7();
|
||||
|
||||
var pushed = await client.PostContractAsync(
|
||||
PushUrl(vaultId),
|
||||
new SyncPushRequest([KeyOperation(keyId, expectedVersion: null, envelope: [9, 8, 7])]));
|
||||
|
||||
var results = await pushed.Content.ReadContractAsync<SyncPushResponse>();
|
||||
results.Results.ShouldHaveSingleItem().Status.ShouldBe(SyncOperationStatus.Applied);
|
||||
|
||||
var pulled = await client.PostContractAsync(
|
||||
PullUrl(vaultId),
|
||||
new SyncPullRequest(null, null, [SyncEntityType.SshKey]));
|
||||
|
||||
var page = await pulled.Content.ReadContractAsync<SyncPullResponse>();
|
||||
var change = page.Changes.ShouldHaveSingleItem();
|
||||
|
||||
change.EntityType.ShouldBe(SyncEntityType.SshKey);
|
||||
change.EntityId.ShouldBe(keyId);
|
||||
change.Payload.ShouldNotBeNull().Envelope.ShouldBe(new byte[] { 9, 8, 7 });
|
||||
|
||||
// The point of the type. A key has no relay, so it has no plaintext columns at all — and null
|
||||
// rather than an all-defaults instance, which would still put "relayEnabled": false on the wire and
|
||||
// invite a reader to think the setting exists and is off.
|
||||
change.PlaintextFields.ShouldBeNull();
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public async Task AnSshKeyCarryingARelayTarget_IsRefused()
|
||||
{
|
||||
var (subject, vaultId) = await SeedUserWithVaultAsync();
|
||||
var client = fixture.CreateClientFor(subject);
|
||||
|
||||
var operation = KeyOperation(Guid.CreateVersion7(), null, [1])
|
||||
with
|
||||
{ PlaintextFields = new SyncPlaintextFields(RelayEnabled: true, Hostname: "db.internal", Port: 22) };
|
||||
|
||||
var pushed = await client.PostContractAsync(PushUrl(vaultId), new SyncPushRequest([operation]));
|
||||
|
||||
var result = (await pushed.Content.ReadContractAsync<SyncPushResponse>()).Results.ShouldHaveSingleItem();
|
||||
|
||||
result.Status.ShouldBe(SyncOperationStatus.Invalid);
|
||||
result.Detail.ShouldContain("no relay target");
|
||||
}
|
||||
|
||||
/// <remarks>
|
||||
/// The path most likely to break: a page of changes mixing item types has to load each type's rows
|
||||
/// separately and then put them back in the log's order, because a client's cursor cannot resume from a
|
||||
/// sequence that was regrouped.
|
||||
/// </remarks>
|
||||
[Fact]
|
||||
public async Task APullMixingHostsAndKeys_ReturnsBothInLogOrder()
|
||||
{
|
||||
var (subject, vaultId) = await SeedUserWithVaultAsync();
|
||||
var client = fixture.CreateClientFor(subject);
|
||||
|
||||
var hostId = Guid.CreateVersion7();
|
||||
var keyId = Guid.CreateVersion7();
|
||||
|
||||
var pushed = await client.PostContractAsync(
|
||||
PushUrl(vaultId),
|
||||
new SyncPushRequest(
|
||||
[
|
||||
NewOperation(hostId, expectedVersion: null, envelope: [1, 1]),
|
||||
KeyOperation(keyId, expectedVersion: null, envelope: [2, 2]),
|
||||
]));
|
||||
|
||||
(await pushed.Content.ReadContractAsync<SyncPushResponse>()).Results
|
||||
.ShouldAllBe(result => result.Status == SyncOperationStatus.Applied);
|
||||
|
||||
var pulled = await client.PostContractAsync(
|
||||
PullUrl(vaultId),
|
||||
new SyncPullRequest(null, null, null));
|
||||
|
||||
var page = await pulled.Content.ReadContractAsync<SyncPullResponse>();
|
||||
|
||||
page.Changes.Count.ShouldBe(2);
|
||||
page.Changes.Select(change => change.ChangeSequence)
|
||||
.ShouldBeInOrder(Shouldly.SortDirection.Ascending);
|
||||
|
||||
var host = page.Changes.Single(change => change.EntityId == hostId);
|
||||
var key = page.Changes.Single(change => change.EntityId == keyId);
|
||||
|
||||
host.EntityType.ShouldBe(SyncEntityType.Host);
|
||||
host.Payload.ShouldNotBeNull().Envelope.ShouldBe(new byte[] { 1, 1 });
|
||||
host.PlaintextFields.ShouldNotBeNull();
|
||||
|
||||
key.EntityType.ShouldBe(SyncEntityType.SshKey);
|
||||
key.Payload.ShouldNotBeNull().Envelope.ShouldBe(new byte[] { 2, 2 });
|
||||
key.PlaintextFields.ShouldBeNull();
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public async Task DeletingAnSshKey_TombstonesItWithoutAPayload()
|
||||
{
|
||||
var (subject, vaultId) = await SeedUserWithVaultAsync();
|
||||
var client = fixture.CreateClientFor(subject);
|
||||
|
||||
var keyId = Guid.CreateVersion7();
|
||||
|
||||
await client.PostContractAsync(
|
||||
PushUrl(vaultId),
|
||||
new SyncPushRequest([KeyOperation(keyId, null, [5])]));
|
||||
|
||||
var deleted = await client.PostContractAsync(
|
||||
PushUrl(vaultId),
|
||||
new SyncPushRequest(
|
||||
[
|
||||
new SyncPushOperation(
|
||||
Guid.CreateVersion7(),
|
||||
SyncEntityType.SshKey,
|
||||
keyId,
|
||||
SyncOperation.Delete,
|
||||
ExpectedVersion: 1,
|
||||
Payload: null,
|
||||
PlaintextFields: null),
|
||||
]));
|
||||
|
||||
(await deleted.Content.ReadContractAsync<SyncPushResponse>()).Results
|
||||
.ShouldHaveSingleItem().Status.ShouldBe(SyncOperationStatus.Applied);
|
||||
|
||||
var pulled = await client.PostContractAsync(
|
||||
PullUrl(vaultId),
|
||||
new SyncPullRequest(null, null, [SyncEntityType.SshKey]));
|
||||
|
||||
var page = await pulled.Content.ReadContractAsync<SyncPullResponse>();
|
||||
var last = page.Changes[^1];
|
||||
|
||||
last.Operation.ShouldBe(SyncOperation.Delete);
|
||||
last.Payload.ShouldBeNull("a tombstone must not ship the key material it replaced");
|
||||
}
|
||||
|
||||
// ---- Helpers ----
|
||||
|
||||
private static string PullUrl(Guid vaultId) => $"/api/v1/vaults/{vaultId}/sync/pull";
|
||||
@@ -633,6 +772,20 @@ public sealed class SyncEndpointTests(ApiFixture fixture)
|
||||
Payload(envelope),
|
||||
new SyncPlaintextFields());
|
||||
|
||||
/// <remarks>
|
||||
/// <c>PlaintextFields: null</c> rather than an empty instance, which is what a real client sends for a
|
||||
/// type with no plaintext columns — and what the server must accept without inventing defaults.
|
||||
/// </remarks>
|
||||
private static SyncPushOperation KeyOperation(Guid entityId, int? expectedVersion, byte[] envelope) =>
|
||||
new(
|
||||
Guid.CreateVersion7(),
|
||||
SyncEntityType.SshKey,
|
||||
entityId,
|
||||
SyncOperation.Upsert,
|
||||
expectedVersion,
|
||||
Payload(envelope),
|
||||
PlaintextFields: null);
|
||||
|
||||
private static SyncPushRequest NewCreateBatch() =>
|
||||
new([NewOperation(Guid.CreateVersion7(), expectedVersion: null, envelope: [1, 2, 3, 4])]);
|
||||
|
||||
|
||||
Reference in New Issue
Block a user