diff --git a/README.md b/README.md index fd34f05..72534e8 100644 --- a/README.md +++ b/README.md @@ -128,14 +128,14 @@ skipped and cannot be recovered from the server. You can then add a host and ope admin console is at `http://localhost:18080` (`admin` / `admin`). 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. +private key, then edit a host and pick that key from its **key** dropdown. From then on that host +authenticates with it — on every machine, since the choice travels inside the host's encrypted payload — +and its password box disappears. 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. +yet (keys are — passwords are not); 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. ### End-to-end verification @@ -203,10 +203,15 @@ off-Windows. Known gaps in the client, stated rather than implied by the interface: credentials are not a synced 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. + synced, and binding one to a host is the way to connect without typing anything; 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. + + Binding a key introduced the first payload schema version bump, and it is worth knowing how it behaves: + a host is written at the *lowest* schema version that can represent it, so only hosts that actually bind + a key are written at version 2 and become read-only on an older build. Hosts that do not are still + written at version 1, byte-identically to before the field existed — which is what keeps upgrading one + machine from making a team's whole vault uneditable everywhere else. - **M2 — full personal vault**, robust sync, relay. - **M3 — teams**, sharing, ACLs. - **M4 — hardening and ops**, packaging, self-hosting guide. diff --git a/src/DodoSSH.Client.App/ViewModels/VaultViewModel.cs b/src/DodoSSH.Client.App/ViewModels/VaultViewModel.cs index d6cbafd..2effe80 100644 --- a/src/DodoSSH.Client.App/ViewModels/VaultViewModel.cs +++ b/src/DodoSSH.Client.App/ViewModels/VaultViewModel.cs @@ -36,10 +36,42 @@ internal sealed class HostRowViewModel(VaultItem host) internal bool IsReadOnly => host.IsReadOnly; + /// How this host authenticates, in one word. + /// + /// Worth a word in the list because the two behave differently at the moment of connecting: one needs + /// the password box filled in and the other does not, and a user staring at an empty password box on a + /// key-authenticated host has no other way to know it is not needed. + /// + internal string Authentication => host.Secret.SshKeyId is null ? "password" : "key"; + /// A short marker for the row, so the list says what it knows without a tooltip. internal string Badge => ItemBadge.For(host.IsBlocked, host.IsReadOnly, host.HasUnsyncedChanges); } +/// An entry in the host editor's key picker. +/// The key's item id, or null for password authentication. +/// What to show. +/// +/// A sentinel entry rather than a nullable selection, because a ComboBox with nothing selected and a +/// ComboBox meaning "no key" look identical and are not the same thing — the first is a host whose binding +/// has not been decided, the second is a decision. +/// +internal sealed record SshKeyChoice(Guid? EntityId, string Label) +{ + /// The "use a password" entry, always first. + internal static SshKeyChoice None { get; } = new(null, "Password (no key)"); + + /// + /// A stand-in for a key the host names and the vault no longer has. + /// + /// + /// Kept in the list, and kept selected, so that opening a host to change its port does not silently + /// convert it to password authentication on save. The id is preserved; only the label admits the + /// problem. + /// + internal static SshKeyChoice Missing(Guid entityId) => new(entityId, "(a key that is no longer here)"); +} + /// One SSH key, as a row in the list. /// /// @@ -147,10 +179,10 @@ internal sealed class ConflictRowViewModel(ConflictNotice notice) /// 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. +/// A key belongs to a host. Each host names the key it authenticates with, or none, and that choice +/// is a field in its encrypted payload — so it follows the host to every machine rather than being made +/// again per connection. The cost is a payload schema version, paid only by hosts that actually bind a key: +/// see HostSecretCodec.CurrentSchemaVersion. /// /// internal sealed partial class VaultViewModel( @@ -227,6 +259,20 @@ internal sealed partial class VaultViewModel( [ObservableProperty] private bool editorRelayEnabled; + /// + /// What the key picker offers: password, then every key in the vault. + /// + /// + /// Rebuilt when the editor opens rather than kept in step with the key list. A background sync could + /// pull a new key while a host is being edited, and having the picker's contents change under the user + /// mid-edit is worse than the list being a minute stale — the two editors cannot be open at once, so + /// the only way to add a key is to close this one anyway. + /// + internal ObservableCollection EditorKeyChoices { get; } = []; + + [ObservableProperty] + private SshKeyChoice? editorSelectedKey; + /// The item being edited, or null when creating. private Guid? editingEntityId; @@ -269,16 +315,8 @@ 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; + /// Whether the selected host authenticates with a key, so the password box can say so. + internal bool SelectedHostUsesAKey => SelectedHost?.Host.SshKeyId is not null; [ObservableProperty] private HostKeyPresentation? pendingHostKey; @@ -545,6 +583,7 @@ internal sealed partial class VaultViewModel( EditorUsername = string.Empty; EditorNotes = string.Empty; EditorRelayEnabled = false; + BuildKeyChoices(boundKeyId: null); IsEditing = true; Status = "Adding a host."; } @@ -573,6 +612,7 @@ internal sealed partial class VaultViewModel( EditorUsername = row.Host.Username ?? string.Empty; EditorNotes = row.Host.Notes ?? string.Empty; EditorRelayEnabled = row.Host.RelayEnabled; + BuildKeyChoices(row.Host.SshKeyId); IsEditing = true; Status = $"Editing {row.Label}."; } @@ -796,12 +836,13 @@ internal sealed partial class VaultViewModel( return; } - if (UseKeyAuthentication && SelectedKey is null) + if (TryBuildCredential(row.Host) is not { } credential) { - // 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."; + // Refused rather than quietly falling back to the password box. A host set up for key-only + // access that silently starts offering a password is the failure worth ruling out — the user + // asked for one thing and got another, and the host is the last place that would say so. + Status = $"'{row.Label}' authenticates with an SSH key that is not in this vault any more. " + + "Edit the host to choose another key, or set it back to a password."; return; } @@ -810,7 +851,7 @@ internal sealed partial class VaultViewModel( await RunAsync( $"Connecting to {row.Label}…", - () => OpenSessionAsync(row, cancellationToken)).ConfigureAwait(true); + () => OpenSessionAsync(row, credential, cancellationToken)).ConfigureAwait(true); } /// Pins the offered host key and retries. @@ -891,7 +932,10 @@ internal sealed partial class VaultViewModel( /// output at a terminal that was never created. That wait is bounded and takes this command's token, so /// a renderer that never arrives ends as a message rather than as a window stuck on "Connecting…". /// - private async Task OpenSessionAsync(HostRowViewModel row, CancellationToken cancellationToken) + private async Task OpenSessionAsync( + HostRowViewModel row, + SshCredential credential, + CancellationToken cancellationToken) { try { @@ -901,7 +945,7 @@ internal sealed partial class VaultViewModel( row.Host.Hostname, row.Host.Port, row.Host.Username!, - BuildCredential()); + credential); await workspace .OpenSessionAsync(request, TerminalSize.Default, cancellationToken) @@ -937,27 +981,36 @@ internal sealed partial class VaultViewModel( } /// - /// How the next connection authenticates. + /// How this host authenticates, or null when it names a key the vault does not have. /// /// + /// /// 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. + /// of a vault, and SshKeySecret says why. It is passed straight through with no empty-to-null + /// check, because SshKeySecret.Passphrase cannot hold an empty string. + /// /// - /// 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. + /// Null is a refusal, not a fallback, and the caller must treat it as one. A dangling reference means a + /// key was deleted on another machine — plausible, and no reason to start sending a password to a host + /// somebody deliberately set up not to accept one. /// /// - private SshCredential BuildCredential() + private SshCredential? TryBuildCredential(HostSecret host) { - if (!UseKeyAuthentication || SelectedKey is not { } row) + if (host.SshKeyId is not { } keyId) { return new SshPasswordCredential(ConnectPassword); } + if (Keys.FirstOrDefault(row => row.EntityId == keyId) is not { } key) + { + return null; + } + return new SshPrivateKeyCredential( - Encoding.UTF8.GetBytes(row.Key.PrivateKeyPem), row.Key.Passphrase); + Encoding.UTF8.GetBytes(key.Key.PrivateKeyPem), key.Key.Passphrase); } private HostSecret BuildHost() => @@ -969,8 +1022,42 @@ internal sealed partial class VaultViewModel( Username = string.IsNullOrWhiteSpace(EditorUsername) ? null : EditorUsername.Trim(), Notes = string.IsNullOrWhiteSpace(EditorNotes) ? null : EditorNotes, RelayEnabled = EditorRelayEnabled, + + // Whatever the picker holds, including the id of a key that has gone missing. Reading it from + // the picker rather than carrying the original through is what lets a binding be removed at all, + // and preserving a missing id is what stops an unrelated edit removing one by accident. + SshKeyId = EditorSelectedKey?.EntityId, }; + /// + /// Fills the key picker, keeping whatever the host is currently bound to selectable. + /// + /// The key the host names, or null for password authentication. + /// + /// A bound key that is no longer in the vault gets a placeholder entry rather than being dropped. Without + /// one the picker would open on "Password (no key)", and someone editing the host's port would convert it + /// to password authentication by saving — which is the quiet version of the failure the connect path + /// refuses outright. + /// + private void BuildKeyChoices(Guid? boundKeyId) + { + EditorKeyChoices.Clear(); + EditorKeyChoices.Add(SshKeyChoice.None); + + foreach (var key in Keys) + { + EditorKeyChoices.Add(new SshKeyChoice(key.EntityId, key.Label)); + } + + if (boundKeyId is { } bound && EditorKeyChoices.All(choice => choice.EntityId != bound)) + { + EditorKeyChoices.Add(SshKeyChoice.Missing(bound)); + } + + EditorSelectedKey = EditorKeyChoices.FirstOrDefault(choice => choice.EntityId == boundKeyId) + ?? SshKeyChoice.None; + } + /// /// 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 @@ -1129,6 +1216,9 @@ internal sealed partial class VaultViewModel( } } + partial void OnSelectedHostChanged(HostRowViewModel? value) => + OnPropertyChanged(nameof(SelectedHostUsesAKey)); + partial void OnPendingHostKeyChanged(HostKeyPresentation? value) => OnPropertyChanged(nameof(HasPendingHostKey)); diff --git a/src/DodoSSH.Client.App/Views/MainWindow.axaml b/src/DodoSSH.Client.App/Views/MainWindow.axaml index bd413df..9c033da 100644 --- a/src/DodoSSH.Client.App/Views/MainWindow.axaml +++ b/src/DodoSSH.Client.App/Views/MainWindow.axaml @@ -111,8 +111,16 @@ VerticalAlignment="Center" /> - + + + + + @@ -128,6 +136,21 @@ + + + + + + + + + IsVisible="{Binding !Vault.SelectedHostUsesAKey}" /> - - +