feat(auth): the keychain setting starts doing something, on every platform including Windows - #1532
Merged
Merged
Conversation
Contributor
Quality GateResult: ✅ all floors passed
Config: .github/quality-gate.yml. Workflow: |
diillson
force-pushed
the
feat/keychain-windows
branch
3 times, most recently
from
September 5, 2026 20:06
627aa70 to
615a640
Compare
…tform including Windows CHATCLI_KEYCHAIN_BACKEND was read into a store that nothing ever called. The credential-encryption key was a file no matter what the setting said, on every platform, and the config screen reported a preference the runtime never acted on. Windows had a second problem underneath that one: even the unused store had no Windows branch, so its native case fell through to false and would have gone to the file anyway. The key now lives where the setting says. The file backend stays the file, always. The keychain backend uses the keychain, migrating a key that is already on disk exactly once and removing the file only after the keychain has handed that key back — a write that appears to succeed and a read that returns nothing would otherwise leave credentials that no future process can decrypt. The default keeps using an existing file key without moving it: relocating a working installation's key without being asked is not a default's business. Only a key being created for the first time goes to the keychain, and only where one is available. Every failure keeps the file. A keychain that cannot be reached, refuses the write, loses it, or returns something that is not a 32-byte key leaves the on-disk key exactly where it was, with one warning per process rather than one per credential read. Windows reaches the Credential Manager through the advapi32 credential API, because there is no CLI route: cmdkey can create and list credentials and never reveals a secret. Entries are stored per machine rather than roaming — a key that follows a profile onto other machines is a wider secret than the one the user asked to store — and a machine where advapi32 cannot be loaded falls back to the file, the same shape as a Linux box with no secret-tool. The store now takes its platform behind an interface. Which backend a key ends up in is the decision worth testing, and it is untestable on any single machine while the platform is reached directly. The macOS path was also exercised against the real keychain on a development machine; the Windows path compiles and vets for its target and has not been run on one.
diillson
force-pushed
the
feat/keychain-windows
branch
from
September 5, 2026 20:09
615a640 to
0b78ab3
Compare
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
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.
Fourth PR from the
features/securityaudit. The finding here turned out to be larger than the one I set out to fix.What the audit found
The page documents
CHATCLI_KEYCHAIN_BACKENDwith a per-OS table, and says Windows uses the Credential Manager.useNative()had no Windows branch, so it returnedfalsethere and silently used the file.Chasing that turned up the bigger problem:
NewKeychainStoreis never called anywhere in the codebase.grep -rn "NewKeychainStore" --include='*.go' .returns only its own definition. The variable was read, a store was built, and nothing ever asked it for anything — the key was a file on every platform, regardless of the setting, while/configdisplayed a preference the runtime never acted on.What this delivers
The setting is wired.
loadOrCreateKey— the key that encrypts~/.chatcli/auth-profiles.json— now resolves through the backend:file: the file, always. The keychain is never consulted.keychain: the keychain. A key already on disk is migrated once, and the file is removed only after the keychain has handed that key back. A write that appears to succeed and a read that returns nothing would otherwise leave credentials no future process can decrypt.auto(default): an existing file key keeps being used, untouched. Only a first-time key goes to the keychain, and only where one is available. Moving a working installation's key without being asked is not a default's business.Every failure keeps the file. Unreachable keychain, refused write, lost write, or a stored value that is not a 32-byte key — the on-disk key stays where it is, with one warning per process rather than one per credential read.
Windows Credential Manager, through the advapi32 credential API (
CredReadW/CredWriteW/CredDeleteW). There is no CLI route:cmdkeycan create and list credentials but never reveals a secret. Entries areCRED_PERSIST_LOCAL_MACHINErather than roaming, and a machine where advapi32 cannot be loaded falls back to the file — the same shape as a Linux box with nosecret-tool. No new dependency:golang.org/x/sysis already direct.The store takes its platform behind an interface. Which backend a key ends up in is the decision worth testing, and it is untestable on any single machine while the platform is reached directly.
Verification
9 tests over the backend-selection logic: the default not migrating, a new key going to the keychain with no copy left on disk, the file backend never reading the keychain, the verified migration, and the three failure modes each keeping the file.
The macOS path was exercised against the real keychain on a development machine (round-trip under a throwaway account, deleted after).
It compiles and vets under
GOOS=windows, and there is no Windows job in CI. Given the terminal-rendering post-mortem, I am not willing to call an unrun Windows path verified. It is designed so the worst case is today's behaviour — any failure falls back to the file — but please run it on a Windows box before we let the docs claim it. I will hold that line in the docs PR until you confirm.