diff --git a/docs/adr/0007-device-key-protection.md b/docs/adr/0007-device-key-protection.md index b8f38f5..11fb21d 100644 --- a/docs/adr/0007-device-key-protection.md +++ b/docs/adr/0007-device-key-protection.md @@ -148,6 +148,17 @@ would have become false under DPAPI alone. A gesture is still something the atta 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 by construction. -- **Revocation must delete the server row**, and un-enrolling the machine in front of the user must not - be able to lock them out: [ADR 0001](0001-e2ee-trust-model.md) makes an enrolled device a recovery - path, so it is now load-bearing for more than convenience. +- **Revocation deletes the server row**, through `DELETE /api/v1/me/devices/{id}`, and the wrap goes with it + on the foreign key's cascade. The device row is not the dangerous half: a `kind=device` wrap left behind is + the user's identity bundle still sealed to a key somebody may hold. It is never refused for being the last + device — [ADR 0001](0001-e2ee-trust-model.md) makes an enrolled device a recovery path, so removing the + last one does cost something, but the machine being revoked is most likely the one just lost and a server + that argued would be refusing the one request that has to work immediately. The passphrase wrap is + untouched, so this can never lock anyone out. +- **Offline revocation does the local half and says so.** What decides whether a machine may unlock itself is + entirely local — the unlock path never asks the server — so the useful half always happens, and only the + account being told can be out of reach. +- **A machine is a device, so registering again replaces rather than adds.** The server is idempotent on the + public key, but the client generates a fresh key pair each time and the keystore holds one, so a second + registration left the account listing a device whose private half had just been overwritten — an orphaned + wrap of exactly the kind revocation exists to remove. Registering now withdraws the previous device. diff --git a/src/DodoSSH.Api/Features/Identity/DeviceService.cs b/src/DodoSSH.Api/Features/Identity/DeviceService.cs index a50f847..8ec868f 100644 --- a/src/DodoSSH.Api/Features/Identity/DeviceService.cs +++ b/src/DodoSSH.Api/Features/Identity/DeviceService.cs @@ -81,6 +81,46 @@ internal sealed class DeviceService(DodoDbContext database, TimeProvider clock) return new RegisterDeviceResponse(registered.Id, now); } + /// + /// Removes a device and the wrap that let it unlock. + /// + /// Whether there was one to remove. + /// + /// + /// Scoped to the caller's own account, which is the whole authorisation check: the id is a v7 GUID and + /// unguessable, but "unguessable" is not a permission, and a user must not be able to revoke somebody + /// else's laptop by pasting an id they saw once. + /// + /// + /// The wrap goes with it through the foreign key's cascade rather than a second statement, and that is + /// worth naming because the device row is not the dangerous half. A device nobody can use is untidy; a + /// wrap left behind is the user's identity bundle still sealed to a + /// key somebody may hold. RevokingADevice_TakesItsWrapWithIt asserts the cascade rather than + /// trusting the configuration to keep saying so. + /// + /// + /// Never refused for being the last device. ADR 0001 makes an enrolled device a recovery + /// path, so removing the last one does cost the user something — but the machine somebody is revoking is + /// most likely the one they have just lost, and a server that argued about it would be refusing the one + /// request that has to work immediately. The passphrase wrap is untouched either way, so this can never + /// lock anyone out of their own vault. + /// + /// + internal async Task RevokeAsync( + UserAccount user, + Guid deviceId, + CancellationToken cancellationToken) + { + ArgumentNullException.ThrowIfNull(user); + + var removed = await database.Devices + .Where(device => device.Id == deviceId && device.UserId == user.Id) + .ExecuteDeleteAsync(cancellationToken) + .ConfigureAwait(false); + + return removed > 0; + } + /// Rounds a timestamp down to what the database can actually hold. /// /// reports 100-nanosecond ticks and PostgreSQL's timestamp with time diff --git a/src/DodoSSH.Api/Features/Identity/IdentityEndpoints.cs b/src/DodoSSH.Api/Features/Identity/IdentityEndpoints.cs index 0b35adc..ac945b5 100644 --- a/src/DodoSSH.Api/Features/Identity/IdentityEndpoints.cs +++ b/src/DodoSSH.Api/Features/Identity/IdentityEndpoints.cs @@ -39,6 +39,14 @@ internal static class IdentityEndpoints .WithName("RegisterDevice") .WithSummary("Registers a device key so this machine can unlock without the passphrase."); + // On the group's ordinary policy, unlike registering. Registering needs a bundle to seal, so + // demanding enrollment says something true; revoking needs nothing but the account, and a user whose + // enrollment state is somehow in doubt is exactly who should still be able to withdraw a laptop they + // have lost. Not enrolled means no devices, which this answers as 404 and no harm done. + group.MapDelete("/devices/{deviceId:guid}", RevokeDeviceAsync) + .WithName("RevokeDevice") + .WithSummary("Withdraws a device key, so that machine can no longer unlock without the passphrase."); + return app; } @@ -125,6 +133,25 @@ internal static class IdentityEndpoints } } + /// + /// 404 for a device that is not there, rather than a bland 204. A revocation is one of the few calls + /// where succeeding on a typo would be a real disservice — "revoked" is what the user reads, and reading + /// it about the wrong id is worse than being told to look again. Clients that are only driving towards + /// "this machine cannot unlock" can treat 404 as having arrived, which is what the desktop client does. + /// + private static async Task> RevokeDeviceAsync( + Guid deviceId, + ICurrentUserContext currentUser, + DeviceService devices, + CancellationToken cancellationToken) + { + var user = await currentUser.GetOrProvisionAsync(cancellationToken).ConfigureAwait(false); + + var revoked = await devices.RevokeAsync(user, deviceId, cancellationToken).ConfigureAwait(false); + + return revoked ? TypedResults.NoContent() : TypedResults.NotFound(); + } + private static ProblemHttpResult Problem(int statusCode, string code, string detail) => TypedResults.Problem( detail: detail, diff --git a/src/DodoSSH.Client.Api/DodoSshApiClient.cs b/src/DodoSSH.Client.Api/DodoSshApiClient.cs index 953a9a5..a5edd25 100644 --- a/src/DodoSSH.Client.Api/DodoSshApiClient.cs +++ b/src/DodoSSH.Client.Api/DodoSshApiClient.cs @@ -1,3 +1,4 @@ +using System.Globalization; using System.Net; using System.Net.Http.Headers; using System.Net.Http.Json; @@ -45,6 +46,16 @@ public interface IAccountApi Task RegisterDeviceAsync( RegisterDeviceRequest request, CancellationToken cancellationToken); + + /// + /// Withdraws a device key, so that machine can no longer unlock without the passphrase. + /// + /// + /// Whether the account had that device. False means it did not, which a caller withdrawing its own + /// device should treat as having arrived rather than as a failure — another machine may have revoked it + /// first, and the goal state is the same either way. + /// + Task RevokeDeviceAsync(Guid deviceId, CancellationToken cancellationToken); } /// @@ -157,6 +168,12 @@ public sealed class DodoSshApiClient(HttpClient http, IAccessTokenProvider token DodoSshJsonContext.Default.RegisterDeviceResponse, cancellationToken); + /// + public Task RevokeDeviceAsync(Guid deviceId, CancellationToken cancellationToken) => + DeleteAsync( + string.Create(CultureInfo.InvariantCulture, $"{DevicesPath}/{deviceId}"), + cancellationToken); + /// Reads vault changes after a cursor. /// /// A POST despite being a read: the filters live in the body, cursors are opaque, and no caching is @@ -216,6 +233,41 @@ public sealed class DodoSshApiClient(HttpClient http, IAccessTokenProvider token return await SendCoreAsync(request, typeInfo, cancellationToken).ConfigureAwait(false); } + /// + /// Sends a delete whose success carries no body. + /// + /// True for a 2xx, false for a 404; anything else throws. + /// + /// Its own path rather than with some empty response type, because the two + /// disagree about what a missing body means. Everywhere else a 200 with nothing in it is a server bug + /// worth an exception; here it is the answer. + /// + private async Task DeleteAsync(string path, CancellationToken cancellationToken) + { + using var request = new HttpRequestMessage(HttpMethod.Delete, path); + + var token = await tokens.GetAccessTokenAsync(cancellationToken).ConfigureAwait(false); + request.Headers.Authorization = new AuthenticationHeaderValue("Bearer", token); + + using var response = await http.SendAsync(request, cancellationToken).ConfigureAwait(false); + + if (response.StatusCode == HttpStatusCode.NotFound) + { + return false; + } + + if (!response.IsSuccessStatusCode) + { + var body = await response.Content + .ReadAsStringAsync(cancellationToken) + .ConfigureAwait(false); + + throw DodoSshApiException.FromResponse(response.StatusCode, body); + } + + return true; + } + private async Task SendCoreAsync( HttpRequestMessage request, System.Text.Json.Serialization.Metadata.JsonTypeInfo typeInfo, diff --git a/src/DodoSSH.Client.App/ViewModels/MainWindowViewModel.cs b/src/DodoSSH.Client.App/ViewModels/MainWindowViewModel.cs index a448c77..a818d5d 100644 --- a/src/DodoSSH.Client.App/ViewModels/MainWindowViewModel.cs +++ b/src/DodoSSH.Client.App/ViewModels/MainWindowViewModel.cs @@ -130,6 +130,10 @@ internal sealed partial class MainWindowViewModel : ObservableObject, IAsyncDisp [ObservableProperty] private bool canRegisterDevice; + /// Whether this machine has a device key to withdraw. + [ObservableProperty] + private bool canForgetDevice; + /// /// 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 @@ -453,10 +457,59 @@ internal sealed partial class MainWindowViewModel : ObservableObject, IAsyncDisp } CanRegisterDevice = false; + CanForgetDevice = true; StatusMessage = $"'{name}' can now unlock without your passphrase."; }).ConfigureAwait(true); } + /// + /// Withdraws this machine's device key, here and on the account. + /// + /// + /// + /// Offered without a confirmation prompt, which is deliberate. The cost of pressing it by accident is one + /// passphrase and one re-registration; the cost of a confirmation dialog is a moment's hesitation at the + /// point somebody has realised a machine is in the wrong hands. Reversible and urgent beats guarded. + /// + /// + /// Works offline, and says so. What decides whether this machine may unlock itself is entirely local, so + /// the useful half always happens — the account being told is the half that can be out of reach. + /// + /// + [RelayCommand] + private async Task ForgetDeviceAsync(CancellationToken cancellationToken) + { + if (Vault is not { } vault) + { + return; + } + + await RunAsync( + "Waiting for Windows…", + async () => + { + var revocation = await vault.Session + .ForgetDeviceAsync(connection?.Account, deviceKeys, cancellationToken) + .ConfigureAwait(true); + + CanForgetDevice = false; + + // Not re-offered here even though it is now true, because registering probes the TPM and + // this is not the moment to do it: somebody who has just withdrawn a device is not about to + // add one back, and the offer reappears on the next unlock. + StatusMessage = revocation switch + { + DeviceRevocation.Complete => + "This machine no longer unlocks without your passphrase, and the account no longer " + + "lists it.", + DeviceRevocation.LocalOnly => + "This machine no longer unlocks without your passphrase. You are offline, so the " + + "account still lists it — sign in and withdraw it again to finish.", + _ => "There was no device key on this machine.", + }; + }).ConfigureAwait(true); + } + /// /// Takes ownership of a freshly opened session, whichever door opened it. /// @@ -491,6 +544,11 @@ internal sealed partial class MainWindowViewModel : ObservableObject, IAsyncDisp CanRegisterDevice = session.Profile.DeviceWrappedPrivateKey is null && await deviceKeys.IsAvailableAsync(cancellationToken).ConfigureAwait(true); + // The other side of the same fact, and it needs its own flag rather than the negation of that one: + // "not offered because this machine has no TPM" and "not offered because it is already registered" + // are both !CanRegisterDevice, and only the second has anything to withdraw. + CanForgetDevice = session.Profile.DeviceWrappedPrivateKey is not null; + 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 diff --git a/src/DodoSSH.Client.App/Views/MainWindow.axaml b/src/DodoSSH.Client.App/Views/MainWindow.axaml index 5aad3ef..c7a9d88 100644 --- a/src/DodoSSH.Client.App/Views/MainWindow.axaml +++ b/src/DodoSSH.Client.App/Views/MainWindow.axaml @@ -60,6 +60,15 @@ IsEnabled="{Binding !IsBusy}" IsVisible="{Binding CanRegisterDevice}" ToolTip.Tip="Registers this machine so a later launch can open the vault with a Windows confirmation instead of your passphrase. Your passphrase keeps working." /> + +