416f23365713bb65425ea7e7f317553615628c44
3
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 jaap-jan
|
f86791e817 |
Finish revoking a device, instead of half of it
ForgetDeviceAsync stopped this machine unlocking without a passphrase and left
the server's row exactly where it was, so the account went on listing a device
nobody could account for. ADR 0007 recorded that as a deliberate gap needing an
endpoint. This is the endpoint, and the two things that turned up behind it.
DELETE /api/v1/me/devices/{id}. The device row is not the dangerous half: a
kind=device wrap is the user's identity bundle sealed to a key somebody may be
holding, and that is what has to go. It goes on the foreign key's cascade rather
than a second statement, and RevokeDevice_TakesItsWrapWithIt asserts the cascade
rather than trusting the configuration to keep saying so.
Scoped to the caller's own account, which is the only authorisation check there
is. The id is an unguessable v7 GUID, but unguessable is not a permission —
without the scope one user could withdraw another's device key by pasting an id
they saw once, and the victim's next launch would ask for a passphrase with no
explanation. 404 rather than 403 for somebody else's device, so a stranger does
not learn the id exists.
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 being revoked 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, which
RevokeDevice_LeavesThePassphraseWrapAlone pins.
--- Two things found on the way ---
Registering twice from one machine left two devices on the account. The server
is idempotent on the public key, but the client generates a fresh key pair every
call and the keystore holds one — so the second registration orphaned a wrap
whose private half had just been overwritten, which is precisely the leftover
this change exists to remove. Registering now withdraws the previous device.
Found by a test that asserted the property and failed.
And the fakes were lying about it. FakeAccountServer's comment claimed the real
service's idempotence while handing back a fresh Guid on every call, which is
invisible until something revokes by id — at which point a test would be
revoking an id the server never issued, and passing. Both fakes now issue one id
per public key and drop the wrap with the device, as the cascade does.
--- Reachable at all ---
ForgetDeviceAsync had exactly one caller and it was a test, so "Stop unlocking
here" now sits in the account bar where "Use Windows Hello here" was. Its own
flag rather than the negation of that one: a machine with no TPM and a machine
that is already registered are both "cannot register", and only the second has
anything to take back.
No confirmation prompt, deliberately. The cost of pressing it by accident is one
passphrase and one re-registration; the cost of a dialog is a moment's
hesitation at the point somebody has realised a machine is in the wrong hands.
Offline it does the local half and says so rather than refusing. Whether this
machine may unlock itself is decided entirely by the local cache and the local
keystore — the unlock path never asks the server — so forgetting here is what
actually revokes, and "you are offline, so this machine will go on unlocking
itself" would be the worst available answer. DeviceRevocation.LocalOnly is what
the interface reports and the status line explains what is left to do.
The local half runs first for the same reason, and the keystore call is the
first thing in the method that can yield: on Windows it raises a consent dialog,
and a dialog wants the thread it was called from. That ordering is currently
load-bearing and shakier than it looks — see the open device-unlock hang.
Four mutations, all caught: dropping the user scope from the server query
(1 test), skipping the stale-device revoke on re-registration (2), skipping the
server call in ForgetDeviceAsync (2), and the earlier version of the client that
never called it at all.
930 tests green across 16 projects, 13 of them new. Zero warnings, format clean.
|
||
|
jaap-jan
|
db4a8ed3d3 |
Let an already-enrolled account register a device key
The first of the three pieces ADR 0007 needs, and the one that was a discovery rather than a plan. EnrollmentService.AddDevice runs only during enrollment, so without an endpoint the device-unlock feature would have reached accounts created after it shipped and no others — which is to say none of the ones that exist. The code even said so: "the devices endpoint sets it properly when it lands." POST /api/v1/me/devices takes a name, an X25519 public key and the bundle sealed to it, and writes a device row plus a UserKeyWrapKind.Device wrap. Possession is proved by construction, so there is no challenge. The wrap is the secret bundle sealed to the supplied public key, and only something that has opened that bundle can produce it. A caller who seals the wrong bytes registers a device that cannot unlock, which harms nobody else; the server cannot tell the difference and must not pretend to, because it holds no key that opens either. That is also why the client must be unlocked to call this at all. It is the one endpoint in the /me group that requires enrollment, and it says so itself rather than relying on the group. The group deliberately does not: GET / and POST /enrollment are how a client discovers it needs to enroll and then does so, and gating those on enrollment would make enrollment unreachable. Adding the stricter policy to this route alone means an unenrolled caller is told "enrollment-required" by the authorization handler rather than getting a 400 about the shape of a request that was fine. Idempotent on the public key, and 200 rather than 201 for the reason enrollment gives: a retry of an identical request returns the same body, so there is no single moment of creation to point a Location header at. A second row for one key would mean a device list with a duplicate in it and two wraps to revoke instead of one. Mutation tested — removing the lookup fails RegisterDevice_TwiceWithTheSameKey_ReturnsTheSameDeviceAndAddsNoSecondWrap and nothing else. That test also found a real defect, in the way these usually surface: two timestamps that print identically and are not equal. TimeProvider reports 100-nanosecond ticks and PostgreSQL's timestamp with time zone keeps microseconds, so the first call returned a value that no later read of the row would ever produce, and the idempotent retry answered with a different timestamp for the same device. Nothing breaks, which is what makes it worth fixing: the service now truncates to the precision the column actually holds, so the response is the same value every time it is asked for. The repo already had a precedent for this class of thing in KeyLogChain.TruncateTimestamp; it just had not been applied here. The platform is deliberately not carried on the wire, which leaves Device.Platform unreported and the stale comment corrected rather than fulfilled. It would be a display-only field, and a Contracts enum mirroring the domain's DevicePlatform is exactly the shape of duplication that has produced three self-consistent bugs in this repository. A device list that wants it can add a mapping table and a test pinning the two together, which is what the sync entity types already do. Its own problem code and exception rather than reusing enrollment's, whose rules it largely shares. Registering a device is not enrolling, and a client showing "your enrollment was rejected" because somebody set up a fingerprint reader would be describing the wrong thing. The validation shares the limit constants — MaximumWrapBytes, MaximumDeviceNameLength, PublicKeySize — and not the four-line guards, which would have had to be parameterised over which exception to throw for less than they cost. Both in-memory fakes implement it properly rather than throwing: they record the wrap so a test can assert it arrived, and refuse before enrollment as the real endpoint's policy does. A fake that answered where the server refuses is a fake that can make a real bug pass. 866 tests green, 8 of them new. Zero warnings, dotnet format clean. Still to come: the protector seam with the wrap cached locally so device unlock works offline, then the Windows Hello implementation and the unlock-screen UI — which is where the Windows target framework lands and where automated testing stops. |
||
|
jaap-jan
|
49f617b450 |
Wire the Avalonia shell to the vault
The host list now comes from the vault instead of from a form. A fresh machine takes a server URL, signs in through the browser, enrolls, and from then on opens with the passphrase alone. DodoSSH.Client.Session is the composition layer: where a profile lives, how it unlocks, and how a machine gets one. ClientPaths picks a non-roaming per-OS directory — %LOCALAPPDATA% and never %APPDATA%, because a SQLite cache that roams between two machines is a corrupt one, and each machine's outbox is its own. SessionOpener needs no transport at all and could not reach one if it wanted to; that is the offline unlock, asserted rather than asserted about. A wrong passphrase, a stale KDF and a grant revoked by a rekey are three different answers, because the remedies are three different things and telling someone to retype a passphrase that was never the problem is worse than saying nothing. The shell's states are the onboarding story. The recovery code gets its own state that cannot be clicked past: it exists for one moment, losing it with the passphrase loses the vault, and there is no server-side reset by design. It is dropped from memory on confirmation rather than merely hidden. Sign-in is a delegate over IVaultServer, so the whole state machine runs in a test against an in-memory server — no browser, no identity provider, no toolkit. The view models are plain observable objects, which is what makes that possible. What it does not cover is whether the XAML binds to the right names; that needs a rendered tree and Avalonia.Headless, and is its own piece of work. Three things found by doing it rather than by reading it: - Pooled SQLite connections keep the database file open after the last context is disposed. On Windows that means locked, so the application could never replace its own cache — and a test could not clean up after itself, which is how it surfaced. Dispose now clears the pool. - EF's SQLite provider puts the database in WAL mode, so the cache is three files. A comment in ClientCacheFactory claimed the opposite; reading PRAGMA journal_mode off a real launch settled it. WAL is the right mode here — a sync pass writes while the interface reads — so the comment was wrong on the merits as well as on the fact. - Enrolling a device key with nowhere to keep the private half would put a wrap on the server nobody can open and make the device list claim this machine can unlock without a passphrase. Device binding is now optional and the shell declines it until the OS keystore is wired. Verified on Windows: the client created %LOCALAPPDATA%\DodoSSH\cache.db and migrated it on first launch, and msedgewebview2 held an established connection to the data plane while the unlock overlay covered it — which is the point of covering the WebView rather than collapsing it, since a NativeWebView that is never laid out is never realised. 630 tests, up from 593. The recovery-code gate and the offline unlock were each verified by breaking them and watching the right test fail. Still to do for M1's actual definition of done: the manual run against the real API and a real Keycloak. Credentials are not a synced entity type yet, so a connection still asks for a password, and the interface says so rather than implying otherwise. |