Skip to content

fix(secrets): trust developer-id signed binary in macOS keychain ACLs#903

Merged
steipete merged 1 commit into
mainfrom
fix/keychain-trust-signed
Jul 6, 2026
Merged

fix(secrets): trust developer-id signed binary in macOS keychain ACLs#903
steipete merged 1 commit into
mainfrom
fix/keychain-trust-signed

Conversation

@steipete

@steipete steipete commented Jul 5, 2026

Copy link
Copy Markdown
Collaborator

Problem

Every gog keychain item is created with KeychainTrustApplication: false, so each of the (three-per-account) token items triggers a macOS "Always Allow + password" prompt on first access — and again after some upgrades. That trade-off exists to protect ad-hoc builds (#86), whose designated requirement changes every rebuild, but the official Homebrew release is Developer-ID signed (stable identity, hardened runtime) and pays the prompt tax for no benefit.

Fix

  • Auto-detect at keyring-open time whether the running executable has a stable signing identity (codesign -dv: not unsigned, not Signature=adhoc, non-empty TeamIdentifier), cached per process; enable KeychainTrustApplication only then. Ad-hoc/source builds keep today's behavior.
  • GOG_KEYCHAIN_TRUST_APPLICATION={auto|true|false} override, plumbed like the other GOG_* envs; documented in docs/spec.md.
  • gog auth doctor reports the effective mode as a keychain.trust check (darwin + keychain backend only).
  • Decision-table unit tests with an injected codesign runner (no real codesign in tests); doctor JSON coverage.

Proof

  • go test ./internal/secrets/... ./internal/cmd/... — ok
  • make lint — 0 issues
  • Live: source build reports keychain.trust = application trust disabled (ad-hoc or unsigned binary); with GOG_KEYCHAIN_TRUST_APPLICATION=true reports forced on via GOG_KEYCHAIN_TRUST_APPLICATION.

Storage format, item names, and existing items' ACLs are untouched; existing items still need their one-time "Always Allow".

🤖 Generated with Claude Code

@clawsweeper

clawsweeper Bot commented Jul 5, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs maintainer review before merge. Reviewed July 5, 2026, 7:52 PM ET / 23:52 UTC.

Summary
The branch conditionally enables macOS Keychain KeychainTrustApplication for stably signed binaries, adds GOG_KEYCHAIN_TRUST_APPLICATION, reports the effective mode in gog auth doctor, and adds docs and tests.

Reproducibility: no. high-confidence runtime reproduction was established. Source inspection confirms current main hard-codes KeychainTrustApplication: false, matching the reported prompt trade-off, but I did not run the signed macOS upgrade path.

Review metrics: 2 noteworthy metrics.

  • Touched Surface: 8 files, +314/-39. The branch changes core keyring policy plus docs and tests, so maintainer review should include upgrade behavior rather than only unit coverage.
  • New Operator Surface: 1 env override added. GOG_KEYCHAIN_TRUST_APPLICATION gives users a way to bypass the automatic policy, which affects compatibility expectations for source and ad-hoc builds.

Merge readiness
Overall: 🦐 gold shrimp
Proof: 🦐 gold shrimp
Patch quality: 🐚 platinum hermit
Result: ready for maintainer review.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Rank-up moves:

  • [P2] Add or link redacted macOS proof from a signed release/Homebrew-style build showing token creation and token read across an upgrade or reinstall.
  • Confirm whether any stable TeamIdentifier is acceptable or whether auto trust should be restricted to the official release identity.

Risk before merge

  • [P1] The PR changes macOS Keychain ACL behavior for newly created OAuth token items in signed builds; unit tests prove the resolver logic, but the PR body does not show a Developer-ID/Homebrew-signed binary creating and reading tokens across an upgrade.
  • [P1] The explicit GOG_KEYCHAIN_TRUST_APPLICATION=true override can intentionally re-enable the older ad-hoc/source-build upgrade failure mode, so the release docs and doctor output need to make that operator trade-off clear.

Maintainer options:

  1. Prove The Signed Release Path (recommended)
    Before merge, run a Developer-ID/Homebrew-signed build through token creation, upgrade or reinstall, and token read to confirm prompts and ACL behavior match the policy.
  2. Accept The Policy Risk
    Maintainers can land based on resolver tests and collaborator live doctor output, accepting that signed-release ACL behavior is validated after merge.
  3. Pause Or Narrow
    If the signing boundary is not settled, pause this branch or narrow it to an explicit env override without changing the default.

Next step before merge

  • [P2] No narrow repair is indicated; the remaining blocker is maintainer acceptance and real signed macOS upgrade proof for the Keychain ACL policy.

Maintainer decision needed

  • Question: Should gogcli automatically enable macOS Keychain application trust for binaries with a stable signing TeamIdentifier, or require narrower release-identity gating or signed-upgrade proof before merge?
  • Rationale: This changes credential ACL behavior for OAuth tokens, and CI cannot prove the real macOS Keychain upgrade boundary that motivated the prior false default.
  • Likely owner: steipete — He owns most current keyring/auth-doctor history and authored the current PR direction.
  • Options:
    • Require Signed-Upgrade Proof (recommended): Merge after a Developer-ID/Homebrew-signed build shows token creation, reinstall or upgrade, and token read behavior matching the intended prompt policy.
    • Narrow The Signing Gate: Restrict auto trust to the official release identity if maintainers do not want any non-ad-hoc TeamIdentifier to receive application trust.
    • Keep Default False: Leave current behavior unchanged and ship only an explicit override if the automatic ACL policy is not settled.

Security
Cleared: The diff is security-sensitive because it changes Keychain ACL policy, but I found no concrete code execution, dependency, secret-handling, or supply-chain defect in the patch.

Review details

Best possible solution:

Land the focused resolver only after maintainers accept the Keychain ACL policy and signed macOS release/upgrade proof confirms the prompt and token-read behavior; otherwise keep the false default with an explicit opt-in override.

Do we have a high-confidence way to reproduce the issue?

No high-confidence runtime reproduction was established. Source inspection confirms current main hard-codes KeychainTrustApplication: false, matching the reported prompt trade-off, but I did not run the signed macOS upgrade path.

Is this the best way to solve the issue?

Unclear until maintainer policy is confirmed. The implementation is narrow and preserves ad-hoc defaults, but the best solution needs signed-release upgrade proof because the change is in credential ACL behavior.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against c0aac99f6be6.

Label changes

Label changes:

  • add P2: This is a normal-priority macOS auth/keychain improvement with limited platform scope but real credential-access impact.
  • add merge-risk: 🚨 compatibility: Changing KeychainTrustApplication for signed builds can affect how newly created token items behave across upgrades or when switching between signed and source builds.
  • add merge-risk: 🚨 security-boundary: The PR changes the local Keychain ACL trust decision for stored OAuth tokens, which is a credential access boundary.
  • add rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🦐 gold shrimp and patch quality is 🐚 platinum hermit.
  • add status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Not applicable: The external-contributor proof gate does not apply to this collaborator-authored PR; the body includes copied live doctor output for source/forced modes but not the signed-release upgrade path.

Label justifications:

  • P2: This is a normal-priority macOS auth/keychain improvement with limited platform scope but real credential-access impact.
  • merge-risk: 🚨 compatibility: Changing KeychainTrustApplication for signed builds can affect how newly created token items behave across upgrades or when switching between signed and source builds.
  • merge-risk: 🚨 security-boundary: The PR changes the local Keychain ACL trust decision for stored OAuth tokens, which is a credential access boundary.
  • rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🦐 gold shrimp and patch quality is 🐚 platinum hermit.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Not applicable: The external-contributor proof gate does not apply to this collaborator-authored PR; the body includes copied live doctor output for source/forced modes but not the signed-release upgrade path.
Evidence reviewed

What I checked:

Likely related people:

  • steipete: Peter Steinberger introduced the original Keychain trust default, merged the prior Homebrew-upgrade fix, later split keyring backend policy into the current file, and authored auth doctor diagnostics. (role: recent area contributor; confidence: high; commits: 3f2f85882093, 716f6c2cf14a, 7b9197ebd118; files: internal/secrets/store.go, internal/secrets/backend.go, internal/cmd/auth_doctor.go)
  • salmonumbrella: salmonumbrella authored the merged fix that disabled KeychainTrustApplication for the Homebrew upgrade token-persistence issue this PR now refines. (role: introduced current safety behavior; confidence: medium; commits: 0a7dd586afea; files: internal/secrets/store.go, internal/secrets/store_test.go)
What the crustacean ranks mean
  • 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
  • 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
  • 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
  • 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
  • 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
  • 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
  • 🌊 off-meta tidepool: rating does not apply to this item.

Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

How this review workflow works
  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

@clawsweeper clawsweeper Bot added rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. P2 Normal priority bug or improvement with limited blast radius. merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. merge-risk: 🚨 security-boundary 🚨 Merging this PR could weaken sandboxing, authorization, credentials, or sensitive data. labels Jul 5, 2026
@steipete
steipete merged commit 088198a into main Jul 6, 2026
11 checks passed
@clawsweeper clawsweeper Bot mentioned this pull request Jul 6, 2026
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. merge-risk: 🚨 security-boundary 🚨 Merging this PR could weaken sandboxing, authorization, credentials, or sensitive data. P2 Normal priority bug or improvement with limited blast radius. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@steipete