feat(encryption): choose where this device's key is kept - #13
Merged
Conversation
Where the key is stored and whether it is passphrase-wrapped are now independent. `identity.toml` names one of four locations — the two existing inline fields, `keyring_account`, or `keys_command` — and for the two that don't say so themselves, `keys_encrypted` records the format. The inline fields are untouched, so existing files load unchanged and files written for the inline location still load on older builds. Retrieval and unwrapping had to split. A `keys_command` may prompt on the TTY while a passphrase-protected identity prompts inside the TUI, and both cannot own the terminal; the key is now fetched before raw mode is entered and unwrapped afterwards. Reconciling a remote disable moved ahead of the fetch so a retired key is never retrieved just to be thrown away. `write_stored_identity` writes new material back to wherever the key already lives rather than always inline, so rotating or re-wrapping cannot quietly drag the key out of the store the user chose. The one case that cannot work is a `keys_command` with no `keys_store_command`: it refuses up front, before a rotation appends the roster op it would have to roll back. The rotation snapshot now captures the key as well as the file. For an external key the file is only a pointer at something the rotation is about to overwrite, which made the on-disk rescue copy — documented as the only pre-rotation key — useless in exactly the case it exists for. Also replaces the combinatorial mutual-exclusivity match with a collect-and- count, and stops routing field validation through serde, where our own errors became indistinguishable from a parse failure and were flattened into "malformed". An unsupported schema version now says so.
The backup callout claimed identity.toml is the only thing that can decrypt, which stops being true once the key moves to a keychain or a secret manager. It now leads with `export-key`, and says restoring is copying the file back. Adds a section for the two axes with recipes for 1Password, pass, Vault and mounted secrets — the OS keychains are left out because `key-source keyring` reaches them directly.
Fills in the backend the key-source plumbing was already written against, and makes the keychain the default for new identities where one is reachable: it keeps the key out of a file that backups and sync tools copy without asking. `encryption enable` offers the choice and falls through to the identity file where there is no keychain, which is Android, iSH, and any headless session without D-Bus. The dependency is target-gated rather than feature-gated, like arboard, so no build needs a special flag and the platforms without a keychain pull in none of it. Linux and the BSDs use the async Secret Service backend: it speaks D-Bus through zbus, which is pure Rust and already a Linux dependency here, while the sync backend needs libdbus and would not cross-compile to musl, i686, armv7 or riscv64. zbus is pinned to async-io so it doesn't drag in tokio. Retiring an identity now inlines the key into the copy it renames aside before forgetting the stored one. That rename is documented as recoverable rather than a delete, which it wasn't for a key held anywhere but the file — the retired copy was a pointer at an item nothing referenced any more. The tests use an in-memory stand-in rather than the real keychain, so they don't write to a developer's login keychain and still run in CI, which has none. keyring's own mock builds a fresh credential per entry, so it cannot round-trip a store and a fetch.
Key retrieval is hoisted out of the unlock so it happens before the TUI takes the terminal: a `keys_command` may run pinentry or a hardware-token prompt, and a passphrase-protected identity prompts inside the TUI, so the two must never both be able to own the terminal. Consuming the prefetch with `take()` broke that on the retry path. A wrong passphrase left nothing cached, so the next attempt fetched again — from inside raw mode and the alternate screen, where a fetch command inherits a terminal crossterm is reading, and its prompt can neither be seen nor answered. Release the prefetch only once an unlock succeeds; `?` propagates before the clear, so a rejected passphrase leaves it in place. Drop `key_is_external` with it. It was the companion to the prefetch and never acquired a caller: the TUI asks the narrower question of whether the source can prompt, which is not the same predicate.
`Entry::new` only assembles a struct — every backend defers contact with the OS until a get or set — so probing availability with it answered `Ok` on any supported platform, keychain or not. The documented fallback to the identity file where nothing is reachable therefore never fired, and a headless session was offered a keychain it could not use. Probe by reading an account that is never written: a miss returns before any unlock or ACL check, so it costs one round trip and cannot raise a dialog. The error mapping was inverted against what `keyring` documents. `NoStorageAccess` is the store refusing — "it might be that the credential store is locked" — while an unreachable D-Bus arrives as `PlatformFailure`. Mapping the former to "no OS keyring is available here" told someone with a locked keychain to go set up a fetch command, and withheld that suggestion from the one person who needed it. A locked keychain now says so and asks to be unlocked, and counts as available: storing into it prompts to unlock, and callers treat a failed move as recoverable.
`store` drained the child's stdout to EOF and only then read its stderr, both on one thread. A store command that fills the stderr pipe buffer while holding stdout open blocks against a drain that will not reach it until stdout closes, and `wait` is never called — a hang with no timeout. Give each pipe its own thread, as the read path already does; neither stream can then be made to wait on the other. Moving a key to the keychain writes it under a freshly minted account and only afterwards records that account in the identity file. A failure between the two — a readback that disagrees, a file write that cannot complete — left a full copy of the key in the OS keychain under a label nothing referenced and nothing would ever collect. Unwind that write on every path out.
Every parse failure reported "key material is malformed". For a file that only points at a keychain or a fetch command there is no key material in it at all, so someone who mistyped their `keys_command` was told their key was corrupt and sent looking for a backup they did not need. The parser's own message cannot be passed on to say better: toml quotes the offending source line in every error shape, including unknown-field errors, and a parse that failed proves nothing about where a secret sits in the file. Report the line number instead — an offset carries nothing — and say the file is unreadable rather than accusing the key. `keys_store_command` without `keys_command` was accepted and then dropped: it names one location, so nothing rejected it, and the next write erased it from disk. That quietly turns a writable key source into one that cannot rotate or re-wrap. Refuse it.
Retiring the identity file has to inline the key first when it lives in the keychain or a secret manager, because the file alone is only a pointer and disabling drops what it points at. Nothing exercised that: the retired copy could have been a pointer at nothing, and the journal's own history would have become unreadable with no error to show for it. Drives it through a fetch command rather than the keychain — these tests link `notema-encryption` built without `cfg(test)`, so its in-memory double is not in play and a keyring source would reach the developer's own login keychain.
`device` had grown to eleven subcommands spanning two unrelated things — the roster of who may read the journal, and this device's own key — in three naming shapes at once. Split them: `device` keeps the roster, a new `key` group takes source, passphrase, rotate and export, and `status` answers "is it on, where is my key, who can read this" in one place instead of requiring two commands and still not covering it. `key status` rather than `key show`: it reports where the key is kept and whether a passphrase protects it, and never prints the key. `--store`'s help claimed it was run once and not recorded. It is recorded, as both the code and the docs said; only the help was wrong. Clap now requires `--read` for `command` instead of a hand-written check, and naming either flag for a location that does not fetch is refused rather than silently ignored. New keys mint where they are asked to. Without a terminal to ask on that is the identity file, matching how `confirm` already treats a missing tty and reversing a default that silently put keys in a keychain a script could not reach; `--key-source` asks for one deliberately. `enroll` mints an identity too, so it offers the same choice `enable` does. A keychain that turns out to be unreachable no longer fails the command. By that point the store is encrypted and the key is safely in the identity file, which is a complete and working end state, and no probe is reliable enough to rule the case out in advance.
The keychain-backed key path is covered by an in-memory double, which is right for the logic built on top but leaves the platform backends and the mapping from `keyring::Error` onto ours entirely unexecuted — and that mapping is where two defects have already hidden. A developer's machine is the wrong place to close the gap: the tests would write to their login keychain, and on macOS an unsigned test binary has no stable code identity, so reading back what it just wrote raises a GUI prompt that would hang the suite. So the tests opt in via `NOTEMA_TEST_REAL_KEYRING`, and CI runs them on Linux against a throwaway gnome-keyring inside a session bus that exists only for that job. They go through the public store API rather than the keyring module, so they cover the path a user takes — including the readback that has to fetch from the keychain to pass — and both move the key back out afterwards so nothing is left behind.
Several claims no longer matched, or never did. Retrieval was said to always happen before the app takes the terminal, which the unlock retry broke. "New identities default to the keychain" covered `enable` only, while `enroll` mints an identity too. The command reference still used the old `device key-source` and `device export-key` spellings, and the version that first understood key locations was guessed at a release that has not happened. Three things were true and undocumented, all of them the kind a reader would want to know before choosing the keychain. Creating a keychain-backed key writes it to `identity.toml` first and moves it after, so the file is replaced rather than shredded. A rotation keeps a self-contained rescue copy in the config directory for its duration. And `keys_command` is a shell line run on every unlock, which makes a writable identity file code execution — worth saying plainly for a file that was inert data until key locations existed. `identity.toml` had only a field table where `config.toml` and `state.toml` get worked examples; give it one per source, since it is the file the docs tell you to back up.
`key status` printed a strict subset of what `encryption status` already prints, so it was a second way to ask one question. The only case it answered better was a roster that would not verify — and that was really a defect in `encryption status`, which printed the local half of the report and then abandoned it with a non-zero exit. A roster that cannot be verified is a fact about the encryption state, so report it as one. The local half stays knowable and is exactly what someone in that position needs; the recipients are deliberately not listed, since they are precisely what could not be vouched for. `print_device_roster` now does every fallible read before printing, so a caller that reports its error rather than propagating it cannot contradict output already on screen. That leaves `key` as four verbs, which is what it is for.
"Source" describes only the read direction. The location is written to as well — needing a separate command for that is the evidence — so `key store` names it for what it is, and matches how the docs already talk about the secret managers behind it. That leaves `--store` meaning something different from the subcommand above it, so the flags become `--read` and `--write`: a symmetric pair, and no word doing two jobs in one invocation. The `keys_store_command` field in `identity.toml` keeps its name. A key is not kept *in* a command, so `KeySource::whereabouts` replaces `label`: a whole clause, letting the command case read "fetched by" where the other two read "kept in". "Stored unprotected" was only ever true of the identity file. A keychain guards what it holds with the login password, and a secret manager with whatever unlocks it, so say which one is doing the guarding — and for a fetch command, that notema cannot vouch for it either way.
paviro
marked this pull request as ready for review
August 4, 2026 00:40
The CLI calls it a key store; the library still called it a source, which only ever described the read direction. `KeySource` becomes `KeyStore`, `DeviceIdentityInfo::source` becomes `store`, and the two error variants follow. `KeyTarget`'s write command is `write`, matching the flag that sets it. No behaviour change, and the on-disk `keys_store_command` keeps its name.
Reject an identity file that names a key location without saying whether what it holds is passphrase-wrapped, rather than defaulting to cleartext and reporting the mismatch as corruption. Put the previous key back when a passphrase change reached an external store but the identity file could not record the new format, and say so when even that fails. Say what disabling encryption leaves behind: a key a --write command holds is named rather than reached into, and a key that can no longer be fetched no longer blocks the disable — the retired copy just says it is only a pointer. Let `key export` take a directory, and refuse to overwrite an existing file without --force. Run the real-keychain tests behind --ignored so they stay out of the default run, and fix the CI job that drives them.
A `keys_command` is a shell line run with the ambient environment every time the key is needed, so anyone who can write `identity.toml` can run code as its owner. The file was created 0600 and never checked again. On Unix, refuse to run one out of a file that is group- or world-writable, or owned by someone else. Owner and mode come from an fstat on the open handle, so the file that was judged is the file that gets read and a symlink is judged by its target. The parent directory is not checked: nobody can leave a file owned by us there, and a umask of 002 hands out a group-writable config directory. Refuse rather than repair. Tightening the mode would not undo a command already swapped in, and would erase the only sign that one could have been — so the error names the file, the mode and the chmod, and asks for the recorded lines to be checked first. Inline and keychain keys still open from a loose file. A lax mode there is a confidentiality problem, and locking someone out of their journal over a bit they can fix protects nothing once the key is readable; `encryption status` reports it instead, and prints the fetch command, which is the only defence against an identity file that arrived with someone's dotfiles.
`create_device_identity` is the single place a new key is minted, for both `initialize_store_identity` and `request_store_access`, and it only checked the device name. Overwriting is unrecoverable, and worse than losing the file: `write_stored_identity` reuses `current_location()`, so minting over a keyring- or command-backed identity writes through to that store too. First-run setup reaches it. `should_offer_encryption` gated on an empty journal list and no roster, neither of which excludes a config dir holding a restored `identity.toml` whose journal folder has not synced its roster yet — the state ENCRYPTION.md describes as normal mid-sync. It now skips the offer and points at `encryption device enroll` instead, matching what `encryption enable` already refuses to do.
`set_key_store` reads the key back from its new home before dropping the old copy, and ENCRYPTION.md promises that unconditionally. It only held for `key store`: `write_stored_identity` and `restore_identity` wrote and moved on, so `key rotate` and `key passphrase` had no such check. That matters most for a store command. `key_command::store` treats a command that exits 0 without reading all of its input as success, and a few-hundred-byte key fits the pipe buffer, so one that reads nothing at all still returns `Ok`. Rotation would then delete its rescue copy and re-encrypt the store to a key nothing holds. The check belongs in `KeyLocation::write` rather than in each caller, so no write path can skip it. `fetch_stored` now shares the same retrieval arm instead of repeating the match.
`disable_identity_file` inlined the key into the copy it renames aside, so the copy holds the key rather than pointing at a store nothing references. It fetched that key itself, from two callers that cannot answer a prompt: `resolve_access`, which the TUI calls inside raw mode and the alternate screen, and `reconcile_disabled_encryption`, which runs on every CLI subcommand. `key_command::run` has no timeout, by design — a Touch ID or pinentry prompt is legitimately unbounded — so a command-backed key hung the launch with the prompt drawn over the alternate screen. It now takes the material as a parameter and cannot retrieve anything. `unlock` keeps the `FetchedKey` it already retrieved instead of dropping it, which is what the retiring paths pass; it is the still-wrapped stored form, alongside the opened identity the store was keeping anyway. Without a key the renamed file is only a pointer, and both automatic paths took `.path` off the result and discarded the rest — so the TUI told the user their old key was "renamed aside, recoverable" in exactly the case it was not. `retired_key` carries that distinction now rather than a bool, and the warnings reach the CLI notice too.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Moves the age identity key out of
identity.tomland behind a configurable keystore — the OS keychain, or a command that fetches it from a secret manager —
and reorganises the encryption CLI around the two things it actually manages.
Key stores
identity.tomlkeepsdevice_nameand gains exactly one field naming where thekey lives:
plain_keys/encrypted_keys(inline, as before),keyring_account,or
keys_commandwith an optionalkeys_store_command. Format and store stayindependent: every store holds either a cleartext or a passphrase-wrapped bundle,
and moving a key never changes its format.
Before dropping the old copy, the move writes the key to its new home, reads it
back, and checks the same key comes out — a write command that quietly did
nothing can't leave you locked out. If that check fails, the copy it just made is
removed again.
CLI
devicehad grown to eleven subcommands spanning the roster and this device'sown key. Now:
encryption statusis the only place that reports state, and it answers thewhole question at once. It never prints the key and never fetches it — both
facts come from
identity.tomlalone, so it won't run yourop reador raise akeychain prompt just to tell you where the key is.
keyis four verbs; aread-only command there would only have repeated
status.statusreports rather than fails. A device roster that won't verify is a factabout the encryption state, so it appears in the output instead of aborting a
report that was already half printed — the local half stays knowable, which is
exactly what someone in that position needs. The recipients are deliberately not
listed in that case, since they are precisely what could not be vouched for.
New keys mint where they're asked to. Without a terminal to ask on that's the
identity file — the portable answer, since a keychain chosen for a script may be
unreachable in the session that has to open the key.
--key-storeasks for onedeliberately.
Wording
A key isn't kept in a command, so the report varies by store: "kept in the
identity file", "kept in the OS keychain", "fetched by an external command".
"Stored unprotected" was only ever true of the identity file — a keychain guards
what it holds with the login password, and a secret manager with whatever unlocks
it. So a key with no passphrase now says which thing is doing the guarding, and
for a fetch command admits that notema cannot vouch for it either way: