From dcaed9934e5cca79c85eab2502fc51490b0b062a Mon Sep 17 00:00:00 2001 From: Fredrik Johansson Date: Sun, 12 Jul 2026 19:54:45 +0200 Subject: [PATCH] Fix documentation drift from the resolved-open-questions pass MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit IMPLEMENTATION.md hadn't been updated after the second implementation pass — the data model SQL was missing prev_ciphertext/prev_nonce, can_write, and the entire vault_grants_previous table; the Rotation section described the old "overwritten, not versioned" behavior (the opposite of what got built); the crypto scheme section still said "read implies write" while the Resolved design decisions section below it said the opposite; the CLI surface listing was missing --purge/--previous/--read-only; and "What was verified" only covered the first verification pass, not the read-only/rollback/purge one. README's Core model was missing any mention of the retention/purge behavior or read/write grant scoping. INTEGRATION.md now recommends --read-only for deploy identities specifically, now that the capability exists. Co-Authored-By: Claude Sonnet 5 --- IMPLEMENTATION.md | 142 +++++++++++++++++++++++++++++++++------------- INTEGRATION.md | 5 ++ README.md | 8 ++- 3 files changed, 113 insertions(+), 42 deletions(-) diff --git a/IMPLEMENTATION.md b/IMPLEMENTATION.md index 2a66243..0bfa38d 100644 --- a/IMPLEMENTATION.md +++ b/IMPLEMENTATION.md @@ -48,11 +48,9 @@ README's "Core model") to libsodium primitives: who's pushing/pulling, for the access log) and encryption (wrapping). - **Push authentication**: every push is signed by the pusher's Ed25519 key so the access log records a real identity, not just "someone with - write access to this vault." Whether *anyone* with any registered - identity can push to a vault they're a recipient of, or whether push - requires a separate "can write" grant distinct from "can read," was an - open question at design time — resolved as "read implies write" for v1 - (see Open Questions). + write access to this vault." Whether having a read grant should imply + write too was an open question at design time — resolved as *no*; see + "Resolved design decisions" below. ## Data model @@ -61,11 +59,15 @@ own key string, not a foreign-keyed `projects`/`environments` pair: ```sql CREATE TABLE IF NOT EXISTS vaults ( - vault_key TEXT PRIMARY KEY, -- e.g. "myapp/production" — free-form, app-chosen - ciphertext BLOB NOT NULL, -- crypto_secretbox(payload, vault_symmetric_key) - nonce BLOB NOT NULL, - updated_at INTEGER NOT NULL, - updated_by TEXT NOT NULL -- recipient_id of whoever last pushed + vault_key TEXT PRIMARY KEY, -- e.g. "myapp/production" — free-form, app-chosen + ciphertext BLOB NOT NULL, -- crypto_secretbox(payload, vault_symmetric_key) + nonce BLOB NOT NULL, + -- Exactly one previous version, retained as a rollback safety net. + -- NULL if there is none, or the last push explicitly purged it. + prev_ciphertext BLOB, + prev_nonce BLOB, + updated_at INTEGER NOT NULL, + updated_by TEXT NOT NULL -- recipient_id of whoever last pushed ); CREATE TABLE IF NOT EXISTS recipients ( @@ -82,17 +84,29 @@ CREATE TABLE IF NOT EXISTS vault_grants ( vault_key TEXT NOT NULL REFERENCES vaults(vault_key), recipient_id TEXT NOT NULL REFERENCES recipients(recipient_id), wrapped_key BLOB NOT NULL, -- crypto_box_seal(vault_symmetric_key, recipient_pubkey) + can_write INTEGER NOT NULL DEFAULT 1, -- read-only grants can pull but not push/grant granted_at INTEGER NOT NULL, PRIMARY KEY (vault_key, recipient_id) ); +-- Mirrors vault_grants for the one retained previous version — wrapped +-- keys as they stood at the time of the push that superseded them, so a +-- recipient who had access then can still unwrap it via --previous even +-- if their current grant has since changed. +CREATE TABLE IF NOT EXISTS vault_grants_previous ( + vault_key TEXT NOT NULL, + recipient_id TEXT NOT NULL, + wrapped_key TEXT NOT NULL, + PRIMARY KEY (vault_key, recipient_id) +); + -- Access log — metadata only, exactly what the threat model says the -- server is allowed to know. CREATE TABLE IF NOT EXISTS access_log ( id INTEGER PRIMARY KEY AUTOINCREMENT, vault_key TEXT NOT NULL, recipient_id TEXT NOT NULL, - action TEXT NOT NULL, -- 'pull' | 'push' + action TEXT NOT NULL, -- 'pull' | 'push' | 'grant' | 'revoke' accessed_at INTEGER NOT NULL ); ``` @@ -105,19 +119,42 @@ to it are separate steps, so a vault can't accidentally be pushed ## Rotation -`keep push ` with new secret values: +`keep push ` with new secret values, run by a recipient holding a +*write* grant (or by anyone, for a brand-new vault key — see "Push +authorization" below): 1. Generate a fresh random symmetric key. 2. Encrypt the new payload under it. -3. Look up every current row in `vault_grants` for this vault. +3. Fetch the current `vault_grants` rows for this vault (via `GET + /vaults/:key/recipients`, itself grant-gated) — each recipient's + public key and current `can_write` flag. 4. Re-wrap the new symmetric key for every one of those recipients' - public keys (already have them via `recipients.public_key`). -5. Replace `vaults.ciphertext`/`nonce` and all `vault_grants.wrapped_key` - rows for this vault, atomically (single transaction). + public keys, preserving each one's `can_write` flag as-is — a + rotation never silently upgrades or downgrades anyone's write access. +5. Unless `--purge`: copy the *outgoing* `vaults.ciphertext`/`nonce` into + `prev_ciphertext`/`prev_nonce`, and copy the outgoing `vault_grants` + rows into `vault_grants_previous`, before overwriting them. With + `--purge`: skip that copy, and also clear out whatever previous + version already existed — a purge means nothing from before this push + survives, not just that this push doesn't add to a stack. +6. Replace `vaults.ciphertext`/`nonce` and all `vault_grants` rows for + this vault, atomically (single transaction covering steps 5 and 6). -The old symmetric key and old wrapped copies are simply gone — -overwritten, not versioned, in v1 (see Open Questions on whether -version history is worth adding). +## Rollback and purge + +`keep pull --previous` fetches `prev_ciphertext`/`prev_nonce` +plus the caller's row in `vault_grants_previous`, if any — a plain 404 if +there is no previous version, or the caller didn't have access to it. +This is a *safety net* for "I pushed a typo'd value," not an audit trail: +exactly one previous version is kept, not a stack. + +`keep push --purge` is the compromise-response path: it rotates +the vault the same as a normal push, except it never retains the +outgoing version as "previous" and actively deletes whatever previous +version already existed. The distinction matters because the entire +point of rotating in response to a leak is that the *old* value stops +being retrievable by anyone — a routine push's one-step rollback safety +net would directly undermine that if it applied here too. ## Revocation @@ -125,11 +162,15 @@ Deleting one `vault_grants` row (`vault_key`, `recipient_id`) removes that recipient's ability to `pull` — the very next pull attempt with their identity finds no wrapped-key row and is rejected. No re-encryption of the payload needed; every other recipient's access is untouched. +(`vault_grants_previous` rows are left alone by a plain revoke — access +to the previous version isn't retroactively pulled just because current +access was revoked; a `--purge` push is what actually removes that.) -As stated in the threat model: this stops *future* pulls only. If -revocation is happening because a machine may have been compromised, -follow it with `keep push` to actually rotate the secret values, not just -the grant. +As stated in the threat model: revocation alone stops *future* pulls +only. If revocation is happening because a machine may have been +compromised, follow it with `keep push --purge` to actually +rotate the secret values and ensure the old ones aren't retrievable via +`--previous` either — not just remove the grant. ## CLI surface @@ -138,9 +179,9 @@ keep identity init -- generate + persist a local Ed25519 keep identity show -- print this machine's public key, for registering as a recipient keep identity set-id -- record the id an admin assigned after registering this identity -keep push [--file .env] -- encrypt + upload; requires this identity to already be a grantee -keep pull [--format env|json] [--out ] -- fetch + decrypt; requires a grant for this identity -keep grant -- add a new recipient to a vault you already have access to +keep push [--file .env] [--purge] -- encrypt + upload; requires a WRITE grant on an existing vault +keep pull [--format env|json] [--out ] [--previous] -- fetch + decrypt; requires a grant +keep grant [--read-only] -- add a recipient to a vault you have WRITE access to keep log -- tail the access log for one vault # admin operations — gated by ADMIN_PASSWORD, see below @@ -217,22 +258,41 @@ doesn't fit an existing deploy setup. ## What was verified end-to-end -Two independent local identities against a real running server (and -separately against the built Docker image): registration, a push, -pulling as the pusher, pulling as an ungranted second identity (correctly -rejected), granting the second identity access without re-pushing, -pulling as the now-granted second identity (correctly decrypts the same -underlying secret via its own sealed key copy), admin revocation, a -subsequent rotation confirming the revoked identity is excluded from the -new wrap set, and rejection of both missing and malformed signed-request -auth. +**Initial implementation**, two independent local identities against a +real running server (and separately against the built Docker image): +registration, a push, pulling as the pusher, pulling as an ungranted +second identity (correctly rejected), granting the second identity +access without re-pushing, pulling as the now-granted second identity +(correctly decrypts the same underlying secret via its own sealed key +copy), admin revocation, a subsequent rotation confirming the revoked +identity is excluded from the new wrap set, and rejection of both +missing and malformed signed-request auth. -One real implementation bug caught during this process, worth recording: -Express's `req.path` inside a sub-router is relative to that router's -mount point (e.g. `/vaults/x/pull` instead of `/api/vaults/x/pull`), -which would have silently mismatched a client that signs the full request -path. Fixed by verifying against `req.originalUrl` (path portion only) -instead, which stays consistent regardless of router nesting. +**Resolved-decisions pass**, three independent identities: a read-only +grant correctly blocked from both `push` and `grant`; write access and +read-only status both correctly preserved across a rotation (confirmed +by the "wrapped for N recipients" count and by the read-only recipient +still being able to pull but not push afterward); `--previous` returning +the prior version for a recipient who had access at that time; a +`--purge` push confirmed to make the previously-current version +unavailable via `--previous` for every recipient, not just the pusher; +re-verified against a fresh Docker build. + +Two real implementation bugs caught during this process, not just +written up: + +- Express's `req.path` inside a sub-router is relative to that router's + mount point (e.g. `/vaults/x/pull` instead of `/api/vaults/x/pull`), + which would have silently mismatched a client that signs the full + request path. Fixed by verifying against `req.originalUrl` (path + portion only) instead, which stays consistent regardless of router + nesting. +- The CLI's `--previous` flag initially signed a path that included its + own query string (`?previous=1`), but the server verifies against + `req.originalUrl` with the query string stripped — every `--previous` + request would have failed signature verification. Fixed by splitting + the signed path from the request URL in `signedFetch`, signing only + the former. ## Resolved design decisions diff --git a/INTEGRATION.md b/INTEGRATION.md index e7499d7..5df7066 100644 --- a/INTEGRATION.md +++ b/INTEGRATION.md @@ -26,6 +26,11 @@ printed public key to whoever runs `keep recipient add`, then deploy from that machine just works — no more "wait, what's the DB password again" when setting up a new box. +A deploy identity should almost always be granted **read-only** access +(`keep grant --read-only`) — it only ever +needs to `pull`, and a read-only grant means a compromised deploy box +can't also rotate the vault or add itself more recipients. + ## Pattern: docker-compose reading a local `.env` A common shape — `docker compose up -d` reads `environment:` values from diff --git a/README.md b/README.md index 19a5066..32c293c 100644 --- a/README.md +++ b/README.md @@ -65,7 +65,13 @@ recipient's public key — into a live service instead of a static file: the payload and every other recipient's access are untouched. 6. Rotating a secret is `keep push ` again — a new random symmetric key, re-wrapped for every currently-granted recipient. Cheap, - since the recipient list rarely changes. + since the recipient list rarely changes. By default the outgoing + version is kept as a one-step rollback safety net (`keep pull + --previous`); `keep push --purge` skips that for compromise-driven + rotations where the old value should stop being retrievable, full stop. +7. Grants distinguish read from write — a recipient can be given + read-only access (`keep grant --read-only`), which can pull a vault + but can't push to it or grant others access. Full design — data model, the atomic-grant/revoke details, and what this explicitly does and doesn't protect against — is in