feat: add darwin file master key fallback for keychain writes#285
feat: add darwin file master key fallback for keychain writes#285liangshuo-1 merged 7 commits intomainfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a file-backed AES-256 “master.key.file” fallback for macOS keychain operations, introduces overridable Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant platformSet
participant getFileMasterKey
participant FileSystem
participant SystemKeychain as keyringSet
participant encryptData
Caller->>platformSet: platformSet(service, secret)
platformSet->>getFileMasterKey: getFileMasterKey(service, allowCreate=false)
alt File master key exists
getFileMasterKey->>FileSystem: read master.key.file
FileSystem-->>getFileMasterKey: key bytes
getFileMasterKey-->>platformSet: file master key
platformSet->>encryptData: encrypt with file master key
else File master key missing
getFileMasterKey-->>platformSet: errNotInitialized
platformSet->>SystemKeychain: keyringSet(create)
alt Keychain create succeeds
SystemKeychain-->>platformSet: system master key
platformSet->>encryptData: encrypt with system master key
else Keychain create fails
platformSet->>getFileMasterKey: getFileMasterKey(service, allowCreate=true)
getFileMasterKey->>FileSystem: generate & write master.key.file
FileSystem-->>getFileMasterKey: key bytes
getFileMasterKey-->>platformSet: file master key
platformSet->>encryptData: encrypt with file master key
end
end
encryptData-->>platformSet: encrypted payload
platformSet->>FileSystem: atomic write payload
FileSystem-->>platformSet: success
platformSet-->>Caller: done
sequenceDiagram
participant Caller
participant platformGet
participant getFileMasterKey
participant FileSystem
participant SystemKeychain as keyringGet
participant decryptData
Caller->>platformGet: platformGet(service)
platformGet->>getFileMasterKey: getFileMasterKey(service, allowCreate=false)
alt File master key exists
getFileMasterKey->>FileSystem: read master.key.file
FileSystem-->>getFileMasterKey: key bytes
getFileMasterKey-->>platformGet: file master key
platformGet->>FileSystem: read encrypted payload
FileSystem-->>platformGet: encrypted data
platformGet->>decryptData: attempt decrypt with file master key
alt Decrypt succeeds
decryptData-->>platformGet: secret
else Decrypt fails
platformGet->>SystemKeychain: keyringGet()
SystemKeychain-->>platformGet: system master key
platformGet->>decryptData: decrypt with system master key
decryptData-->>platformGet: secret
end
else File master key missing
getFileMasterKey-->>platformGet: errNotInitialized
platformGet->>SystemKeychain: keyringGet()
SystemKeychain-->>platformGet: system master key
platformGet->>FileSystem: read encrypted payload
FileSystem-->>platformGet: encrypted data
platformGet->>decryptData: decrypt with system master key
decryptData-->>platformGet: secret
end
platformGet-->>Caller: secret
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Comment |
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@872aa63f1c7551e54e38bc14a0cf5d9c01a46d77🧩 Skill updatenpx skills add larksuite/cli#feat/macos_keychain_write_fail_downgrade -y -g |
Greptile SummaryThis PR adds a file-backed AES-256 master-key fallback for macOS Keychain writes using One remaining issue: after a successful Confidence Score: 4/5Safe to merge with low risk; one P2 finding with a latent mixed-key-state correctness edge case that requires a transient I/O failure after a successful write. Prior P1 concerns (concurrent clobber, false-corrupted error) are resolved by the O_EXCL + retry approach. The remaining finding is labeled P2 but touches data-integrity (mixed encryption key state if a post-write read fails transiently), so it is treated as P1 for scoring, yielding 4/5. internal/keychain/keychain_darwin.go lines 186–200 (post-write re-read in getFileMasterKey) Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A([platformSet]) --> B{File master key\nexists on disk?}
B -- Yes --> E[Encrypt with file key]
B -- No --> C{Try system\nkeychain create}
C -- Success --> E
C -- Failure --> D["Create file master key\n(O_EXCL + retry)"]
D --> E
E --> F[Write encrypted data\natomic rename]
G([platformGet]) --> H[Read encrypted\ndata file]
H --> I{File master\nkey on disk?}
I -- Yes --> J{Decrypt\nsucceeds?}
J -- Yes --> K([Return plaintext])
J -- No --> L[Try keychain\nmaster key]
I -- No --> L
L --> M{Decrypt\nsucceeds?}
M -- Yes --> K
M -- No --> N([Return error])
Reviews (7): Last reviewed commit: "refactor(keychain): replace os package w..." | Re-trigger Greptile |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/keychain/keychain_darwin.go`:
- Around line 228-233: The current flow in getMasterKey -> getFileMasterKey
masks the original macOS keychain error so callers of
internal/keychain/keychain.go:Set can't detect that storage fell back to
master.key.file; change the API to return a sentinel/backend indicator alongside
the error (e.g., an enum or distinct error type) so Set can signal whether the
secret was persisted to the OS keychain or to the file. Update getMasterKey and
getFileMasterKey to return (key, backendResult, err) (or similar), propagate
that result through Set, and use a clear sentinel value (e.g., BackendKeychain
vs BackendFile) so callers can emit the required warning when write escaped the
macOS Keychain.
- Around line 208-212: The current logic silently treats any error from
getFileMasterKey(service, false) as "no file key" and falls back to the
keychain; change both the decrypt path (where decryptData is called) and the
encrypt/write path to only fall back to the keychain when getFileMasterKey
returns errNotInitialized, and otherwise return the getFileMasterKey error (or
propagate decryptData errors) immediately; locate the calls to getFileMasterKey,
decryptData, and any write/encrypt functions in keychain_darwin.go and update
the error handling so non-errNotInitialized errors are not suppressed and are
returned to the caller.
- Around line 137-148: The current code writes a temp file (tmpKeyPath) and uses
os.Rename to move it to keyPath, which can overwrite an existing master key;
change the rename to create a hard link with os.Link(tmpKeyPath, keyPath) and
handle the error: if os.Link returns nil then remove tmpKeyPath and return the
written key, if os.Link fails with EEXIST then read keyPath (using os.ReadFile)
and if the existing key length equals masterKeyBytes return that key, otherwise
return the link error; ensure tmpKeyPath is always cleaned up (defer
os.Remove(tmpKeyPath)) and keep the existing existingKey/readErr checks and
return semantics.
🪄 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: 07e875f8-df81-4ed2-ac85-a0ebb400948e
📒 Files selected for processing (2)
internal/keychain/keychain_darwin.gointernal/keychain/keychain_darwin_test.go
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/keychain/keychain_other.go`:
- Around line 76-118: getMasterKey (and the fallback getFileMasterKey) currently
creates the master key directly at its final path which can expose a partial
file to other processes; instead, write the key to a temporary file in the same
directory with mode 0600, fsync the temp file (or close it), then atomically
move it into place with os.Rename to publish the key, and preserve the existing
post-rename validation logic (reading canonicalKey/existingKey and returning
"keychain is corrupted" when length mismatches); if os.Rename fails because the
final file already exists, fall back to reading the existing file as the current
code does and remove the temp file on any write/rename failure to avoid leaving
stray temp files.
🪄 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: ec25c513-2a7f-4617-807a-e8c2f6a3ebe2
📒 Files selected for processing (3)
internal/keychain/keychain_darwin.gointernal/keychain/keychain_darwin_test.gointernal/keychain/keychain_other.go
✅ Files skipped from review due to trivial changes (1)
- internal/keychain/keychain_darwin_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/keychain/keychain_darwin.go
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
internal/keychain/keychain_darwin.go (2)
261-265:⚠️ Potential issue | 🟠 MajorCorruption and permission errors from file master key are silently swallowed.
When
getFileMasterKey(service, false)fails with a corruption error (e.g., wrong key length) or permission error, the code silently falls through to the keychain path instead of returning the error. This masks data corruption and can lead to a mixed-backend state where the file key exists but is ignored.Only
errNotInitialized(file doesn't exist) should trigger the keychain fallback; other errors should be propagated.🛡️ Proposed fix to narrow fallback to missing-file case
- if key, ferr := getFileMasterKey(service, false); ferr == nil { - if plaintext, derr := decryptData(data, key); derr == nil { - return plaintext, nil - } - } + key, ferr := getFileMasterKey(service, false) + if ferr == nil { + plaintext, derr := decryptData(data, key) + if derr == nil { + return plaintext, nil + } + // File key exists but decryption failed - don't fall back + return "", derr + } else if !errors.Is(ferr, errNotInitialized) { + // File key error other than "not initialized" - propagate + return "", ferr + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/keychain/keychain_darwin.go` around lines 261 - 265, The current branch swallows errors from getFileMasterKey and decryptData; change the logic in the block using getFileMasterKey(service, false) and decryptData so that: if ferr == nil then attempt decryptData and on any decrypt error return that error (do not fall back); if ferr != nil, only allow fallback to the keychain path when ferr == errNotInitialized; for any other ferr (e.g., corruption or permission errors) immediately return ferr. Reference getFileMasterKey, decryptData, and errNotInitialized when making this change.
279-288:⚠️ Potential issue | 🟠 MajorNon-errNotInitialized errors from file master key should not fall through to keychain.
Similar to
platformGet, ifgetFileMasterKey(service, false)returns a corruption or permission error, the code should return that error rather than attempting keychain creation. OnlyerrNotInitializedshould trigger the fallback path.Additionally, when keychain write fails and file fallback succeeds (lines 282-287), callers cannot distinguish between a successful keychain write and a fallback to file storage. Consider returning a sentinel or logging a warning so the behavior is observable.
🛡️ Proposed fix to handle errors correctly
key, err := getFileMasterKey(service, false) - if err != nil { + if errors.Is(err, errNotInitialized) { key, err = getMasterKey(service, true) if err != nil { key, err = getFileMasterKey(service, true) if err != nil { return err } } + } else if err != nil { + return err }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/keychain/keychain_darwin.go` around lines 279 - 288, The current fallback logic in the block using getFileMasterKey(service, false) incorrectly treats all errors as if they were errNotInitialized; change it so only err == errNotInitialized triggers the fallback to getMasterKey/getFileMasterKey with force=true, and any other error (e.g., corruption or permission errors returned by getFileMasterKey) is returned immediately from the caller; additionally, make keychain write failures observable by returning a sentinel value or error (or emitting a warning-level log) when getMasterKey (the keychain write) fails but a subsequent file fallback succeeds so callers can distinguish "keychain write failed, using file fallback" versus a full success — update logic around getMasterKey and the second getFileMasterKey calls to implement these behaviors and reference errNotInitialized, getFileMasterKey, and getMasterKey.
🧹 Nitpick comments (1)
internal/keychain/keychain_darwin.go (1)
129-150: File operations useos.*instead ofvfs.*as required by coding guidelines.Multiple file operations in
getFileMasterKeyuseos.ReadFile,os.MkdirAll,os.OpenFile, andos.Remove. Additionally,keyPathderived from theserviceparameter is not validated withvalidate.SafeInputPathbefore use.As per coding guidelines: "Use
vfs.*instead ofos.*for all filesystem access" and "Validate paths usingvalidate.SafeInputPathbefore any file I/O operations".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/keychain/keychain_darwin.go` around lines 129 - 150, getFileMasterKey uses os.* calls and an unchecked keyPath derived from service; replace os.ReadFile, os.MkdirAll, os.OpenFile and os.Remove with the vfs equivalents (e.g., vfs.ReadFile, vfs.MkdirAll, vfs.OpenFile, vfs.Remove) and propagate their errors the same way, and before constructing/using keyPath validate the service-derived path with validate.SafeInputPath (or SafeInputPath on the resulting path variable) to prevent unsafe input; ensure file open flags and permission bits are set equivalently when switching to vfs and that dir/keyPath variables (and any defered closes/removals) use the vfs file handles.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/keychain/keychain_darwin.go`:
- Around line 185-202: The final "return key" is unreachable and inconsistent
with the verification logic; update the function to remove the dead return and
ensure you return the verified read-back value (canonicalKey or existingKey)
instead of the original generated key; specifically, keep the existing branches
that return canonicalKey or existingKey when their lengths equal masterKeyBytes,
remove the final unreachable "return key" and any dangling paths so all code
paths return the validated canonicalKey/existingKey or the appropriate error
(reference: canonicalKey, existingKey, keyPath, masterKeyBytes, key).
---
Duplicate comments:
In `@internal/keychain/keychain_darwin.go`:
- Around line 261-265: The current branch swallows errors from getFileMasterKey
and decryptData; change the logic in the block using getFileMasterKey(service,
false) and decryptData so that: if ferr == nil then attempt decryptData and on
any decrypt error return that error (do not fall back); if ferr != nil, only
allow fallback to the keychain path when ferr == errNotInitialized; for any
other ferr (e.g., corruption or permission errors) immediately return ferr.
Reference getFileMasterKey, decryptData, and errNotInitialized when making this
change.
- Around line 279-288: The current fallback logic in the block using
getFileMasterKey(service, false) incorrectly treats all errors as if they were
errNotInitialized; change it so only err == errNotInitialized triggers the
fallback to getMasterKey/getFileMasterKey with force=true, and any other error
(e.g., corruption or permission errors returned by getFileMasterKey) is returned
immediately from the caller; additionally, make keychain write failures
observable by returning a sentinel value or error (or emitting a warning-level
log) when getMasterKey (the keychain write) fails but a subsequent file fallback
succeeds so callers can distinguish "keychain write failed, using file fallback"
versus a full success — update logic around getMasterKey and the second
getFileMasterKey calls to implement these behaviors and reference
errNotInitialized, getFileMasterKey, and getMasterKey.
---
Nitpick comments:
In `@internal/keychain/keychain_darwin.go`:
- Around line 129-150: getFileMasterKey uses os.* calls and an unchecked keyPath
derived from service; replace os.ReadFile, os.MkdirAll, os.OpenFile and
os.Remove with the vfs equivalents (e.g., vfs.ReadFile, vfs.MkdirAll,
vfs.OpenFile, vfs.Remove) and propagate their errors the same way, and before
constructing/using keyPath validate the service-derived path with
validate.SafeInputPath (or SafeInputPath on the resulting path variable) to
prevent unsafe input; ensure file open flags and permission bits are set
equivalently when switching to vfs and that dir/keyPath variables (and any
defered closes/removals) use the vfs file handles.
🪄 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: c0b42eee-2fd5-49b4-9087-77186a3687fc
📒 Files selected for processing (1)
internal/keychain/keychain_darwin.go
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
internal/keychain/keychain_darwin.go (2)
258-262:⚠️ Potential issue | 🟠 MajorOnly treat
errNotInitializedas “no file key yet”.Line 258 and Line 276 still collapse every
getFileMasterKey(service, false)failure into the keychain branch. That masks a corrupt or unreadablemaster.key.fileon reads, and it lets writes switch back to the keychain even after the file backend is supposed to stay sticky.🔧 Suggested fix
func platformGet(service, account string) (string, error) { path := filepath.Join(StorageDir(service), safeFileName(account)) data, err := os.ReadFile(path) if errors.Is(err, os.ErrNotExist) { return "", nil } if err != nil { return "", err } if key, ferr := getFileMasterKey(service, false); ferr == nil { if plaintext, derr := decryptData(data, key); derr == nil { return plaintext, nil } + } else if !errors.Is(ferr, errNotInitialized) { + return "", ferr } key, err := getMasterKey(service, false) if err != nil { return "", err }func platformSet(service, account, data string) error { key, err := getFileMasterKey(service, false) - if err != nil { + if errors.Is(err, errNotInitialized) { key, err = getMasterKey(service, true) if err != nil { key, err = getFileMasterKey(service, true) if err != nil { return err } } + } else if err != nil { + return err }Also applies to: 276-284
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/keychain/keychain_darwin.go` around lines 258 - 262, The current getFileMasterKey(service, false) call treats any error as “no file key” and falls back to the keychain; change the logic in the blocks around getFileMasterKey and decryptData so you only treat errNotInitialized as the “no file key yet” case—if getFileMasterKey returns any other error, propagate/return that error (don’t fall back to keychain) and ensure writes remain sticky to the file backend; update the code paths that call getFileMasterKey (both the decrypt/read branch using decryptData and the later write branch) to check for errors explicitly (if err == errNotInitialized then continue to keychain fallback else return the error).
129-135:⚠️ Potential issue | 🟡 MinorKeep the exclusive create, but publish the file atomically.
O_EXCLavoids clobbering an existing master key, but Line 150 still makesmaster.key.filevisible before Line 176 writes the 32 bytes. Because Lines 129-135 treat any short read as corruption, a concurrentgetFileMasterKey(..., false)can fail during that window. Writing to a temp file and linking it into place only after close avoids exposing a zero-byte or partial key.Also applies to: 150-183
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/keychain/keychain_darwin.go` around lines 129 - 135, The code currently uses O_EXCL to avoid clobbering the master key but creates the target master.key.file before writing, which exposes a zero/partial file and causes getFileMasterKey(...) to treat short reads as corruption; change the write path in the functions handling master key creation (refer to getFileMasterKey, the code that opens/creates master.key.file and the block around the create at lines corresponding to the 150-183 diff) to instead write the 32-byte key to a uniquely named temporary file in the same directory (use secure permissions), fsync the temp file and its directory, then atomically rename/link the temp file to master.key.file (preserving the exclusive-create semantics by failing if the final target already exists), so concurrent readers never see a partial file and O_EXCL behavior is preserved.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/keychain/keychain_darwin.go`:
- Around line 125-199: The fallback file-master-key logic in getFileMasterKey
(which builds dir via StorageDir and keyPath with fileMasterKeyName) uses raw
os.* calls and lacks a validate.SafeInputPath check; update this function to
call validate.SafeInputPath on the derived dir and keyPath before any I/O and
replace all os.* usage with the vfs package equivalents (vfs.ReadFile,
vfs.MkdirAll, vfs.OpenFile, vfs.Remove, and vfs file operations) while
preserving the same existence/error checks (errors.Is(..., os.ErrNotExist) ->
errors.Is(..., fs.ErrNotExist) or the appropriate sentinel if vfs exposes one)
and file mode semantics (0700/0600), and ensure the same retry/read-after-create
logic uses vfs.ReadFile so the code honors the repository VFS abstraction and
path validation.
---
Duplicate comments:
In `@internal/keychain/keychain_darwin.go`:
- Around line 258-262: The current getFileMasterKey(service, false) call treats
any error as “no file key” and falls back to the keychain; change the logic in
the blocks around getFileMasterKey and decryptData so you only treat
errNotInitialized as the “no file key yet” case—if getFileMasterKey returns any
other error, propagate/return that error (don’t fall back to keychain) and
ensure writes remain sticky to the file backend; update the code paths that call
getFileMasterKey (both the decrypt/read branch using decryptData and the later
write branch) to check for errors explicitly (if err == errNotInitialized then
continue to keychain fallback else return the error).
- Around line 129-135: The code currently uses O_EXCL to avoid clobbering the
master key but creates the target master.key.file before writing, which exposes
a zero/partial file and causes getFileMasterKey(...) to treat short reads as
corruption; change the write path in the functions handling master key creation
(refer to getFileMasterKey, the code that opens/creates master.key.file and the
block around the create at lines corresponding to the 150-183 diff) to instead
write the 32-byte key to a uniquely named temporary file in the same directory
(use secure permissions), fsync the temp file and its directory, then atomically
rename/link the temp file to master.key.file (preserving the exclusive-create
semantics by failing if the final target already exists), so concurrent readers
never see a partial file and O_EXCL behavior is preserved.
🪄 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: 4a1beb0c-138e-494b-a621-52049c47b324
📒 Files selected for processing (1)
internal/keychain/keychain_darwin.go
…hecks - Replace temporary file approach with direct file creation - Add explicit corruption checks for existing keys - Ensure atomic operations and proper cleanup on failure
Add descriptive comments to explain the purpose of timeout, crypto parameters, and test variables in the macOS keychain implementation.
Add retry mechanism when reading existing master key file to handle potential race conditions. Return early if read error occurs instead of waiting for all retries.
Restructure the key validation flow to reduce redundant checks and improve readability. The corrupted key check is moved after the error handling block for better logical flow.
1e6fe7e to
645eef4
Compare
Use vfs package instead of os for file operations to improve testability and abstract filesystem access. This change makes it easier to mock filesystem operations in tests and provides a consistent interface for file handling.
Summary
On macOS, when writing to the system Keychain fails, the CLI now falls back to a file-stored master key while keeping AES-GCM encryption unchanged. If a file master key already exists, subsequent writes use it directly; reads always prefer the file master key before trying the Keychain.
Changes
Test Plan
go test ./internal/keychain -v)go test ./...)./lark-cli config init,./lark-cli auth login)Related Issues
Summary by CodeRabbit
New Features
Tests