fix(keychain): prefer file-based master key to fix background/automat…#1059
fix(keychain): prefer file-based master key to fix background/automat…#1059Zhang-986 wants to merge 1 commit into
Conversation
…ion context on macOS
📝 WalkthroughWalkthroughThis PR adds automatic migration of secrets from the macOS system keychain to file-based master key storage. When ChangesmacOS Keychain Secret Migration
Sequence DiagramsequenceDiagram
participant platformGet
participant systemKeychain as System Keychain Master Key
participant migrateToFileMasterKey
participant fileMasterKey as File-Based Master Key
participant filesystem as File System
platformGet->>systemKeychain: decrypt ciphertext
systemKeychain-->>platformGet: plaintext
platformGet->>migrateToFileMasterKey: plaintext
migrateToFileMasterKey->>fileMasterKey: get or create
fileMasterKey-->>migrateToFileMasterKey: file-based key
migrateToFileMasterKey->>migrateToFileMasterKey: encrypt plaintext (AES-GCM)
migrateToFileMasterKey->>filesystem: write temp file
filesystem-->>migrateToFileMasterKey: temp path
migrateToFileMasterKey->>filesystem: atomic rename to final path
filesystem-->>migrateToFileMasterKey: success
platformGet-->>caller: plaintext
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
github-actions
Bot
added
the
size/M
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR updates the macOS keychain implementation to prefer a file-based master key for background/automation compatibility, and adds logic to migrate existing secrets from the system keychain master key on successful reads.
Changes:
- Change
platformSetfallback order to create/use a file-based master key before attempting system keychain access. - Add best-effort migration on
platformGetto re-encrypt secrets with the file-based master key after decrypting with the system keychain key. - Add Darwin-specific tests covering “file key created first” and “migration on read” behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| internal/keychain/keychain_darwin.go | Adds migration helper and adjusts master-key selection order to prefer file-based key. |
| internal/keychain/keychain_darwin_test.go | Adds tests validating file-first behavior and migration away from system keychain. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![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/keychain/keychain_darwin.go (1)
278-298: 💤 Low valueConsider using
deferfor temp file cleanup to matchplatformSetpattern.If
WriteFilefails after partially creating the file, the temp file won't be cleaned up. While unlikely to cause issues (UUID-named files), usingdeferwould be consistent withplatformSet(line 323).♻️ Suggested change for consistency
dir := StorageDir(service) targetPath := filepath.Join(dir, safeFileName(account)) tmpPath := targetPath + "." + uuid.New().String() + ".tmp" + defer vfs.Remove(tmpPath) if vfs.WriteFile(tmpPath, encrypted, 0600) != nil { return } - if vfs.Rename(tmpPath, targetPath) != nil { - _ = vfs.Remove(tmpPath) - } + _ = vfs.Rename(tmpPath, targetPath) }🤖 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/keychain/keychain_darwin.go` around lines 278 - 298, migrateToFileMasterKey currently creates a temp file tmpPath with vfs.WriteFile and only removes it on Rename failure; add a defer immediately after creating tmpPath (after the WriteFile succeeds) to ensure the temp file is removed on any early return or error (similar to platformSet pattern). Specifically, after vfs.WriteFile(tmpPath, ...) check for error, then set defer func() { _ = vfs.Remove(tmpPath) }(), then proceed to vfs.Rename(tmpPath, targetPath) and only cancel/remove the defer if Rename succeeds (or keep it and no-op on missing file); reference migrateToFileMasterKey, tmpPath, vfs.WriteFile, vfs.Rename, vfs.Remove and platformSet for locating the pattern.
🤖 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/keychain/keychain_darwin_test.go`:
- Around line 65-68: Remove the unused keychainKey variable and its population
loop: delete the declaration "keychainKey := make([]byte, masterKeyBytes)" and
the for loop that assigns to keychainKey (the byte-filling loop referencing
masterKeyBytes and keychainKey). This eliminates dead code in
keychain_darwin_test.go and keeps the test focused on verifying that the system
keychain is not accessed.
---
Nitpick comments:
In `@internal/keychain/keychain_darwin.go`:
- Around line 278-298: migrateToFileMasterKey currently creates a temp file
tmpPath with vfs.WriteFile and only removes it on Rename failure; add a defer
immediately after creating tmpPath (after the WriteFile succeeds) to ensure the
temp file is removed on any early return or error (similar to platformSet
pattern). Specifically, after vfs.WriteFile(tmpPath, ...) check for error, then
set defer func() { _ = vfs.Remove(tmpPath) }(), then proceed to
vfs.Rename(tmpPath, targetPath) and only cancel/remove the defer if Rename
succeeds (or keep it and no-op on missing file); reference
migrateToFileMasterKey, tmpPath, vfs.WriteFile, vfs.Rename, vfs.Remove and
platformSet for locating the pattern.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: bfd21dc6-b966-4862-8293-b517a47d6a87
📒 Files selected for processing (2)
internal/keychain/keychain_darwin.gointernal/keychain/keychain_darwin_test.go
|
@Zhang-986 This PR auto-migrates inside Discoverability and reversibility — the user opts in knowingly, the OS Keychain entry is preserved as a cold backup, and the action appears in shell history. |
背景
Closes #1038
macOS 上在后台/自动化场景跑
auth status --verify会报 "keychain not initialized"，但同一个 profile 在交互式终端里完全正常。原因是
platformSet优先用 system keychain 的 master key 来加密凭据，而 system keychain 在没有 GUI session 的情况下根本访问不了。用户在交互式终端里config init存的密钥，到了自动化脚本里就读不出来了。改了什么
1.
platformSet里调换了 key 优先级原来：读已有 file key → system keychain（允许创建）→ 创建 file key
现在：读已有 file key → 创建 file key → system keychain（允许创建）
这样新存的凭据一律用 file key 加密，后台和交互式都能读。
2.
platformGet里加了自动迁移当
platformGet通过 system keychain 成功解密后，会自动用 file key 重新加密。老用户不需要重新配置，下次读取就自动迁移了。What changed
1. Swapped key priority in
platformSetBefore: existing file key → system keychain (allow create) → create file key
After: existing file key → create file key → system keychain (allow create)
New credentials are now always encrypted with the file-based key, which works in both interactive and background contexts.
2. Added automatic migration in
platformGetWhen a secret is successfully decrypted using the system keychain key, it gets re-encrypted with the file-based key. Existing users are migrated on their next read — no manual reconfiguration needed.
测试
go test ./internal/keychain/... -race— 5 个测试全过go vet/gofmt— 没问题TestPlatformSetCreatesFileMasterKeyFirst— 验证新凭据用 file key，不碰 system keychainTestMigrateToFileMasterKey— 验证迁移：system keychain 解密 → file key 重加密 → 断掉 system keychain 后仍能读取Testing
go test ./internal/keychain/... -race— all 5 tests passgo vet/gofmt— cleanTestPlatformSetCreatesFileMasterKeyFirst— new credentials use file key, not system keychainTestMigrateToFileMasterKey— migration works: decrypt with system key → re-encrypt with file key → read works without system keychainSummary by CodeRabbit
New Features
Bug Fixes
Tests