240aadb746c720c0ab016e355b683fe5fbd582d8
8
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 jaap-jan
|
94e11f5e38 | update packages | ||
|
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
|
573f5d5668 |
Keep the device key in the TPM, behind a consent Windows enforces
The last of ADR 0007's three pieces, and it does not implement what that ADR originally decided — because writing it exposed a flaw in the decision. The ADR said "a Windows Hello gesture gating a protected blob". That does not deliver what the rest of the document claims for it: a gate inside the process is not a gate. A store that showed a prompt and then read a DPAPI blob would be bypassed by malware that skipped the prompt, read the file and called CryptUnprotectData itself — which is exactly the attacker the whole decision was made against, and exactly the reason DPAPI alone was rejected. 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. That is strictly stronger than the ADR described, and most of what option D was being saved for: the wrapping key genuinely never leaves hardware. The X25519 device key still lands in memory to open the wrap, because DSH1 fixes that wrap at a curve the TPM cannot do — the remaining gap, and now a smaller step than it was. CngKey is in-box, so this needed no WinRT projection and no Windows target framework. Which is worth stating plainly because the opposite was planned: the piece was scoped as "where the Windows TFM lands", and it turned out a platform guard on one class was enough. Client.App and its two test projects stay on net10.0. Two things were measured on real hardware rather than assumed, and the second changed the shape of the work. The platform provider works here and holds an RSA key — confirmed by creating and deleting one before writing anything that depended on it. And ProtectKey prompts at key *creation*, not only at use. The comment in the first draft of this file said the opposite, with a confident explanation: sealing uses only the public half, so it should be silent. It is not. 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. Found by writing tests around save and forget and watching the suite hang for ten minutes waiting for somebody to type one. That has two consequences worth knowing before touching this file. SaveAsync is user-facing code — it belongs on a UI thread, behind a button somebody pressed, never on a background pass. And almost nothing in the store can be covered automatically: two tests remain, availability and the empty-blob case, both of which provably reach no dialog. Disabling the UI policy to make the rest testable would remove the one property worth having. The interface offers two things and hides both where they cannot work. "Use Windows Hello" appears on the unlock screen only when this machine has a cached wrap and a keystore still willing to release the key; "Use Windows Hello here" appears in the account bar only when the machine can keep a key and has not already registered one, so it is spent once used. Absent rather than disabled, in both cases: a greyed-out button on a machine that never had a TPM reads as something broken, and the passphrase box beside it is not a fallback — it is the ordinary way in. Both unlock paths now share AdoptAsync rather than each opening the known-host store, building the vault and starting auto-sync. The ordering in there is load-bearing and a second copy would be a second chance to get it wrong. The shell's tests drive a fake keystore. Not for speed: the real one prompts on every save and load, so a suite using it would block forever. What the shell has to get right is which buttons appear and what happens when one is pressed, and a fake answers exactly that. It is shared from Client.Session.Tests by source link rather than reimplemented. 882 tests green, 6 of them new. Zero warnings, dotnet format clean. Not verified, and not verifiable here: the dialogs. Whether the consent prompt appears at the right moments, reads sensibly, and returns to a usable window when declined needs the application run by a person on a machine with a TPM. That is the remaining half of outstanding item #7, and it is now the only thing between this feature and being finished. |
||
|
jaap-jan
|
1faea42b94 |
Unlock with this machine's device key, without a passphrase or a network
The second of ADR 0007's three pieces: the seam a keystore plugs into, the wrap
cached where an offline unlock can reach it, and the unlock path itself. What is
still missing is the keystore — UnavailableDeviceKeyStore is what the application
composes for now, so behaviour is unchanged until piece three lands.
IDeviceKeyStore holds exactly 32 bytes, and only because the cache key moved
first. It would have had to hold the local cache key alongside the X25519 scalar —
a second live secret at rest, going stale on every passphrase change — had
|
||
|
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
|
211eba0666 |
Keep host key trust in the vault, and make it withdrawable
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
|
||
|
jaap-jan
|
e3fd3e1728 |
Sync and authenticate with SSH keys on the client
Completes the client half of SSH keys: they sync alongside hosts, appear in their own list, and can be selected to authenticate a connection instead of typing a password. The reconciler and the repository were Host-typed throughout, so the choice was to generalise them or to keep a second copy per item type. Generalised, because ItemReconciler's whole premise is that the pull and the push paths must answer the same collision the same way — two copies would drift the first time one of them was fixed. What is genuinely per-type now arrives through IItemKind<TSecret>: the cipher, the merge, the plaintext columns, and the noun to use when telling a person what happened to their item. Generic where the server's IItemKind is not, and for the reason that reverses there — the client needs the concrete type, because it merges field by field. The pull filter is derived from the same registry that builds the reconcilers. That is the specific failure being designed out: an item type that encrypts, merges and lists perfectly and is never once requested from the server, so it works on the machine that made it and exists nowhere else. No client cache migration. The item table's primary key and the outbox's unique index already carry the entity type, and AadResourceTypes already mapped SshKey — so a host and a key may share an id and never see each other's rows, which SshKeySyncTests now arranges deliberately. A key hands the server nothing in plaintext. There is a public_key_fingerprint column and it would be accepted; leaving it null is deliberate. A fingerprint is not secret but it is a stable identifier for a key pair, so filling it would let an operator tell which of their users hold the same key and correlate one across vaults, for a column nothing reads. The design allows itself one plaintext concession — the relay address, which the relay cannot work without — and this is not that. A key is chosen per connection rather than bound to a host, which works the way ssh -i does. Binding one needs a field on HostSecret and therefore a payload schema bump, which makes every host written afterwards read-only on an older build; worth doing deliberately rather than as a side effect of adding keys. Three things this found, all of them by being falsified rather than by review: - Making the reconciler generic silently turned a record comparison into reference equality, because == on a type parameter is not value equality. The effect would have been a conflict recorded on every pass for an unacknowledged create that had in fact landed. Sabotaging the fix left all 73 tests passing — nothing covered that branch — so ConflictMatrixTests now has AnUnacknowledgedCreateThatDidLand_IsDroppedQuietly, which fails without it. - A test asserting that a blank passphrase reaches SSH.NET as null was vacuous: it exercised the editor, not the credential path, and passed with the guard deleted. Resolved by making SshKeySecret.Passphrase normalise an empty string to null, so there is one spelling of one state — which also keeps two clients from producing different payload bytes for an identical key. That exposed a wider gap: SshKeySecret, its codec and its merge had no direct unit tests at all. They have 25 now. - The reason first given for that normalisation was false. It claimed SSH.NET rejects a passphrase supplied for an unprotected key; measured against a real sshd it ignores it and authenticates anyway. Corrected everywhere it was stated and recorded in docs/platform-flags.md. The same test file also closes a real hole: SshPrivateKeyCredential had never been exercised against a server, because the existing key test builds SSH.NET's auth method directly and bypasses the path a vault-held key actually takes. Only one editor may be open at a time. Both sit in the same 340-pixel column as Auto rows and their heights together exceed it at the window's minimum size, so two open editors put the lower one's Save and Cancel past the bottom edge — the same failure this window already shipped once with the setup screens. Expressed as a state rule because that is the only form of it this repository can check: nothing here loads a .axaml. The refusal keeps what was typed, since in the key editor that is a pasted private key the user may have nowhere else. The end-to-end slice now carries a key as well as a host, so both item types go through the real API, the real PostgreSQL and the real crypto in one pass — the three hand-kept mappings between enums that do not line up are the reason that is worth doing rather than trusting the unit suites. 735 tests green, including the container-backed SSH and end-to-end suites. Zero warnings, dotnet format clean. |
||
|
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. |