From 211eba066646539bab1e5044b06629f81a802533 Mon Sep 17 00:00:00 2001 From: Jaap-Jan de Wit | DodoTech Date: Thu, 30 Jul 2026 11:00:39 +0200 Subject: [PATCH] Keep host key trust in the vault, and make it withdrawable MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A fingerprint approved once is now approved on every machine and survives a restart, because host key trust is a vault item type rather than a dictionary that dies with the process. InMemoryKnownHostStore was what shipped, so the user was asked to verify a fingerprint on every single connection — which is the gap most likely to train somebody to click through the one warning that actually matters. A warning that appears when nothing is wrong teaches that nothing is ever wrong. The fourth item type, and like the third it cost no sync logic: a row, an EF configuration, a migration, a server kind; a secret, a codec, a merge, a cipher, a repository facade and a session property. One row in the client registry. The reconciler, the mirror, the repository, the outbox and the pull filter were not touched. SyncEntityType.KnownHostKey and AadResourceType.KnownHostKey were already reserved, so neither the contract nor docs/crypto.md changed. One item per (host, port, algorithm), because a server legitimately offers several host keys and which one gets negotiated is not ours to predict. Pinning per endpoint would make an algorithm change indistinguishable from an attack. The label is derived rather than stored, which is the one place this type departs from the other three. A user never names a pin — there is nothing to name it after but the three fields it already has — and a stored label is a second copy of data that can disagree with the first after a merge. Relabel returns the secret unchanged, and says why. The store answers the handshake without touching the disk. SshNetConnectionFactory calls FindAsync from inside SSH.NET's synchronous HostKeyReceived event, over .GetAwaiter().GetResult(), which cannot be avoided; doing SQLite I/O plus an AEAD open per lookup there would put the handshake behind the cache. So decryption happens in OpenAsync and RefreshAsync — on unlock and after each sync pass, exactly where the host and key lists already reload — and FindAsync is a dictionary read under a lock with no await inside it. That snapshot is where the one real bug in this change lived. Install originally merged the live pins over the freshly loaded snapshot, to protect a TrustAsync that had landed while the read was in flight. It would also have resurrected every pin the user had just forgotten, and stopped a withdrawal made on another machine from ever taking effect — the store would have healed the deletion back into existence on every refresh. Replacing wholesale and discarding the read instead is correct because writes are the rare case: every write bumps a generation counter, and a refresh whose stamp is stale throws itself away rather than winning. Nothing found this but reading the method again; it is the kind of mistake that passes every test written before it, because the test that catches it is the one the bug tells you to write. Forgetting is new, and persistence is what made it mandatory rather than convenient. A mismatch is a hard refusal with no way to continue — deliberately, and that stays — so pinning a key permanently is also a way to make a legitimately rebuilt server permanently unreachable. Before this change the pin died at exit and the problem solved itself; now it does not. ForgetAsync drops every algorithm for an endpoint, and it is reachable from the host editor rather than from the warning. Putting it on the mismatch banner would have made it two clicks from "this may be an attack" to "connect anyway", which is the affordance the hard refusal exists to deny. The banner already promised the key could be removed in the host's settings; that promise is now true and points at the button. Trust recorded on another machine becomes visible at the next sync pass, not immediately, and that is a decision rather than an oversight. The failure it produces is a first-contact prompt for a host a colleague approved a minute ago: answerable, and self-correcting on the next pass. The opposite trade — polling the vault on the handshake thread to close a one-minute window — buys nothing and costs the property above. The dangerous direction is not reachable at all: a pin recorded here enters the snapshot as part of recording it, so a refresh can never discard a local trust decision. The server learns nothing, and this is the item type where the temptation was real. A plaintext host column would let a known-hosts screen sort and page without decrypting anything, and it would hand the operator the map of every user's estate — assembled, as these things are, out of facts that are each individually harmless. A host row concedes an address only when relay is switched on and the database refuses to store one otherwise (ADR 0004); there is no equivalent excuse here. The table has no column to put one in, and the EF configuration says so where somebody adding it would be standing. Two things about the migration in this commit are worth knowing, because both came out of getting it wrong. It was hand-written first, including its .Designer.cs, and that version is not what is here. Verifying it turned up something that had been quietly assumed: Migration_AppliedCleanly_WithNoPendingModelChanges does not check the model snapshot. It asserts that migrations applied and that none are pending, which a wrong snapshot satisfies perfectly — the snapshot only matters as the diff base for the *next* migrations add, so an incorrect one passes the whole suite and corrupts the following migration instead. The real check is to generate a throwaway migration and confirm its Up and Down come out empty. They did, and the generated designer was byte-identical to the transcribed one across all 1255 lines, so the hand-written work was in fact correct. Then dotnet ef migrations remove --no-build deleted the wrong migration. With --no-build the tool reads the previously compiled assembly rather than the files on disk, and the probe had just changed which migration was last, so it removed AddKnownHostKeyItem and reverted the snapshot. That turned out to leave exactly the right diff base, so the migration here is EF's own output rather than a transcription — a better outcome than the one that was interrupted, arrived at by accident. Never pass --no-build to migrations remove. Mutation tested, all three sabotages detected: dropping the algorithm from KnownHostIdentity.For, merging instead of replacing in Install, and pointing KnownHostKeyCipher at PortForward — which is what a cast from the wire enum's 10 would silently produce. Each is caught both by an assertion about the mechanism and by a behavioural test that never mentions it; the resource-type sabotage is caught by the table from d10a38d and nothing else, which is what that table is for. The end-to-end slice now approves the real sshd's host key through the vault, pushes it, and reads it back on the second simulated machine — including a check that the server learned no address, and that the second machine answers null for an algorithm never offered. 845 tests green. Zero warnings, dotnet format clean. Three things are deliberately not fixed. A tombstone queued over a create that was never pushed is refused by the server as Invalid and parked; that is pre-existing for all four item types, and the fix belongs in VaultItemRepository.DeleteAsync rather than here. Deleting a host, or changing its address, orphans its pins — both are correct as trust decisions, since a pin describes an endpoint and not a bookmark, but nothing surfaces the leftovers. And there is no interface listing pins at all: trust is created at the connect prompt and withdrawn in the host editor. A known-hosts list is where the orphans would become visible, and it wants the vault column rework first, for the same reason the credential editor does. --- README.md | 37 +- src/DodoSSH.Api/Features/Sync/ItemKinds.cs | 110 +- src/DodoSSH.Client.App/App.axaml.cs | 9 +- .../ViewModels/MainWindowViewModel.cs | 36 +- .../ViewModels/VaultViewModel.cs | 127 +- src/DodoSSH.Client.App/Views/MainWindow.axaml | 23 +- src/DodoSSH.Client.App/packages.lock.json | 1 + src/DodoSSH.Client.Domain/KnownHostSecret.cs | 106 ++ .../KnownHostSecretCodec.cs | 121 ++ .../KnownHostSecretMerge.cs | 121 ++ .../DodoSSH.Client.Session.csproj | 8 + .../VaultKnownHostStore.cs | 364 +++++ src/DodoSSH.Client.Session/VaultSession.cs | 9 + src/DodoSSH.Client.Session/packages.lock.json | 22 + src/DodoSSH.Client.Ssh/HostKeyTrust.cs | 105 +- src/DodoSSH.Client.Sync/ItemKinds.cs | 91 +- src/DodoSSH.Client.Sync/KnownHostKeyCipher.cs | 131 ++ .../KnownHostRepository.cs | 51 + src/DodoSSH.Domain/Hosts.cs | 82 ++ .../HostAndSyncConfigurations.cs | 44 +- src/DodoSSH.Infrastructure/DodoDbContext.cs | 3 + ...0730075732_AddKnownHostKeyItem.Designer.cs | 1255 +++++++++++++++++ .../20260730075732_AddKnownHostKeyItem.cs | 70 + .../Migrations/DodoDbContextModelSnapshot.cs | 93 ++ tests/DodoSSH.Api.Tests/SyncEndpointTests.cs | 92 ++ .../ShellFlowTests.cs | 142 +- .../packages.lock.json | 1 + .../KnownHostSecretTests.cs | 208 +++ .../VaultKnownHostStoreTests.cs | 351 +++++ .../packages.lock.json | 23 + .../KnownHostStoreTests.cs | 139 ++ .../AadResourceTypeTests.cs | 44 + .../FakeVaultServer.cs | 95 +- .../ItemKindsTests.cs | 7 +- .../KnownHostSyncTests.cs | 201 +++ .../DodoSSH.Client.Sync.Tests/SyncHarness.cs | 45 + .../M1VerticalSliceTests.cs | 90 +- tests/DodoSSH.SystemTests/packages.lock.json | 1 + 38 files changed, 4363 insertions(+), 95 deletions(-) create mode 100644 src/DodoSSH.Client.Domain/KnownHostSecret.cs create mode 100644 src/DodoSSH.Client.Domain/KnownHostSecretCodec.cs create mode 100644 src/DodoSSH.Client.Domain/KnownHostSecretMerge.cs create mode 100644 src/DodoSSH.Client.Session/VaultKnownHostStore.cs create mode 100644 src/DodoSSH.Client.Sync/KnownHostKeyCipher.cs create mode 100644 src/DodoSSH.Client.Sync/KnownHostRepository.cs create mode 100644 src/DodoSSH.Infrastructure/Migrations/20260730075732_AddKnownHostKeyItem.Designer.cs create mode 100644 src/DodoSSH.Infrastructure/Migrations/20260730075732_AddKnownHostKeyItem.cs create mode 100644 tests/DodoSSH.Client.Domain.Tests/KnownHostSecretTests.cs create mode 100644 tests/DodoSSH.Client.Session.Tests/VaultKnownHostStoreTests.cs create mode 100644 tests/DodoSSH.Client.Ssh.Tests/KnownHostStoreTests.cs create mode 100644 tests/DodoSSH.Client.Sync.Tests/KnownHostSyncTests.cs diff --git a/README.md b/README.md index 72534e8..9da831c 100644 --- a/README.md +++ b/README.md @@ -132,10 +132,16 @@ private key, then edit a host and pick that key from its **key** dropdown. From 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); 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. +The first time you connect to a host you are asked to check its key fingerprint. That decision is stored in +the vault, so it is asked once per host rather than once per launch and it reaches your other machines with +the next sync. If a server is legitimately rebuilt and offers a new key, the connection is refused outright +with no way to continue from the warning — edit the host and choose **Forget host key**, which is deliberately +somewhere you have to go on purpose. + +Two 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 nothing in the interface can create a vault +credential yet (they do sync — there is just no editor for one); and unlock asks for the passphrase on every +launch, because no device key is registered. ### End-to-end verification @@ -148,9 +154,9 @@ dotnet test tests/DodoSSH.SystemTests It brings up PostgreSQL, Keycloak and an OpenSSH server in containers, applies the committed migrations, starts the API as a child process out of its own build output, and then drives the real client: sign in -through Keycloak, enroll, unlock, create a host, sync it, read it back on a second simulated machine, -unlock again with no network, and open a shell on the `sshd`. Roughly 25 seconds once the images are -pulled. +through Keycloak, enroll, unlock, create an SSH key and a host bound to it, sync them, open a shell on the +`sshd` and approve its host key at the real first-contact refusal, then read all three back on a second +simulated machine and unlock again with no network. Roughly 25 seconds once the images are pulled. What makes it worth its weight is that it consumes the artefacts that ship — the realm file from `deploy/keycloak`, the EF migrations, the API's own `appsettings` — rather than a fixture written to match @@ -198,14 +204,19 @@ off-Windows. they are unverified. *Verified end to end:* `tests/DodoSSH.SystemTests` drives the whole slice against a real Keycloak, a real API, a real PostgreSQL and a real `sshd` — sign-in, the identity-provider key binding, enrollment, - offline unlock, a host through the vault to a second machine, and an interactive shell. See + offline unlock, a host and an SSH key through the vault to a second machine, an interactive shell, and the + host key approved at that shell's prompt reaching the second machine as well. See [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 password authentication still asks for the password each time — SSH keys *are* - 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. + Known gaps in the client, stated rather than implied by the interface: nothing in the interface can create + a vault credential yet, so password authentication still asks for the password each time — SSH keys *are* + editable, and binding one to a host is the way to connect without typing anything; and no device key is + registered, so the passphrase is needed on every launch until the OS keystore is wired. + + Host key trust *is* in the vault, which is what makes trust-on-first-use worth having: a fingerprint + approved on one machine is approved on all of them and survives a restart, and the server cannot drop a + pin to force a fresh first-use decision without the item visibly going missing. A changed host key stays a + hard refusal with no way past it; withdrawing a pin is a separate, deliberate act in the host's editor. 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 diff --git a/src/DodoSSH.Api/Features/Sync/ItemKinds.cs b/src/DodoSSH.Api/Features/Sync/ItemKinds.cs index b25f2b6..9f2af5b 100644 --- a/src/DodoSSH.Api/Features/Sync/ItemKinds.cs +++ b/src/DodoSSH.Api/Features/Sync/ItemKinds.cs @@ -68,8 +68,10 @@ internal interface IItemKind internal static class ItemKinds { private static readonly Dictionary Supported = - new[] { (IItemKind)new HostKind(), new SshKeyKind(), new CredentialKind() } - .ToDictionary(kind => kind.WireType); + new[] + { + (IItemKind)new HostKind(), new SshKeyKind(), new CredentialKind(), new KnownHostKeyKind(), + }.ToDictionary(kind => kind.WireType); /// The kind for a wire type, or null when this server does not synchronise it yet. /// @@ -295,9 +297,9 @@ internal sealed class SshKeyKind : IItemKind /// Credentials: an envelope and nothing else. /// -/// The strictest of the three kinds about plaintext, and the reason is not symmetry. A key at least has a -/// fingerprint that is public by nature; a password has no part that is safe to expose, so this kind accepts -/// no plaintext fields at all and hydrates none. +/// As strict as a kind gets about plaintext — is the other one — and the +/// reason is not symmetry. A key at least has a fingerprint that is public by nature; a password has no part +/// that is safe to expose, so this kind accepts no plaintext fields at all and hydrates none. /// internal sealed class CredentialKind : IItemKind { @@ -387,3 +389,101 @@ internal sealed class CredentialKind : IItemKind /// public SyncPlaintextFields? Hydrate(IVaultItem item) => null; } + +/// Known host keys: an envelope and nothing else. +/// +/// As strict as about plaintext, for a reason that is about aggregation rather +/// than secrecy. A fingerprint is published so it can be compared and an address may already sit in a +/// relay-enabled host's columns — but the set of endpoints one user has approved is a map of their estate, +/// and this server has nothing to do with it. +/// +internal sealed class KnownHostKeyKind : IItemKind +{ + /// + public SyncEntityType WireType => SyncEntityType.KnownHostKey; + + /// + public ChangeEntityType ChangeType => ChangeEntityType.KnownHostKey; + + /// + public async Task FindAsync( + DodoDbContext database, + Guid id, + CancellationToken cancellationToken) => + await database.KnownHostKeys.SingleOrDefaultAsync(k => k.Id == id, cancellationToken) + .ConfigureAwait(false); + + /// + public async Task> LoadAsync( + DodoDbContext database, + Guid vaultId, + Guid[] ids, + CancellationToken cancellationToken) + { + var rows = await database.KnownHostKeys + .Where(k => k.VaultId == vaultId && ids.Contains(k.Id)) + .ToListAsync(cancellationToken) + .ConfigureAwait(false); + + return rows.ToDictionary(row => row.Id, row => (IVaultItem)row); + } + + /// + public IVaultItem Add(DodoDbContext database, Guid id, Guid vaultId) + { + var knownHost = new VaultKnownHostKey { Id = id, VaultId = vaultId }; + + database.KnownHostKeys.Add(knownHost); + + return knownHost; + } + + /// + /// Refuses every plaintext field there is. + /// + /// + /// The relay fields are refused although this type is the one that does hold an address, and + /// that is the point: the address belongs in the ciphertext. A client sending it here is either confused + /// or trying to get the server to keep a list it has no business keeping, and either way it should be + /// told rather than have the value quietly dropped. + /// + /// + public bool ValidateFields(SyncPlaintextFields fields, out string error) + { + ArgumentNullException.ThrowIfNull(fields); + + error = string.Empty; + + if (fields.RelayEnabled || fields.Hostname is not null || fields.Port is not null) + { + error = "A known host key is not something the server dials; its address stays encrypted."; + return false; + } + + if (fields.PublicKeyFingerprint is not null) + { + error = "A known host key's fingerprint stays inside its payload."; + return false; + } + + return true; + } + + /// Nothing to copy: this type has no plaintext columns to copy anything into. + /// + public void ApplyFields(IVaultItem item, SyncPlaintextFields fields) + { + } + + /// + public void ClearFieldsOnDelete(IVaultItem item) + { + } + + /// + /// Always null, as for a credential: this type has no plaintext columns, so there is nothing a pull could + /// hydrate even in principle. + /// + /// + public SyncPlaintextFields? Hydrate(IVaultItem item) => null; +} diff --git a/src/DodoSSH.Client.App/App.axaml.cs b/src/DodoSSH.Client.App/App.axaml.cs index 4edf364..425a2ac 100644 --- a/src/DodoSSH.Client.App/App.axaml.cs +++ b/src/DodoSSH.Client.App/App.axaml.cs @@ -52,10 +52,11 @@ internal sealed partial class DodoSshApp : Application var paths = ClientPaths.Default; var caches = ClientCacheFactory.ForFile(paths.CacheFile); - // Known hosts are still in memory. The plan puts them in the vault as a synced entity so trust - // follows the user to every device, and SyncEntityType.KnownHostKey is reserved for it — but that - // entity type is not synced yet, so trust currently lasts one session. - var knownHosts = new InMemoryKnownHostStore(); + // Known hosts live in the vault, so trust survives a restart and follows the user to every device. + // Composed here, once, because the connection factory below needs it now and outlives every unlock; + // the vault behind it is attached and detached as one is opened and locked. See VaultKnownHostStore + // for why the handshake is answered from a snapshot rather than by reading the vault per lookup. + var knownHosts = new VaultKnownHostStore(); var workspace = new TerminalWorkspace( new AvaloniaTerminalAssetProvider(), diff --git a/src/DodoSSH.Client.App/ViewModels/MainWindowViewModel.cs b/src/DodoSSH.Client.App/ViewModels/MainWindowViewModel.cs index f1af6f4..ae549c8 100644 --- a/src/DodoSSH.Client.App/ViewModels/MainWindowViewModel.cs +++ b/src/DodoSSH.Client.App/ViewModels/MainWindowViewModel.cs @@ -3,7 +3,6 @@ using CommunityToolkit.Mvvm.ComponentModel; using CommunityToolkit.Mvvm.Input; using DodoSSH.Client.Auth; using DodoSSH.Client.Session; -using DodoSSH.Client.Ssh; using DodoSSH.Client.Storage; using DodoSSH.Client.Terminal; using DodoSSH.Crypto; @@ -60,7 +59,15 @@ internal sealed partial class MainWindowViewModel : ObservableObject, IAsyncDisp private readonly ClientPaths paths; private readonly ClientCacheFactory caches; private readonly TerminalWorkspace workspace; - private readonly IKnownHostStore knownHosts; + + /// + /// The concrete store rather than IKnownHostStore, because this is where its lifecycle belongs: + /// the interface is what the handshake asks, and opening a vault behind it, refreshing it and forgetting + /// it are this state machine's business. The same instance was handed to the connection factory when the + /// application was composed. + /// + private readonly VaultKnownHostStore knownHosts; + private readonly SignInHandler signIn; private readonly TimeProvider clock; private readonly Argon2Profile? passphraseProfile; @@ -83,7 +90,7 @@ internal sealed partial class MainWindowViewModel : ObservableObject, IAsyncDisp ClientPaths paths, ClientCacheFactory caches, TerminalWorkspace workspace, - IKnownHostStore knownHosts, + VaultKnownHostStore knownHosts, SignInHandler signIn, TimeProvider clock, Argon2Profile? passphraseProfile = null) @@ -355,6 +362,22 @@ internal sealed partial class MainWindowViewModel : ObservableObject, IAsyncDisp Passphrase = string.Empty; + // Before the vault view model, so the first connection after an unlock already knows which + // host keys this user has approved. Reading them is one listing; doing it here rather than + // lazily is what keeps it off the SSH handshake thread. + try + { + await knownHosts.OpenAsync(outcome.Session!, cancellationToken).ConfigureAwait(true); + } + catch + { + // Nothing owns the session yet, so nothing else would ever dispose it — and an + // undisposed session is vault keys left in memory for the life of the process, which is + // precisely what unlocking must be able to undo. + await outcome.Session!.DisposeAsync().ConfigureAwait(true); + throw; + } + Vault = new VaultViewModel(outcome.Session!, workspace, knownHosts, () => connection); State = ShellState.Unlocked; @@ -398,6 +421,11 @@ internal sealed partial class MainWindowViewModel : ObservableObject, IAsyncDisp [RelayCommand] private async Task LockAsync() { + // First, and before the session it read from goes: a synchronisation pass may be in flight, and it + // ends by refreshing this store. Detaching now makes that refresh a no-op instead of a set of pins + // reappearing behind a lock screen. + knownHosts.Close(); + if (Vault is { } open) { Vault = null; @@ -420,6 +448,8 @@ internal sealed partial class MainWindowViewModel : ObservableObject, IAsyncDisp disposed = true; + knownHosts.Close(); + if (Vault is { } open) { await open.DisposeAsync().ConfigureAwait(false); diff --git a/src/DodoSSH.Client.App/ViewModels/VaultViewModel.cs b/src/DodoSSH.Client.App/ViewModels/VaultViewModel.cs index 2effe80..2c96901 100644 --- a/src/DodoSSH.Client.App/ViewModels/VaultViewModel.cs +++ b/src/DodoSSH.Client.App/ViewModels/VaultViewModel.cs @@ -173,10 +173,12 @@ internal sealed class ConflictRowViewModel(ConflictNotice notice) /// A background pass is deliberately quieter than the button: see . /// /// -/// 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. +/// Keys and host key trust are in the vault; passwords are not yet. An SSH key is a synced item, so +/// it is stored once and available on every machine, and so is a known host key — approving a fingerprint +/// here approves it on every device and survives a restart. Credentials are synced as well, but nothing in +/// this interface can create one, so password authentication still asks 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 belongs to a host. Each host names the key it authenticates with, or none, and that choice @@ -188,7 +190,7 @@ internal sealed class ConflictRowViewModel(ConflictNotice notice) internal sealed partial class VaultViewModel( VaultSession session, TerminalWorkspace workspace, - IKnownHostStore knownHosts, + VaultKnownHostStore knownHosts, Func connection) : ObservableObject, IAsyncDisposable { /// @@ -276,6 +278,17 @@ internal sealed partial class VaultViewModel( /// The item being edited, or null when creating. private Guid? editingEntityId; + /// + /// Whether the editor is showing a host that could have a pinned key to forget. + /// + /// + /// Read by the editor to hide the button while a host is being created, where there is nothing to + /// withdraw yet. It does not claim a pin exists — answering that would mean a second question to the + /// known-host store for a button's visibility, and the command already says plainly when there was + /// nothing to forget. + /// + internal bool CanForgetHostKey => IsEditing && editingEntityId is not null; + // ---- 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. @@ -310,7 +323,8 @@ internal sealed partial class VaultViewModel( // ---- Connecting ---- /// - /// Typed per connection because credentials are not a synced entity type yet. Never persisted. + /// Typed per connection because nothing in this interface can create a vault credential yet — not because + /// the vault cannot hold one. Never persisted. /// [ObservableProperty] private string connectPassword = string.Empty; @@ -531,6 +545,11 @@ internal sealed partial class VaultViewModel( await ReloadAsync(cancellationToken).ConfigureAwait(true); + // Host key trust arrives with the rest of the vault, and the store the SSH handshake asks holds a + // snapshot rather than reading per lookup — so a pass that pulled a pin has to hand it over here, + // or a host a colleague approved stays a first-contact prompt until the next unlock. + await knownHosts.RefreshAsync(cancellationToken).ConfigureAwait(true); + return report; } finally @@ -854,7 +873,15 @@ internal sealed partial class VaultViewModel( () => OpenSessionAsync(row, credential, cancellationToken)).ConfigureAwait(true); } - /// Pins the offered host key and retries. + /// + /// Pins the offered host key and retries. + /// + /// + /// The pin goes into the vault, so this writes to the local cache and queues a change for every other + /// machine — which is why the write is guarded and the connection is only retried once it has landed. + /// It used to be a dictionary insert that could not fail; reporting a failed write as a failed + /// connection would send the user looking at the host. + /// [RelayCommand] private async Task TrustHostKeyAsync(CancellationToken cancellationToken) { @@ -863,11 +890,28 @@ internal sealed partial class VaultViewModel( return; } - await knownHosts.TrustAsync(presentation, cancellationToken).ConfigureAwait(true); + try + { + await knownHosts.TrustAsync(presentation, cancellationToken).ConfigureAwait(true); + } + catch (OperationCanceledException) + { + Status = "Cancelled."; + return; + } + catch (Exception exception) + { + Status = $"The host key could not be stored, so nothing was connected: {exception.Message}"; + return; + } PendingHostKey = null; await ConnectAsync(cancellationToken).ConfigureAwait(true); + + // After connecting, not before. A pin is worth pushing straight away — the same host on another + // machine should not ask again — but not at the cost of delaying the connection the user asked for. + await AutoSyncAsync(cancellationToken).ConfigureAwait(true); } /// Dismisses the trust prompt without pinning anything. @@ -878,6 +922,65 @@ internal sealed partial class VaultViewModel( Status = "The host key was not trusted, so nothing was connected."; } + /// + /// Withdraws trust from every key pinned for the host being edited. + /// + /// + /// + /// The counterpart to trust that now outlives the process, and the reason it exists at all: a mismatch is + /// a hard refusal with no way past it, so a server that is legitimately rebuilt would be unreachable for + /// ever without this. It is deliberately here — in the host's editor, reached by choosing to edit + /// a host — and not on the refusal itself. A "forget this key" button next to the warning is the same + /// button as "continue anyway" with two clicks instead of one. + /// + /// + /// Applies to the host's saved address rather than whatever the editor's boxes currently hold. + /// The pin belongs to the endpoint that was actually dialled, and someone halfway through retyping a + /// hostname has not moved it yet. + /// + /// + [RelayCommand] + private async Task ForgetHostKeyAsync(CancellationToken cancellationToken) + { + if (editingEntityId is not { } entityId + || Hosts.FirstOrDefault(row => row.EntityId == entityId) is not { } row) + { + return; + } + + var address = row.Host.Hostname; + var port = row.Host.Port; + + await RunAsync( + $"Forgetting the pinned host key for {address}…", + async () => + { + var forgotten = await knownHosts + .ForgetAsync(address, port, cancellationToken) + .ConfigureAwait(true); + + // The refusal that sent the user here is about a pin that no longer exists. + HostKeyMismatch = null; + + PendingChanges = await session + .PendingChangeCountAsync(cancellationToken) + .ConfigureAwait(true); + + if (forgotten == 0) + { + Status = $"Nothing was pinned for {address}:{port}."; + return; + } + + Status = $"Forgot the pinned host key for {address}:{port}. The next connection will " + + "ask you to check its fingerprint again."; + }).ConfigureAwait(true); + + // Pushed straight away, as a save or a deletion is: a withdrawal that stayed on this machine would + // leave the other ones refusing to connect to a server that has been rebuilt. + await AutoSyncAsync(cancellationToken).ConfigureAwait(true); + } + /// /// Marks every shown conflict as seen. /// @@ -1219,6 +1322,14 @@ internal sealed partial class VaultViewModel( partial void OnSelectedHostChanged(HostRowViewModel? value) => OnPropertyChanged(nameof(SelectedHostUsesAKey)); + /// + /// The editing id is set before this flips in every path that opens the editor, and cleared after it + /// flips back in every path that closes one, so this notification always observes the pair in a + /// consistent state. + /// + partial void OnIsEditingChanged(bool value) => + OnPropertyChanged(nameof(CanForgetHostKey)); + 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 9c033da..6063abf 100644 --- a/src/DodoSSH.Client.App/Views/MainWindow.axaml +++ b/src/DodoSSH.Client.App/Views/MainWindow.axaml @@ -163,6 +163,19 @@