lore-credential: Stop the keychain thrashing in the credential store - #170
Closed
mjansson wants to merge 1 commit into
Closed
lore-credential: Stop the keychain thrashing in the credential store#170mjansson wants to merge 1 commit into
mjansson wants to merge 1 commit into
Conversation
`encrypt_token` persisted a nonce counter into the OS keyring before every seal. The key never changed, but counter and key shared one blob, so every stored token rewrote the keyring entry — and tokens are stored on each authz exchange, not just at login. On macOS that entry is a legacy `SecKeychain` item whose ACL is a list of trusted binaries, so every application linking lore prompted for write access and every rebuild invalidated the grant and prompted again. Separately, `get_secret_from_store` treated any keyring error as "no key", so one denied prompt regenerated the key and called `reset_tokens()` for every lore process on the machine, which then provoked the next prompt. Draw a random 96-bit nonce per seal and carry it in the sealed blob instead. With no counter there is nothing to persist: the key is written once at init and only read from then on, and the keyring writes are gone. Narrow the absence check to `keyring::Error::NoEntry` so an unreadable store is an error rather than an invitation to replace the key. Give the store its own artifacts — `tokenstore.toml` and a `tokenstore_encryption_key` target, the latter also naming the on-disk key fallback — under an `org.lore` service, and migrate nothing. Sharing state is what caused the damage: a client that cannot read a token re-authenticates and rewrites the entry in its own format, and one that cannot read the key clears the store for everyone. Older versions keep their own files untouched, so both can run on one machine without resetting each other. The cost is one login on first use. Removing the counter also removes a real hazard: writing it back was not a compare-and-swap, so two processes could read the same value and seal with the same nonce. `SingleNonceSequence` now structurally prevents a key from sealing twice. With one wire format left, the format marker, the dual-path decode, the padding truncation and the legacy-key adoption all go. Also propagate the RNG failure in `generate_encryption_key`, which starts from a zeroed buffer of the right length and so silently returned an all-zero key that every later check accepts; and fix a latent bug where the keyring entry cache was a single `OnceLock` that returned whichever target was requested first for every subsequent target. Signed-off-by: Mattias Jansson <mjansson@gmail.com>
mjansson
force-pushed
the
mjansson/keyring-fixes
branch
from
August 16, 2026 12:40
2f11b00 to
991f9b6
Compare
|
Imported as Lore CR-366. |
epic-lore-bot
Bot
added
imported
epic-lore-bot Bot
pushed a commit
that referenced
this pull request
Aug 17, 2026
## Motivation `encrypt_token` persisted a nonce counter into the OS keyring before *every* seal. The key never changed — but counter and key shared one blob, so every stored token rewrote the keyring entry, and tokens are stored on each authz exchange, not just at login. On macOS that entry is a `SecKeychain` item whose ACL is a list of trusted binaries. Every application linking lore prompted for write access, and every rebuild invalidated the grant and prompted again. Separately, `get_secret_from_store` treated *any* keyring error as "no key", so a single denied prompt regenerated the key and wiped the token store — for every lore process on the machine, which then provoked the next prompt. ### The fix **Random nonces.** Each seal draws a fresh 96-bit nonce and carries it in the blob. No counter, so nothing to persist: the key is written once at init and only read from then on. The keyring writes are simply gone. **An unreadable store is not an empty one. **Only `NoEntry` counts as "no key"; every other keyring failure propagates. A denied or cancelled prompt can no longer rotate the key out from under anyone. **Its own files.** The store moves to `tokenstore.toml` and the key to a `tokenstore_encryption_key` target (which also renames the on-disk fallback) to avoid interference between old and new clients. ### Why nothing is migrated Sharing state is what caused the damage. An old client that can't read a token re-authenticates and rewrites the entry in its own format; one that can't read the key regenerates it and clears the store for everybody. Copying the old credentials forward would have kept both problems alive, and duplicating one-time-use refresh tokens across two clients would risk tripping rotation reuse-detection and logging out both. So the new store shares nothing and reads nothing older. Old clients keep using `tokens.toml` and their own keyring slot, untouched — the two generations can run side by side on one machine and neither can reset the other. **Cost: one `lore login` on first use.** ### Security Removing the counter also removes a real hazard: the old design had no compare-and-swap on the keyring, so two processes could read the same counter and seal with the same nonce — catastrophic under AES-GCM. Random 96-bit nonces need no shared state, and `SingleNonceSequence` structurally prevents a key from sealing twice. With one wire format left, the marker, the dual-path decode, the padding truncation, and the legacy-key adoption all go away (−263 lines). `generate_encryption_key` now propagates RNG failure rather than returning a zeroed key. An adversarial security review of the final diff found no reportable issues; nonce reuse, format confusion, key lifecycle, lock ordering, and secret exposure in logs were each examined and ruled out. ### Testing 11 new: seal/open round trip, nonce freshness and prefix layout, foreign-key rejection, malformed and short-blob handling) and 13 integration tests pass. ``` Imported-PR: #170 Imported-From: 991f9b6 Imported-Base: 935fccb Imported-Merge: a327586 Imported-Author: Mattias Jansson (mjansson) Signed-off-by: Mattias Jansson <mjansson@gmail.com> GH-URL: #170 ``` Lore-RevId: 586 Lore-Signature: 39954b2a04cb602a4e34dc6a4b523967c1e103ede172610fa91057af149987a9
|
Closed by mirrored commit 0ba8258. |
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.
Motivation
encrypt_tokenpersisted a nonce counter into the OS keyring before every seal. The key never changed — but counter and key shared one blob, so every stored token rewrote the keyring entry, and tokens are stored on each authz exchange, not just at login.On macOS that entry is a
SecKeychainitem whose ACL is a list of trusted binaries. Every application linking lore prompted for write access, and every rebuild invalidated the grant and prompted again.Separately,
get_secret_from_storetreated any keyring error as "no key", so a single denied prompt regenerated the key and wiped the token store — for every lore process on the machine, which then provoked the next prompt.The fix
Random nonces. Each seal draws a fresh 96-bit nonce and carries it in the blob. No counter, so nothing to persist: the key is written once at init and only read from then on. The keyring writes are simply gone.
**An unreadable store is not an empty one. **Only
NoEntrycounts as "no key"; every other keyring failure propagates. A denied or cancelled prompt can no longer rotate the key out from under anyone.Its own files. The store moves to
tokenstore.tomland the key to atokenstore_encryption_keytarget (which also renames the on-disk fallback) to avoid interference between old and new clients.Why nothing is migrated
Sharing state is what caused the damage. An old client that can't read a token re-authenticates and rewrites the entry in its own format; one that can't read the key regenerates it and clears the store for everybody. Copying the old credentials forward would have kept both problems alive, and duplicating one-time-use refresh tokens across two clients would risk tripping rotation reuse-detection and logging out both.
So the new store shares nothing and reads nothing older. Old clients keep using
tokens.tomland their own keyring slot, untouched — the two generations can run side by side on one machine and neither can reset the other. Cost: onelore loginon first use.Security
Removing the counter also removes a real hazard: the old design had no compare-and-swap on the keyring, so two processes could read the same counter and seal with the same nonce — catastrophic under AES-GCM. Random 96-bit nonces need no shared state, and
SingleNonceSequencestructurally prevents a key from sealing twice. With one wire format left, the marker, the dual-path decode, the padding truncation, and the legacy-key adoption all go away (−263 lines).generate_encryption_keynow propagates RNG failure rather than returning a zeroed key.An adversarial security review of the final diff found no reportable issues; nonce reuse, format confusion, key lifecycle, lock ordering, and secret exposure in logs were each examined and ruled out.
Testing
11 new: seal/open round trip, nonce freshness and prefix layout, foreign-key rejection, malformed and short-blob handling) and 13 integration tests pass.