fix(oauth): store keyring tokens as one entry per provider#668
fix(oauth): store keyring tokens as one entry per provider#668euxaristia wants to merge 3 commits into
Conversation
The keyring backend combined every provider and MCP token into a single JSON blob under one keyring entry. On macOS, add-generic-password now writes through security -i, whose command parser caps a single write at 4095 bytes (Gitlawb#574). The combined blob has no such bound: three or more logged-in providers routinely exceeds it, so Set() starts failing for every provider, not just the one that pushed it over. Split storage into one keyring entry per token key, plus a small index entry listing which keys exist (KeyringClient has no list operation). Each write is now bounded to a single token's size, well under the line cap regardless of how many providers are logged in. Installs on the old combined-entry format keep reading correctly via a legacy fallback, and get migrated to per-key entries on the next save.
WalkthroughOAuth tokens are now stored as individually encoded keyring entries coordinated by a chunked index. Reads support legacy combined entries and serialized multi-entry access, while writes reconcile interruptions, deletions, and migration. Tests cover storage limits, corruption, migration, and lock behavior. ChangesOAuth keyring storage
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Store
participant keyringBlob
participant Index
participant TokenEntries
Store->>keyringBlob: Load or Status under read lock
keyringBlob->>Index: Read chunked token index
Index-->>keyringBlob: Return token keys
keyringBlob->>TokenEntries: Read each encoded token
TokenEntries-->>keyringBlob: Return available tokens
Store->>keyringBlob: Save updated store under write lock
keyringBlob->>Index: Publish union index
keyringBlob->>TokenEntries: Write or delete token entries
keyringBlob->>Index: Shrink to exact index and remove legacy entry
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/oauth/store_keyring_test.go (1)
131-172: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider testing the index/entry desync recovery path.
Migration and deletion-reconciliation coverage look solid. One gap:
read()'scontinuewhen a key is listed in the index but its entry is missing (store.go Lines 483-488) — the mechanism the design relies on for surviving interrupted writes — isn't exercised anywhere. A small test seeding an index that references a key with no corresponding entry (mimicking a killed process mid-write) would lock in that behavior.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/oauth/store_keyring_test.go` around lines 131 - 172, Add a focused test for the keyring store’s index/entry desynchronization recovery in the read path, such as the relevant Store read method. Seed the fake keyring with an index referencing one missing entry and at least one valid entry, then verify reading skips the missing key without returning an error and still returns the valid token. Model the setup after TestStoreKeyringMigratesLegacyCombinedEntry and use the existing fake keyring helpers and keyring symbols.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/oauth/store.go`:
- Around line 447-510: Update Store.Load’s keyring path to execute
keyringBlob.read() through blob.withLock, using the same cross-process locking
established for Save/Delete. Ensure the lock covers the complete index-and-entry
read so concurrent writes cannot produce a stale or incomplete result; preserve
existing read errors and returned data.
---
Nitpick comments:
In `@internal/oauth/store_keyring_test.go`:
- Around line 131-172: Add a focused test for the keyring store’s index/entry
desynchronization recovery in the read path, such as the relevant Store read
method. Seed the fake keyring with an index referencing one missing entry and at
least one valid entry, then verify reading skips the missing key without
returning an error and still returns the valid token. Model the setup after
TestStoreKeyringMigratesLegacyCombinedEntry and use the existing fake keyring
helpers and keyring symbols.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 23533b19-6702-40c4-8c89-873eaeb55792
📒 Files selected for processing (2)
internal/oauth/store.gointernal/oauth/store_keyring_test.go
Store.Load and Status read the keyring blob via several separate Get calls (index, then each entry), not one atomic snapshot, but only Save/Delete ran that read-modify-write under blob.withLock. An unlocked Load/Status could run concurrently with another process's Save/Delete mid write and observe a torn state. Route both through withLock like Save/Delete already do. Also add coverage for read()'s index/entry desync recovery: a key listed in the index whose own entry is missing must be skipped, not fail the whole read.
|
Addressed CodeRabbit's review.
go build, go vet, and go test -race -count=1 ./internal/oauth/... ./internal/keyring/... ./internal/credstore/... are all clean. |
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
-
[P1] Bound the key index as well as each token entry
internal/oauth/store.go:560
The newoauth-tokens-indexis still one base64-encoded value written throughKeyring.Set, and therefore through macOSsecurity -i's 4095-byte command-line cap.ValidateKeypermits 137-byte provider keys; an index containing 22 such keys serializes to 3,081 bytes and base64-expands to 4,108 bytes before thesecuritycommand framing, so the index write fails even when every token is tiny. The token entry has already been written, but it is not indexed and cannot be loaded. Use a bounded/chunked enumeration format (or another bounded scheme) and add a cap-aware regression test. -
[P1] Make the multi-entry update recoverable instead of leaving invisible credentials behind
internal/oauth/store.go:549
A write now has several externally visible steps with no durable recovery state. If the process dies after a per-tokenSetbut before the index write, the new credential is unindexed forever; if it dies orDeletefails after line 564 publishes a reduced index, a logged-out credential remains in the OS keychain forever because future cleanup only consults that reduced index. An index-write failure can also return an error after replacing an already-indexed token. This is especially harmful for refresh tokens: the caller can be told login/logout failed while the secret has already changed or remains resident but is unreachable through Zero. Add transactional/recovery metadata (or order the operations with a recoverable invariant) and failure-injection coverage for every interruption boundary. -
[P1] Keep the lock valid for the longer multi-key keyring operation
internal/oauth/store.go:609
The split format holdsoauth-keyring.lockfilewhile it runs one external keyring command per entry, butacquireFileLockreclaims any lock older than 30 seconds and never refreshes its mtime. Each command may legitimately take up to 10 seconds, so a normal read or write with only a few slow entries can exceed 30 seconds; another process then reclaims the live lock and resumes a concurrent read-modify-write, allowing the token-loss race this lock is intended to prevent. Refresh/lease the lock during the operation or use a stale timeout that safely covers the bounded keyring work. -
[P1] Preserve tokens when old and new binaries run during an upgrade or downgrade
internal/oauth/store.go:488
Once a new binary creates the index it ignores the legacy combined entry, while an older running binary continues to read and write only that legacy entry. After migration, an old process can save token C to the legacy blob; the next new-process save reads only the index, rewrites it without C, and deletes the legacy blob, silently losing C. The shared lock serializes the two processes but cannot reconcile the two schemas. Provide a compatibility/dual-write transition or otherwise detect and merge legacy writes before removing the legacy entry. -
[P2] Do not make file-backed reads fail behind a crashed writer's fresh lock
internal/oauth/store.go:279
LoadandStatusnow unconditionally acquire the blob lock, including for the file backend. File writes are atomic renames and reads were intentionally lock-free; after a writer crash leaves its lock file, the new read waits only five seconds and errors while the stale-lock threshold remains 30 seconds. That turns a recoverable process crash into roughly 30 seconds of OAuth read failures even though the last complete token file is readable. Restrict the read-side lock to the multi-get keyring backend, or adjust the file-lock recovery behavior so reads retain the former crash tolerance.
gnanam1990
left a comment
gnanam1990
left a comment
There was a problem hiding this comment.
Local review: built and ran go test ./internal/oauth on darwin/arm64; all pass. One real correctness concern worth confirming.
…xed-version safe - The key index is chunked: continuation entries hold overflow keys and are written before the header that references them, so every index entry stays under the macOS security -i 4095-byte line cap regardless of how many providers are logged in, and a torn chunk write is skipped on read like a missing token entry. - Writes follow a recoverable ordering: the union of the prior and new key sets is published first, token entries are written next, removed entries are deleted while the index still lists them, and only then does the index shrink. Every token entry that exists at any instant is listed in the published index, so an interrupted login/logout can never strand an invisible credential in the OS keychain; the next write reconciles. - A held lock's mtime is refreshed every 10s while the multi-command keyring operation runs, so the 30s stale-reclaim threshold only ever fires for a genuinely crashed holder, not a legitimately slow healthy one. - A legacy combined entry that reappears after migration was written by an old binary still running; its unseen keys are merged before the entry is deleted, so mixed old/new versions during an upgrade no longer lose freshly saved tokens. - Read-side locking is scoped to the keyring backend via a new blob.withReadLock: file-backend reads stay lock-free (writes are atomic renames), restoring crash tolerance when a writer dies holding the lock. - The cross-process lock path falls back to the OS temp directory when no config location resolves, instead of silently degrading to in-process serialization only.
|
Pushed 0b616f7 for jatmn's five findings and gnanam1990's lock-path comment.
go build, go vet, and go test ./internal/oauth pass locally. |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/oauth/store.go (1)
212-222: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winPrefer a per-user fallback before
/tmp.
acquireFileLockalready usesO_EXCLand0600, so symlink-following isn’t the issue, butos.TempDir()still gives a fixed lock path in a shared directory. On multi-user hosts another local user can pre-create or hold that file and make keyring saves time out.os.UserCacheDir()would avoid the shared-path DoS; add a test for the fallback branch.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/oauth/store.go` around lines 212 - 222, Update the fallback lock-path logic in the Store construction to prefer a per-user directory from os.UserCacheDir(), creating or selecting an appropriate cache location before falling back to os.TempDir() only if the user cache directory cannot be resolved. Preserve the resolved store-path behavior, and add coverage for the fallback branch verifying the per-user lock location.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/oauth/store_keyring_test.go`:
- Around line 570-573: Extend the crashed-writer lock test around Status to
verify that Status remains lock-free, rather than merely accepting a successful
call after stale-lock expiry. Add a bounded timing or immediate-return assertion
for s.Status("") while preserving the existing error and single-entry checks;
keep Load’s existing timing assertion unchanged.
---
Outside diff comments:
In `@internal/oauth/store.go`:
- Around line 212-222: Update the fallback lock-path logic in the Store
construction to prefer a per-user directory from os.UserCacheDir(), creating or
selecting an appropriate cache location before falling back to os.TempDir() only
if the user cache directory cannot be resolved. Preserve the resolved store-path
behavior, and add coverage for the fallback branch verifying the per-user lock
location.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6bf70feb-0af6-48ed-a808-dbcac2b2f97f
📒 Files selected for processing (2)
internal/oauth/store.gointernal/oauth/store_keyring_test.go
There was a problem hiding this comment.
Clean approve from me this latest commit addresses the earlier collaborator P1s (chunked index, recoverable multi-step write, lock-lease refresh, mixed-version legacy merge, lock-free file reads), and the interruption-boundary tests cover every write/delete step; build, vet, gofmt, and the oauth suite pass. Two minor things worth a glance, neither blocking: when ResolveStorePath fails the keyring lock falls back to a shared os.TempDir() file, which on a shared /tmp multi-user host could cross users (a per-user cache dir would be safer); and in the mixed-version window a token an old binary saves to the legacy entry stays invisible to new-binary Load until the next new-binary Save (the write path merges it, so it's not lost, just temporarily unreadable).
4 participants
Summary
PR #574 moved the macOS keyring write path to
security -i, which isnecessary to keep the secret out of the process list and out of a
getpass(3) /dev/tty prompt. But
security -i's command parser caps asingle write at 4095 bytes, and the oauth store combines every
provider and MCP token into one JSON blob under a single keyring
entry. That blob has no size bound of its own: three or more
logged-in providers routinely exceeds the cap, so Set() starts failing
for every provider, not just the one that pushed it over the line.
This splits keyring storage into one entry per token key (account =
key), plus a small index entry listing which keys currently exist,
since KeyringClient has no list operation. Each write is now bounded
to a single token's size, comfortably under the 4095-byte cap
regardless of how many providers are logged in.
Installs still on the old combined-entry format keep reading
correctly through a legacy fallback, and get migrated to per-key
entries (with the legacy entry removed) the next time anything is
saved.
Test plan
go build ./...make lint(fmt-check + vet)go test ./internal/oauth/... ./internal/keyring/... ./internal/credstore/... -racetokens and asserts no single keyring entry exceeds a 3000-byte
margin under the line cap
securityCLI in a throwawaytest keychain that the underlying
security -iwrite/readmechanism this builds on works correctly (quoting, round-trip,
length guard)
Summary by CodeRabbit