oauth: opt-in AES-256-GCM encrypted-at-rest token storage#213
Conversation
OAuth Phase 2: add an encrypted token-store backend. The default backend still writes the 0600 plaintext JSON unchanged; setting ZERO_OAUTH_STORAGE=encrypted-file encrypts the token file with AES-256-GCM under a per-user random 32-byte secret persisted 0600 beside the token file (<tokens>.secret). - internal/oauth/encrypt.go: aesGCMCrypter (nonce||ciphertext, GCM tamper detection) + load-or-create secret (atomic 0600 write). Pure stdlib, no dep. - Store gains an optional crypter: readState decrypts before unmarshal, writeState encrypts after marshal. A missing secret, a short blob, or a failed auth tag all fail closed (no silent empty/plaintext fallback). - CLI: newAuthManager selects the backend from ZERO_OAUTH_STORAGE; `zero auth` help documents it. Tests: crypter round-trip + tamper, secret create/persist/0600/wrong-size, store-through-encrypted (ciphertext on disk, round-trip, Status/Delete, tamper + missing-secret fail-closed). Gates: gofmt/vet/build(host+linux+windows)/test -race/staticcheck(no new)/govulncheck(0)/deadcode(no new) all pass. Local run check: device login under ZERO_OAUTH_STORAGE=encrypted-file writes a ciphertext file (no plaintext token), .secret is 0600, and `zero auth status` reads it back.
Zero automated PR reviewVerdict: No blockers found Blockers
Validation
ScopeHead: This deterministic review checks validation status and basic diff hygiene. A human reviewer still owns product judgment and design quality. |
WalkthroughAdds opt-in AES-256-GCM encrypted-at-rest storage for OAuth tokens. A new ChangesEncrypted OAuth Token Storage
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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
🤖 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/encrypt_test.go`:
- Around line 57-59: The code ignores errors from file I/O operations before
dereferencing or using their results, which can cause panics. At the os.Stat
call around line 58, check the error return value before calling info.Mode() on
the result. Similarly, at the os.ReadFile call around line 124, check the error
return value before indexing into the raw slice at line 125. Add proper error
handling (either return early or skip the check) for both locations to prevent
panics when these file operations fail.
🪄 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 Plus
Run ID: b5c8b442-f1b8-4728-93cb-7fbc1d9e94f4
📒 Files selected for processing (4)
internal/cli/auth.gointernal/oauth/encrypt.gointernal/oauth/encrypt_test.gointernal/oauth/store.go
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Verdict: APPROVED ✅
Reviewed locally in worktree review-pr213 at 563f0a3. The actual PR diff (1 commit on top of main) is 382 lines across internal/cli/auth.go (12 +), internal/oauth/encrypt.go (new, 116), internal/oauth/encrypt_test.go (new, 145), and internal/oauth/store.go (28 +). gofmt/vet/build clean; go test ./internal/oauth/... passes (9.2s). Full suite shows only the pre-existing TestRunExecUsesProjectConfigAndOpenAICompatibleProvider failure (reproduces on clean main, not this PR).
What it does: adds an opt-in Encrypted: true flag to oauth.StoreOptions. When set, the JSON token file is replaced by an AES-256-GCM ciphertext blob (nonce || ciphertext), with a separate 32-byte random secret persisted in <filePath>.secret (0600). Activated at the CLI by ZERO_OAUTH_STORAGE=encrypted-file. Default is unchanged (0600 plaintext JSON).
Security invariants verified by reading the diff:
- AES-256-GCM with random nonces via
crypto/rand. 32-byte secret generated on first use, persisted atomically (tmp + rename), 0600 permissions, dir 0700. - Fail-closed on missing secret:
openpassescreate=false, so a missing secret is a hard error rather than silently minting a new key that could never decrypt the existing file. - Tamper detection: GCM auth tag is verified; a flipped byte breaks
gcm.Openand surfaces as"wrong secret or tampered". - Short-blob guard:
len(blob) < gcm.NonceSize()is rejected before attempting decryption. - No key material in error messages: the only error that mentions the secret is
"token secret %s is missing"(path only, not the secret value). - Schema version unchanged: the unified store's
schemaVersion: 1is still validated, so a corrupted/foreign file fails closed.
Tests (encrypt_test.go): round-trip + tamper + missing-secret + short-blob + wrong-sized secret + atomic-rename + dir-creation, all green.
One small observation (not a blocker):
The CLI change wires ZERO_OAUTH_STORAGE=encrypted-file to Encrypted: true and documents it in auth --help. It would be nice to also plumb the keyring backend (from #216) through the same env var as a third option — that way the env var becomes a single switch: plain (default) | encrypted-file | keyring. The PR as-is is consistent with the "encrypted file" addition, and #216's keyring is the natural follow-up to make them siblings.
Merging.
🤖 Generated with Claude Code
|
@CodeRabbit rereview |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "=== PR Info ==="
gh pr view 213 --repo Gitlawb/zero --json title,state,body,files
echo ""
echo "=== PR Diff ==="
gh pr diff 213 --repo Gitlawb/zeroLength of output: 16695
🔴 Medium — TOCTOU race in
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
internal/oauth/encrypt.go (2)
104-106: ⚡ Quick winMissing error context wrapping on
MkdirAll.All other errors in
loadOrCreateSecretare wrapped withfmt.Errorf("oauth: ..."), but this one is returned bare. Wrap it for consistency and debuggability.if err := os.MkdirAll(filepath.Dir(path), 0o700); err != nil { - return nil, err + return nil, fmt.Errorf("oauth: create token secret directory: %w", err) }🤖 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/encrypt.go` around lines 104 - 106, The error returned from the os.MkdirAll call in the loadOrCreateSecret function is not wrapped with fmt.Errorf, while all other errors in this function are consistently wrapped with the "oauth: " prefix for context and debuggability. Wrap the error using fmt.Errorf with an appropriate message like "oauth: " followed by a descriptive message and the error variable to maintain consistency with the rest of the function's error handling.
29-34: 💤 Low valueRedundant
nilcheck fornow.
NewStorealready defaultsoptions.Nowtotime.Nowbefore passing it tonewAESGCMCrypter. This guard is dead code. Remove it or, if you want defense-in-depth, add a brief comment explaining why.🤖 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/encrypt.go` around lines 29 - 34, The nil check for the now parameter in the newAESGCMCrypter function is redundant because NewStore already ensures options.Now defaults to time.Now before calling this function. Either remove the entire if block that checks if now is nil and defaults it to time.Now, or if you prefer defense-in-depth protection, replace it with a brief comment explaining why the check is retained despite the earlier default assignment. The choice depends on whether you want to maintain defensive guard clauses or rely solely on the upstream default.internal/oauth/store.go (1)
270-277: ⚡ Quick winInconsistent newline handling between encrypted and plaintext paths.
Line 270 appends a trailing newline to
data, storing it inpayload. But line 273 sealsdata(without the newline) rather thanpayload. The plaintext backend writespayload(JSON + newline), while the encrypted backend seals onlydata(JSON, no newline).This inconsistency means decrypting and re-encrypting the file changes its content hash, and if you ever need to compare or diff the underlying JSON, the formats diverge. For uniformity, seal the same content:
payload := append(data, '\n') if s.crypter != nil { // Encrypted backend: the on-disk file is opaque ciphertext, not JSON. - payload, err = s.crypter.seal(data) + payload, err = s.crypter.seal(payload) if err != nil { return err } }Alternatively, skip the newline entirely for both paths if it's not meaningful, but consistency is the key point.
🤖 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 270 - 277, The newline is appended to data and stored in payload on line 270, but the encrypted backend only seals the original data without the newline on line 273, while the plaintext backend writes the full payload. To ensure consistency between encrypted and plaintext backends, change the seal call in the if block to seal payload instead of data, so both paths handle the same content with the trailing newline included.
🤖 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/cli/auth.go`:
- Around line 132-135: The oauth.NewStore call in the auth initialization
currently silently defaults to plaintext encryption (Encrypted: false) for any
unrecognized ZERO_OAUTH_STORAGE value, which can leave tokens unencrypted
without user awareness. Before creating the store, validate the
ZERO_OAUTH_STORAGE environment variable by retrieving and trimming its value,
then explicitly check if it is either empty (for plaintext) or equals
"encrypted-file" (for encryption). If the value is non-empty but not
"encrypted-file", return an error immediately to fail fast and alert the user of
the invalid configuration, rather than silently downgrading security.
In `@internal/oauth/encrypt.go`:
- Around line 111-114: The code has a TOCTOU race condition in the token secret
file creation where concurrent invocations can generate different secrets and
race on os.Rename, causing the losing process to use an orphaned secret that no
longer matches the on-disk file. After the successful os.Rename in the rename
error handling block, add logic to re-read the secret file from disk and use
that authoritative version instead of the locally generated secret. This ensures
the in-memory secret always matches what is actually stored on disk, preventing
token encryption/decryption mismatches when multiple processes compete to create
the secret file.
---
Nitpick comments:
In `@internal/oauth/encrypt.go`:
- Around line 104-106: The error returned from the os.MkdirAll call in the
loadOrCreateSecret function is not wrapped with fmt.Errorf, while all other
errors in this function are consistently wrapped with the "oauth: " prefix for
context and debuggability. Wrap the error using fmt.Errorf with an appropriate
message like "oauth: " followed by a descriptive message and the error variable
to maintain consistency with the rest of the function's error handling.
- Around line 29-34: The nil check for the now parameter in the newAESGCMCrypter
function is redundant because NewStore already ensures options.Now defaults to
time.Now before calling this function. Either remove the entire if block that
checks if now is nil and defaults it to time.Now, or if you prefer
defense-in-depth protection, replace it with a brief comment explaining why the
check is retained despite the earlier default assignment. The choice depends on
whether you want to maintain defensive guard clauses or rely solely on the
upstream default.
In `@internal/oauth/store.go`:
- Around line 270-277: The newline is appended to data and stored in payload on
line 270, but the encrypted backend only seals the original data without the
newline on line 273, while the plaintext backend writes the full payload. To
ensure consistency between encrypted and plaintext backends, change the seal
call in the if block to seal payload instead of data, so both paths handle the
same content with the trailing newline included.
🪄 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 Plus
Run ID: 5067943d-635d-4911-afc9-76fddd80a66f
📒 Files selected for processing (4)
internal/cli/auth.gointernal/oauth/encrypt.gointernal/oauth/encrypt_test.gointernal/oauth/store.go
Resolve CodeRabbit review comments on this PR:
- newAuthManager: reject an unsupported ZERO_OAUTH_STORAGE value instead of
silently downgrading to plaintext. A mistyped non-empty value now fails fast
("invalid ZERO_OAUTH_STORAGE ... (supported: encrypted-file)") so a user who
believes encryption is on is never left writing plaintext.
- loadOrCreateSecret: fix the first-run TOCTOU race. os.Rename publishes by
clobbering on POSIX, so two concurrent processes could each generate a secret
and the loser would silently orphan every token it then encrypts. Create the
secret exclusively (O_CREATE|O_EXCL); the loser adopts the winner's on-disk
secret. Factored the read+size-validate into readSecretFile and dropped the
now-unused clock plumbing from the crypter.
- encrypt_test: handle os.Stat / os.ReadFile errors before dereferencing the
result (mode/index) so a real I/O failure surfaces instead of a panic.
Adds a test asserting an invalid ZERO_OAUTH_STORAGE fails fast.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/encrypt.go`:
- Around line 103-105: When encountering os.ErrExist on the file creation
attempt, the current code immediately calls readSecretFile(path) which can fail
if the winning writer hasn't finished writing and closing the file yet. Add a
bounded retry loop around the readSecretFile(path) call in the errors.Is(err,
os.ErrExist) branch to allow time for the concurrent writer to complete. Include
a small backoff delay between retries to avoid spinning, and ensure the retry
count is reasonable (e.g., 3-5 attempts with small millisecond delays) so that
legitimate failures still propagate rather than hanging indefinitely.
🪄 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 Plus
Run ID: 6819520d-9d7e-42c9-93f6-ed6cf3f242ce
📒 Files selected for processing (5)
internal/cli/auth.gointernal/cli/auth_test.gointernal/oauth/encrypt.gointernal/oauth/encrypt_test.gointernal/oauth/store.go
🚧 Files skipped from review as they are similar to previous changes (3)
- internal/oauth/encrypt_test.go
- internal/cli/auth.go
- internal/oauth/store.go
|
Pushed
Added a test that an invalid @coderabbitai review |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "=== Latest PR Diff ==="
gh pr diff 213 --repo Gitlawb/zero
echo ""
echo "=== PR Commits ==="
gh pr view 213 --repo Gitlawb/zero --json commits --jq '.commits[-3:]'Length of output: 18780
✅ Fixed
The TOCTOU fix is the right approach: 🟡 Still open (low severity)1. // current — no context
if err := os.MkdirAll(filepath.Dir(path), 0o700); err != nil {
return nil, err
}
// fix
if err := os.MkdirAll(filepath.Dir(path), 0o700); err != nil {
return nil, fmt.Errorf("oauth: create secret dir: %w", err)
}2. payload := append(data, '\n')
if s.crypter != nil {
payload, err = s.crypter.seal(data) // ← still sealing data, not payloadThe sealed blob omits the trailing newline that plaintext gets. Harmless (GCM decryption and 🔵 Not addressed (UX polish, lowest priority)The mode-switch confusing error — when a user toggles The two remaining items are both minor. The core correctness and security properties are solid — approve when ready. |
CodeRabbit follow-up: the O_EXCL create path read the on-disk secret immediately after EEXIST, but the winner may not have finished writing it yet, so a loser could observe a short file and fail transiently. Retry the read briefly (bounded) so concurrent first-run invocations converge on the winner's secret. Adds a -race concurrency test asserting convergence.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/oauth/encrypt.go (1)
95-96:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winWrap
os.MkdirAllerror with context for consistency.Every other error path in this function wraps errors with
fmt.Errorf("oauth: ..."), but this one returns the raw error. Minor inconsistency that makes debugging harder if directory creation fails.Proposed fix
if err := os.MkdirAll(filepath.Dir(path), 0o700); err != nil { - return nil, err + return nil, fmt.Errorf("oauth: create token secret directory: %w", err) }🤖 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/encrypt.go` around lines 95 - 96, The error from os.MkdirAll is returned raw without wrapping, while all other error paths in this function use fmt.Errorf with an "oauth: " prefix for consistency. Modify the return statement to wrap the error from os.MkdirAll using fmt.Errorf with a descriptive message like "oauth: failed to create directory" to maintain consistency with the error handling pattern used throughout the rest of the function.
🤖 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.
Outside diff comments:
In `@internal/oauth/encrypt.go`:
- Around line 95-96: The error from os.MkdirAll is returned raw without
wrapping, while all other error paths in this function use fmt.Errorf with an
"oauth: " prefix for consistency. Modify the return statement to wrap the error
from os.MkdirAll using fmt.Errorf with a descriptive message like "oauth: failed
to create directory" to maintain consistency with the error handling pattern
used throughout the rest of the function.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: ad9e3dd7-5dcf-4484-a2e8-8619414eb13f
📒 Files selected for processing (2)
internal/oauth/encrypt.gointernal/oauth/encrypt_test.go
Resolve the conflicts from #213 (AES-256-GCM encrypted token store) landing on main, which overlapped #216's keyring/store-unification work in the same files: - internal/oauth/store.go: unify the two backends. Keep #216's blobStore architecture (file vs keyring) and graft #213's optional crypter onto the file path, so the three backends compose: "" / "file" (plaintext), "encrypted-file" (AES-256-GCM, crypter set), and "keyring". StoreOptions keeps Encrypted as a legacy alias for Storage=="encrypted-file"; readState decrypts and writeState seals when crypter != nil. - internal/cli/auth.go: newAuthManager validates ZERO_OAUTH_STORAGE against {file, encrypted-file, keyring} and passes it through as Storage (the auto-merge had kept #213's encrypted-only validation, which rejected keyring); merge the help text to document both encrypted-file and keyring. Both feature suites pass (encrypt + keyring store, the invalid-storage CLI guard, mcp/swarm/keyring); adds a test for the explicit Storage="encrypted-file" selector. The only remaining cli failures are the pre-existing env-dependent doctor/exec tests that pass in CI.
Bring the branch current with main (which now carries #213's AES-256-GCM encrypted token store, #218, #219, #204). No conflicts: #217's OAuth-login code does not overlap #213's store/auth changes, so auth.go and store.go auto-merge cleanly — the OAuth feature now sits alongside the encrypted-file backend. Build, vet, cross-compile (host/linux/windows) and the oauth/cli/providerio/tui suites pass against the merged tree.
2 participants
Summary
OAuth Phase 2 — adds an opt-in encrypted-at-rest token store. The default backend is unchanged (0600 plaintext JSON); setting
ZERO_OAUTH_STORAGE=encrypted-fileencrypts the token file with AES-256-GCM under a per-user random 32-byte secret persisted 0600 beside it (<tokens>.secret). Pure stdlib — no new dependency.Builds on the merged
internal/oauthengine (#210); branches offmain.How it works
internal/oauth/encrypt.go—aesGCMCrypter: seal = random-nonce ‖ ciphertext; GCM gives confidentiality and tamper detection. Secret is load-or-create with an atomic 0600 write.Storegains an optional crypter:readStatedecrypts before unmarshal,writeStateencrypts after marshal. A missing secret, short blob, or failed auth tag all fail closed — never a silent empty/plaintext fallback.newAuthManagerselects the backend fromZERO_OAUTH_STORAGE;zero authhelp documents it.Security
Confidentiality + integrity at rest (AES-256-GCM), per-user random secret, 0600 on both files, fail-closed on tamper/missing-secret. Token material is still never logged (status output unchanged).
Verification
Tests: crypter round-trip + tamper; secret create/persist/0600/wrong-size fail-closed; store-through-encrypted (ciphertext on disk, round-trip,
Status/Delete, tamper + missing-secret fail-closed).Gates: gofmt · vet · build (host + linux + windows) ·
go test ./...incl.-race· staticcheck (no new) · govulncheck (0) · deadcode (no new vs main).Local run check (built binary): a
--devicelogin underZERO_OAUTH_STORAGE=encrypted-fileagainst a local fake OAuth server:Logged in to demo.schemaVersion), binary hexdump;<tokens>.secretpresent at mode 600;zero auth statusdecrypts and reads it back (demo: logged in (refreshable), expires …), leaking no secret material.Follow-ups (unchanged from before)
OS-keyring backend (needs a third-party dep) and unifying the MCP token store onto
internal/oauth's namespaced store (transparent migration) remain separate follow-ups.Summary by CodeRabbit
New Features
ZERO_OAUTH_STORAGE=encrypted-file(AES-256-GCM), storing tokens on disk as ciphertext with a companion secret file.Documentation
ZERO_OAUTH_TOKENS_PATHoverride,0600permissions, and encryption behavior.Tests
ZERO_OAUTH_STORAGEvalues.