diff --git a/docs/adr/0007-device-key-protection.md b/docs/adr/0007-device-key-protection.md index 1a89f6e..b8f38f5 100644 --- a/docs/adr/0007-device-key-protection.md +++ b/docs/adr/0007-device-key-protection.md @@ -33,11 +33,47 @@ leaves hardware" would be false under all of them. ## Decision -**A Windows Hello gesture gating a protected blob, with the passphrase kept as a permanent fallback.** +**A TPM-resident key whose use requires the user's consent, with the passphrase kept as a permanent +fallback.** -The gesture is what carries the security value: it requires **user presence** per unlock. Hello cannot -decrypt, so it is used to gate release of the wrapping key, and the passphrase path remains available -unconditionally. +User presence per unlock is what carries the security value. What changed between this decision and its +implementation is *who enforces the presence*, and the change was a correction rather than a refinement. + +> **Amended 2026-07-30.** This section originally read "a Windows Hello gesture gating a protected blob". +> That design does not deliver what the rest of this document claims for it, and the flaw is worth keeping +> on the record: **a gate inside the process is not a gate.** A store that showed a Hello prompt and then +> read a DPAPI blob would be bypassed by malware that skipped the prompt, read the file and called +> `CryptUnprotectData` itself. The presence requirement has to be a condition of *using the key*, enforced +> below the application, or it is decoration. + +So the device key is encrypted to an RSA key created in the **Microsoft Platform Crypto Provider** — the +TPM — under `CngUIProtectionLevels.ProtectKey`. Windows requires consent to use that key, so the prompt is +not something this code can be talked out of showing. Malware can ask for the key; it cannot answer the +dialog, and the attempt is visible. `System.Security.Cryptography.CngKey` is in-box, so this needs no WinRT +projection and **no Windows target framework** — a plain platform guard is enough. + +RSA rather than an agreement algorithm because the payload is 32 bytes and OAEP over 2048 bits carries 190. +That also keeps the DSH1 device wrap unchanged at X25519: the TPM key protects the device key, it does not +replace it. + +Availability is probed by creating a throwaway key and deleting it, not by asking whether the provider is +registered — it is registered on machines with no usable TPM too, and reports itself present right up to +the point where creating a key fails. + +### What was measured, and what it cost + +Two things were verified on real hardware rather than assumed, and one of them changed the design's shape: + +- **The platform provider works** and holds an RSA key: confirmed by creating and deleting one. +- **`ProtectKey` prompts at key *creation*, not only at use.** `CngKey.Create` blocks on a dialog, because + the policy means "protect this key with a PIN" and Windows asks the user to set that up there and then. + +The second has consequences. Registering a device shows a setup dialog and every unlock shows a consent +dialog, which is the right shape for an opt-in feature — but it means **`SaveAsync` is user-facing code** +that belongs on a UI thread behind a button somebody pressed, and it means almost nothing in the store can +be covered by an automated test. That was found by writing those tests and watching a suite hang for ten +minutes waiting for a PIN. Two tests remain: availability, and the empty case that provably reaches no +dialog. ### Why not DPAPI alone @@ -55,9 +91,14 @@ Neither defends the *local malware* case. The gesture does. ### Why not extend the spec (yet) -The only option that delivers what the TPM is usually credited with is to add a `SealTo` algorithm over -a curve the TPM can do — `alg_id = 4` over P-256 — so the device private key never exists in process -memory at all. That is **the recorded target**, not this decision. +The device *wrapping* key now genuinely never leaves the TPM, which is most of what option D promised. What +remains is that the X25519 device key itself is reassembled in process memory to open the wrap, because DSH1 +fixes that wrap at a curve the TPM cannot do. + +Closing that last gap means adding a `SealTo` algorithm over a curve the TPM can do — `alg_id = 4` over +P-256 — so the device key never exists outside hardware at all. That is **the recorded target**, not this +decision, and it is now a smaller step than it was: the keystore plumbing, the endpoint and the unlock path +would all be unchanged. It is cheaper than "change a frozen spec" sounds, because a device wrap row is read only by the device that created it: not by another client, and not by the server. The envelope already carries `alg_id` @@ -97,11 +138,12 @@ would have become false under DPAPI alone. A gesture is still something the atta ### Operational -- **Hello is not always available.** No biometric hardware falls back to a Hello PIN, which is - TPM-bound and rate-limited and still satisfies the presence requirement. Some machines have no Hello - at all. The passphrase path is therefore required, not a nicety. -- **Hello keys are invalidated when the PIN is reset**, so the blob must be treated as losable at any - time; losing it degrades to a passphrase prompt and never to a locked-out vault. +- **A TPM is not always there.** A machine without one gets a store that reports itself unavailable, so + unlock keeps asking for the passphrase and neither affordance appears in the interface. The passphrase path + is therefore required, not a nicety. +- **The stored key must be treated as losable at any time** — a reset PIN, a cleared TPM, a replaced key. + Every loss degrades to a passphrase prompt and never to a locked-out vault, which is why every failure in + the store returns null rather than throwing and why the three unlock statuses all end in the same advice. - **Registering a device is a separate act from enrolling one.** `EnrollmentService.AddDevice` runs only during enrollment, so every already-enrolled account — which is all of them — needs an endpoint to add a device wrap while unlocked. Producing the wrap requires the bundle, so the client proves possession diff --git a/src/DodoSSH.Client.App/App.axaml.cs b/src/DodoSSH.Client.App/App.axaml.cs index 425a2ac..1b8496a 100644 --- a/src/DodoSSH.Client.App/App.axaml.cs +++ b/src/DodoSSH.Client.App/App.axaml.cs @@ -67,11 +67,17 @@ internal sealed partial class DodoSshApp : Application var browser = new SystemBrowserLauncher(); + // Chosen once, here, because it is a property of the machine and not of any session. A computer with + // a usable TPM gets the store that keeps a device key behind a Windows consent prompt; anything else + // gets one that reports itself unavailable, so unlock keeps asking for the passphrase. See ADR 0007. + var deviceKeys = DeviceKeyStores.ForThisMachine(paths); + var viewModel = new MainWindowViewModel( paths, caches, workspace, knownHosts, + deviceKeys, async (url, cancellationToken) => await ServerConnection .SignInAsync(url, browser, TimeProvider.System, cancellationToken) .ConfigureAwait(false), @@ -84,11 +90,22 @@ internal sealed partial class DodoSshApp : Application // discarding the task here is safe rather than merely convenient. _ = viewModel.StartAsync(CancellationToken.None); + WireShutdown(desktop, viewModel, workspace, caches); + } + + /// + /// Shutdown is deferred rather than blocked on. Sessions hold SSH connections and a listening socket, and + /// blocking the UI thread on their disposal is how an application comes to take several seconds to close — + /// or deadlocks, if any of that disposal needs the UI thread. + /// + private static void WireShutdown( + IClassicDesktopStyleApplicationLifetime desktop, + MainWindowViewModel viewModel, + TerminalWorkspace workspace, + ClientCacheFactory caches) + { var shuttingDown = false; - // Shutdown is deferred rather than blocked on. Sessions hold SSH connections and a listening - // socket, and blocking the UI thread on their disposal is how an application comes to take several - // seconds to close — or deadlocks, if any of that disposal needs the UI thread. desktop.ShutdownRequested += async (_, e) => { if (shuttingDown) diff --git a/src/DodoSSH.Client.App/ViewModels/MainWindowViewModel.cs b/src/DodoSSH.Client.App/ViewModels/MainWindowViewModel.cs index ae549c8..a448c77 100644 --- a/src/DodoSSH.Client.App/ViewModels/MainWindowViewModel.cs +++ b/src/DodoSSH.Client.App/ViewModels/MainWindowViewModel.cs @@ -68,6 +68,13 @@ internal sealed partial class MainWindowViewModel : ObservableObject, IAsyncDisp /// private readonly VaultKnownHostStore knownHosts; + /// + /// Whatever this machine can keep a device key in, chosen once at composition. An interface because the + /// answer is a platform decision — see ADR 0007 — and because a machine with no TPM gets a store that + /// reports itself unavailable rather than a null this state machine would have to check for. + /// + private readonly IDeviceKeyStore deviceKeys; + private readonly SignInHandler signIn; private readonly TimeProvider clock; private readonly Argon2Profile? passphraseProfile; @@ -91,6 +98,7 @@ internal sealed partial class MainWindowViewModel : ObservableObject, IAsyncDisp ClientCacheFactory caches, TerminalWorkspace workspace, VaultKnownHostStore knownHosts, + IDeviceKeyStore deviceKeys, SignInHandler signIn, TimeProvider clock, Argon2Profile? passphraseProfile = null) @@ -99,6 +107,7 @@ internal sealed partial class MainWindowViewModel : ObservableObject, IAsyncDisp this.caches = caches; this.workspace = workspace; this.knownHosts = knownHosts; + this.deviceKeys = deviceKeys; this.signIn = signIn; this.clock = clock; this.passphraseProfile = passphraseProfile; @@ -113,6 +122,14 @@ internal sealed partial class MainWindowViewModel : ObservableObject, IAsyncDisp [ObservableProperty] private bool isBusy; + /// Whether the unlock screen should offer a gesture instead of the passphrase. + [ObservableProperty] + private bool canUnlockWithDevice; + + /// Whether an unlocked vault should offer to register this machine. + [ObservableProperty] + private bool canRegisterDevice; + /// /// The address dotnet run --project src/DodoSSH.Api actually serves, so the first launch after /// a clone works without the user having to know a port. This was https://localhost:7217, which @@ -219,6 +236,13 @@ internal sealed partial class MainWindowViewModel : ObservableObject, IAsyncDisp ServerUrl = profile.ServerUrl; State = ShellState.Locked; StatusMessage = $"Enrolled against {profile.ServerUrl}."; + + // Both halves have to hold: a wrap in the cache, and a machine still willing to hand the key + // back. Offering the button without the second would prompt for a key that is not there; without + // the first it would prompt for a wrap that is not there. Neither failure is one a user could + // make sense of, so the button simply does not appear. + CanUnlockWithDevice = profile.DeviceWrappedPrivateKey is not null + && await deviceKeys.IsAvailableAsync(cancellationToken).ConfigureAwait(true); } catch (Exception exception) when (exception is not OperationCanceledException) { @@ -362,34 +386,119 @@ 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; - - await Vault.LoadAsync(cancellationToken).ConfigureAwait(true); - - // After the first load, so the list is on screen before anything talks to a server. The - // loop is started from the UI thread deliberately: every pass resumes here, which is what - // keeps the observable collections single-threaded. - Vault.StartAutoSync(); + await AdoptAsync(outcome.Session!, cancellationToken).ConfigureAwait(true); }).ConfigureAwait(true); } + /// Opens the vault with this machine's device key instead of the passphrase. + /// + /// No Task.Run, unlike the passphrase path: there is no Argon2 to pay for here, and the work that + /// does block is a Windows consent dialog which belongs on the UI thread anyway. + /// + [RelayCommand] + private async Task UnlockWithDeviceAsync(CancellationToken cancellationToken) + { + await RunAsync( + "Waiting for Windows…", + async () => + { + var outcome = await Opener() + .UnlockWithDeviceAsync(deviceKeys, cancellationToken) + .ConfigureAwait(true); + + StatusMessage = outcome.Message; + + if (!outcome.IsUnlocked) + { + // A declined gesture leaves the passphrase box exactly where it was, which is the whole + // fallback: the user types instead. Nothing about the screen changes but the message. + return; + } + + await AdoptAsync(outcome.Session!, cancellationToken).ConfigureAwait(true); + }).ConfigureAwait(true); + } + + /// + /// Registers this machine so a later launch can unlock with a gesture. + /// + /// + /// Needs a network, because the wrap has to reach the server — a wrap that exists only here would be + /// lost with the cache file and could never be revoked. Needs an unlocked vault too, because only an + /// open session can seal the bundle. + /// + [RelayCommand] + private async Task RegisterDeviceAsync(CancellationToken cancellationToken) + { + if (Vault is not { } vault || connection is null) + { + StatusMessage = "Sign in first: registering this machine has to reach the server."; + return; + } + + await RunAsync( + "Waiting for Windows…", + async () => + { + var name = Environment.MachineName; + + var registered = await vault.Session + .RegisterDeviceAsync(connection.Account, deviceKeys, name, cancellationToken) + .ConfigureAwait(true); + + if (!registered) + { + StatusMessage = "This machine has nowhere to keep a device key."; + return; + } + + CanRegisterDevice = false; + StatusMessage = $"'{name}' can now unlock without your passphrase."; + }).ConfigureAwait(true); + } + + /// + /// Takes ownership of a freshly opened session, whichever door opened it. + /// + /// + /// Shared by both unlock paths rather than duplicated, because the ordering in here is load-bearing and + /// a second copy would be a second chance to get it wrong. + /// + private async Task AdoptAsync(VaultSession session, CancellationToken cancellationToken) + { + // 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(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 session.DisposeAsync().ConfigureAwait(true); + throw; + } + + Vault = new VaultViewModel(session, workspace, knownHosts, () => connection); + State = ShellState.Unlocked; + + // Offered only where it can actually be honoured: a machine that can keep a key, and a profile that + // has not already registered one. Asked once here rather than recomputed, because the answer + // involves a TPM probe. + CanRegisterDevice = session.Profile.DeviceWrappedPrivateKey is null + && await deviceKeys.IsAvailableAsync(cancellationToken).ConfigureAwait(true); + + await Vault.LoadAsync(cancellationToken).ConfigureAwait(true); + + // After the first load, so the list is on screen before anything talks to a server. The loop is + // started from the UI thread deliberately: every pass resumes here, which is what keeps the + // observable collections single-threaded. + Vault.StartAutoSync(); + } + /// /// Closes the vault and forgets every key it held. Open shells keep running. /// diff --git a/src/DodoSSH.Client.App/ViewModels/VaultViewModel.cs b/src/DodoSSH.Client.App/ViewModels/VaultViewModel.cs index 2c96901..4e24d03 100644 --- a/src/DodoSSH.Client.App/ViewModels/VaultViewModel.cs +++ b/src/DodoSSH.Client.App/ViewModels/VaultViewModel.cs @@ -208,6 +208,16 @@ internal sealed partial class VaultViewModel( private Task? autoSyncLoop; private bool disposed; + /// + /// The open vault this view model is showing. + /// + /// + /// Exposed for the operations only an open session can perform — sealing the bundle to a new device key, + /// chiefly — which the shell drives rather than this view model. Ownership does not move: this type + /// disposes it, and a caller must not. + /// + internal VaultSession Session => session; + /// The hosts to show, unpushed local state included. internal ObservableCollection Hosts { get; } = []; diff --git a/src/DodoSSH.Client.App/Views/MainWindow.axaml b/src/DodoSSH.Client.App/Views/MainWindow.axaml index 40a4941..b6dd128 100644 --- a/src/DodoSSH.Client.App/Views/MainWindow.axaml +++ b/src/DodoSSH.Client.App/Views/MainWindow.axaml @@ -50,6 +50,16 @@ IsVisible="{Binding !IsOnline}" />