db4a8ed3d39772855486cf266fa712dde1bee5c2
8
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 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
|
e24012b039 |
Sync credentials as a vault item type, and bind one to a host
Closes the largest remaining M1 gap in the data layer: a username and password can live in the vault, sync between machines, and be named by a host as how it authenticates. What is not here is the interface for creating one — see the end of this message. The third item type, and the first one that cost almost nothing to add. Server: a VaultCredential row, an EF configuration, a migration, and a CredentialKind. Client: a secret, a codec, a merge, a cipher, a kind, a repository facade and a session property. No new reconciliation logic, no change to the sync engine, no client cache migration. That was the whole point of the item-kind seam, and this is the evidence it holds. The narrowest type of the three on plaintext, and not for symmetry. A host has a deliberate concession — the relay needs an address it can resolve. A key has a fingerprint, public by nature, which this client still declines to send. A password has no part that is safe to expose: not its length, not a hash, not a hint. So CredentialKind refuses every plaintext field there is, hydrates none, and the table has no column to put one in. HostSecret.CredentialId is the password counterpart of SshKeyId, and the two are mutually exclusive. SSH itself would happily try a key and fall back to a password, but a host naming both leaves "how does this authenticate?" without a single answer — the interface, the connect path and the user would each be free to guess differently. TryValidate refuses it. One consequence was not anticipated: "a full host" stops being a coherent idea, which is what broke AFullHost_RoundTrips and is now written into that test. The schema version became a ladder rather than a maximum: credential-bound is 3, key-bound is 2, neither is still 1. Adding credentials therefore does not drag every key-bound host in every vault onto a version that clients understanding keys perfectly well would refuse to edit. A test pins exactly that, because it is the property the whole content-dependent-version rule exists to provide, and the obvious implementation would quietly lose it. Two tests had become false and said so: - Push_AnUnsupportedEntityType_IsInvalidNotAFailedBatch used Credential as its example of a type this server does not implement. It now asks the server's own registry what is still missing, so it cannot go stale again, and skips with a reason if that set ever empties. - ThePullFilterNamesEveryTypeThisBuildSynchronises pinned the exact list, which is what it is for. Also fixes ten nullable warnings — eight in SyncEndpointTests, two in a test file added earlier today. Neither set was introduced here; both were invisible until an unrelated change forced their project to recompile, which means the zero-warning claims made earlier in this work only ever covered what happened to be rebuilt. 777 tests green. Zero warnings, dotnet format clean. Not done, and deliberately: the credential interface. The vault column is 340 pixels wide and already holds two lists and two editors, kept from clipping its own buttons at the window's minimum height only by the one-editor-at-a-time rule added earlier today. A third list and a third editor would recreate that defect rather than avoid it, so the column needs a shape decision first. Credentials sync; they cannot yet be created in the interface. |
||
|
jaap-jan
|
e93acc856f |
Sync SSH keys as a vault item type, over a shared write path
The private key now lives in the vault as ciphertext, syncs between a user's machines, and is stored on the server so it can later be shared — sharing itself needs M3's signed grants; this is the storage that makes it possible. More was already reserved than expected: SyncEntityType.SshKey, CryptoSpec.AadResourceType.SshKey, ChangeEntityType.SshKey, SyncPlaintextFields.PublicKeyFingerprint, and SshPrivateKeyCredential wired through PrivateKeyFile over a MemoryStream so a key never touches disk. The frozen contract and crypto spec needed no change at all. What was missing was the server. Rather than copy the push path per item type — version check, change-log append, exactly-once receipt, advisory lock — it is now written once over IVaultItem, with everything type-specific behind IItemKind: which table, which plaintext columns, and what those columns must satisfy. Ten copies of that logic by M5, with a fix applied to nine, is the outcome this avoids. The refactor landed first with no behaviour change, so all 66 existing Host tests were the regression net, and they stayed green. An interface rather than a base class, deliberately: EF Core maps an inheritance hierarchy when it can see one, so a mapped base would quietly become a table-per-hierarchy discriminator across item types — the very arrangement per-type tables exist to avoid. ssh_key mirrors host and pointedly has no relay trio. That is the argument for separate tables rather than one wide item table: the columns a host needs are columns a key must never have, and a shared table could only make them nullable and trust the code. A key carrying a relay target is refused with a reason rather than silently dropped. A key hydrates PlaintextFields as null, not an empty instance — the difference is visible on the wire, because an all-defaults instance still serialises "relayEnabled": false and invites a reader to believe the setting exists and is off. It has none. Two things now defended by tests rather than by comments. Each kind states its own ChangeEntityType instead of casting: the two enums agree numerically but do not even share member names (Host against SshHost), and filing key changes under the host type is silent sync corruption — sabotaging it fails three tests. And EntityTypeAlignmentTests asserts the two enums stay aligned in both directions and in count, which nothing did before. The client half is next: SshKeySecret, its codec and merge, the cipher, a repository, and the UI. Note for that work — SyncEntityType.SshKey is 3 while AadResourceType.SshKey is 6, so a cast between them would seal key ciphertext as a vault and nothing would fail. |
||
|
jaap-jan
|
8d2416a602 |
Add the encrypted local cache and the sync client
Three new client projects, and the wire-contract fix they needed. DodoSSH.Client.Domain holds the decrypted item model and the three-way merge, with no I/O at all — so the suite that decides whether a credential can be lost runs in milliseconds with nothing to mock. Scalars defer to the server on a genuine clash so every replica resolves the same triple identically and two clients cannot ping-pong; directives merge per name so two people each adding one both keep theirs; the jump chain merges as a whole value because its order is the route. Whatever loses is returned rather than dropped. DodoSSH.Client.Storage is EF Core on SQLite, no SQLCipher: the rows are already ciphertext, so an encrypted file would protect protected bytes at the cost of a native dependency. It keeps the server's state and the outbox in separate tables, which is what preserves the common ancestor a merge needs. One pending operation per item, enforced by a unique index. DodoSSH.Client.Sync is the pull/apply/push loop. Pulling never decrypts — a change with no local work pending is plumbed as ciphertext — so a first sync of thousands of items does not run twice as many AEAD operations for nothing. Contracts: EncryptedPayload gains WrappedDataKey and DataKeyId. The specification has required a per-item data key since crypto.md §3, the columns have existed since the first migration and DshAad.ItemPayload binds the id, but this record had nowhere to put either — so a spec-compliant item could not be transmitted at all. Found by writing the client that has to produce one. Also closes a hole in AadResourceType, which had no value for the HostTag and HostCredential that SyncEntityType has always listed. Four bugs the tests found, not review: - SQLite refuses to order or compare its own DateTimeOffset mapping, and throws at execution rather than model build. Collecting tombstones and listing conflicts are both that shape, so this was a crash waiting for the first user with a deleted host. Timestamps are integers now, by convention so a later field cannot be the one left unconverted. - SQLitePCLRaw 2.1.11, which EF resolves, is covered by GHSA-2m69-gcr7-jv3q. Pinned forward as a family. - Resurrecting content from a remote deletion cleared the original before queueing the copy. Two transactions, so a crash between them lost the work; reversed, and the rescued id is derived from the tombstone so a replay coalesces instead of duplicating. - Several equality assertions went through Shouldly's ShouldBe, which compares IEnumerable element-wise and so tested nothing about the Equals these types exist to provide. Corrected; the falsification that caught it went from 2 failures to 6. The push response's cursor is deliberately ignored. It sits after this client's own writes, so adopting it skips anything another client committed at a lower sequence in the window between a pull and a push — permanently. Re-reading one's own writes is idempotent and costs a page. The Contracts doc that invited the shortcut now says so. 593 tests, up from 448. The delete-versus-edit rules, the ancestor retention, the fresh operation id on coalesce and the cursor safeguard were each verified by breaking them and watching the right test fail. |
||
|
jaap-jan
|
a628762cd1 |
Add /me and enrollment with the identity-provider key binding (M1)
The last backend piece of M1. A client can now log in, discover it must enroll, publish its identity key, and get a usable personal vault. Enrollment is one indivisible act. One transaction writes the key, its wraps, the device, the key log entry, the vault and the vault key grant, because none of them is useful alone: a key with no vault leaves a user unable to store anything, and a vault with no grant is a container nobody can ever open -- including its owner, since only the client can wrap the key and it has already moved on. Two independent checks run, and neither substitutes for the other. The Ed25519 self-signature proves possession of the private key. The identity-provider binding proves whose key it is: the client hashed its statement, used the hash as an OIDC nonce, and the resulting ID token is the provider's signature over exactly those public keys. This server cannot mint that signature, so it cannot invent a key for a user who never enrolled -- which is the attack that would otherwise let an operator read every vault by publishing its own key as yours. The binding token is stored verbatim, not just summarised. Clients must repeat the check against the provider's JWKS fetched directly, and storing only our conclusion would ask them to trust the server about the one question the design exists to avoid trusting it about. Key log appends take a deployment-wide advisory lock. The falsification matters more than the passing test: with the lock removed, Enroll_ConcurrentEnrollmentsByDifferentUsers_LeaveAnUnbrokenChain fails with entry 11 linked to the wrong predecessor. Different users trip no unique index, so without serialising they all read the same head and the chain forks -- indistinguishable from the key substitution the log exists to make detectable, and permanent, because the log is append-only. Enrollment is idempotent. Vault ids and keys are client-chosen, so a client whose response was lost re-sends the identical body and gets the identical result. Without that, a lost response leaves a user enrolled against a vault they never learned the id of. Contract change, breaking the v0.1 freeze deliberately. EnrollmentRequest had DevicePublicKey but no wrap to go with it, which is unsatisfiable: only the holder of the secret bundle can seal it, so the server could never fill the gap. Added DeviceWrappedPrivateKey, and PersonalVault so enrollment can be atomic rather than leaving an unopenable vault behind two endpoints that do not exist yet. No client exists and no package is published, which is exactly when PublicAPI.Unshipped.txt expects this. Sync now requires the Enrolled policy, which until now was a stub whose name promised a check it never made. The sync denial tests use enrolled intruders instead of unenrolled ones -- an unenrolled caller is stopped before the vault check runs, which would have left those tests passing without exercising the thing they exist to prove. Also fixed: omitting kdfParameters from the JSON body was a 500. A record's non-nullable parameters are a compile-time promise, not a runtime one. 268 tests pass, zero warnings on a clean rebuild, format clean. |
||
|
jaap-jan
|
3829217e8a |
Add sync engine: cursors, push/pull, and the advisory-lock ordering proof (M1)
The vault write path. Push is the only way items change — no per-entity POST/PUT/DELETE — so one place enforces revisions, the change log and access control. The concurrency hazard, now proven rather than asserted: bigserial assigns sequence values when the INSERT runs, not at commit, so transaction A can take sequence 5 while B takes 6 and commits first. A reader polling in between sees only 6, advances past 5, and never learns about it. AdvisoryLockOrderingTests reproduces that gap WITHOUT the lock first — otherwise the with-lock test proves nothing, since it would pass just as happily if the interleaving never occurred — then shows pg_advisory_xact_lock removes it, and that 12 concurrent writers produce no gaps. Cursors are opaque and HMAC-tagged, and carry their vault id. 29 unit tests cover the rejections, which are the point: an accepted-but-wrong cursor is silent data loss, strictly worse than an error a client can resync from. Rejected: tampered tag, tampered payload, foreign signing key, a legitimately-issued cursor from another vault, truncation, and hostile input (never throws — cursors come from clients). Push semantics: - 200 even on partial failure, with per-operation status, so one stale item cannot block everything a client queued while offline. - Conflict returns the server's current row for client-side three-way merge. The server cannot merge ciphertext, so never last-writer-wins. - opId receipts make retries exactly-once per operation, not per batch — a client retrying a partially-overlapping batch after a timeout would otherwise double-apply what landed. - A tombstone beats a late upsert, and delete clears hostname/port: leaving the address would keep the server able to resolve a host the user believes they deleted. - Relay field validation mirrors the DB CHECK so a bad request is a clear Invalid rather than a constraint violation surfacing as a 500. Authorization goes through IVaultAccessService, which returns the same answer for "absent" and "forbidden" — distinguishing them is an existence oracle for other tenants' vault ids. Team vaults are explicitly denied until M3 rather than falling through to a permissive default. JIT provisioning keys on (issuer, subject), never email, and handles the concurrent-first-request race via the unique index. Renamed two domain types: Host -> SshHost, because Host collides with Microsoft.Extensions.Hosting.Host in every file of a web project, and SyncChange -> VaultChange to stop it colliding with the Contracts DTO of the same name. Aliasing at every use site would have been permanent friction. Worth noting: `ef migrations has-pending-model-changes` reported clean after those renames even though the snapshot still said "DodoSSH.Domain.Host" — it diffs tables, not CLR type names. The snapshot was regenerated and the emitted DDL diffed against the previous artifacts/schema/v0.1.sql to confirm the rename produced no schema change. Also removed ConfigureAwait(false) from test methods: xUnit1030 flags it as bypassing parallelization limits, which is why MA0004 is suppressed in test projects. Verified: 0 warnings on a clean rebuild, 146 tests pass (up from 122), format clean. Endpoint-level tests are the immediate next step: they need a WireMock OIDC/JWKS stub and real JWT minting, so the "wrong user is denied" matrix does not exist yet for these two routes. The service-layer authorization and the concurrency property are covered. |
||
|
jaap-jan
|
d3b14e6bc0 |
Add configuration, OIDC auth wiring and discovery endpoints (M1)
Options, JWT bearer validation, the /meta and .well-known endpoints, and a dev compose stack with Keycloak. Verified end to end: compose up, migrate, run, both discovery endpoints return correct payloads, and readiness reports the schema current. Configuration: - Strongly-typed options for Server, Oidc, Relay and Sync, all ValidateOnStart. A self-hosted server that boots half-configured and fails later per-request is far harder to diagnose than one that refuses to start and names the bad setting. - Cross-field validation the annotations cannot express: relay needs a WebSocketUrl when enabled, idle timeout must be under max session duration, item payload cap under batch cap. - Startup warnings for combinations that are individually valid but dangerous together: RequireHttpsMetadata false outside Development, and AllowEmailLinking (which turns any token bearing a victim's email into account takeover, hence default false). Auth: - JwtBearer with ClockSkew cut to 30s from the 5-minute default; five minutes of slack on a credential granting vault ciphertext access is more than any clock needs. - IncludeErrorDetails off, and a FallbackPolicy so an endpoint without an explicit policy still requires a caller rather than silently being public. Discovery, per ADR 0002: - /api/v1/meta reports versions, features and push caps. - /.well-known/dodossh-configuration is the onboarding story: the user types one server URL and the client discovers OIDC authority, client id, scopes and relay endpoint. Two environment problems found by actually running the stack: - PostgreSQL 18 changed its data mount point. Mounting /var/lib/postgresql/data — correct through 17 — makes the image refuse to start; 18+ wants a single mount at /var/lib/postgresql with the cluster in a subdirectory. - Keycloak moved to host port 18080. An unrelated Apache Tomcat on this machine holds 127.0.0.1:8080, and a loopback-specific bind beats Docker's 0.0.0.0 publish for "localhost". It presents as Keycloak 404ing every realm while its own log says the import succeeded, which is a genuinely misleading failure. Also: CA1848 is enforced, not advisory — warnings are errors, so the .editorconfig comment claiming otherwise was wrong. Startup and health logging now uses [LoggerMessage]. And a clean rebuild is back to zero warnings; the incremental build had been hiding 40 in test projects (banned Guid.NewGuid, an obsolete Testcontainers constructor, and two analyzer families that are genuinely noise under a test host). Verified: 0 warnings on a clean rebuild, 122 tests pass, format clean. |