fix(auth): stabilize macOS Keychain ACL across Homebrew upgrades#182
Merged
fix(auth): stabilize macOS Keychain ACL across Homebrew upgrades#182
Conversation
Pin the release binary's Designated Requirement to the bundle identifier so the Keychain ACL entry a user approved with "Always Allow" keeps matching after `brew upgrade bkt`. The previous 0.21.0 claim that the stable identifier alone preserved approvals was incomplete — without `-r=`, codesign still derived a cdhash-based DR that changed on every rebuild. Also set KeychainTrustApplication and KeychainAccessibleWhenUnlocked on darwin so fresh items trust the calling binary at create time and survive login/unlock cycles without re-prompting, and refresh the stored item on `bkt auth login` by deleting before setting (99designs/keyring's update path deliberately preserves existing ACLs, which left stale entries from older binaries prompting forever). Add `bkt auth doctor [host]` that reports binary signature, Designated Requirement, trust flags, and per-host Keychain presence. Uses `security find-generic-password` without `-g` so it never reads the stored secret; probe failures surface as `keychain=unknown` and an explicit next-step diagnosis instead of silently claiming absence. Widen `secret.Store.Delete` to also treat `os.ErrNotExist` as success so the encrypted file backend matches Keychain semantics. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
Exercises writeDoctorText on both darwin and non-darwin reports, the URL-style and unknown-selector branches of chooseHostKeys, and describeBackend. Raises patch coverage above codecov's 43% gate. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Make runCmd a package-level var so tests can stub codesign and security invocations with canned output. Adds parser coverage for the adhoc/cdhash DR case, the stable identifier-based DR case, and each of the three keychainItemPresent outcomes (present, not-found, inconclusive). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Use absolute Apple-tool paths so a poisoned \$PATH cannot swap in a malicious probe. Also surface the codesign requirement-dump error via the returned reqErr instead of silently dropping it so inspectCodesign failures reach the doctor ProbeErrors report. Addresses security-auditor finding on PR #182. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
6 tasks
… keychain Per Apple's kSecAttrAccessible docs, that attribute only applies to data-protection keychain items or synchronizable items on macOS. 99designs/keyring uses the file-based path, so the flag is harmless but currently does nothing. Set for forward compatibility. No functional change. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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
1 participant
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.
Fixes #181.
Summary
scripts/codesign-macos.sh(-r='designated => identifier "..."'). Previously codesign derived acdhash-based DR that changed on every rebuild, which invalidated the Keychain ACL entry the user approved with "Always Allow" on the nextbrew upgrade bkt. The 0.21.0 changelog claim that the stable identifier alone preserved approvals was incomplete — this PR makes it actually true. Security trade-off documented in the script: any ad-hoc binary claiming the same identifier satisfies the DR. Developer ID signing remains future work.KeychainTrustApplicationandKeychainAccessibleWhenUnlockedon darwin insecret.Open()so fresh items trust the calling binary at create time and survive unlock cycles without re-prompting.pkg/cmd/auth/auth.godeletes any stale Keychain entry before writing on darwin.99designs/keyring'supdateItemdeliberately preserves existing ACLs, so users had to logout-then-login to benefit from the trust flags. Now a singlebkt auth loginafter upgrade is enough.secret.Store.Deleteto treatos.ErrNotExistas success (the file backend returns a rawPathErrorinstead ofkeyring.ErrKeyNotFound).bkt auth doctor [host]reports binary signature, Designated Requirement, trust flags, and per-host Keychain presence. Usessecurity find-generic-passwordwithout-gso it never reads the secret or triggers a Keychain prompt. DistinguisheserrSecItemNotFound(exit 44) from probe failures — probe failures surface askeychain=unknownplus a probe-inconclusive diagnosis; they are never silently collapsed into "missing".How to verify
Before merging, on macOS:
The first invocation on a go-run binary (cdhash DR) should diagnose the upgrade-reprompt scenario; after building and signing with the updated script,
bkt auth doctorshould reportDR stable: yesand the "stable Designated Requirement" diagnosis.Not in this PR
99designs/keyringdoes not support them; no evidence they're involved in the observed symptom.internal/secret/darwin_lock.gois kept as-is.Test plan
go test ./...greengo vet ./...cleango build ./cmd/bktsucceedscodesign-macos.sh;codesign -d -r-reportsdesignated => identifier "io.github.avivsinai.bitbucket-cli"(stable)bkt auth doctorrenders both diagnoses (cdhash-bound vs stable DR)brew upgrade bktfollowed by a command that reads the token does not re-prompt (only the one-time re-login after upgrade is required)🤖 Generated with Claude Code