diff --git a/README.md b/README.md index fa09f1b..fd34f05 100644 --- a/README.md +++ b/README.md @@ -127,10 +127,15 @@ In the app, enter `http://localhost:5233` as the server. Your browser opens for skipped and cannot be recovered from the server. You can then add a host and open a shell on it. Keycloak's admin console is at `http://localhost:18080` (`admin` / `admin`). -Three of M1's known gaps are visible immediately, so they are worth expecting rather than diagnosing: a -connection asks for the host's password every time, because credentials are not a synced entity type yet; -host key trust lasts one session, because known hosts do not live in the vault yet; and unlock asks for the -passphrase on every launch, because no device key is registered. +You can also add an SSH key, which is stored in the vault like a host and synced the same way: paste the +private key, tick **Use key** next to Connect, and the selected key authenticates instead of a password. + +Three of M1's known gaps are visible immediately, so they are worth expecting rather than diagnosing: +password authentication asks for the password every time, because credentials are not a synced entity type +yet (keys are — passwords are not); a key is chosen per connection rather than remembered per host, because +binding one to a host needs a new field on the host payload and so a schema version bump; and unlock asks +for the passphrase on every launch, because no device key is registered. Host key trust also lasts one +session, because known hosts do not live in the vault yet. ### End-to-end verification @@ -197,9 +202,11 @@ off-Windows. [End-to-end verification](#end-to-end-verification). Known gaps in the client, stated rather than implied by the interface: credentials are not a synced - entity type yet, so a connection still asks for a password; known host keys live in memory for one - session instead of in the vault; and no device key is registered, so the passphrase is needed on every - launch until the OS keystore is wired. + entity type yet, so password authentication still asks for the password each time — SSH keys *are* + synced, and are the way to connect without typing anything; a key is picked per connection rather than + bound to a host, which needs a field on the host payload and therefore a schema version bump; known host + keys live in memory for one session instead of in the vault; and no device key is registered, so the + passphrase is needed on every launch until the OS keystore is wired. - **M2 — full personal vault**, robust sync, relay. - **M3 — teams**, sharing, ACLs. - **M4 — hardening and ops**, packaging, self-hosting guide. diff --git a/docs/platform-flags.md b/docs/platform-flags.md index e9dd98c..c6291fb 100644 --- a/docs/platform-flags.md +++ b/docs/platform-flags.md @@ -259,6 +259,17 @@ the blocking `Read` on a thread-pool thread, so every open session parks one thr it is idle. Fine for the handful of tabs M1 targets; revisit before advertising many concurrent sessions, since the fix is either an upstream change or driving `IChannelSession` directly. +**A passphrase supplied for an unprotected private key is silently ignored, not refused.** +`PrivateKeyFile(stream, passphrase)` on an unencrypted PKCS#1 RSA key loads it and the connection +authenticates exactly as if no passphrase had been given — measured against a real `sshd` in +`KeyAuthenticationTests.APassphraseOnAnUnprotectedKey_IsIgnoredRatherThanRefused`, which was written +expecting the opposite and corrected to match. Two consequences, and the second is the one that bites: a +stray passphrase does no harm, so nothing downstream needs to defend against it; but equally nothing +downstream will *report* one, so if a user swears they set a passphrase and the key opens without it, no +error will ever say so. Only established for that armour and that algorithm; whether the OpenSSH format's +`none` cipher path behaves the same way is untested. `SshKeySecret.Passphrase` still normalises an empty +string to null, for the reasons stated there — one representation of one state — and not for this. + **SSH.NET cannot share one connection between `SshClient` and `SftpClient`.** A shell plus SFTP to the same host means two TCP connections, two authentications and — later — two relay sockets. Connect SFTP lazily and reuse the cached decrypted credential so the user is not prompted twice. diff --git a/src/DodoSSH.Client.App/ViewModels/VaultViewModel.cs b/src/DodoSSH.Client.App/ViewModels/VaultViewModel.cs index bf440b4..d6cbafd 100644 --- a/src/DodoSSH.Client.App/ViewModels/VaultViewModel.cs +++ b/src/DodoSSH.Client.App/ViewModels/VaultViewModel.cs @@ -1,5 +1,6 @@ using System.Collections.ObjectModel; using System.Globalization; +using System.Text; using CommunityToolkit.Mvvm.ComponentModel; using CommunityToolkit.Mvvm.Input; using DodoSSH.Client.Api; @@ -17,17 +18,17 @@ namespace DodoSSH.Client.App.ViewModels; /// the flags the list has to show: an edit this machine has not pushed, a change the server refused, and /// an item a newer client wrote that must not be re-encoded here. /// -internal sealed class HostRowViewModel(VaultHost host) +internal sealed class HostRowViewModel(VaultItem host) { internal Guid EntityId => host.EntityId; - internal HostSecret Host => host.Host; + internal HostSecret Host => host.Secret; - internal string Label => host.Host.Label; + internal string Label => host.Secret.Label; internal string Address => string.Create( CultureInfo.InvariantCulture, - $"{host.Host.Username ?? "—"}@{host.Host.Hostname}:{host.Host.Port}"); + $"{host.Secret.Username ?? "—"}@{host.Secret.Hostname}:{host.Secret.Port}"); internal bool HasUnsyncedChanges => host.HasUnsyncedChanges; @@ -36,13 +37,65 @@ internal sealed class HostRowViewModel(VaultHost host) internal bool IsReadOnly => host.IsReadOnly; /// A short marker for the row, so the list says what it knows without a tooltip. - internal string Badge => host switch + internal string Badge => ItemBadge.For(host.IsBlocked, host.IsReadOnly, host.HasUnsyncedChanges); +} + +/// One SSH key, as a row in the list. +/// +/// +/// Carries the decrypted , as the host row carries its host, so opening the +/// editor or connecting with the key needs no second decryption. +/// +/// +/// Nothing here exposes the private key to the view. is what the editor and the +/// connect path read, and the members the XAML binds are the label, a description and a badge. That is not +/// a security boundary — the same object holds the material either way — but it does mean no template, +/// tooltip or accessibility surface can end up rendering a private key by being pointed at the obvious +/// property. +/// +/// +internal sealed class SshKeyRowViewModel(VaultItem key) +{ + internal Guid EntityId => key.EntityId; + + internal SshKeySecret Key => key.Secret; + + internal string Label => key.Secret.Label; + + /// What the list shows under the name: what is known about the key, never the key. + internal string Description => key.Secret switch { - { IsBlocked: true } => "rejected", - { IsReadOnly: true } => "newer version", - { HasUnsyncedChanges: true } => "not synced", - _ => string.Empty, + { Passphrase: not null, PublicKey: not null } => "passphrase · public half stored", + { Passphrase: not null } => "passphrase · no public half", + { PublicKey: not null } => "no passphrase · public half stored", + _ => "no passphrase · no public half", }; + + internal bool HasUnsyncedChanges => key.HasUnsyncedChanges; + + internal bool IsBlocked => key.IsBlocked; + + internal bool IsReadOnly => key.IsReadOnly; + + internal string Badge => ItemBadge.For(key.IsBlocked, key.IsReadOnly, key.HasUnsyncedChanges); +} + +/// The one-word marker a row shows for its sync state. +/// +/// Shared by both row types rather than written twice, because the three states mean the same thing for +/// every item type and a list where one kind said "not synced" and the other "unsynced" would read as two +/// different conditions. +/// +internal static class ItemBadge +{ + internal static string For(bool isBlocked, bool isReadOnly, bool hasUnsyncedChanges) => + (isBlocked, isReadOnly, hasUnsyncedChanges) switch + { + (true, _, _) => "rejected", + (_, true, _) => "newer version", + (_, _, true) => "not synced", + _ => string.Empty, + }; } /// A conflict, as a row. @@ -88,10 +141,16 @@ internal sealed class ConflictRowViewModel(ConflictNotice notice) /// A background pass is deliberately quieter than the button: see . /// /// -/// Credentials are not in the vault yet. SyncEntityType.Credential exists in the contract -/// but is not synced, so connecting still asks for a password each time. That is a real M1 limitation -/// rather than a design choice, and the interface says so rather than implying the vault holds more than -/// it does. +/// Keys are in the vault; passwords are not. An SSH key is a synced item, so it is stored once and +/// available on every machine. SyncEntityType.Credential exists in the contract and is still not +/// synced, so password authentication asks for the password each time. That is a real M1 limitation rather +/// than a design choice, and the interface says so rather than implying the vault holds more than it does. +/// +/// +/// A key is chosen per connection, not per host. Binding a key to a host is the better answer and it +/// is not free: it means a new field on HostSecret, which means bumping the payload schema version, +/// which makes every host written afterwards read-only on an older build. Worth doing deliberately rather +/// than as a side effect of adding keys, so for now this works the way ssh -i does. /// /// internal sealed partial class VaultViewModel( @@ -118,6 +177,9 @@ internal sealed partial class VaultViewModel( /// The hosts to show, unpushed local state included. internal ObservableCollection Hosts { get; } = []; + /// The SSH keys to show, unpushed local state included. + internal ObservableCollection Keys { get; } = []; + /// Whatever the merge had to override and the user has not acknowledged. internal ObservableCollection Conflicts { get; } = []; @@ -127,6 +189,9 @@ internal sealed partial class VaultViewModel( [ObservableProperty] private HostRowViewModel? selectedHost; + [ObservableProperty] + private SshKeyRowViewModel? selectedKey; + [ObservableProperty] private string status = string.Empty; @@ -165,6 +230,37 @@ internal sealed partial class VaultViewModel( /// The item being edited, or null when creating. private Guid? editingEntityId; + // ---- The key editor ---- + // A second set of editor state rather than a shared one. The two editors hold unrelated fields, and + // sharing them would mean a half-typed host reappearing inside a key editor. + + [ObservableProperty] + private bool isEditingKey; + + [ObservableProperty] + private string keyEditorLabel = string.Empty; + + /// + /// Bound to a text box the user pastes a private key into, so this holds key material for as long as + /// the editor is open, and clears it. Neither that nor anything else here + /// can wipe it — see SshKeySecret, which explains why a .NET string is the honest choice for + /// this and what it does not buy. + /// + [ObservableProperty] + private string keyEditorPrivateKey = string.Empty; + + [ObservableProperty] + private string keyEditorPassphrase = string.Empty; + + [ObservableProperty] + private string keyEditorPublicKey = string.Empty; + + [ObservableProperty] + private string keyEditorNotes = string.Empty; + + /// The key being edited, or null when creating. + private Guid? editingKeyId; + // ---- Connecting ---- /// @@ -173,6 +269,17 @@ internal sealed partial class VaultViewModel( [ObservableProperty] private string connectPassword = string.Empty; + /// + /// Whether to authenticate with the selected key rather than a password. + /// + /// + /// An explicit switch rather than "use the key if one happens to be selected". The key list's selection + /// exists to edit and delete keys, and letting it silently change how the next connection authenticates + /// would make clicking a row to rename it alter what Connect does. + /// + [ObservableProperty] + private bool useKeyAuthentication; + [ObservableProperty] private HostKeyPresentation? pendingHostKey; @@ -202,9 +309,13 @@ internal sealed partial class VaultViewModel( { await ReloadAsync(cancellationToken).ConfigureAwait(true); - Status = Hosts.Count == 0 - ? "No hosts yet. Add one." - : $"{Hosts.Count} host(s) in {VaultName}."; + Status = (Hosts.Count, Keys.Count) switch + { + (0, 0) => "No hosts yet. Add one.", + (0, var keys) => $"No hosts yet, and {keys} key(s) in {VaultName}.", + (var hosts, 0) => $"{hosts} host(s) in {VaultName}.", + var (hosts, keys) => $"{hosts} host(s) and {keys} key(s) in {VaultName}.", + }; } /// @@ -217,6 +328,19 @@ internal sealed partial class VaultViewModel( /// "the background pass is quiet" false on the one path that mattered. /// private async Task ReloadAsync(CancellationToken cancellationToken) + { + var unreadable = await ReloadHostsAsync(cancellationToken).ConfigureAwait(true); + + unreadable += await ReloadKeysAsync(cancellationToken).ConfigureAwait(true); + + UnreadableItems = unreadable; + PendingChanges = await session.PendingChangeCountAsync(cancellationToken).ConfigureAwait(true); + + await LoadConflictsAsync(cancellationToken).ConfigureAwait(true); + } + + /// How many hosts would not decrypt. + private async Task ReloadHostsAsync(CancellationToken cancellationToken) { var listing = await session.Hosts .ListAsync(session.ActiveVaultId, cancellationToken) @@ -226,7 +350,7 @@ internal sealed partial class VaultViewModel( Hosts.Clear(); - foreach (var host in listing.Hosts.OrderBy(host => host.Host.Label, StringComparer.CurrentCulture)) + foreach (var host in listing.Items.OrderBy(host => host.Secret.Label, StringComparer.CurrentCulture)) { Hosts.Add(new HostRowViewModel(host)); } @@ -235,10 +359,33 @@ internal sealed partial class VaultViewModel( // under the user. SelectedHost = Hosts.FirstOrDefault(row => row.EntityId == selectedId) ?? Hosts.FirstOrDefault(); - UnreadableItems = listing.Unreadable; - PendingChanges = await session.PendingChangeCountAsync(cancellationToken).ConfigureAwait(true); + return listing.Unreadable; + } - await LoadConflictsAsync(cancellationToken).ConfigureAwait(true); + /// How many keys would not decrypt. + /// + /// Unlike the host list, the selection is not defaulted to the first row. A key selection is + /// what authenticates with, and quietly selecting one on load would + /// mean a connection made with a key the user never chose. + /// + private async Task ReloadKeysAsync(CancellationToken cancellationToken) + { + var listing = await session.SshKeys + .ListAsync(session.ActiveVaultId, cancellationToken) + .ConfigureAwait(true); + + var selectedId = SelectedKey?.EntityId; + + Keys.Clear(); + + foreach (var key in listing.Items.OrderBy(key => key.Secret.Label, StringComparer.CurrentCulture)) + { + Keys.Add(new SshKeyRowViewModel(key)); + } + + SelectedKey = Keys.FirstOrDefault(row => row.EntityId == selectedId); + + return listing.Unreadable; } /// Runs a synchronisation pass, if there is a server to talk to. @@ -386,6 +533,11 @@ internal sealed partial class VaultViewModel( [RelayCommand] private void NewHost() { + if (KeyEditorIsInTheWay()) + { + return; + } + editingEntityId = null; EditorLabel = string.Empty; EditorHostname = string.Empty; @@ -401,7 +553,7 @@ internal sealed partial class VaultViewModel( [RelayCommand] private void EditSelectedHost() { - if (SelectedHost is not { } row) + if (SelectedHost is not { } row || KeyEditorIsInTheWay()) { return; } @@ -505,6 +657,129 @@ internal sealed partial class VaultViewModel( await AutoSyncAsync(cancellationToken).ConfigureAwait(true); } + /// Starts a new SSH key. + [RelayCommand] + private void NewKey() + { + if (HostEditorIsInTheWay()) + { + return; + } + + editingKeyId = null; + ClearKeyEditor(); + IsEditingKey = true; + Status = "Adding an SSH key."; + } + + /// Opens the selected key for editing. + /// + /// The private key is loaded into the editor, which is the only way an edit can preserve it: the + /// codec has no notion of a partial update, so saving re-encodes every field. + /// + [RelayCommand] + private void EditSelectedKey() + { + if (SelectedKey is not { } row || HostEditorIsInTheWay()) + { + return; + } + + if (row.IsReadOnly) + { + Status = "This key was written by a newer version of DodoSSH. Update before editing it."; + return; + } + + editingKeyId = row.EntityId; + KeyEditorLabel = row.Key.Label; + KeyEditorPrivateKey = row.Key.PrivateKeyPem; + KeyEditorPassphrase = row.Key.Passphrase ?? string.Empty; + KeyEditorPublicKey = row.Key.PublicKey ?? string.Empty; + KeyEditorNotes = row.Key.Notes ?? string.Empty; + IsEditingKey = true; + Status = $"Editing {row.Label}."; + } + + /// Abandons the key editor, clearing the material out of it. + [RelayCommand] + private void CancelKeyEdit() + { + IsEditingKey = false; + editingKeyId = null; + ClearKeyEditor(); + Status = string.Empty; + } + + /// Stores the key editor's contents, encrypted, and queues it for the server. + [RelayCommand] + private async Task SaveKeyAsync(CancellationToken cancellationToken) + { + var key = BuildKey(); + + if (!key.TryValidate(out var reason)) + { + Status = reason; + return; + } + + await RunAsync( + "Saving…", + async () => + { + if (editingKeyId is { } entityId) + { + await session.SshKeys + .UpdateAsync(session.ActiveVaultId, entityId, key, cancellationToken) + .ConfigureAwait(true); + } + else + { + editingKeyId = await session.SshKeys + .CreateAsync(session.ActiveVaultId, key, cancellationToken) + .ConfigureAwait(true); + } + + IsEditingKey = false; + ClearKeyEditor(); + + await ReloadAsync(cancellationToken).ConfigureAwait(true); + + SelectedKey = Keys.FirstOrDefault(row => row.EntityId == editingKeyId); + editingKeyId = null; + + Status = connection() is null + ? $"Saved '{key.Label}'. It will sync when you are online." + : $"Saved '{key.Label}'."; + }).ConfigureAwait(true); + + await AutoSyncAsync(cancellationToken).ConfigureAwait(true); + } + + /// Queues a tombstone for the selected key. + [RelayCommand] + private async Task DeleteKeyAsync(CancellationToken cancellationToken) + { + if (SelectedKey is not { } row) + { + return; + } + + await RunAsync( + "Deleting…", + async () => + { + await session.SshKeys + .DeleteAsync(session.ActiveVaultId, row.EntityId, cancellationToken) + .ConfigureAwait(true); + + await ReloadAsync(cancellationToken).ConfigureAwait(true); + Status = $"Deleted '{row.Label}'."; + }).ConfigureAwait(true); + + await AutoSyncAsync(cancellationToken).ConfigureAwait(true); + } + /// Opens a terminal on the selected host. [RelayCommand] private async Task ConnectAsync(CancellationToken cancellationToken) @@ -521,6 +796,15 @@ internal sealed partial class VaultViewModel( return; } + if (UseKeyAuthentication && SelectedKey is null) + { + // Refused rather than quietly falling back to the password box. Silently authenticating a + // different way than the user asked for is how a password reaches a host that was meant to + // only ever see a key. + Status = "Choose a key to authenticate with, or turn key authentication off."; + return; + } + PendingHostKey = null; HostKeyMismatch = null; @@ -617,7 +901,7 @@ internal sealed partial class VaultViewModel( row.Host.Hostname, row.Host.Port, row.Host.Username!, - new SshPasswordCredential(ConnectPassword)); + BuildCredential()); await workspace .OpenSessionAsync(request, TerminalSize.Default, cancellationToken) @@ -652,6 +936,30 @@ internal sealed partial class VaultViewModel( } } + /// + /// How the next connection authenticates. + /// + /// + /// The key material is handed over as UTF-8 bytes, which is what PrivateKeyFile reads from a + /// MemoryStream — so the key reaches SSH.NET without ever becoming a file on disk. The + /// passphrase goes with it: a key stored in the vault together with its passphrase is the whole point + /// of a vault, and SshKeySecret says why. + /// + /// The passphrase is passed straight through, with no empty-to-null check, because + /// SshKeySecret.Passphrase cannot hold an empty string — it normalises one to null on the way in. + /// + /// + private SshCredential BuildCredential() + { + if (!UseKeyAuthentication || SelectedKey is not { } row) + { + return new SshPasswordCredential(ConnectPassword); + } + + return new SshPrivateKeyCredential( + Encoding.UTF8.GetBytes(row.Key.PrivateKeyPem), row.Key.Passphrase); + } + private HostSecret BuildHost() => new() { @@ -663,6 +971,76 @@ internal sealed partial class VaultViewModel( RelayEnabled = EditorRelayEnabled, }; + /// + /// The private key is not trimmed. Its armour is whitespace-significant and a client that tidied it up + /// would eventually tidy a format it did not fully understand — the same reason + /// SshKeySecret.PrivateKeyPem stores it verbatim. Everything else is trimmed, because a label + /// with a trailing space sorts oddly and reads as a different name. + /// + private SshKeySecret BuildKey() => + new() + { + Label = KeyEditorLabel.Trim(), + PrivateKeyPem = KeyEditorPrivateKey, + + // Not trimmed and not emptied: leading or trailing spaces are legitimate in a passphrase, and + // the record turns an empty one into null on its own. + Passphrase = KeyEditorPassphrase, + PublicKey = string.IsNullOrWhiteSpace(KeyEditorPublicKey) ? null : KeyEditorPublicKey.Trim(), + Notes = string.IsNullOrWhiteSpace(KeyEditorNotes) ? null : KeyEditorNotes, + }; + + /// + /// Whether the key editor has to be dealt with before another one can open. + /// + /// + /// + /// Only one editor open at a time, and this is a layout constraint rather than a style rule. Both + /// editors sit in the same 340-pixel column as Auto rows, and their desired heights together + /// exceed the column at the window's minimum height — so opening both pushes the lower one's Save and + /// Cancel past the bottom edge, where they cannot be clicked. That is the same failure this window has + /// already shipped once, when the setup screens rendered sliced with their buttons unreachable, and it is + /// the failure that nothing in this repository can catch: no test loads a .axaml. Making it a + /// state rule instead of a sizing hope is what makes it testable at all. + /// + /// + /// Refused rather than resolved by closing the other editor, because closing it would silently discard + /// what was typed there — and in the key editor that is a pasted private key the user may have nowhere + /// else. One sentence and one click is the cheaper of the two. + /// + /// + private bool KeyEditorIsInTheWay() + { + if (!IsEditingKey) + { + return false; + } + + Status = "Finish or cancel the SSH key you are editing first."; + return true; + } + + /// + private bool HostEditorIsInTheWay() + { + if (!IsEditing) + { + return false; + } + + Status = "Finish or cancel the host you are editing first."; + return true; + } + + private void ClearKeyEditor() + { + KeyEditorLabel = string.Empty; + KeyEditorPrivateKey = string.Empty; + KeyEditorPassphrase = string.Empty; + KeyEditorPublicKey = string.Empty; + KeyEditorNotes = string.Empty; + } + private async Task LoadConflictsAsync(CancellationToken cancellationToken) { var notices = await session.ReadConflictsAsync(cancellationToken).ConfigureAwait(true); @@ -693,9 +1071,11 @@ internal sealed partial class VaultViewModel( var notes = new List(); + // "item(s)", not "host(s)": a vault now holds keys as well, and a report that named the wrong kind + // would send someone looking through the wrong list for something that was not there. if (report.Resurrected > 0) { - notes.Add($"{report.Resurrected} host(s) deleted elsewhere were kept under a new name"); + notes.Add($"{report.Resurrected} item(s) deleted elsewhere were kept under a new name"); } if (report.DeletesAbandoned > 0) diff --git a/src/DodoSSH.Client.App/Views/MainWindow.axaml b/src/DodoSSH.Client.App/Views/MainWindow.axaml index 2e6712e..bd413df 100644 --- a/src/DodoSSH.Client.App/Views/MainWindow.axaml +++ b/src/DodoSSH.Client.App/Views/MainWindow.axaml @@ -78,8 +78,21 @@ - - + @@ -138,6 +151,77 @@ /// + /// /// Kept with the key rather than typed per connection, which is the entire point of a vault: the /// passphrase defends the key file on a disk, and inside a vault the key is not on a disk. Storing both /// together means the vault passphrase is what protects them, which is the guarantee this product is /// built to make. A user who wants the second factor can leave this null and be prompted. + /// + /// + /// An empty string is normalised to null, so there is exactly one way to say "no passphrase". + /// Two spellings of one state cost more than they look: two clients that agree about a key and disagree + /// only about which spelling they used would produce different payload bytes for an identical key and a + /// spurious field conflict out of the merge, and Passphrase is not null would stop being a + /// reliable answer to "is this key protected?" — which is what the interface reads to describe a key. + /// Normalising here rather than at each call site means every way one can arrive lands on the same + /// value: an editor whose box was left blank, a codec decoding another client's "", a merge + /// picking one side. + /// + /// + /// It is not a defence against SSH.NET, which was the original reason given here and turned out + /// to be false: a passphrase handed to PrivateKeyFile for a key that has none is ignored, not + /// rejected, and the connection succeeds. See docs/platform-flags.md. + /// /// - public string? Passphrase { get; init; } + public string? Passphrase + { + get => passphrase; + init => passphrase = string.IsNullOrEmpty(value) ? null : value; + } /// /// The public half, in authorized_keys form, when it is known. @@ -78,17 +101,17 @@ public sealed record SshKeySecret /// produces a vault item that looks fine and fails at connection time with an authentication error that /// says nothing about which file you chose. /// - public bool TryValidate([NotNullWhen(false)] out string? error) + public bool TryValidate([NotNullWhen(false)] out string? reason) { if (string.IsNullOrWhiteSpace(Label)) { - error = "A key needs a name."; + reason = "A key needs a name."; return false; } if (string.IsNullOrWhiteSpace(PrivateKeyPem)) { - error = "A key needs its private key material."; + reason = "A key needs its private key material."; return false; } @@ -97,17 +120,17 @@ public sealed record SshKeySecret if (material.StartsWith("ssh-", StringComparison.Ordinal) || material.StartsWith("ecdsa-", StringComparison.Ordinal)) { - error = "That is a public key. Paste the private key — the file without the .pub extension."; + reason = "That is a public key. Paste the private key — the file without the .pub extension."; return false; } if (!material.StartsWith("-----BEGIN", StringComparison.Ordinal)) { - error = "That does not look like a private key; it should begin with \"-----BEGIN\"."; + reason = "That does not look like a private key; it should begin with \"-----BEGIN\"."; return false; } - error = null; + reason = null; return true; } } diff --git a/src/DodoSSH.Client.Domain/VaultSecrets.cs b/src/DodoSSH.Client.Domain/VaultSecrets.cs new file mode 100644 index 0000000..233ca0f --- /dev/null +++ b/src/DodoSSH.Client.Domain/VaultSecrets.cs @@ -0,0 +1,37 @@ +using System.Diagnostics.CodeAnalysis; + +namespace DodoSSH.Client.Domain; + +/// +/// What every kind of decrypted vault item has in common. +/// +/// +/// +/// Two members, and both are here because the sync layer needs them for every item type it handles: a +/// name to put in a message to a person, and the check that must pass before the item is sealed. +/// Everything else about an item — how it is encoded, how two versions of it merge, which plaintext +/// columns the server is allowed to see — is per-type behaviour that lives in the sync layer's item +/// kinds rather than on the record. The split is not arbitrary: a label and a validity rule are +/// intrinsic to the thing, whereas how it is encrypted is a decision about how it is stored. +/// +/// +/// An interface rather than a base record, because the implementations share no field — a base record +/// would exist solely to declare two abstract members, and would put a type in the equality contract of +/// records that have nothing else in common. The server's IVaultItem is an interface for a +/// sharper reason (EF Core maps a visible base class as a TPH hierarchy) but reaches the same place. +/// +/// +public interface IVaultSecret +{ + /// What the user calls this item. The only name it has anywhere. + string Label { get; } + + /// Whether this is storable, and why not if it is not. + /// + /// The out parameter is reason rather than the error every implementation used before + /// this interface existed, because CA1716 refuses a reserved word from another CLR language on an + /// interface member. Renamed at the implementations too: a parameter name that differs from the one it + /// implements is legal and reads as an oversight. + /// + bool TryValidate([NotNullWhen(false)] out string? reason); +} diff --git a/src/DodoSSH.Client.Session/VaultSession.cs b/src/DodoSSH.Client.Session/VaultSession.cs index 6649145..2286176 100644 --- a/src/DodoSSH.Client.Session/VaultSession.cs +++ b/src/DodoSSH.Client.Session/VaultSession.cs @@ -75,6 +75,7 @@ public sealed class VaultSession : IAsyncDisposable Conflicts = new ConflictStore(caches, protector, clock); Vault = new VaultStore(caches, clock); Hosts = new HostRepository(Items, Outbox, keyring); + SshKeys = new SshKeyRepository(Items, Outbox, keyring); } /// Who this session belongs to, and the material that unlocked it. @@ -89,6 +90,13 @@ public sealed class VaultSession : IAsyncDisposable /// Hosts, decrypted, with unpushed local changes laid over them. public HostRepository Hosts { get; } + /// SSH keys, decrypted, with unpushed local changes laid over them. + /// + /// Shares the item store and outbox with , so one synchronisation pass carries + /// both and a key edit made offline queues behind a host edit in the order the user made them. + /// + public SshKeyRepository SshKeys { get; } + /// Vaults whose grant could not be opened, so their items cannot be read. public IReadOnlyList UnreadableVaults => keyring.Unopened; diff --git a/src/DodoSSH.Client.Sync/HostRepository.cs b/src/DodoSSH.Client.Sync/HostRepository.cs index 9a69753..19c93e4 100644 --- a/src/DodoSSH.Client.Sync/HostRepository.cs +++ b/src/DodoSSH.Client.Sync/HostRepository.cs @@ -1,299 +1,40 @@ using DodoSSH.Client.Domain; using DodoSSH.Client.Storage; -using DodoSSH.Contracts; namespace DodoSSH.Client.Sync; -/// A host as the interface should show it. -/// The item id. -/// The decrypted host. -/// -/// The server version this is based on. Zero for an item that has never been accepted. -/// -/// -/// Whether this reflects a local edit the server has not accepted yet. Worth showing: it is the -/// difference between "saved" and "saved here". -/// -/// -/// Whether the pending change was refused and is waiting on a person, so it will not retry on its own. -/// -/// -/// Whether this host was written by a newer client and so must not be edited here, because re-encoding -/// it would drop fields this build cannot represent. -/// -public sealed record VaultHost( - Guid EntityId, - HostSecret Host, - int Version, - bool HasUnsyncedChanges, - bool IsBlocked, - bool IsReadOnly); - -/// The hosts in a vault, and what could not be read. -/// The readable hosts, newest change last. -/// -/// How many items would not decrypt. Surfaced rather than swallowed: a non-zero count here after a -/// rekey is the signal that new grants are needed. -/// -public sealed record HostListing(IReadOnlyList Hosts, int Unreadable); - /// -/// Reading and writing hosts, as the interface sees them. +/// The hosts in a vault, decrypted, with unpushed local changes laid over them. /// /// -/// -/// The view is the mirror of the server's state with the outbox laid over it, which is what makes the -/// application feel local: an edit appears immediately and a delete disappears immediately, whether or -/// not the network is there. Nothing here talks to the server; the sync engine reconciles later. -/// -/// -/// Writes never touch the mirror. That separation is load-bearing — the mirror is the common ancestor a -/// three-way merge needs, and a repository that updated it on save would destroy the very state that -/// lets a conflict be merged instead of arbitrated. -/// +/// A named facade over , which holds the logic and is shared +/// with . Two reasons it is a facade rather than the generic class itself: +/// callers read better for having asked for hosts by name, and the item kind that parameterises the +/// generic is internal to this assembly — exposing it would make the encoding and merge of every item +/// type part of the public surface for the sake of a constructor argument. /// public sealed class HostRepository(ItemStore items, OutboxStore outbox, VaultKeyring keyring) { - /// Reads every host the user should see in a vault. - public async Task ListAsync(Guid vaultId, CancellationToken cancellationToken) - { - if (!keyring.TryGet(vaultId, out var vaultKey, out _)) - { - throw new VaultUnreadableException(vaultId); - } + private readonly VaultItemRepository hosts = + new(HostKind.Instance, items, outbox, keyring); - var mirrored = await items - .ListAsync(vaultId, SyncEntityType.Host, includeDeleted: true, cancellationToken) - .ConfigureAwait(false); + /// + public Task> ListAsync(Guid vaultId, CancellationToken cancellationToken) => + hosts.ListAsync(vaultId, cancellationToken); - var pending = await outbox.ListAllAsync(vaultId, cancellationToken).ConfigureAwait(false); + /// + public Task CreateAsync(Guid vaultId, HostSecret host, CancellationToken cancellationToken) => + hosts.CreateAsync(vaultId, host, cancellationToken); - var pendingByEntity = pending - .Where(operation => operation.EntityType == SyncEntityType.Host) - .ToDictionary(operation => operation.EntityId); - - var hosts = new List(); - var unreadable = 0; - - foreach (var item in mirrored) - { - if (pendingByEntity.Remove(item.EntityId, out var local)) - { - AddPending(hosts, ref unreadable, vaultKey, local); - continue; - } - - if (item.IsDeleted || item.Payload is null) - { - continue; - } - - var opened = HostCipher.TryOpen(item.Payload, vaultKey.Span, item.EntityId, item.Version); - - if (opened is null) - { - unreadable++; - continue; - } - - hosts.Add(new VaultHost( - item.EntityId, opened.Host, item.Version, false, false, opened.IsReadOnly)); - } - - // Whatever is left has no mirror row yet: items created here and not yet accepted. - foreach (var local in pendingByEntity.Values) - { - AddPending(hosts, ref unreadable, vaultKey, local); - } - - return new HostListing(hosts, unreadable); - } - - /// - /// Adds a host, returning the id it was given. - /// - /// - /// The id is generated here, not by the server, which is what lets a host be created with no network - /// at all — the point of the whole outbox. UUIDv7 so that ids sort by creation time, which keeps - /// index locality reasonable on the server side. - /// - public async Task CreateAsync( - Guid vaultId, - HostSecret host, - CancellationToken cancellationToken) - { - ArgumentNullException.ThrowIfNull(host); - Validate(host); - - var (vaultKey, generation) = Key(vaultId); - var entityId = Guid.CreateVersion7(); - - await outbox.QueueAsync( - new QueuedChange( - vaultId, - SyncEntityType.Host, - entityId, - SyncOperation.Upsert, - ExpectedVersion: null, - HostCipher.Seal(host, vaultKey.Span, entityId, generation, itemVersion: 1), - HostFields.From(host), - Ancestor: null), - cancellationToken).ConfigureAwait(false); - - return entityId; - } - - /// - /// Replaces a host's contents. - /// - /// - /// The base is taken from the pending operation when there is one, and from the mirror otherwise. - /// Reading it the other way round would seal the payload at a version that does not match the - /// expectedVersion the coalesced row keeps — and because the AAD binds the item version, the - /// result would encrypt cleanly and never decrypt again. - /// - public async Task UpdateAsync( + /// + public Task UpdateAsync( Guid vaultId, Guid entityId, HostSecret host, - CancellationToken cancellationToken) - { - ArgumentNullException.ThrowIfNull(host); - Validate(host); + CancellationToken cancellationToken) => + hosts.UpdateAsync(vaultId, entityId, host, cancellationToken); - var (vaultKey, generation) = Key(vaultId); - - var pending = await outbox - .FindAsync(vaultId, SyncEntityType.Host, entityId, cancellationToken) - .ConfigureAwait(false); - - var expectedVersion = pending is not null - ? pending.ExpectedVersion - : await MirrorVersionAsync(vaultId, entityId, cancellationToken).ConfigureAwait(false); - - var ancestor = pending?.Ancestor - ?? await MirrorAncestorAsync(vaultId, entityId, cancellationToken).ConfigureAwait(false); - - await outbox.QueueAsync( - new QueuedChange( - vaultId, - SyncEntityType.Host, - entityId, - SyncOperation.Upsert, - expectedVersion, - HostCipher.Seal( - host, vaultKey.Span, entityId, generation, SyncVersions.NextVersion(expectedVersion)), - HostFields.From(host), - ancestor), - cancellationToken).ConfigureAwait(false); - } - - /// - /// Deletes a host. - /// - /// - /// Queued as a tombstone, never a local removal. An offline client that simply forgot the row would - /// be unable to tell the server anything, and the item would come back on the next pull. - /// - public async Task DeleteAsync(Guid vaultId, Guid entityId, CancellationToken cancellationToken) - { - var pending = await outbox - .FindAsync(vaultId, SyncEntityType.Host, entityId, cancellationToken) - .ConfigureAwait(false); - - var expectedVersion = pending is not null - ? pending.ExpectedVersion - : await MirrorVersionAsync(vaultId, entityId, cancellationToken).ConfigureAwait(false); - - var ancestor = pending?.Ancestor - ?? await MirrorAncestorAsync(vaultId, entityId, cancellationToken).ConfigureAwait(false); - - await outbox.QueueAsync( - new QueuedChange( - vaultId, - SyncEntityType.Host, - entityId, - SyncOperation.Delete, - expectedVersion, - Payload: null, - Fields: null, - ancestor), - cancellationToken).ConfigureAwait(false); - } - - private static void Validate(HostSecret host) - { - if (!host.TryValidate(out var error)) - { - throw new ArgumentException(error, nameof(host)); - } - } - - private static void AddPending( - List hosts, - ref int unreadable, - ReadOnlyMemory vaultKey, - PendingOperation local) - { - if (local.Operation == SyncOperation.Delete) - { - // Gone as far as this machine is concerned, even before the server agrees. - return; - } - - if (local.Payload is null) - { - unreadable++; - return; - } - - var version = SyncVersions.NextVersion(local.ExpectedVersion); - var opened = HostCipher.TryOpen(local.Payload, vaultKey.Span, local.EntityId, version); - - if (opened is null) - { - unreadable++; - return; - } - - hosts.Add(new VaultHost( - local.EntityId, - opened.Host, - local.ExpectedVersion ?? 0, - HasUnsyncedChanges: true, - local.IsParked, - opened.IsReadOnly)); - } - - private (ReadOnlyMemory VaultKey, uint Generation) Key(Guid vaultId) => - keyring.TryGet(vaultId, out var vaultKey, out var generation) - ? (vaultKey, generation) - : throw new VaultUnreadableException(vaultId); - - private async Task MirrorVersionAsync( - Guid vaultId, - Guid entityId, - CancellationToken cancellationToken) - { - var item = await items - .FindAsync(vaultId, SyncEntityType.Host, entityId, cancellationToken) - .ConfigureAwait(false); - - // A null means the server has never seen this item, which is exactly what "create" is. - return item?.Version; - } - - private async Task MirrorAncestorAsync( - Guid vaultId, - Guid entityId, - CancellationToken cancellationToken) - { - var item = await items - .FindAsync(vaultId, SyncEntityType.Host, entityId, cancellationToken) - .ConfigureAwait(false); - - return item?.Payload is null - ? null - : new StoredAncestor(item.Version, item.Payload, item.Fields); - } + /// + public Task DeleteAsync(Guid vaultId, Guid entityId, CancellationToken cancellationToken) => + hosts.DeleteAsync(vaultId, entityId, cancellationToken); } diff --git a/src/DodoSSH.Client.Sync/ItemKinds.cs b/src/DodoSSH.Client.Sync/ItemKinds.cs new file mode 100644 index 0000000..57f788a --- /dev/null +++ b/src/DodoSSH.Client.Sync/ItemKinds.cs @@ -0,0 +1,250 @@ +using DodoSSH.Client.Domain; +using DodoSSH.Client.Storage; +using DodoSSH.Contracts; + +namespace DodoSSH.Client.Sync; + +/// A decrypted item, and whether this build may write it back. +/// The item. +/// +/// Whether a newer client wrote it, in which case re-encoding it here would drop fields this build has +/// no concept of. +/// +internal sealed record OpenedItem(TSecret Secret, bool IsReadOnly) + where TSecret : class, IVaultSecret; + +/// A merged item, and everything that had to be overridden to produce it. +/// The item to store and push. +/// Empty when the two sides were reconcilable field by field. +internal sealed record MergedItem( + TSecret Merged, + IReadOnlyList Conflicts) + where TSecret : class, IVaultSecret; + +/// +/// Everything about one item type that the shared sync path cannot know. +/// +/// +/// +/// The reconciler holds the six answers a collision can have — merge, adopt, resurrect, abandon, park, +/// refuse — and every one of them is identical for a host and for an SSH key. Only the encoding, the +/// merge and the plaintext columns differ, and those arrive through here. A second copy of the +/// reconciler per item type is the alternative, and it is not a real one: the file's whole premise is +/// that the pull and push paths must answer the same situation the same way, and two copies would drift +/// the moment one of them was fixed. +/// +/// +/// Generic, unlike the server's IItemKind, and for a reason that reverses there: the client +/// does need the concrete type. It merges two versions of an item field by field and hands the +/// result to a codec, so erasing the type would only move the downcasts inside the reconciler, where +/// they would be a cast per branch instead of none. +/// +/// +internal interface IItemKind + where TSecret : class, IVaultSecret +{ + /// The type as the wire contract names it. + SyncEntityType EntityType { get; } + + /// + /// What to call one of these when telling a person what happened to it. + /// + /// + /// Lower case and singular, because every use is mid-sentence. This exists because the conflict log + /// is read by people: "this host could not be decrypted" is actively misleading when the item was a + /// private key, and a user who is told the wrong noun looks in the wrong place. + /// + string Noun { get; } + + /// + OpenedItem? TryOpen( + EncryptedPayload payload, + ReadOnlySpan vaultKey, + Guid entityId, + int itemVersion); + + /// + EncryptedPayload Seal( + TSecret secret, + ReadOnlySpan vaultKey, + Guid entityId, + uint keyGeneration, + int itemVersion); + + /// + /// The plaintext columns the server gets, or null when this type gives it nothing. + /// + /// + /// Nullable rather than an all-defaults record, because the difference is visible on the wire and to + /// a reader: SyncPlaintextFields with nothing set still serialises relayEnabled: false, + /// which invites the belief that the type has a relay setting which happens to be off. + /// + SyncPlaintextFields? Fields(TSecret secret); + + /// Merges two divergent versions against the version they both started from. + MergedItem Merge(TSecret ancestor, TSecret local, TSecret remote); + + /// The same item under a new name, for a resurrection. + TSecret Relabel(TSecret secret, string label); +} + +/// +/// The item types this client synchronises. +/// +/// +/// +/// One list, and the pull filter is derived from it. The engine asks the server for exactly the +/// types in and refuses to apply a change of any other type, so adding a kind +/// cannot leave the filter behind — which is the specific way this would otherwise break: an item type +/// that reads and writes perfectly in every unit test and is never once requested from the server. +/// +/// +/// A reconciler per type rather than one shared instance, because each closes the generic over its own +/// secret type. They are cheap — four fields and no state — and building them once per engine keeps the +/// per-change path a dictionary lookup. +/// +/// +internal static class ItemKinds +{ + private static readonly (SyncEntityType Type, ReconcilerFactory Create)[] Registry = + [ + (SyncEntityType.Host, static (outbox, conflicts, keyring) => + new ItemReconciler(HostKind.Instance, outbox, conflicts, keyring)), + + (SyncEntityType.SshKey, static (outbox, conflicts, keyring) => + new ItemReconciler(SshKeyKind.Instance, outbox, conflicts, keyring)), + ]; + + /// The types to ask the server for, in a fixed order. + internal static IReadOnlyList SyncedTypes { get; } = + [.. Registry.Select(entry => entry.Type)]; + + private delegate IItemReconciler ReconcilerFactory( + OutboxStore outbox, + ConflictStore conflicts, + VaultKeyring keyring); + + /// Builds one reconciler per synchronised type. + internal static Dictionary Reconcilers( + OutboxStore outbox, + ConflictStore conflicts, + VaultKeyring keyring) => + Registry.ToDictionary( + entry => entry.Type, + entry => entry.Create(outbox, conflicts, keyring)); +} + +/// Hosts. +internal sealed class HostKind : IItemKind +{ + internal static HostKind Instance { get; } = new(); + + /// + public SyncEntityType EntityType => SyncEntityType.Host; + + /// + public string Noun => "host"; + + /// + public OpenedItem? TryOpen( + EncryptedPayload payload, + ReadOnlySpan vaultKey, + Guid entityId, + int itemVersion) + { + var document = HostCipher.TryOpen(payload, vaultKey, entityId, itemVersion); + + return document is null ? null : new OpenedItem(document.Host, document.IsReadOnly); + } + + /// + public EncryptedPayload Seal( + HostSecret secret, + ReadOnlySpan vaultKey, + Guid entityId, + uint keyGeneration, + int itemVersion) => + HostCipher.Seal(secret, vaultKey, entityId, keyGeneration, itemVersion); + + /// + public SyncPlaintextFields? Fields(HostSecret secret) => HostFields.From(secret); + + /// + public MergedItem Merge(HostSecret ancestor, HostSecret local, HostSecret remote) + { + var merged = HostSecretMerge.Merge(ancestor, local, remote); + + return new MergedItem(merged.Merged, merged.Conflicts); + } + + /// + public HostSecret Relabel(HostSecret secret, string label) + { + ArgumentNullException.ThrowIfNull(secret); + + return secret with { Label = label }; + } +} + +/// SSH keys. +internal sealed class SshKeyKind : IItemKind +{ + internal static SshKeyKind Instance { get; } = new(); + + /// + public SyncEntityType EntityType => SyncEntityType.SshKey; + + /// + public string Noun => "SSH key"; + + /// + public OpenedItem? TryOpen( + EncryptedPayload payload, + ReadOnlySpan vaultKey, + Guid entityId, + int itemVersion) + { + var document = SshKeyCipher.TryOpen(payload, vaultKey, entityId, itemVersion); + + return document is null ? null : new OpenedItem(document.Key, document.IsReadOnly); + } + + /// + public EncryptedPayload Seal( + SshKeySecret secret, + ReadOnlySpan vaultKey, + Guid entityId, + uint keyGeneration, + int itemVersion) => + SshKeyCipher.Seal(secret, vaultKey, entityId, keyGeneration, itemVersion); + + /// + /// Nothing at all. + /// + /// + /// The server has a public_key_fingerprint column and would accept one here, and this client + /// deliberately declines to fill it. A fingerprint is not a secret, but it is a stable identifier for + /// a key pair, and handing it over would let the operator tell which of their users hold the same key + /// and correlate one key across vaults — for a column nothing in the product reads. The design allows + /// itself exactly one plaintext concession, the relay address, and it is a concession because the + /// relay cannot work without it. This is not that. See ADR 0004. + /// + /// + public SyncPlaintextFields? Fields(SshKeySecret secret) => null; + + /// + public MergedItem Merge(SshKeySecret ancestor, SshKeySecret local, SshKeySecret remote) + { + var merged = SshKeySecretMerge.Merge(ancestor, local, remote); + + return new MergedItem(merged.Merged, merged.Conflicts); + } + + /// + public SshKeySecret Relabel(SshKeySecret secret, string label) + { + ArgumentNullException.ThrowIfNull(secret); + + return secret with { Label = label }; + } +} diff --git a/src/DodoSSH.Client.Sync/ItemReconciler.cs b/src/DodoSSH.Client.Sync/ItemReconciler.cs index 150207f..789ac1d 100644 --- a/src/DodoSSH.Client.Sync/ItemReconciler.cs +++ b/src/DodoSSH.Client.Sync/ItemReconciler.cs @@ -13,7 +13,7 @@ namespace DodoSSH.Client.Sync; /// Deterministic, from the original id and the version of the tombstone that displaced it. That matters /// because applying a pulled change is at-least-once: the cursor is saved after the changes are applied, /// so a process that dies in between re-applies them on the next start. A random id would resurrect the -/// same host twice and leave the user with duplicates to sort out; this way the second attempt produces +/// same item twice and leave the user with duplicates to sort out; this way the second attempt produces /// the same id and coalesces into the same outbox row. /// /// Not a UUIDv7, and that is fine — the server treats item ids as opaque, and the time ordering a v7 id @@ -47,6 +47,32 @@ internal static class ResurrectionId } } +/// +/// Reconciles one item type, with the secret type erased so the engine can hold a table of them. +/// +/// +/// The engine never needs the concrete type — it dispatches on the entity type a change carries and lets +/// the reconciler do the rest — so this interface is what it stores. The two members are the two places +/// the push and pull paths need type-specific crypto. +/// +internal interface IItemReconciler +{ + /// Reconciles a remote change against the operation pending for the same item. + Task ReconcileAsync( + Guid vaultId, + SyncChange remote, + PendingOperation pending, + SyncReportBuilder report, + CancellationToken cancellationToken); + + /// Re-seals a queued change as a create, for a server that says it has no such row. + /// Null on success, or the reason the change could not be re-offered. + Task ReofferAsCreateAsync( + Guid vaultId, + PendingOperation pending, + CancellationToken cancellationToken); +} + /// /// Decides what happens when a remote change collides with an unpushed local one. /// @@ -54,7 +80,10 @@ internal static class ResurrectionId /// /// Shared by the pull and the push paths, because both meet the same six situations and must answer them /// identically — a pull that merged one way and a push that merged the other would make the outcome -/// depend on which side happened to notice first. +/// depend on which side happened to notice first. Shared across item types for the same reason: a host +/// and an SSH key meet those six situations in exactly the same way, and the only differences — +/// encoding, merge, plaintext columns, what to call the thing — arrive through +/// . /// /// /// The governing rule is that nothing is discarded silently. Where the two sides can be @@ -64,20 +93,29 @@ internal static class ResurrectionId /// reconstruct. /// /// -internal sealed class ItemReconciler( - ItemStore items, +/// +/// Takes no , which is worth noticing rather than reading as an omission: nothing +/// here writes the mirror. Reconciling only ever revises the outbox and records conflicts, and the +/// server's own version of an item is written by before this is called. +/// +internal sealed class ItemReconciler( + IItemKind kind, OutboxStore outbox, ConflictStore conflicts, - VaultKeyring keyring) + VaultKeyring keyring) : IItemReconciler + where TSecret : class, IVaultSecret { /// Reconciles a remote change against the operation pending for the same item. - internal Task ReconcileAsync( + public Task ReconcileAsync( Guid vaultId, SyncChange remote, PendingOperation pending, SyncReportBuilder report, CancellationToken cancellationToken) { + ArgumentNullException.ThrowIfNull(remote); + ArgumentNullException.ThrowIfNull(pending); + if (remote.Operation == SyncOperation.Delete) { return pending.Operation == SyncOperation.Delete @@ -91,6 +129,49 @@ internal sealed class ItemReconciler( : MergeAsync(vaultId, remote, pending, report, cancellationToken); } + /// + /// Re-seals a queued change as a create, for a server that says it has no such row. + /// + /// + /// The payload has to be re-sealed rather than re-sent: it was sealed at the version this client + /// predicted, and a create produces version 1, which the AAD binds. + /// + /// Null on success, or the reason the change could not be re-offered. + public async Task ReofferAsCreateAsync( + Guid vaultId, + PendingOperation pending, + CancellationToken cancellationToken) + { + ArgumentNullException.ThrowIfNull(pending); + + if (!keyring.TryGet(vaultId, out var vaultKey, out var generation) || pending.Payload is null) + { + return "This item has no usable vault key."; + } + + var local = kind.TryOpen( + pending.Payload, + vaultKey.Span, + pending.EntityId, + SyncVersions.NextVersion(pending.ExpectedVersion)); + + if (local is null) + { + return "The queued change could not be decrypted, so it could not be re-offered."; + } + + await outbox.ReviseAsync( + pending.Sequence, + SyncOperation.Upsert, + expectedVersion: null, + kind.Seal(local.Secret, vaultKey.Span, pending.EntityId, generation, itemVersion: 1), + kind.Fields(local.Secret), + ancestor: null, + cancellationToken).ConfigureAwait(false); + + return null; + } + /// /// Reconciles a pending create that the server says already exists. /// @@ -98,7 +179,7 @@ internal sealed class ItemReconciler( /// In practice this means an earlier push of the same create did land and its acknowledgement was /// lost — a timeout, a dropped connection — after which the local row may also have been edited. The /// resolution adopts the server's row as the base and re-offers the local content as an update, so - /// the newer local state wins and no duplicate host appears. A genuine id collision between two + /// the newer local state wins and no duplicate item appears. A genuine id collision between two /// clients is the other reading, and is not achievable with UUIDv7; if it happened, the server's /// values would be in the conflict log rather than gone. /// @@ -117,11 +198,14 @@ internal sealed class ItemReconciler( return; } - var (local, remoteHost, vaultKey, generation) = opened.Value; + var (local, remoteSecret, vaultKey, generation) = opened.Value; - if (local == remoteHost) + // Through the comparer, not ==. Both secrets are records with value equality, but TSecret is a + // type parameter, so == would bind to reference equality at compile time and never be true — + // turning "our own create coming back" into a conflict record on every single pass. + if (EqualityComparer.Default.Equals(local, remoteSecret)) { - // Byte-for-byte the same host: this is our own create coming back. Nothing to do but stop + // Field for field the same item: this is our own create coming back. Nothing to do but stop // trying to send it again. await outbox.CompleteAsync(pending.Sequence, cancellationToken).ConfigureAwait(false); return; @@ -133,7 +217,7 @@ internal sealed class ItemReconciler( await conflicts.RecordAsync( vaultId, - SyncEntityType.Host, + kind.EntityType, remote.EntityId, ConflictKind.FieldOverridden, ConflictDetails.Encode( @@ -167,9 +251,9 @@ internal sealed class ItemReconciler( return; } - var (local, remoteHost, vaultKey, generation) = opened.Value; + var (local, remoteSecret, vaultKey, generation) = opened.Value; - var ancestor = HostCipher.TryOpen( + var ancestor = kind.TryOpen( pending.Ancestor.Payload, vaultKey.Span, remote.EntityId, pending.Ancestor.Version); if (ancestor is null) @@ -182,17 +266,17 @@ internal sealed class ItemReconciler( return; } - var merged = HostSecretMerge.Merge(ancestor.Host, local, remoteHost); + var merged = kind.Merge(ancestor.Secret, local, remoteSecret); await ReviseAsUpdateAsync( vaultId, remote, pending, merged.Merged, vaultKey, generation, cancellationToken) .ConfigureAwait(false); - if (merged.HasConflicts) + if (merged.Conflicts.Count > 0) { await conflicts.RecordAsync( vaultId, - SyncEntityType.Host, + kind.EntityType, remote.EntityId, ConflictKind.FieldOverridden, ConflictDetails.Encode( @@ -211,7 +295,7 @@ internal sealed class ItemReconciler( /// /// The tombstone is accepted — arguing with it would conflict for ever, since a delete beats a late /// upsert on the server — and the local content is re-offered under a fresh id, labelled so the user - /// can see what happened. That is the whole of "never silently drop a host": the original goes, the + /// can see what happened. That is the whole of "never silently drop an item": the original goes, the /// work does not. /// private async Task ResurrectAsync( @@ -230,7 +314,7 @@ internal sealed class ItemReconciler( var local = pending.Payload is null ? null - : HostCipher.TryOpen( + : kind.TryOpen( pending.Payload, vaultKey.Span, remote.EntityId, @@ -244,7 +328,7 @@ internal sealed class ItemReconciler( } var restoredId = ResurrectionId.For(remote.EntityId, remote.Version); - var restored = local.Host with { Label = $"{local.Host.Label} (restored)" }; + var restored = kind.Relabel(local.Secret, $"{local.Secret.Label} (restored)"); // Queued before the original is cleared, and that order matters. These are two separate // transactions, so a process that dies between them has to fail in the direction that keeps the @@ -254,12 +338,12 @@ internal sealed class ItemReconciler( await outbox.QueueAsync( new QueuedChange( vaultId, - SyncEntityType.Host, + kind.EntityType, restoredId, SyncOperation.Upsert, ExpectedVersion: null, - HostCipher.Seal(restored, vaultKey.Span, restoredId, generation, itemVersion: 1), - HostFields.From(restored), + kind.Seal(restored, vaultKey.Span, restoredId, generation, itemVersion: 1), + kind.Fields(restored), Ancestor: null), cancellationToken).ConfigureAwait(false); @@ -268,11 +352,11 @@ internal sealed class ItemReconciler( await conflicts.RecordAsync( vaultId, - SyncEntityType.Host, + kind.EntityType, remote.EntityId, ConflictKind.RemoteDeleteResurrected, ConflictDetails.Encode( - $"'{local.Host.Label}' was deleted elsewhere while this machine had unsaved changes. " + $"'{local.Secret.Label}' was deleted elsewhere while this machine had unsaved changes. " + $"The deletion stands and the local version was kept as '{restored.Label}'."), cancellationToken).ConfigureAwait(false); @@ -291,23 +375,23 @@ internal sealed class ItemReconciler( await conflicts.RecordAsync( vaultId, - SyncEntityType.Host, + kind.EntityType, remote.EntityId, ConflictKind.LocalDeleteOverridden, ConflictDetails.Encode( - "This host was edited elsewhere after it was deleted here, so the deletion was not " - + "applied. Delete it again if that is still what you want."), + $"This {kind.Noun} was edited elsewhere after it was deleted here, so the deletion was " + + "not applied. Delete it again if that is still what you want."), cancellationToken).ConfigureAwait(false); report.DeletesAbandoned++; } - /// Re-offers a host as an update against the server's current version. + /// Re-offers an item as an update against the server's current version. private async Task ReviseAsUpdateAsync( Guid vaultId, SyncChange remote, PendingOperation pending, - HostSecret host, + TSecret secret, ReadOnlyMemory vaultKey, uint generation, CancellationToken cancellationToken) @@ -318,14 +402,14 @@ internal sealed class ItemReconciler( pending.Sequence, SyncOperation.Upsert, expectedVersion: remote.Version, - HostCipher.Seal(host, vaultKey.Span, remote.EntityId, generation, nextVersion), - HostFields.From(host), + kind.Seal(secret, vaultKey.Span, remote.EntityId, generation, nextVersion), + kind.Fields(secret), new StoredAncestor(remote.Version, remote.Payload!, remote.PlaintextFields), cancellationToken).ConfigureAwait(false); } /// Opens both sides of a collision, parking the operation if either will not open. - private async Task<(HostSecret Local, HostSecret Remote, ReadOnlyMemory VaultKey, uint Generation)?> + private async Task<(TSecret Local, TSecret Remote, ReadOnlyMemory VaultKey, uint Generation)?> OpenPairAsync( Guid vaultId, SyncChange remote, @@ -342,47 +426,63 @@ internal sealed class ItemReconciler( return null; } - var local = HostCipher.TryOpen( + var local = kind.TryOpen( pending.Payload, vaultKey.Span, remote.EntityId, SyncVersions.NextVersion(pending.ExpectedVersion)); - var remoteHost = HostCipher.TryOpen( + var remoteSecret = kind.TryOpen( remote.Payload, vaultKey.Span, remote.EntityId, remote.Version); - if (local is null || remoteHost is null) + if (local is null || remoteSecret is null) { await ParkAsync(vaultId, remote.EntityId, pending, report, cancellationToken) .ConfigureAwait(false); return null; } - if (local.IsReadOnly || remoteHost.IsReadOnly) + if (local.IsReadOnly || remoteSecret.IsReadOnly) { - // A newer client wrote fields this build cannot represent. Re-encoding would drop them, so - // the item is left alone until this client is updated. - await outbox.ParkAsync( - pending.Sequence, - "Written by a newer version of DodoSSH; update before editing this host.", - cancellationToken).ConfigureAwait(false); - - await conflicts.RecordAsync( - vaultId, - SyncEntityType.Host, - remote.EntityId, - ConflictKind.TooNewToEdit, - ConflictDetails.Encode( - "This host was written by a newer version of DodoSSH. It can be read but not " - + "merged here, because saving it would discard fields this version does not know " - + "about."), - cancellationToken).ConfigureAwait(false); - - report.Parked++; + await ParkAsTooNewAsync(vaultId, remote.EntityId, pending, report, cancellationToken) + .ConfigureAwait(false); return null; } - return (local.Host, remoteHost.Host, vaultKey, generation); + return (local.Secret, remoteSecret.Secret, vaultKey, generation); + } + + /// + /// Leaves an item alone because a newer client wrote it. + /// + /// + /// Re-encoding would drop fields this build cannot represent, so the item waits until this client is + /// updated. Parked rather than merged-and-hoped: the dropped field could be the one that matters. + /// + private async Task ParkAsTooNewAsync( + Guid vaultId, + Guid entityId, + PendingOperation pending, + SyncReportBuilder report, + CancellationToken cancellationToken) + { + await outbox.ParkAsync( + pending.Sequence, + $"Written by a newer version of DodoSSH; update before editing this {kind.Noun}.", + cancellationToken).ConfigureAwait(false); + + await conflicts.RecordAsync( + vaultId, + kind.EntityType, + entityId, + ConflictKind.TooNewToEdit, + ConflictDetails.Encode( + $"This {kind.Noun} was written by a newer version of DodoSSH. It can be read but not " + + "merged here, because saving it would discard fields this version does not know " + + "about."), + cancellationToken).ConfigureAwait(false); + + report.Parked++; } private async Task ParkAsync( @@ -394,16 +494,16 @@ internal sealed class ItemReconciler( { await outbox.ParkAsync( pending.Sequence, - "The local or the server copy of this host could not be decrypted.", + $"The local or the server copy of this {kind.Noun} could not be decrypted.", cancellationToken).ConfigureAwait(false); await conflicts.RecordAsync( vaultId, - SyncEntityType.Host, + kind.EntityType, entityId, ConflictKind.Undecryptable, ConflictDetails.Encode( - "This host could not be decrypted, so the change made here could not be merged. " + $"This {kind.Noun} could not be decrypted, so the change made here could not be merged. " + "The vault key may have been rotated, or the stored payload may not belong to this " + "item."), cancellationToken).ConfigureAwait(false); @@ -411,9 +511,22 @@ internal sealed class ItemReconciler( report.Unreadable++; report.Parked++; } +} - /// Writes the server's version of an item into the local mirror. - internal Task MirrorAsync(Guid vaultId, SyncChange change, CancellationToken cancellationToken) => +/// Writes the server's version of an item into the local mirror. +/// +/// Type-agnostic on purpose, and separate from the reconcilers for that reason: mirroring copies +/// ciphertext into a row and never decrypts, so there is nothing here for an item kind to decide. Making +/// it a method on a reconciler would have meant picking one arbitrarily, or having the engine look one up +/// for a change it can mirror without knowing anything about. +/// +internal static class ItemMirror +{ + internal static Task WriteAsync( + ItemStore items, + Guid vaultId, + SyncChange change, + CancellationToken cancellationToken) => items.SaveAsync( new StoredItem( vaultId, diff --git a/src/DodoSSH.Client.Sync/SshKeyRepository.cs b/src/DodoSSH.Client.Sync/SshKeyRepository.cs new file mode 100644 index 0000000..0ff69c4 --- /dev/null +++ b/src/DodoSSH.Client.Sync/SshKeyRepository.cs @@ -0,0 +1,49 @@ +using DodoSSH.Client.Domain; +using DodoSSH.Client.Storage; + +namespace DodoSSH.Client.Sync; + +/// +/// The SSH keys in a vault, decrypted, with unpushed local changes laid over them. +/// +/// +/// +/// Identical in shape to and identical in implementation, because both are +/// facades over the same generic repository. The only thing that differs is the item kind, and with it +/// the cipher, the merge, and the fact that a key sends the server no plaintext columns at all. +/// +/// +/// A key listed here has its private key in memory. Listing is not a cheap metadata read: it +/// decrypts every key in the vault, so the caller holds the material for as long as it holds the listing. +/// That is the same bargain makes for passwords in notes and the reason +/// SshKeySecret documents what managed strings do and do not give you — but it is worth stating +/// where the decryption actually happens, which is here. +/// +/// +public sealed class SshKeyRepository(ItemStore items, OutboxStore outbox, VaultKeyring keyring) +{ + private readonly VaultItemRepository keys = + new(SshKeyKind.Instance, items, outbox, keyring); + + /// + public Task> ListAsync( + Guid vaultId, + CancellationToken cancellationToken) => + keys.ListAsync(vaultId, cancellationToken); + + /// + public Task CreateAsync(Guid vaultId, SshKeySecret key, CancellationToken cancellationToken) => + keys.CreateAsync(vaultId, key, cancellationToken); + + /// + public Task UpdateAsync( + Guid vaultId, + Guid entityId, + SshKeySecret key, + CancellationToken cancellationToken) => + keys.UpdateAsync(vaultId, entityId, key, cancellationToken); + + /// + public Task DeleteAsync(Guid vaultId, Guid entityId, CancellationToken cancellationToken) => + keys.DeleteAsync(vaultId, entityId, cancellationToken); +} diff --git a/src/DodoSSH.Client.Sync/SyncEngine.cs b/src/DodoSSH.Client.Sync/SyncEngine.cs index b39a84a..6160169 100644 --- a/src/DodoSSH.Client.Sync/SyncEngine.cs +++ b/src/DodoSSH.Client.Sync/SyncEngine.cs @@ -33,7 +33,13 @@ public sealed class SyncEngine private readonly VaultKeyring keyring; private readonly TimeProvider clock; private readonly SyncOptions options; - private readonly ItemReconciler reconciler; + + /// + /// One per synchronised item type, built once. The keys are also the pull filter — see + /// — so a type this engine cannot reconcile is never requested, and a type it + /// can reconcile cannot be left out of the request. + /// + private readonly Dictionary reconcilers; /// Creates the engine. public SyncEngine( @@ -63,7 +69,7 @@ public sealed class SyncEngine this.clock = clock; this.options = options ?? SyncOptions.Default; - reconciler = new ItemReconciler(items, outbox, conflicts, keyring); + reconcilers = ItemKinds.Reconcilers(outbox, conflicts, keyring); } /// Runs a full pass over one vault. @@ -127,7 +133,7 @@ public sealed class SyncEngine { var response = await api.SyncPullAsync( vaultId, - new SyncPullRequest(state.Cursor, options.PullPageSize, [SyncEntityType.Host]), + new SyncPullRequest(state.Cursor, options.PullPageSize, ItemKinds.SyncedTypes), cancellationToken).ConfigureAwait(false); foreach (var change in response.Changes) @@ -187,14 +193,16 @@ public sealed class SyncEngine SyncReportBuilder report, CancellationToken cancellationToken) { - if (change.EntityType != SyncEntityType.Host) + if (!reconcilers.TryGetValue(change.EntityType, out var reconciler)) { - // Reserved in the contract but not yet syncable. Ignoring it keeps a newer server's extra - // entity types from breaking an older client's pull. + // Reserved in the contract but not yet syncable here. The pull filter already asks for only + // the types this build handles, so reaching this means a newer server sent something extra — + // and ignoring it keeps that from breaking an older client's pull. Not mirrored either: a row + // this build can never read is cache with no reader. return; } - await reconciler.MirrorAsync(vaultId, change, cancellationToken).ConfigureAwait(false); + await ItemMirror.WriteAsync(items, vaultId, change, cancellationToken).ConfigureAwait(false); var pending = await outbox .FindAsync(vaultId, change.EntityType, change.EntityId, cancellationToken) @@ -394,14 +402,29 @@ public sealed class SyncEngine return false; } + if (!reconcilers.TryGetValue(operation.EntityType, out var reconciler)) + { + // Only reachable if something queued a type this build does not synchronise, which the + // repositories cannot do. Parked rather than dropped, so the change is visible to a user + // instead of retried for ever against a path that cannot handle it. + await RejectAsync( + vaultId, + operation, + $"This version of DodoSSH cannot reconcile items of type {operation.EntityType}.", + report, + cancellationToken).ConfigureAwait(false); + + return false; + } + if (result.ServerEntity is null) { // The version check failed but the server has no such row. Re-offer it as a create. - return await RetryAsCreateAsync(vaultId, operation, report, cancellationToken) + return await RetryAsCreateAsync(vaultId, reconciler, operation, report, cancellationToken) .ConfigureAwait(false); } - await reconciler.MirrorAsync(vaultId, result.ServerEntity, cancellationToken) + await ItemMirror.WriteAsync(items, vaultId, result.ServerEntity, cancellationToken) .ConfigureAwait(false); await reconciler @@ -413,6 +436,7 @@ public sealed class SyncEngine private async Task RetryAsCreateAsync( Guid vaultId, + IItemReconciler reconciler, PendingOperation operation, SyncReportBuilder report, CancellationToken cancellationToken) @@ -424,44 +448,20 @@ public sealed class SyncEngine return false; } - if (!keyring.TryGet(vaultId, out var vaultKey, out var generation) - || operation.Payload is null) + // Re-sealing needs the item's own cipher, so the reconciler does it. A reason back means the + // change can never be sent, not that it should be retried. + var failure = await reconciler + .ReofferAsCreateAsync(vaultId, operation, cancellationToken) + .ConfigureAwait(false); + + if (failure is null) { - await RejectAsync( - vaultId, operation, "This item has no usable vault key.", report, cancellationToken) - .ConfigureAwait(false); - return false; + return true; } - var local = HostCipher.TryOpen( - operation.Payload, - vaultKey.Span, - operation.EntityId, - SyncVersions.NextVersion(operation.ExpectedVersion)); + await RejectAsync(vaultId, operation, failure, report, cancellationToken).ConfigureAwait(false); - if (local is null) - { - await RejectAsync( - vaultId, - operation, - "The queued change could not be decrypted, so it could not be re-offered.", - report, - cancellationToken).ConfigureAwait(false); - return false; - } - - // Re-sealed at version 1, because that is what the server assigns to a create and the AAD binds - // the version. - await outbox.ReviseAsync( - operation.Sequence, - SyncOperation.Upsert, - expectedVersion: null, - HostCipher.Seal(local.Host, vaultKey.Span, operation.EntityId, generation, itemVersion: 1), - HostFields.From(local.Host), - ancestor: null, - cancellationToken).ConfigureAwait(false); - - return true; + return false; } /// Parks an operation the server will never accept, and says why. diff --git a/src/DodoSSH.Client.Sync/VaultItemRepository.cs b/src/DodoSSH.Client.Sync/VaultItemRepository.cs new file mode 100644 index 0000000..ee808da --- /dev/null +++ b/src/DodoSSH.Client.Sync/VaultItemRepository.cs @@ -0,0 +1,317 @@ +using DodoSSH.Client.Domain; +using DodoSSH.Client.Storage; +using DodoSSH.Contracts; + +namespace DodoSSH.Client.Sync; + +/// One vault item as the interface should show it. +/// The item id. +/// The decrypted item. +/// +/// The server version this is based on. Zero for an item that has never been accepted. +/// +/// +/// Whether this reflects a local edit the server has not accepted yet. Worth showing: it is the +/// difference between "saved" and "saved here". +/// +/// +/// Whether the pending change was refused and is waiting on a person, so it will not retry on its own. +/// +/// +/// Whether this item was written by a newer client and so must not be edited here, because re-encoding +/// it would drop fields this build cannot represent. +/// +public sealed record VaultItem( + Guid EntityId, + TSecret Secret, + int Version, + bool HasUnsyncedChanges, + bool IsBlocked, + bool IsReadOnly) + where TSecret : class, IVaultSecret; + +/// The items of one kind in a vault, and what could not be read. +/// The readable items. +/// +/// How many items would not decrypt. Surfaced rather than swallowed: a non-zero count here after a +/// rekey is the signal that new grants are needed. +/// +public sealed record ItemListing(IReadOnlyList> Items, int Unreadable) + where TSecret : class, IVaultSecret; + +/// +/// Reading and writing one kind of vault item, as the interface sees them. +/// +/// +/// +/// The view is the mirror of the server's state with the outbox laid over it, which is what makes the +/// application feel local: an edit appears immediately and a delete disappears immediately, whether or +/// not the network is there. Nothing here talks to the server; the sync engine reconciles later. +/// +/// +/// Writes never touch the mirror. That separation is load-bearing — the mirror is the common ancestor a +/// three-way merge needs, and a repository that updated it on save would destroy the very state that +/// lets a conflict be merged instead of arbitrated. +/// +/// +/// Every read and write is scoped to , which is also what +/// keeps two kinds apart in storage: the item table is keyed on the type as well as the id, so a host and +/// a key could share an id and never see each other's rows. +/// +/// +internal sealed class VaultItemRepository( + IItemKind kind, + ItemStore items, + OutboxStore outbox, + VaultKeyring keyring) + where TSecret : class, IVaultSecret +{ + /// Reads every item of this kind the user should see in a vault. + internal async Task> ListAsync( + Guid vaultId, + CancellationToken cancellationToken) + { + if (!keyring.TryGet(vaultId, out var vaultKey, out _)) + { + throw new VaultUnreadableException(vaultId); + } + + var mirrored = await items + .ListAsync(vaultId, kind.EntityType, includeDeleted: true, cancellationToken) + .ConfigureAwait(false); + + var pending = await outbox.ListAllAsync(vaultId, cancellationToken).ConfigureAwait(false); + + var pendingByEntity = pending + .Where(operation => operation.EntityType == kind.EntityType) + .ToDictionary(operation => operation.EntityId); + + var listed = new List>(); + var unreadable = 0; + + foreach (var item in mirrored) + { + if (pendingByEntity.Remove(item.EntityId, out var local)) + { + AddPending(listed, ref unreadable, vaultKey, local); + continue; + } + + if (item.IsDeleted || item.Payload is null) + { + continue; + } + + var opened = kind.TryOpen(item.Payload, vaultKey.Span, item.EntityId, item.Version); + + if (opened is null) + { + unreadable++; + continue; + } + + listed.Add(new VaultItem( + item.EntityId, opened.Secret, item.Version, false, false, opened.IsReadOnly)); + } + + // Whatever is left has no mirror row yet: items created here and not yet accepted. + foreach (var local in pendingByEntity.Values) + { + AddPending(listed, ref unreadable, vaultKey, local); + } + + return new ItemListing(listed, unreadable); + } + + /// + /// Adds an item, returning the id it was given. + /// + /// + /// The id is generated here, not by the server, which is what lets an item be created with no network + /// at all — the point of the whole outbox. UUIDv7 so that ids sort by creation time, which keeps + /// index locality reasonable on the server side. + /// + internal async Task CreateAsync( + Guid vaultId, + TSecret secret, + CancellationToken cancellationToken) + { + ArgumentNullException.ThrowIfNull(secret); + Validate(secret); + + var (vaultKey, generation) = Key(vaultId); + var entityId = Guid.CreateVersion7(); + + await outbox.QueueAsync( + new QueuedChange( + vaultId, + kind.EntityType, + entityId, + SyncOperation.Upsert, + ExpectedVersion: null, + kind.Seal(secret, vaultKey.Span, entityId, generation, itemVersion: 1), + kind.Fields(secret), + Ancestor: null), + cancellationToken).ConfigureAwait(false); + + return entityId; + } + + /// + /// Replaces an item's contents. + /// + /// + /// The base is taken from the pending operation when there is one, and from the mirror otherwise. + /// Reading it the other way round would seal the payload at a version that does not match the + /// expectedVersion the coalesced row keeps — and because the AAD binds the item version, the + /// result would encrypt cleanly and never decrypt again. + /// + internal async Task UpdateAsync( + Guid vaultId, + Guid entityId, + TSecret secret, + CancellationToken cancellationToken) + { + ArgumentNullException.ThrowIfNull(secret); + Validate(secret); + + var (vaultKey, generation) = Key(vaultId); + + var pending = await outbox + .FindAsync(vaultId, kind.EntityType, entityId, cancellationToken) + .ConfigureAwait(false); + + var expectedVersion = pending is not null + ? pending.ExpectedVersion + : await MirrorVersionAsync(vaultId, entityId, cancellationToken).ConfigureAwait(false); + + var ancestor = pending?.Ancestor + ?? await MirrorAncestorAsync(vaultId, entityId, cancellationToken).ConfigureAwait(false); + + await outbox.QueueAsync( + new QueuedChange( + vaultId, + kind.EntityType, + entityId, + SyncOperation.Upsert, + expectedVersion, + kind.Seal( + secret, + vaultKey.Span, + entityId, + generation, + SyncVersions.NextVersion(expectedVersion)), + kind.Fields(secret), + ancestor), + cancellationToken).ConfigureAwait(false); + } + + /// + /// Deletes an item. + /// + /// + /// Queued as a tombstone, never a local removal. An offline client that simply forgot the row would + /// be unable to tell the server anything, and the item would come back on the next pull. + /// + internal async Task DeleteAsync(Guid vaultId, Guid entityId, CancellationToken cancellationToken) + { + var pending = await outbox + .FindAsync(vaultId, kind.EntityType, entityId, cancellationToken) + .ConfigureAwait(false); + + var expectedVersion = pending is not null + ? pending.ExpectedVersion + : await MirrorVersionAsync(vaultId, entityId, cancellationToken).ConfigureAwait(false); + + var ancestor = pending?.Ancestor + ?? await MirrorAncestorAsync(vaultId, entityId, cancellationToken).ConfigureAwait(false); + + await outbox.QueueAsync( + new QueuedChange( + vaultId, + kind.EntityType, + entityId, + SyncOperation.Delete, + expectedVersion, + Payload: null, + Fields: null, + ancestor), + cancellationToken).ConfigureAwait(false); + } + + private static void Validate(TSecret secret) + { + if (!secret.TryValidate(out var error)) + { + throw new ArgumentException(error, nameof(secret)); + } + } + + private void AddPending( + List> listed, + ref int unreadable, + ReadOnlyMemory vaultKey, + PendingOperation local) + { + if (local.Operation == SyncOperation.Delete) + { + // Gone as far as this machine is concerned, even before the server agrees. + return; + } + + if (local.Payload is null) + { + unreadable++; + return; + } + + var version = SyncVersions.NextVersion(local.ExpectedVersion); + var opened = kind.TryOpen(local.Payload, vaultKey.Span, local.EntityId, version); + + if (opened is null) + { + unreadable++; + return; + } + + listed.Add(new VaultItem( + local.EntityId, + opened.Secret, + local.ExpectedVersion ?? 0, + HasUnsyncedChanges: true, + local.IsParked, + opened.IsReadOnly)); + } + + private (ReadOnlyMemory VaultKey, uint Generation) Key(Guid vaultId) => + keyring.TryGet(vaultId, out var vaultKey, out var generation) + ? (vaultKey, generation) + : throw new VaultUnreadableException(vaultId); + + private async Task MirrorVersionAsync( + Guid vaultId, + Guid entityId, + CancellationToken cancellationToken) + { + var item = await items + .FindAsync(vaultId, kind.EntityType, entityId, cancellationToken) + .ConfigureAwait(false); + + // A null means the server has never seen this item, which is exactly what "create" is. + return item?.Version; + } + + private async Task MirrorAncestorAsync( + Guid vaultId, + Guid entityId, + CancellationToken cancellationToken) + { + var item = await items + .FindAsync(vaultId, kind.EntityType, entityId, cancellationToken) + .ConfigureAwait(false); + + return item?.Payload is null + ? null + : new StoredAncestor(item.Version, item.Payload, item.Fields); + } +} diff --git a/tests/DodoSSH.Client.App.Tests/FakeVaultServer.cs b/tests/DodoSSH.Client.App.Tests/FakeVaultServer.cs index 7ddb598..dbb7f3b 100644 --- a/tests/DodoSSH.Client.App.Tests/FakeVaultServer.cs +++ b/tests/DodoSSH.Client.App.Tests/FakeVaultServer.cs @@ -20,7 +20,13 @@ namespace DodoSSH.Client.App.Tests; internal sealed class FakeVaultServer : IVaultServer, IAccountApi, ISyncApi, IKeyBindingAuthorizer { private readonly List log = []; - private readonly Dictionary rows = []; + + /// + /// Keyed on the entity type as well as the id, as the server's tables and the client's cache both are. + /// Ids are UUIDv7 so a collision between two types will not happen by accident — but a fake that would + /// treat a host and a key with one id as one row is a fake that could make a real bug pass. + /// + private readonly Dictionary<(SyncEntityType Type, Guid EntityId), SyncChange> rows = []; private KeyStatement? statement; private byte[]? wrappedPrivateKey; @@ -172,7 +178,7 @@ internal sealed class FakeVaultServer : IVaultServer, IAccountApi, ISyncApi, IKe private SyncPushResult Apply(SyncPushOperation operation) { - rows.TryGetValue(operation.EntityId, out var existing); + rows.TryGetValue((operation.EntityType, operation.EntityId), out var existing); var current = existing?.Operation == SyncOperation.Delete ? null : existing; @@ -201,7 +207,7 @@ internal sealed class FakeVaultServer : IVaultServer, IAccountApi, ISyncApi, IKe : operation.PlaintextFields, UpdatedAt: DateTimeOffset.FromUnixTimeSeconds(1_750_000_000 + sequence)); - rows[operation.EntityId] = change; + rows[(operation.EntityType, operation.EntityId)] = change; log.Add(change); return new SyncPushResult( diff --git a/tests/DodoSSH.Client.App.Tests/ShellFlowTests.cs b/tests/DodoSSH.Client.App.Tests/ShellFlowTests.cs index 0053017..d6e44e3 100644 --- a/tests/DodoSSH.Client.App.Tests/ShellFlowTests.cs +++ b/tests/DodoSSH.Client.App.Tests/ShellFlowTests.cs @@ -706,10 +706,300 @@ public sealed class ShellFlowTests : IAsyncLifetime shell.HasLiveSessions.ShouldBeFalse("an ordinary lock must not warn about nothing"); } + // ---- SSH keys ---- + + [Fact] + public async Task AddingAKey_ShowsItImmediatelyAndPushesIt() + { + await UnlockedAsync(); + var vault = shell.Vault!; + + vault.NewKeyCommand.Execute(null); + vault.IsEditingKey.ShouldBeTrue(); + + vault.KeyEditorLabel = "deploy"; + vault.KeyEditorPrivateKey = PrivateKey("MATERIAL"); + vault.KeyEditorPassphrase = "hunter2"; + + await vault.SaveKeyCommand.ExecuteAsync(null); + + vault.IsEditingKey.ShouldBeFalse(); + + var row = vault.Keys.ShouldHaveSingleItem(); + row.Label.ShouldBe("deploy"); + row.Description.ShouldBe("passphrase · no public half"); + row.HasUnsyncedChanges.ShouldBeFalse("saving pushes, so nothing should still be pending"); + + vault.PendingChanges.ShouldBe(0); + server.LiveRowCount.ShouldBe(1, "a saved key should reach the server without pressing Sync"); + + // And the host list is untouched, so the two lists are genuinely separate. + vault.Hosts.ShouldBeEmpty(); + } + + [Fact] + public async Task AddingAKey_DoesNotSelectItForAuthenticationByItself() + { + // Loading or saving must not decide how the next connection authenticates. The alternative — the + // host list's habit of selecting the first row — would mean a key nobody chose being offered to a + // host, which is a credential leaving the vault by accident. + await UnlockedAsync(); + var vault = shell.Vault!; + + await AddKeyAsync(vault, "deploy"); + + vault.UseKeyAuthentication.ShouldBeFalse(); + + await vault.SyncCommand.ExecuteAsync(null); + + vault.Keys.ShouldHaveSingleItem(); + vault.SelectedKey.ShouldNotBeNull("saving selects the key it just saved, so it can be edited"); + + // But a reload that did not save anything leaves the selection alone rather than inventing one. + vault.SelectedKey = null; + await vault.SyncCommand.ExecuteAsync(null); + vault.SelectedKey.ShouldBeNull(); + } + + [Fact] + public async Task EditingAKey_RoundTripsThroughTheEditorIncludingTheMaterial() + { + await UnlockedAsync(); + var vault = shell.Vault!; + + await AddKeyAsync(vault, "deploy"); + await vault.SyncCommand.ExecuteAsync(null); + + vault.SelectedKey = vault.Keys[0]; + vault.EditSelectedKeyCommand.Execute(null); + + // The material has to come back into the editor. The codec has no partial update, so a save + // re-encodes every field — an editor that loaded a blank private key would erase it. + vault.KeyEditorLabel.ShouldBe("deploy"); + vault.KeyEditorPrivateKey.ShouldBe(PrivateKey("MATERIAL")); + vault.KeyEditorPassphrase.ShouldBe("hunter2"); + + vault.KeyEditorNotes = "rotate quarterly"; + await vault.SaveKeyCommand.ExecuteAsync(null); + await vault.SyncCommand.ExecuteAsync(null); + + var saved = vault.Keys.ShouldHaveSingleItem().Key; + saved.Notes.ShouldBe("rotate quarterly"); + saved.PrivateKeyPem.ShouldBe(PrivateKey("MATERIAL")); + saved.Passphrase.ShouldBe("hunter2"); + } + + [Fact] + public async Task CancellingTheKeyEditor_LeavesNoMaterialBehindInIt() + { + // The editor holds a private key in a bound property for as long as it is open. It cannot be wiped + // — see SshKeySecret — but it can stop being referenced, and an abandoned editor that kept the key + // would hand it to whatever opened next. + await UnlockedAsync(); + var vault = shell.Vault!; + + vault.NewKeyCommand.Execute(null); + vault.KeyEditorLabel = "deploy"; + vault.KeyEditorPrivateKey = PrivateKey("ABANDONED"); + vault.KeyEditorPassphrase = "hunter2"; + + vault.CancelKeyEditCommand.Execute(null); + + vault.IsEditingKey.ShouldBeFalse(); + vault.KeyEditorPrivateKey.ShouldBeEmpty(); + vault.KeyEditorPassphrase.ShouldBeEmpty(); + vault.KeyEditorLabel.ShouldBeEmpty(); + vault.Keys.ShouldBeEmpty(); + } + + [Fact] + public async Task APublicKeyPastedIntoThePrivateField_NamesTheActualMistake() + { + // ssh-keygen writes two files whose names differ by four characters. The message has to say which + // one to pick, because the alternative is an authentication failure at connect time that says + // nothing about the file. + await UnlockedAsync(); + var vault = shell.Vault!; + + vault.NewKeyCommand.Execute(null); + vault.KeyEditorLabel = "deploy"; + vault.KeyEditorPrivateKey = "ssh-ed25519 AAAAC3NzaC1lZDI1NTE5 deploy@laptop"; + + await vault.SaveKeyCommand.ExecuteAsync(null); + + vault.Status.ShouldContain(".pub"); + vault.Keys.ShouldBeEmpty(); + vault.IsEditingKey.ShouldBeTrue("the editor stays open so the paste can be corrected"); + } + + [Fact] + public async Task DeletingAKey_RemovesItLocallyAndPushesTheTombstone() + { + await UnlockedAsync(); + var vault = shell.Vault!; + + await AddKeyAsync(vault, "deploy"); + + vault.SelectedKey = vault.Keys[0]; + await vault.DeleteKeyCommand.ExecuteAsync(null); + + vault.Keys.ShouldBeEmpty(); + server.LiveRowCount.ShouldBe(0); + vault.PendingChanges.ShouldBe(0); + } + + /// + /// A layout invariant expressed as a state one, because it is the only form of it this repository can + /// check: nothing here loads a .axaml, and both editors are Auto rows in the same + /// 340-pixel column whose combined height exceeds the column at the window's minimum size. Two open + /// editors put the lower one's buttons past the bottom edge — the same failure this window shipped once + /// already, with the setup screens sliced and unclickable. + /// + [Fact] + public async Task OnlyOneEditorOpensAtATime_AndTheRefusalKeepsWhatWasTyped() + { + await UnlockedAsync(); + var vault = shell.Vault!; + + vault.NewKeyCommand.Execute(null); + vault.KeyEditorPrivateKey = PrivateKey("PASTED-AND-NOWHERE-ELSE"); + + vault.NewHostCommand.Execute(null); + + vault.IsEditing.ShouldBeFalse("the host editor must not open over the key editor"); + vault.IsEditingKey.ShouldBeTrue(); + vault.Status.ShouldContain("SSH key"); + + // The refusal is worth nothing if it costs the paste. + vault.KeyEditorPrivateKey.ShouldBe(PrivateKey("PASTED-AND-NOWHERE-ELSE")); + + // And it is a refusal, not a lockout. + vault.CancelKeyEditCommand.Execute(null); + vault.NewHostCommand.Execute(null); + vault.IsEditing.ShouldBeTrue(); + + // Symmetrically, with the host editor holding the column. + vault.EditorLabel = "half-typed"; + vault.NewKeyCommand.Execute(null); + + vault.IsEditingKey.ShouldBeFalse(); + vault.EditorLabel.ShouldBe("half-typed"); + vault.Status.ShouldContain("host"); + } + + [Fact] + public async Task EditingAnExistingItem_IsRefusedByTheOtherEditorToo() + { + // The Edit commands are a second door into the same column, and guarding only the Add ones would + // leave it wide open. + await UnlockedAsync(); + var vault = shell.Vault!; + + await AddHostAsync(vault, "prod-db"); + await AddKeyAsync(vault, "deploy"); + + vault.SelectedHost = vault.Hosts[0]; + vault.SelectedKey = vault.Keys[0]; + + vault.NewKeyCommand.Execute(null); + vault.EditSelectedHostCommand.Execute(null); + vault.IsEditing.ShouldBeFalse(); + + vault.CancelKeyEditCommand.Execute(null); + + vault.NewHostCommand.Execute(null); + vault.EditSelectedKeyCommand.Execute(null); + vault.IsEditingKey.ShouldBeFalse(); + } + + // ---- Authenticating with a key ---- + + [Fact] + public async Task ConnectingWithoutKeyAuthentication_UsesThePassword() + { + var vault = await ReadyToConnectAsync(); + await AddKeyAsync(vault, "deploy"); + + // A key exists and is even selected. Without the switch it must still be the password that is used. + vault.SelectedKey = vault.Keys[0]; + vault.ConnectPassword = "typed-in"; + + await ConnectWithRendererAsync(vault); + + var credential = ssh.Requests.ShouldHaveSingleItem().Credential; + credential.ShouldBeOfType().Password.ShouldBe("typed-in"); + } + + [Fact] + public async Task ConnectingWithKeyAuthentication_HandsTheSshStackTheKeyAndItsPassphrase() + { + var vault = await ReadyToConnectAsync(); + await AddKeyAsync(vault, "deploy"); + + vault.SelectedKey = vault.Keys[0]; + vault.UseKeyAuthentication = true; + vault.ConnectPassword = "should-not-be-used"; + + await ConnectWithRendererAsync(vault); + + var credential = ssh.Requests.ShouldHaveSingleItem().Credential + .ShouldBeOfType(); + + System.Text.Encoding.UTF8.GetString(credential.PrivateKeyPem) + .ShouldBe(PrivateKey("MATERIAL")); + + credential.Passphrase.ShouldBe("hunter2"); + } + + [Fact] + public async Task AKeyWithABlankPassphraseBox_IsAKeyWithNoPassphrase() + { + // Blank and absent are one state, from the editor all the way to the credential. The list has to say + // so too, because "passphrase" against a key that has none sends someone hunting for one they never + // set — and SSH.NET will not correct them: it ignores a passphrase on an unprotected key rather than + // refusing it. See docs/platform-flags.md. + var vault = await ReadyToConnectAsync(); + await AddKeyAsync(vault, "deploy", passphrase: string.Empty); + + var row = vault.Keys.ShouldHaveSingleItem(); + row.Description.ShouldStartWith("no passphrase"); + + vault.SelectedKey = row; + vault.UseKeyAuthentication = true; + + await ConnectWithRendererAsync(vault); + + ssh.Requests.ShouldHaveSingleItem().Credential + .ShouldBeOfType() + .Passphrase.ShouldBeNull(); + } + + [Fact] + public async Task KeyAuthenticationWithNoKeyChosen_RefusesRatherThanFallingBackToThePassword() + { + // The failure this prevents is silent: a user who asked for key authentication and got password + // authentication has sent a password to a host that was meant never to see one. + var vault = await ReadyToConnectAsync(); + await AddKeyAsync(vault, "deploy"); + + vault.SelectedKey = null; + vault.UseKeyAuthentication = true; + vault.ConnectPassword = "must-not-be-sent"; + + await vault.ConnectCommand.ExecuteAsync(null); + + ssh.Requests.ShouldBeEmpty("nothing should have been dialled at all"); + vault.Status.ShouldContain("key"); + } + // ---- Helpers ---- private static CancellationToken Token => TestContext.Current.CancellationToken; + /// Armoured material of a plausible shape, and deliberately not a usable key. + private static string PrivateKey(string body) => + $"-----BEGIN OPENSSH PRIVATE KEY-----\n{body}\n-----END OPENSSH PRIVATE KEY-----\n"; + private Task SignInAsync(Uri serverUrl, CancellationToken cancellationToken) { // Counted so a test can assert that a rejected URL never got this far. Reaching here means a @@ -770,6 +1060,30 @@ public sealed class ShellFlowTests : IAsyncLifetime await vault.SaveHostCommand.ExecuteAsync(null); } + private static async Task AddKeyAsync( + VaultViewModel vault, + string label, + string material = "MATERIAL", + string passphrase = "hunter2") + { + vault.NewKeyCommand.Execute(null); + vault.KeyEditorLabel = label; + vault.KeyEditorPrivateKey = PrivateKey(material); + vault.KeyEditorPassphrase = passphrase; + + await vault.SaveKeyCommand.ExecuteAsync(null); + } + + /// Connects with a renderer attached, which the data plane requires before a session opens. + private async Task ConnectWithRendererAsync(VaultViewModel vault) + { + await using var renderer = await FakeRenderer.AttachAsync(workspace, Token); + + await vault.ConnectCommand.ExecuteAsync(null); + + vault.Status.ShouldContain("Connected", Case.Insensitive); + } + /// An unlocked vault with one selected host and a renderer attached. private async Task ReadyToConnectAsync() { diff --git a/tests/DodoSSH.Client.Domain.Tests/SshKeySecretTests.cs b/tests/DodoSSH.Client.Domain.Tests/SshKeySecretTests.cs new file mode 100644 index 0000000..b7ec642 --- /dev/null +++ b/tests/DodoSSH.Client.Domain.Tests/SshKeySecretTests.cs @@ -0,0 +1,261 @@ +namespace DodoSSH.Client.Domain.Tests; + +/// +/// The SSH key record, its codec and its merge. +/// +/// +/// The codec is the point at which a private key becomes bytes and comes back, so a bug here is a key that +/// either does not survive a round trip or survives it in a form SSH.NET will not load. The sync suite +/// exercises all of this through two devices and a server, which is the right place for the reconciliation +/// rules — but it cannot say which of these types was wrong when it fails. +/// +public sealed class SshKeySecretTests +{ + private const string Material = + "-----BEGIN OPENSSH PRIVATE KEY-----\nb3BlbnNzaC1rZXktdjEA\n-----END OPENSSH PRIVATE KEY-----\n"; + + // ---- The record ---- + + [Fact] + public void AnEmptyPassphrase_IsTheSameAsNone() + { + // One spelling of one state. The two that follow are what it buys: identical keys encode + // identically, so they cannot produce a spurious merge conflict, and "is this key protected?" has a + // single reliable answer for the interface to read. + Key(passphrase: string.Empty).Passphrase.ShouldBeNull(); + Key(passphrase: null).Passphrase.ShouldBeNull(); + Key(passphrase: "hunter2").Passphrase.ShouldBe("hunter2"); + } + + [Fact] + public void APassphraseOfSpaces_IsKept() + { + // Whitespace is a legal passphrase, so this is deliberately not IsNullOrWhiteSpace. Trimming it + // would silently change the passphrase of a key someone can still open elsewhere. + Key(passphrase: " ").Passphrase.ShouldBe(" "); + } + + [Fact] + public void AnEmptyPassphraseAndNone_AreEqual() + { + // Follows from the normalisation, and it is the property the merge depends on: it compares the two + // sides for equality to decide whether anything changed at all. + Key(passphrase: string.Empty).ShouldBe(Key(passphrase: null)); + } + + [Theory] + [InlineData("", Material, "needs a name")] + [InlineData(" ", Material, "needs a name")] + [InlineData("deploy", "", "private key material")] + [InlineData("deploy", "ssh-ed25519 AAAAC3NzaC1lZDI1NTE5 deploy@laptop", ".pub")] + [InlineData("deploy", "ecdsa-sha2-nistp256 AAAAE2VjZHNh deploy@laptop", ".pub")] + [InlineData("deploy", "not a key at all", "-----BEGIN")] + public void AnInvalidKey_SaysWhatIsWrongWithIt(string label, string material, string expected) + { + var key = new SshKeySecret { Label = label, PrivateKeyPem = material }; + + key.TryValidate(out var reason).ShouldBeFalse(); + reason.ShouldNotBeNull().ShouldContain(expected); + } + + [Fact] + public void AKeyWithLeadingWhitespace_IsStillRecognised() + { + // A paste out of a terminal or an editor arrives with a newline in front of it more often than not. + var key = new SshKeySecret { Label = "deploy", PrivateKeyPem = "\n " + Material }; + + key.TryValidate(out var reason).ShouldBeTrue(reason); + } + + // ---- The codec ---- + + [Fact] + public void AKey_SurvivesARoundTrip() + { + var key = Key(passphrase: "hunter2") with + { + PublicKey = "ssh-ed25519 AAAAC3NzaC1lZDI1NTE5 deploy@laptop", + Notes = "rotate in June", + }; + + var encoded = SshKeySecretCodec.Encode(key); + + SshKeySecretCodec.TryDecode(encoded, out var document).ShouldBeTrue(); + document.ShouldNotBeNull(); + document.Key.ShouldBe(key); + document.SchemaVersion.ShouldBe(SshKeySecretCodec.CurrentSchemaVersion); + document.IsReadOnly.ShouldBeFalse(); + } + + [Fact] + public void TheMaterialIsNotReformatted() + { + // Verbatim, including the trailing newline. OpenSSH, PKCS#1 and PKCS#8 all round-trip untouched + // because nothing here parses them, and a client that normalised the armour would eventually + // normalise a format it did not fully understand. + var awkward = "-----BEGIN RSA PRIVATE KEY-----\r\nMIIBOgIBAAJB\r\n-----END RSA PRIVATE KEY-----"; + + var encoded = SshKeySecretCodec.Encode(Key() with { PrivateKeyPem = awkward }); + + SshKeySecretCodec.TryDecode(encoded, out var document).ShouldBeTrue(); + document.ShouldNotBeNull().Key.PrivateKeyPem.ShouldBe(awkward); + } + + [Fact] + public void EncodingIsDeterministic() + { + // An unchanged key must not look like a change to the sync engine, which compares ciphertext-bearing + // payloads derived from these bytes. + SshKeySecretCodec.Encode(Key(passphrase: "hunter2")) + .ShouldBe(SshKeySecretCodec.Encode(Key(passphrase: "hunter2"))); + } + + [Fact] + public void AnEmptyPassphraseIsNotWrittenAtAll() + { + // The normalisation reaches the wire: a key saved with a blank box is byte-identical to one saved + // with no passphrase, so the two cannot diverge into a spurious conflict on another machine. + SshKeySecretCodec.Encode(Key(passphrase: string.Empty)) + .ShouldBe(SshKeySecretCodec.Encode(Key(passphrase: null))); + } + + [Fact] + public void AnEmptyPassphraseWrittenByAnotherClient_DecodesAsNone() + { + var payload = System.Text.Encoding.UTF8.GetBytes( + $$""" + {"schemaVersion":1,"label":"deploy","privateKeyPem":{{System.Text.Json.JsonSerializer.Serialize(Material)}},"passphrase":""} + """); + + SshKeySecretCodec.TryDecode(payload, out var document).ShouldBeTrue(); + document.ShouldNotBeNull().Key.Passphrase.ShouldBeNull(); + } + + [Theory] + [InlineData("not json at all")] + [InlineData("{}")] + [InlineData("""{"schemaVersion":0,"label":"deploy","privateKeyPem":"x"}""")] + [InlineData("""{"schemaVersion":1,"label":"deploy"}""")] + [InlineData("""{"schemaVersion":1,"privateKeyPem":"-----BEGIN X-----"}""")] + public void APayloadThatIsNotAKey_DoesNotDecode(string json) + { + // False rather than a throw, and rather than a half-built key. A decode failure is what a rotated + // vault key and a server handing back the wrong bytes both look like from here, and neither must + // abort a sync pass. + SshKeySecretCodec + .TryDecode(System.Text.Encoding.UTF8.GetBytes(json), out var document) + .ShouldBeFalse(); + + document.ShouldBeNull(); + } + + [Fact] + public void AKeyFromANewerClient_IsReadableButNotWritable() + { + var payload = System.Text.Encoding.UTF8.GetBytes( + $$""" + {"schemaVersion":99,"label":"deploy","privateKeyPem":{{System.Text.Json.JsonSerializer.Serialize(Material)}},"certificate":"something this build has never heard of"} + """); + + SshKeySecretCodec.TryDecode(payload, out var document).ShouldBeTrue(); + + document.ShouldNotBeNull(); + document.SchemaVersion.ShouldBe(99); + document.IsReadOnly.ShouldBeTrue( + "re-encoding would drop the field, leaving a key that still decrypts and no longer works"); + } + + // ---- The merge ---- + + [Fact] + public void EachSideEditingADifferentField_KeepsBoth() + { + var ancestor = Key(); + var local = ancestor with { Label = "deploy-laptop" }; + var remote = ancestor with { Notes = "from the desktop" }; + + var merged = SshKeySecretMerge.Merge(ancestor, local, remote); + + merged.HasConflicts.ShouldBeFalse(); + merged.Merged.Label.ShouldBe("deploy-laptop"); + merged.Merged.Notes.ShouldBe("from the desktop"); + merged.Merged.PrivateKeyPem.ShouldBe(ancestor.PrivateKeyPem); + } + + [Fact] + public void BothSidesReplacingTheMaterial_ReportsTheClashWithoutQuotingEitherKey() + { + var ancestor = Key(); + var local = ancestor with { PrivateKeyPem = Armour("LAPTOP-SECRET") }; + var remote = ancestor with { PrivateKeyPem = Armour("DESKTOP-SECRET") }; + + var merged = SshKeySecretMerge.Merge(ancestor, local, remote); + + merged.HasConflicts.ShouldBeTrue(); + + var conflict = merged.Conflicts.ShouldHaveSingleItem(); + conflict.Field.ShouldBe(nameof(SshKeySecret.PrivateKeyPem)); + + // Named, so the user knows what clashed. Not quoted, because the conflict log is stored to be read + // and is deliberately kept after acknowledgement. + conflict.Kept.ShouldNotContain("LAPTOP-SECRET"); + conflict.Kept.ShouldNotContain("DESKTOP-SECRET"); + conflict.Discarded.ShouldNotBeNull().ShouldNotContain("LAPTOP-SECRET"); + conflict.Discarded.ShouldNotContain("DESKTOP-SECRET"); + + // And the surviving key is a real one — redacting the report must not redact the value. + merged.Merged.PrivateKeyPem.ShouldBeOneOf(local.PrivateKeyPem, remote.PrivateKeyPem); + } + + [Fact] + public void BothSidesChangingThePassphrase_IsAlsoRedacted() + { + var ancestor = Key(passphrase: "original"); + var local = ancestor with { Passphrase = "laptop-passphrase" }; + var remote = ancestor with { Passphrase = "desktop-passphrase" }; + + var merged = SshKeySecretMerge.Merge(ancestor, local, remote); + + var conflict = merged.Conflicts.ShouldHaveSingleItem(); + conflict.Field.ShouldBe(nameof(SshKeySecret.Passphrase)); + conflict.Kept.ShouldNotContain("passphrase-"); + conflict.Kept.ShouldNotContain("laptop-passphrase"); + conflict.Discarded.ShouldNotBeNull().ShouldNotContain("desktop-passphrase"); + } + + [Fact] + public void ALabelClash_IsShownInFull() + { + // The counterpart to the redaction: a label is not a secret, and hiding it would leave the user + // unable to tell which name was discarded. + var ancestor = Key(); + var local = ancestor with { Label = "deploy-laptop" }; + var remote = ancestor with { Label = "deploy-desktop" }; + + var merged = SshKeySecretMerge.Merge(ancestor, local, remote); + + var conflict = merged.Conflicts.ShouldHaveSingleItem(); + conflict.Field.ShouldBe(nameof(SshKeySecret.Label)); + + new[] { conflict.Kept, conflict.Discarded } + .ShouldBe(["deploy-laptop", "deploy-desktop"], ignoreOrder: true); + } + + [Fact] + public void BothSidesMakingTheSameEdit_IsNotAConflict() + { + var ancestor = Key(); + var edited = ancestor with { Notes = "rotate in June" }; + + var merged = SshKeySecretMerge.Merge(ancestor, edited, edited); + + merged.HasConflicts.ShouldBeFalse(); + merged.Merged.ShouldBe(edited); + } + + private static string Armour(string body) => + $"-----BEGIN OPENSSH PRIVATE KEY-----\n{body}\n-----END OPENSSH PRIVATE KEY-----\n"; + + private static SshKeySecret Key(string? passphrase = null) => + new() { Label = "deploy", PrivateKeyPem = Material, Passphrase = passphrase }; +} diff --git a/tests/DodoSSH.Client.Session.Tests/SessionLifecycleTests.cs b/tests/DodoSSH.Client.Session.Tests/SessionLifecycleTests.cs index 96381c5..9357f90 100644 --- a/tests/DodoSSH.Client.Session.Tests/SessionLifecycleTests.cs +++ b/tests/DodoSSH.Client.Session.Tests/SessionLifecycleTests.cs @@ -220,9 +220,9 @@ public sealed class SessionLifecycleTests : IAsyncLifetime var listing = await session.Hosts.ListAsync(session.ActiveVaultId, Token); - var host = listing.Hosts.ShouldHaveSingleItem(); + var host = listing.Items.ShouldHaveSingleItem(); host.EntityId.ShouldBe(entityId); - host.Host.Label.ShouldBe("prod-db"); + host.Secret.Label.ShouldBe("prod-db"); host.HasUnsyncedChanges.ShouldBeTrue(); (await session.PendingChangeCountAsync(Token)).ShouldBe(1); diff --git a/tests/DodoSSH.Client.Ssh.Tests/KeyAuthenticationTests.cs b/tests/DodoSSH.Client.Ssh.Tests/KeyAuthenticationTests.cs new file mode 100644 index 0000000..1668aa5 --- /dev/null +++ b/tests/DodoSSH.Client.Ssh.Tests/KeyAuthenticationTests.cs @@ -0,0 +1,117 @@ +using System.Security.Cryptography; +using System.Text; +using Renci.SshNet.Common; + +namespace DodoSSH.Client.Ssh.Tests; + +/// +/// Public-key authentication through the path the vault actually uses, against a real sshd. +/// +/// +/// +/// PtyAndResizeSpikeTests also authenticates with this fixture's key, and it does so by building +/// SSH.NET's PrivateKeyAuthenticationMethod itself — correct for a spike whose subject is the PTY, +/// and it leaves the application's own path unexercised. What runs here is what a vault-held key goes +/// through: carrying PEM bytes rather than a path, into +/// , which hands them to PrivateKeyFile as a +/// MemoryStream. That indirection is the reason a key in this product never becomes a file on disk, +/// and until now nothing established that it authenticates. +/// +/// +/// The fixture's key is RSA because there is no BCL Ed25519, and the fixture has to render the public half +/// in authorized_keys form to install it. Which algorithm it is does not matter to anything under +/// test here — the client never parses the key, it forwards it. +/// +/// +[Collection(SshCollection.Name)] +public sealed class KeyAuthenticationTests(SshServerFixture fixture) +{ + private static CancellationToken Token => TestContext.Current.CancellationToken; + + [Fact] + public async Task AKeyHeldAsBytes_AuthenticatesAndOpensAShell() + { + await using var connection = await ConnectTrustedAsync( + new SshPrivateKeyCredential(Pkcs1(fixture.ClientKey), Passphrase: null)); + + connection.IsConnected.ShouldBeTrue(); + + // Authenticated is not the same as usable: a channel has to open on the connection too. + await using var shell = await connection.OpenShellAsync(TerminalSize.Default, Token); + + shell.IsOpen.ShouldBeTrue(); + } + + [Fact] + public async Task TheSameKeyInPkcs8Armour_AlsoAuthenticates() + { + // SshKeySecret stores whatever armour it was given, verbatim, and declines to normalise it. This is + // the half of that claim which is about SSH.NET rather than the codec: the two commonest forms + // ssh-keygen and openssl produce both load without the client knowing which it has. + await using var connection = await ConnectTrustedAsync( + new SshPrivateKeyCredential(Pkcs8(fixture.ClientKey), Passphrase: null)); + + connection.IsConnected.ShouldBeTrue(); + } + + [Fact] + public async Task AKeyTheServerDoesNotKnow_FailsAsAnAuthenticationError() + { + // The specific failure worth pinning is a misreport. The host key is already trusted here, so the + // factory's gate must translate nothing and let the authentication error through — if it answered + // with SshHostKeyUnknownException instead, the user would be shown a fingerprint to approve for a + // problem that approving it cannot fix. + using var stranger = RSA.Create(2048); + + var knownHosts = await TrustedStoreAsync(); + var factory = new SshNetConnectionFactory(knownHosts); + + await Should.ThrowAsync(async () => + await factory.ConnectAsync(Request(new SshPrivateKeyCredential(Pkcs1(stranger), null)), Token)); + } + + [Fact] + public async Task APassphraseOnAnUnprotectedKey_IsIgnoredRatherThanRefused() + { + // Written expecting the opposite, and it records what SSH.NET measurably does: PrivateKeyFile + // accepts a passphrase for a key that has none, and the connection authenticates as if it had not + // been given. See docs/platform-flags.md — the consequence is that nothing downstream will catch a + // stray passphrase, so a client that wants that caught has to notice it itself, and a client that + // does not can stop worrying about the case. + await using var connection = await ConnectTrustedAsync( + new SshPrivateKeyCredential(Pkcs1(fixture.ClientKey), "a passphrase this key does not have")); + + connection.IsConnected.ShouldBeTrue(); + } + + private static byte[] Pkcs1(RSA key) => Encoding.UTF8.GetBytes(key.ExportRSAPrivateKeyPem()); + + private static byte[] Pkcs8(RSA key) => Encoding.UTF8.GetBytes(key.ExportPkcs8PrivateKeyPem()); + + private SshConnectionRequest Request(SshCredential credential) => + new(fixture.Host, fixture.Port, SshServerFixture.Username, credential); + + /// A store that already trusts the container's host key, so first contact is not the subject. + private async Task TrustedStoreAsync() + { + var knownHosts = new InMemoryKnownHostStore(); + var factory = new SshNetConnectionFactory(knownHosts); + + // Learned by being refused, which is the only way this client learns a host key. + var unknown = await Should.ThrowAsync(async () => + await factory.ConnectAsync( + Request(new SshPasswordCredential(SshServerFixture.Password)), Token)); + + await knownHosts.TrustAsync(unknown.Presentation, Token); + + return knownHosts; + } + + private async Task ConnectTrustedAsync(SshCredential credential) + { + var knownHosts = await TrustedStoreAsync(); + + return await new SshNetConnectionFactory(knownHosts) + .ConnectAsync(Request(credential), Token); + } +} diff --git a/tests/DodoSSH.Client.Sync.Tests/ConflictMatrixTests.cs b/tests/DodoSSH.Client.Sync.Tests/ConflictMatrixTests.cs index 6807ea7..74c2b1a 100644 --- a/tests/DodoSSH.Client.Sync.Tests/ConflictMatrixTests.cs +++ b/tests/DodoSSH.Client.Sync.Tests/ConflictMatrixTests.cs @@ -36,8 +36,8 @@ public sealed class ConflictMatrixTests : IAsyncLifetime await harness.SettleAsync(); var seen = await harness.Second.FindAsync(entityId); - seen.Host.Label.ShouldBe("prod-db"); - seen.Host.Notes.ShouldBe("primary"); + seen.Secret.Label.ShouldBe("prod-db"); + seen.Secret.Notes.ShouldBe("primary"); seen.HasUnsyncedChanges.ShouldBeFalse(); harness.Server.RowCount.ShouldBe(1); } @@ -49,7 +49,7 @@ public sealed class ConflictMatrixTests : IAsyncLifetime var entityId = await harness.First.CreateAsync(Host("prod-db")); var local = await harness.First.FindAsync(entityId); - local.Host.Label.ShouldBe("prod-db"); + local.Secret.Label.ShouldBe("prod-db"); local.HasUnsyncedChanges.ShouldBeTrue(); harness.Server.RowCount.ShouldBe(0); @@ -64,7 +64,7 @@ public sealed class ConflictMatrixTests : IAsyncLifetime await harness.First.UpdateAsync(entityId, Host("prod-db", notes: "rotate quarterly")); await harness.SettleAsync(); - (await harness.Second.FindAsync(entityId)).Host.Notes.ShouldBe("rotate quarterly"); + (await harness.Second.FindAsync(entityId)).Secret.Notes.ShouldBe("rotate quarterly"); (await harness.First.ConflictsAsync()).ShouldBeEmpty(); } @@ -77,7 +77,7 @@ public sealed class ConflictMatrixTests : IAsyncLifetime await harness.Second.UpdateAsync(entityId, Host("prod-db", username: "postgres")); await harness.SettleAsync(); - (await harness.First.FindAsync(entityId)).Host.Username.ShouldBe("postgres"); + (await harness.First.FindAsync(entityId)).Secret.Username.ShouldBe("postgres"); (await harness.First.ConflictsAsync()).ShouldBeEmpty(); } @@ -95,11 +95,11 @@ public sealed class ConflictMatrixTests : IAsyncLifetime await harness.SettleAsync(); - var merged = (await harness.First.FindAsync(entityId)).Host; + var merged = (await harness.First.FindAsync(entityId)).Secret; merged.Notes.ShouldBe("from the laptop"); merged.Username.ShouldBe("postgres"); - (await harness.Second.FindAsync(entityId)).Host.ShouldBe(merged); + (await harness.Second.FindAsync(entityId)).Secret.ShouldBe(merged); (await harness.First.ConflictsAsync()).ShouldBeEmpty(); } @@ -116,7 +116,7 @@ public sealed class ConflictMatrixTests : IAsyncLifetime await harness.SettleAsync(); - var merged = (await harness.First.FindAsync(entityId)).Host; + var merged = (await harness.First.FindAsync(entityId)).Secret; merged.Options.Count.ShouldBe(2); merged.Options.TryGetValue("Compression", out _).ShouldBeTrue(); merged.Options.TryGetValue("ServerAliveInterval", out _).ShouldBeTrue(); @@ -135,8 +135,8 @@ public sealed class ConflictMatrixTests : IAsyncLifetime await harness.SettleAsync(); - var first = (await harness.First.FindAsync(entityId)).Host; - var second = (await harness.Second.FindAsync(entityId)).Host; + var first = (await harness.First.FindAsync(entityId)).Secret; + var second = (await harness.Second.FindAsync(entityId)).Secret; first.ShouldBe(second); @@ -172,7 +172,7 @@ public sealed class ConflictMatrixTests : IAsyncLifetime await harness.SettleAsync(); - (await harness.First.FindAsync(entityId)).Host.Hostname.ShouldBe("db.internal"); + (await harness.First.FindAsync(entityId)).Secret.Hostname.ShouldBe("db.internal"); (await ConflictsAcrossDevicesAsync()).ShouldBeEmpty(); } @@ -187,8 +187,8 @@ public sealed class ConflictMatrixTests : IAsyncLifetime await harness.First.DeleteAsync(entityId); await harness.SettleAsync(); - (await harness.First.ListAsync()).Hosts.ShouldBeEmpty(); - (await harness.Second.ListAsync()).Hosts.ShouldBeEmpty(); + (await harness.First.ListAsync()).Items.ShouldBeEmpty(); + (await harness.Second.ListAsync()).Items.ShouldBeEmpty(); harness.Server.RowCount.ShouldBe(0); } @@ -203,8 +203,8 @@ public sealed class ConflictMatrixTests : IAsyncLifetime await harness.SettleAsync(); - (await harness.First.ListAsync()).Hosts.ShouldBeEmpty(); - (await harness.Second.ListAsync()).Hosts.ShouldBeEmpty(); + (await harness.First.ListAsync()).Items.ShouldBeEmpty(); + (await harness.Second.ListAsync()).Items.ShouldBeEmpty(); (await ConflictsAcrossDevicesAsync()).ShouldBeEmpty(); } @@ -227,16 +227,16 @@ public sealed class ConflictMatrixTests : IAsyncLifetime var listing = await harness.First.ListAsync(); - var restored = listing.Hosts.ShouldHaveSingleItem(); + var restored = listing.Items.ShouldHaveSingleItem(); restored.EntityId.ShouldNotBe(entityId); - restored.Host.Label.ShouldBe("prod-db (restored)"); - restored.Host.Notes.ShouldBe("credentials rotated, do not delete"); + restored.Secret.Label.ShouldBe("prod-db (restored)"); + restored.Secret.Notes.ShouldBe("credentials rotated, do not delete"); (await ConflictsAcrossDevicesAsync()) .ShouldContain(kind => kind == ConflictKind.RemoteDeleteResurrected); // And the other machine sees it too, so the rescue is not local-only. - (await harness.Second.ListAsync()).Hosts.ShouldHaveSingleItem() + (await harness.Second.ListAsync()).Items.ShouldHaveSingleItem() .EntityId.ShouldBe(restored.EntityId); } @@ -261,7 +261,7 @@ public sealed class ConflictMatrixTests : IAsyncLifetime await harness.SettleAsync(); - (await harness.First.ListAsync()).Hosts.Count.ShouldBe(1); + (await harness.First.ListAsync()).Items.Count.ShouldBe(1); harness.Server.RowCount.ShouldBe(1); } @@ -279,7 +279,7 @@ public sealed class ConflictMatrixTests : IAsyncLifetime await harness.SettleAsync(); var survivor = await harness.First.FindAsync(entityId); - survivor.Host.Notes.ShouldBe("still needed"); + survivor.Secret.Notes.ShouldBe("still needed"); (await ConflictsAcrossDevicesAsync()) .ShouldContain(kind => kind == ConflictKind.LocalDeleteOverridden); @@ -296,9 +296,9 @@ public sealed class ConflictMatrixTests : IAsyncLifetime await harness.SettleAsync(); - var order = (await harness.Second.ListAsync()).Hosts + var order = (await harness.Second.ListAsync()).Items .OrderBy(host => host.Version) - .ThenBy(host => host.Host.Label, StringComparer.Ordinal) + .ThenBy(host => host.Secret.Label, StringComparer.Ordinal) .Select(host => host.EntityId) .ToArray(); @@ -341,8 +341,38 @@ public sealed class ConflictMatrixTests : IAsyncLifetime await harness.SettleAsync(); harness.Server.RowCount.ShouldBe(1); - (await harness.First.FindAsync(entityId)).Host.Notes.ShouldBe("second"); - (await harness.Second.FindAsync(entityId)).Host.Notes.ShouldBe("second"); + (await harness.First.FindAsync(entityId)).Secret.Notes.ShouldBe("second"); + (await harness.Second.FindAsync(entityId)).Secret.Notes.ShouldBe("second"); + } + + [Fact] + public async Task AnUnacknowledgedCreateThatDidLand_IsDroppedQuietly() + { + // The same lost acknowledgement as above, but with no subsequent edit — which is the common case, + // since a timeout is far more likely than a timeout followed by a change. The queued create meets + // the server's own copy of itself, and the only correct answer is to stop trying to send it. In + // particular this must not be reported as a conflict: there is nothing for a person to decide, and a + // vault that produced a conflict notice every time a push timed out would train people to ignore + // them. + // + // This is the one path that compares two decrypted items for equality, and the comparison has to go + // through EqualityComparer rather than ==, because the reconciler is generic over the secret type + // and == on a type parameter is reference equality. + var entityId = await harness.First.CreateAsync(Host("prod-db", notes: "first")); + + await PushBehindTheEnginesBackAsync(entityId); + + await harness.SettleAsync(); + + (await ConflictsAcrossDevicesAsync()).ShouldBeEmpty( + "an item that came back exactly as it was sent is not something to arbitrate"); + + // And the operation is gone rather than still being offered. + (await harness.First.Outbox.TakeAsync(VaultId, 100, TestContext.Current.CancellationToken)) + .ShouldBeEmpty(); + + harness.Server.RowCount.ShouldBe(1); + (await harness.First.FindAsync(entityId)).HasUnsyncedChanges.ShouldBeFalse(); } [Fact] @@ -397,7 +427,7 @@ public sealed class ConflictMatrixTests : IAsyncLifetime await harness.First.SyncAsync(); var host = await harness.First.FindAsync(entityId); - host.Host.Notes.ShouldBe("mine"); + host.Secret.Notes.ShouldBe("mine"); host.IsBlocked.ShouldBeTrue(); host.HasUnsyncedChanges.ShouldBeTrue(); } diff --git a/tests/DodoSSH.Client.Sync.Tests/FakeVaultServer.cs b/tests/DodoSSH.Client.Sync.Tests/FakeVaultServer.cs index 2e1bfb6..c1d1ef6 100644 --- a/tests/DodoSSH.Client.Sync.Tests/FakeVaultServer.cs +++ b/tests/DodoSSH.Client.Sync.Tests/FakeVaultServer.cs @@ -11,19 +11,28 @@ namespace DodoSSH.Client.Sync.Tests; /// /// A faithful reimplementation of DodoSSH.Api.Features.Sync.SyncService's decision table: the /// version check, the tombstone-beats-late-upsert rule, idempotent deletes, operation receipts, the -/// change log, and cursors that are opaque to the client. It is not a stub that returns canned answers — -/// if it were, none of the conflict tests would mean anything, because the interesting behaviour is -/// exactly the server's refusal to apply a stale write. +/// change log, cursors that are opaque to the client, the pull filter, and the per-type rules about which +/// plaintext columns an item may carry. It is not a stub that returns canned answers — if it were, none of +/// the conflict tests would mean anything, because the interesting behaviour is exactly the server's +/// refusal to apply a stale write. /// /// /// The duplication against the real service is deliberate and is the point of the exercise: two /// independent expressions of the same rules, and SyncEndpointTests checks the other one against /// real Postgres. A shared implementation would let a misreading of the protocol pass on both sides. /// +/// +/// Rows are keyed on the entity type as well as the id, as the server's separate tables are and as the +/// client's cache is. Keying on the id alone would work for every test that uses one item type and would +/// silently make a host and a key with the same id the same row. +/// /// internal sealed class FakeVaultServer : ISyncApi { - private readonly Dictionary rows = []; + /// The item types this fake knows, mirroring the server's own registry. + private static readonly SyncEntityType[] Supported = [SyncEntityType.Host, SyncEntityType.SshKey]; + + private readonly Dictionary<(SyncEntityType Type, Guid EntityId), Row> rows = []; private readonly List log = []; private readonly Dictionary receipts = []; @@ -49,6 +58,9 @@ internal sealed class FakeVaultServer : ISyncApi /// Pushes received, so a test can prove a retry did or did not happen. internal int PushCount { get; private set; } + /// The entity-type filter of the last pull, so a test can assert what was asked for. + internal IReadOnlyList? LastPullTypes { get; private set; } + /// /// Runs just before a push is applied, so a test can land another client's write in the window /// between one client's pull and its push. That window is the whole subject of the cursor-gap test. @@ -66,7 +78,16 @@ internal sealed class FakeVaultServer : ISyncApi var after = DecodeCursor(request.Cursor); var limit = Math.Clamp(request.Limit ?? MaxPullLimit, 1, MaxPullLimit); - var page = log.Where(entry => entry.Sequence > after).Take(limit + 1).ToList(); + LastPullTypes = request.EntityTypes; + + // Empty or absent means every type, as the contract says. + var wanted = request.EntityTypes is { Count: > 0 } types ? types : null; + + var page = log + .Where(entry => entry.Sequence > after) + .Where(entry => wanted is null || wanted.Contains(entry.EntityType)) + .Take(limit + 1) + .ToList(); var hasMore = page.Count > limit; if (hasMore) @@ -109,18 +130,22 @@ internal sealed class FakeVaultServer : ISyncApi } /// Applies a change as if another client had made it. - internal int ExternalUpsert(Guid entityId, EncryptedPayload payload, SyncPlaintextFields? fields) + internal int ExternalUpsert( + Guid entityId, + EncryptedPayload payload, + SyncPlaintextFields? fields, + SyncEntityType entityType = SyncEntityType.Host) { var result = Apply(new SyncPushOperation( Guid.CreateVersion7(), - SyncEntityType.Host, + entityType, entityId, SyncOperation.Upsert, - rows.TryGetValue(entityId, out var existing) && !existing.IsDeleted + rows.TryGetValue((entityType, entityId), out var existing) && !existing.IsDeleted ? existing.Version : null, payload, - fields ?? new SyncPlaintextFields())); + fields)); if (result.Status != SyncOperationStatus.Applied) { @@ -132,13 +157,13 @@ internal sealed class FakeVaultServer : ISyncApi } /// Deletes as if another client had done it. - internal void ExternalDelete(Guid entityId) + internal void ExternalDelete(Guid entityId, SyncEntityType entityType = SyncEntityType.Host) { - var existing = rows[entityId]; + var existing = rows[(entityType, entityId)]; var result = Apply(new SyncPushOperation( Guid.CreateVersion7(), - SyncEntityType.Host, + entityType, entityId, SyncOperation.Delete, existing.Version, @@ -151,7 +176,8 @@ internal sealed class FakeVaultServer : ISyncApi } } - internal Row? Find(Guid entityId) => rows.TryGetValue(entityId, out var row) ? row : null; + internal Row? Find(Guid entityId, SyncEntityType entityType = SyncEntityType.Host) => + rows.TryGetValue((entityType, entityId), out var row) ? row : null; private long Head => log.Count == 0 ? 0 : log[^1].Sequence; @@ -159,7 +185,7 @@ internal sealed class FakeVaultServer : ISyncApi private SyncPushResult Apply(SyncPushOperation operation) { - if (operation.EntityType != SyncEntityType.Host) + if (!Supported.Contains(operation.EntityType)) { return Invalid(operation, $"Entity type {operation.EntityType} is not yet supported."); } @@ -181,7 +207,7 @@ internal sealed class FakeVaultServer : ISyncApi operation.OperationId, SyncOperationStatus.Forbidden, null, null, null, null); } - rows.TryGetValue(operation.EntityId, out var existing); + rows.TryGetValue((operation.EntityType, operation.EntityId), out var existing); return operation.Operation == SyncOperation.Delete ? ApplyDelete(operation, existing) @@ -202,14 +228,9 @@ internal sealed class FakeVaultServer : ISyncApi var fields = operation.PlaintextFields ?? new SyncPlaintextFields(); - if (!fields.RelayEnabled && (fields.Hostname is not null || fields.Port is not null)) + if (!ValidateFields(operation.EntityType, fields, out var fieldError)) { - return Invalid(operation, "An address may only be supplied when relay is enabled."); - } - - if (fields.RelayEnabled && (string.IsNullOrWhiteSpace(fields.Hostname) || fields.Port is null)) - { - return Invalid(operation, "Relay-enabled hosts require both a hostname and a port."); + return Invalid(operation, fieldError); } if (existing is null || existing.IsDeleted) @@ -233,6 +254,45 @@ internal sealed class FakeVaultServer : ISyncApi return Commit(operation, updated, SyncOperation.Upsert); } + /// The per-type rules about which plaintext columns an item may carry. + /// + /// A key's are stricter than a host's rather than merely different, and that asymmetry is the point: + /// the relay concession belongs to hosts alone, so a key arriving with an address is a client bug and + /// is refused with a reason instead of being quietly dropped. + /// + private static bool ValidateFields( + SyncEntityType entityType, + SyncPlaintextFields fields, + out string error) + { + error = string.Empty; + + if (entityType == SyncEntityType.SshKey) + { + if (fields.RelayEnabled || fields.Hostname is not null || fields.Port is not null) + { + error = "An SSH key has no relay target; relay fields may only be set on a host."; + return false; + } + + return true; + } + + if (!fields.RelayEnabled && (fields.Hostname is not null || fields.Port is not null)) + { + error = "An address may only be supplied when relay is enabled."; + return false; + } + + if (fields.RelayEnabled && (string.IsNullOrWhiteSpace(fields.Hostname) || fields.Port is null)) + { + error = "Relay-enabled hosts require both a hostname and a port."; + return false; + } + + return true; + } + private SyncPushResult Create(SyncPushOperation operation, Row? existing, SyncPlaintextFields fields) { // A tombstone beats a late upsert. The client is told so it can resurrect the item deliberately @@ -248,7 +308,9 @@ internal sealed class FakeVaultServer : ISyncApi return Conflict(operation, existing: null); } - var created = new Row(operation.EntityId, 1, 0, operation.Payload!, fields, false); + var created = new Row( + operation.EntityType, operation.EntityId, 1, 0, operation.Payload!, fields, false); + return Commit(operation, created, SyncOperation.Upsert); } @@ -293,8 +355,8 @@ internal sealed class FakeVaultServer : ISyncApi { var sequence = Head + 1; - log.Add(new LogEntry(sequence, row.EntityId, change, row.Version, Now)); - rows[row.EntityId] = row with { ChangeSequence = sequence }; + log.Add(new LogEntry(sequence, row.EntityType, row.EntityId, change, row.Version, Now)); + rows[(row.EntityType, row.EntityId)] = row with { ChangeSequence = sequence }; receipts[operation.OperationId] = new Receipt(row.Version, sequence); return new SyncPushResult( @@ -315,13 +377,13 @@ internal sealed class FakeVaultServer : ISyncApi private SyncChange Hydrate(LogEntry entry) { - var row = rows[entry.EntityId]; + var row = rows[(entry.EntityType, entry.EntityId)]; return ToChange(row, entry.Sequence, entry.Revision, entry.OccurredAt); } private SyncChange ToChange(Row row, long? sequence = null, int? version = null, DateTimeOffset? at = null) => new( - SyncEntityType.Host, + row.EntityType, row.EntityId, row.IsDeleted ? SyncOperation.Delete : SyncOperation.Upsert, version ?? row.Version, @@ -359,6 +421,7 @@ internal sealed class FakeVaultServer : ISyncApi } internal sealed record Row( + SyncEntityType EntityType, Guid EntityId, int Version, long ChangeSequence, @@ -368,6 +431,7 @@ internal sealed class FakeVaultServer : ISyncApi private sealed record LogEntry( long Sequence, + SyncEntityType EntityType, Guid EntityId, SyncOperation Operation, int Revision, diff --git a/tests/DodoSSH.Client.Sync.Tests/ItemKindsTests.cs b/tests/DodoSSH.Client.Sync.Tests/ItemKindsTests.cs new file mode 100644 index 0000000..7df4b87 --- /dev/null +++ b/tests/DodoSSH.Client.Sync.Tests/ItemKindsTests.cs @@ -0,0 +1,44 @@ +using DodoSSH.Contracts; + +namespace DodoSSH.Client.Sync.Tests; + +/// +/// The registry of synchronised item types, and the one property that has to hold about it. +/// +/// +/// Adding an item type touches a cipher, a codec, a merge, a repository and a view. The failure this pins is +/// the one that none of those would reveal: a type that can be created, encrypted, merged and listed +/// perfectly, and is never asked for in a pull — so it works on the machine that made it and exists nowhere +/// else. Deriving the filter from the registry is what prevents it; these tests are what notice if the +/// derivation stops holding. +/// +public sealed class ItemKindsTests +{ + [Fact] + public void ThePullFilterNamesEveryTypeThisBuildSynchronises() + { + ItemKinds.SyncedTypes.ShouldBe([SyncEntityType.Host, SyncEntityType.SshKey]); + } + + [Fact] + public void ThePullFilterIsNotEmpty() + { + // Stated separately from the list above because the consequence of an empty one is quiet rather + // than loud: the contract says an empty filter means every type, so the client would ask the server + // for everything it holds and then discard most of the answer in ApplyAsync. The way it could + // actually become empty is a static initialisation order slip — SyncedTypes projects Registry, and a + // reordering of the two declarations would leave it reading an unassigned array. + ItemKinds.SyncedTypes.ShouldNotBeEmpty(); + } + + [Fact] + public async Task EveryTypeInThePullFilter_HasAReconciler() + { + using var harness = await SyncHarness.CreateAsync(); + + var reconcilers = ItemKinds.Reconcilers( + harness.First.Outbox, harness.First.Conflicts, harness.First.Keyring); + + reconcilers.Keys.Order().ShouldBe(ItemKinds.SyncedTypes.Order()); + } +} diff --git a/tests/DodoSSH.Client.Sync.Tests/SshKeySyncTests.cs b/tests/DodoSSH.Client.Sync.Tests/SshKeySyncTests.cs new file mode 100644 index 0000000..515c0f2 --- /dev/null +++ b/tests/DodoSSH.Client.Sync.Tests/SshKeySyncTests.cs @@ -0,0 +1,311 @@ +using DodoSSH.Client.Storage; +using DodoSSH.Contracts; +using static DodoSSH.Client.Sync.Tests.SyncHarness; + +namespace DodoSSH.Client.Sync.Tests; + +/// +/// SSH keys through the same two-machine harness as hosts. +/// +/// +/// +/// Deliberately not a copy of with the nouns changed. The six collision +/// outcomes are decided by ItemReconciler<TSecret>, which is one implementation shared by both +/// item types, so re-asserting all of them per type would test the same code twice and grow with every type +/// added. What is tested here is what is genuinely different about a key: its cipher, its merge, the fact +/// that it hands the server nothing in plaintext, that the reconciler's messages call it a key, and that its +/// items cannot be confused with a host's. +/// +/// +/// The two collision cases that are repeated — resurrection and an abandoned delete — are here +/// because they are the two that touch key material: one re-seals it under a new id, the other decides +/// whether a private key survives a deletion. +/// +/// +public sealed class SshKeySyncTests : IAsyncLifetime +{ + private SyncHarness harness = null!; + + private static CancellationToken Token => TestContext.Current.CancellationToken; + + /// + public async ValueTask InitializeAsync() => harness = await CreateAsync(); + + /// + public ValueTask DisposeAsync() + { + harness.Dispose(); + return ValueTask.CompletedTask; + } + + // ---- The uncontested paths ---- + + [Fact] + public async Task AKeyCreatedOnOneMachine_ReachesTheOther() + { + var entityId = await harness.First.CreateKeyAsync( + Key("deploy", material: "LAPTOP-MATERIAL", passphrase: "hunter2", notes: "rotate in June")); + + await harness.SettleAsync(); + + var seen = await harness.Second.FindKeyAsync(entityId); + + seen.Secret.Label.ShouldBe("deploy"); + seen.Secret.PrivateKeyPem.ShouldContain("LAPTOP-MATERIAL"); + seen.Secret.Passphrase.ShouldBe("hunter2"); + seen.Secret.Notes.ShouldBe("rotate in June"); + seen.HasUnsyncedChanges.ShouldBeFalse(); + } + + [Fact] + public async Task ThePull_AsksForKeysAsWellAsHosts() + { + await harness.First.SyncAsync(); + + var asked = harness.Server.LastPullTypes.ShouldNotBeNull(); + + asked.ShouldContain(SyncEntityType.Host); + asked.ShouldContain( + SyncEntityType.SshKey, + "a type the engine can reconcile but never requests would work in every unit test and never " + + "sync"); + } + + [Fact] + public async Task AHostAndAKeyQueuedTogether_BothGoInOnePush() + { + var hostId = await harness.First.CreateAsync(Host("prod-db")); + var keyId = await harness.First.CreateKeyAsync(Key("deploy")); + + await harness.First.SyncAsync(); + + harness.Server.PushCount.ShouldBe(1, "one outbox, one batch, whatever the item types in it"); + + await harness.Second.SyncAsync(); + + (await harness.Second.FindAsync(hostId)).Secret.Label.ShouldBe("prod-db"); + (await harness.Second.FindKeyAsync(keyId)).Secret.Label.ShouldBe("deploy"); + } + + [Fact] + public async Task AKeyList_DoesNotShowHosts() + { + await harness.First.CreateAsync(Host("prod-db")); + await harness.First.CreateKeyAsync(Key("deploy")); + + await harness.SettleAsync(); + + (await harness.Second.ListKeysAsync()).Items.ShouldHaveSingleItem() + .Secret.Label.ShouldBe("deploy"); + + (await harness.Second.ListAsync()).Items.ShouldHaveSingleItem() + .Secret.Label.ShouldBe("prod-db"); + } + + // ---- What the server is told ---- + + [Fact] + public async Task AKeyHandsTheServerNothingInPlaintext() + { + // The public half is supplied, which is the case where a fingerprint could have been derived and + // sent. The server has a column for one and would accept it; this client does not fill it, because a + // fingerprint is a stable identifier for a key pair and nothing in the product reads the column. + var entityId = await harness.First.CreateKeyAsync( + Key("deploy", publicKey: "ssh-ed25519 AAAAC3Nz deploy@laptop")); + + var queued = await harness.First.Outbox + .FindAsync(VaultId, SyncEntityType.SshKey, entityId, Token); + + queued.ShouldNotBeNull(); + queued.Fields.ShouldBeNull("a key sends no plaintext fields at all, not an empty set of them"); + + await harness.SettleAsync(); + + var row = harness.Server.Find(entityId, SyncEntityType.SshKey).ShouldNotBeNull(); + + row.Fields.PublicKeyFingerprint.ShouldBeNull(); + row.Fields.RelayEnabled.ShouldBeFalse(); + row.Fields.Hostname.ShouldBeNull(); + } + + [Fact] + public async Task AHostAndAKeyWithTheSameId_AreDifferentItems() + { + // Not reachable through the repositories, which mint UUIDv7s, so it is arranged on the server. The + // point is that two things defend the separation independently: the item table is keyed on the type + // as well as the id, and the payload's AAD binds a resource type — so neither payload can be opened + // as the other even if a lookup did confuse them. + var sharedId = Guid.CreateVersion7(); + + harness.First.Keyring.TryGet(VaultId, out var vaultKey, out var generation).ShouldBeTrue(); + + harness.Server.ExternalUpsert( + sharedId, + HostCipher.Seal(Host("prod-db"), vaultKey.Span, sharedId, generation, itemVersion: 1), + new SyncPlaintextFields(), + SyncEntityType.Host); + + harness.Server.ExternalUpsert( + sharedId, + SshKeyCipher.Seal(Key("deploy"), vaultKey.Span, sharedId, generation, itemVersion: 1), + null, + SyncEntityType.SshKey); + + await harness.Second.SyncAsync(); + + var hosts = await harness.Second.ListAsync(); + var keys = await harness.Second.ListKeysAsync(); + + hosts.Items.ShouldHaveSingleItem().Secret.Label.ShouldBe("prod-db"); + keys.Items.ShouldHaveSingleItem().Secret.Label.ShouldBe("deploy"); + + hosts.Unreadable.ShouldBe(0); + keys.Unreadable.ShouldBe(0); + } + + // ---- Merging ---- + + [Fact] + public async Task TwoMachinesEditingDifferentFieldsOfAKey_BothSurvive() + { + var entityId = await harness.First.CreateKeyAsync(Key("deploy", material: "SHARED")); + await harness.SettleAsync(); + + await harness.First.UpdateKeyAsync(entityId, Key("deploy-laptop", material: "SHARED")); + await harness.Second.UpdateKeyAsync( + entityId, Key("deploy", material: "SHARED", notes: "from the desktop")); + + await harness.SettleAsync(); + + var first = (await harness.First.FindKeyAsync(entityId)).Secret; + var second = (await harness.Second.FindKeyAsync(entityId)).Secret; + + first.ShouldBe(second); + first.Label.ShouldBe("deploy-laptop"); + first.Notes.ShouldBe("from the desktop"); + first.PrivateKeyPem.ShouldContain("SHARED"); + + (await ConflictKindsAsync()).ShouldBeEmpty(); + } + + [Fact] + public async Task BothReplacedTheKeyMaterial_NeitherKeyIsWrittenToTheConflictLog() + { + // The reason SshKeySecretMerge redacts. A host conflict records the value that lost so the user can + // put it back; doing that with a private key would copy a secret into a log that is designed to be + // read and is deliberately kept after acknowledgement. + var entityId = await harness.First.CreateKeyAsync(Key("deploy", material: "ORIGINAL")); + await harness.SettleAsync(); + + await harness.First.UpdateKeyAsync(entityId, Key("deploy", material: "LAPTOP-SECRET")); + await harness.Second.UpdateKeyAsync(entityId, Key("deploy", material: "DESKTOP-SECRET")); + + await harness.SettleAsync(); + + (await ConflictKindsAsync()).ShouldContain(kind => kind == ConflictKind.FieldOverridden); + + var details = await ConflictDetailsAsync(); + + details.ShouldContain( + detail => detail.Contains("PrivateKeyPem", StringComparison.Ordinal), + "the user still has to be told which field clashed"); + + foreach (var detail in details) + { + detail.ShouldNotContain("LAPTOP-SECRET"); + detail.ShouldNotContain("DESKTOP-SECRET"); + } + } + + [Fact] + public async Task BothChangedThePassphrase_ThePassphraseIsNotInTheLogEither() + { + var entityId = await harness.First.CreateKeyAsync(Key("deploy", passphrase: "original")); + await harness.SettleAsync(); + + await harness.First.UpdateKeyAsync(entityId, Key("deploy", passphrase: "laptop-passphrase")); + await harness.Second.UpdateKeyAsync(entityId, Key("deploy", passphrase: "desktop-passphrase")); + + await harness.SettleAsync(); + + foreach (var detail in await ConflictDetailsAsync()) + { + detail.ShouldNotContain("laptop-passphrase"); + detail.ShouldNotContain("desktop-passphrase"); + } + } + + // ---- Deletes, where key material can be lost ---- + + [Fact] + public async Task AKeyDeletedElsewhereWhileEditedHere_KeepsTheMaterialUnderANewName() + { + var entityId = await harness.First.CreateKeyAsync(Key("deploy", material: "IRREPLACEABLE")); + await harness.SettleAsync(); + + await harness.First.DeleteKeyAsync(entityId); + await harness.Second.UpdateKeyAsync( + entityId, Key("deploy", material: "IRREPLACEABLE", notes: "still in use")); + + await harness.SettleAsync(); + + var restored = (await harness.First.ListKeysAsync()).Items.ShouldHaveSingleItem(); + + restored.EntityId.ShouldNotBe(entityId); + restored.Secret.Label.ShouldBe("deploy (restored)"); + restored.Secret.Notes.ShouldBe("still in use"); + restored.Secret.PrivateKeyPem.ShouldContain( + "IRREPLACEABLE", Case.Sensitive, "a resurrection that lost the key would rescue nothing"); + + (await ConflictKindsAsync()).ShouldContain(kind => kind == ConflictKind.RemoteDeleteResurrected); + } + + [Fact] + public async Task AKeyEditedElsewhereAfterBeingDeletedHere_SurvivesAndIsCalledAKey() + { + var entityId = await harness.First.CreateKeyAsync(Key("deploy")); + await harness.SettleAsync(); + + await harness.First.UpdateKeyAsync(entityId, Key("deploy", notes: "still in use")); + await harness.Second.DeleteKeyAsync(entityId); + + await harness.SettleAsync(); + + (await harness.First.FindKeyAsync(entityId)).Secret.Notes.ShouldBe("still in use"); + + (await ConflictKindsAsync()).ShouldContain(kind => kind == ConflictKind.LocalDeleteOverridden); + + // The noun matters: someone told a host was edited elsewhere goes looking in the host list. + var details = await ConflictDetailsAsync(); + + details.ShouldContain(detail => detail.Contains("This SSH key was edited", StringComparison.Ordinal)); + details.ShouldNotContain(detail => detail.Contains("This host was edited", StringComparison.Ordinal)); + } + + // ---- Helpers ---- + + private async Task> ConflictKindsAsync() + { + var first = await harness.First.ConflictsAsync(); + var second = await harness.Second.ConflictsAsync(); + + return [.. first.Concat(second).Select(conflict => conflict.Kind)]; + } + + /// + /// The raw stored detail, decoded as text rather than parsed. The redaction claims are claims about what + /// is absent from the bytes, and reading through the JSON model would only prove the material + /// is absent from the fields the model happens to name. + /// + private async Task> ConflictDetailsAsync() + { + var first = await harness.First.ConflictsAsync(); + var second = await harness.Second.ConflictsAsync(); + + return + [ + .. first.Concat(second) + .Select(conflict => System.Text.Encoding.UTF8.GetString(conflict.Detail)), + ]; + } +} diff --git a/tests/DodoSSH.Client.Sync.Tests/SyncEngineTests.cs b/tests/DodoSSH.Client.Sync.Tests/SyncEngineTests.cs index 17b23ab..d61d64e 100644 --- a/tests/DodoSSH.Client.Sync.Tests/SyncEngineTests.cs +++ b/tests/DodoSSH.Client.Sync.Tests/SyncEngineTests.cs @@ -31,7 +31,7 @@ public sealed class SyncEngineTests var report = await harness.Second.SyncAsync(); report.Pulled.ShouldBe(7); - (await harness.Second.ListAsync()).Hosts.Count.ShouldBe(7); + (await harness.Second.ListAsync()).Items.Count.ShouldBe(7); } [Fact] @@ -133,7 +133,7 @@ public sealed class SyncEngineTests report.ServerTimeSkewMs.ShouldBeGreaterThan(2 * 60 * 60 * 1000); // The item still round-trips, so nothing downstream depended on the timestamp. - (await harness.First.FindAsync(entityId)).Host.Label.ShouldBe("prod-db"); + (await harness.First.FindAsync(entityId)).Secret.Label.ShouldBe("prod-db"); } [Fact] diff --git a/tests/DodoSSH.Client.Sync.Tests/SyncHarness.cs b/tests/DodoSSH.Client.Sync.Tests/SyncHarness.cs index 0c5f493..4f86781 100644 --- a/tests/DodoSSH.Client.Sync.Tests/SyncHarness.cs +++ b/tests/DodoSSH.Client.Sync.Tests/SyncHarness.cs @@ -37,6 +37,7 @@ internal sealed class SyncDevice : IDisposable SyncState = new SyncStateStore(factory); Conflicts = new ConflictStore(factory, protector, TimeProvider.System); Hosts = new HostRepository(Items, Outbox, keyring); + SshKeys = new SshKeyRepository(Items, Outbox, keyring); Engine = new SyncEngine( server, Items, Outbox, SyncState, Conflicts, keyring, TimeProvider.System, options); @@ -56,6 +57,8 @@ internal sealed class SyncDevice : IDisposable internal HostRepository Hosts { get; } + internal SshKeyRepository SshKeys { get; } + internal SyncEngine Engine { get; } internal static async Task CreateAsync( @@ -90,21 +93,21 @@ internal sealed class SyncDevice : IDisposable internal Task SyncAsync() => Engine.SyncAsync(SyncHarness.VaultId, TestContext.Current.CancellationToken); - internal Task ListAsync() => + internal Task> ListAsync() => Hosts.ListAsync(SyncHarness.VaultId, TestContext.Current.CancellationToken); internal async Task> HostsSortedAsync() { var listing = await ListAsync(); - return [.. listing.Hosts.Select(h => h.Host).OrderBy(h => h.Label, StringComparer.Ordinal)]; + return [.. listing.Items.Select(h => h.Secret).OrderBy(h => h.Label, StringComparer.Ordinal)]; } - internal async Task FindAsync(Guid entityId) + internal async Task> FindAsync(Guid entityId) { var listing = await ListAsync(); - return listing.Hosts.SingleOrDefault(host => host.EntityId == entityId) + return listing.Items.SingleOrDefault(host => host.EntityId == entityId) ?? throw new InvalidOperationException($"{Name} cannot see host {entityId}."); } @@ -117,6 +120,28 @@ internal sealed class SyncDevice : IDisposable internal Task DeleteAsync(Guid entityId) => Hosts.DeleteAsync(SyncHarness.VaultId, entityId, TestContext.Current.CancellationToken); + // ---- The same four operations, on SSH keys ---- + + internal Task> ListKeysAsync() => + SshKeys.ListAsync(SyncHarness.VaultId, TestContext.Current.CancellationToken); + + internal async Task> FindKeyAsync(Guid entityId) + { + var listing = await ListKeysAsync(); + + return listing.Items.SingleOrDefault(key => key.EntityId == entityId) + ?? throw new InvalidOperationException($"{Name} cannot see key {entityId}."); + } + + internal Task CreateKeyAsync(SshKeySecret key) => + SshKeys.CreateAsync(SyncHarness.VaultId, key, TestContext.Current.CancellationToken); + + internal Task UpdateKeyAsync(Guid entityId, SshKeySecret key) => + SshKeys.UpdateAsync(SyncHarness.VaultId, entityId, key, TestContext.Current.CancellationToken); + + internal Task DeleteKeyAsync(Guid entityId) => + SshKeys.DeleteAsync(SyncHarness.VaultId, entityId, TestContext.Current.CancellationToken); + internal Task> ConflictsAsync() => Conflicts.ListAsync(SyncHarness.VaultId, false, TestContext.Current.CancellationToken); @@ -249,4 +274,29 @@ internal sealed class SyncHarness : IDisposable : HostOptions.Create(options.Select(o => new HostOption(o.Name, o.Value))), RelayEnabled = relayEnabled, }; + + /// + /// An SSH key whose material is a plausible shape but not a real key. + /// + /// + /// Not a valid Ed25519 key, and deliberately so: nothing in the sync path parses the material, and a + /// real private key checked into a test repository is a real private key on the internet regardless of + /// what it was used for. SshKeySecret.TryValidate only requires the armour, and the tests that + /// need a key SSH.NET can actually load live in DodoSSH.Client.Ssh.Tests where one is generated. + /// + internal static SshKeySecret Key( + string label, + string material = "deploy-key-material", + string? passphrase = null, + string? publicKey = null, + string? notes = null) => + new() + { + Label = label, + PrivateKeyPem = $"-----BEGIN OPENSSH PRIVATE KEY-----\n{material}\n" + + "-----END OPENSSH PRIVATE KEY-----\n", + Passphrase = passphrase, + PublicKey = publicKey, + Notes = notes, + }; } diff --git a/tests/DodoSSH.SystemTests/M1VerticalSliceTests.cs b/tests/DodoSSH.SystemTests/M1VerticalSliceTests.cs index 7033903..b092012 100644 --- a/tests/DodoSSH.SystemTests/M1VerticalSliceTests.cs +++ b/tests/DodoSSH.SystemTests/M1VerticalSliceTests.cs @@ -74,13 +74,22 @@ public sealed class M1VerticalSliceTests(DevStack stack) : IClassFixture + /// The relay concession is the host's alone. A key has no address to resolve, so the server is given + /// nothing at all about it — not even the public-key fingerprint its own schema has a column for, which + /// it would have accepted. A fingerprint is not secret but it is a stable identifier for a key pair, and + /// nothing in the product reads that column; see the note on SshKeyKind.Fields. + /// + private static async Task AssertTheServerLearnsNothingAboutTheKeyAsync( + ServerConnection connection, + Guid keyId) + { + var vaultId = (await connection.Account.GetMeAsync(Token)).Vaults.Single().VaultId; + + var page = await connection.Sync.SyncPullAsync( + vaultId, new SyncPullRequest(null, 100, [SyncEntityType.SshKey]), Token); + + // Asked for keys, and got only keys back — so the filter the client relies on is honoured by the + // real endpoint and not merely by the in-memory one the unit suites use. + page.Changes.ShouldAllBe(change => change.EntityType == SyncEntityType.SshKey); + + var change = page.Changes.Single(c => c.EntityId == keyId); + + change.PlaintextFields.ShouldBeNull( + "a key gives the server no plaintext columns, so it hydrates to nothing at all"); + + change.Payload.ShouldNotBeNull(); + change.Payload.WrappedDataKey.ShouldNotBeEmpty(); + change.Payload.DataKeyId.ShouldNotBe(Guid.Empty); + } + private async Task ReadOnASecondMachineAsync( ServerConnection connection, HostSecret expected, - Guid entityId) + Guid entityId, + SshKeySecret expectedKey, + Guid keyId) { using var desktopCache = await OpenCacheAsync(); @@ -178,19 +218,30 @@ public sealed class M1VerticalSliceTests(DevStack stack) : IClassFixture + /// Armour of the right shape around material that is not a key. The shell at the end of this test + /// authenticates with a password, because what is under test here is the key's journey through the vault + /// — and a real private key committed to a repository is a real private key on the internet whatever it + /// was for. That SSH.NET can authenticate with a key delivered this way, as bytes rather than a file, is + /// established against a real sshd in KeyAuthenticationTests. + /// + private static SshKeySecret BuildKey() => + new() + { + Label = "e2e-deploy-key", + PrivateKeyPem = + "-----BEGIN OPENSSH PRIVATE KEY-----\nnot-a-real-key\n-----END OPENSSH PRIVATE KEY-----\n", + Passphrase = "an end to end key passphrase", + PublicKey = "ssh-ed25519 AAAAC3NzaC1lZDI1NTE5 e2e@dodossh", + Notes = "created by the end-to-end slice", + }; + private async Task OpenCacheAsync() { var directory = Path.Combine(Path.GetTempPath(), $"dodossh-e2e-{Guid.CreateVersion7():N}");