feat(tokencache): opt-in file token storage for cgo source builds - #82
feat(tokencache): opt-in file token storage for cgo source builds#82omarshahine wants to merge 1 commit into
Conversation
|
🦞👀 Pull request received. I will update this pull request when review starts. ClawSweeper review completeClawSweeper finished reviewing this revision. The review result is being finalized. |
|
Codex review: blocked before merge. Reviewed September 8, 2026, 3:17 AM ET / 07:17 UTC. ClawSweeper reviewWhat this changesAdds a configuration and environment setting that directs cached-token reads and writes to file storage, documents its protection limits, and tests logout across backend switches. Merge readiness⛔ Blocked before merge - 5 items remain The selector remains distinct work, with no blocking code defect found. The separately merged logout repair resolves the earlier compatibility concern; the outstanding choice is acceptance of the opt-in credential-storage boundary. Priority: P2 Review scores
Verification
How this fits togethereightctl caches Eight Sleep authentication tokens so commands and scheduled actions can reuse them. The storage selector controls where tokens are saved and loaded before the client sends authenticated API requests; logout still clears both reachable stores. flowchart TD
A[Configuration or environment] --> B{File storage selected?}
B -->|Yes| C[File token cache]
B -->|No| D[Native keyring with file fallback]
C --> E[Authenticated API client]
D --> E
F[Logout] --> C
F --> D
Decision needed
Why: The implementation deliberately broadens credential access for opted-in users, and the repository owner has explicitly left that policy choice open. Before merge
Findings
Agent review detailsSecurityNeeds attention: No accidental authorization or supply-chain regression was found, but the intentional credential-storage tradeoff requires owner acceptance. Review metrics
Merge-risk optionsMaintainer options:
Technical reviewBest possible solution: Offer the documented file-storage escape hatch only with explicit acceptance of its filesystem trust boundary, preserving native-first defaults and cleanup across both stores. Do we have a high-confidence way to reproduce the issue? Not applicable to the selector as a new capability. The macOS source-build prompt is described in the discussion, but this review did not reproduce it. Is this the best way to solve the issue? Unclear as the preferred product direction: this is a narrow reuse of the existing backend, while the proposed signing alternative remains unverified. AGENTS.md: not found in the target repository. Codex review notes: model internal, reasoning medium; reviewed against db84b936e0ba. LabelsLabel justifications:
EvidenceSecurity concerns:
What I checked:
Likely related people:
Rank-up movesOptional improvements that raise the rating; they are not merge blockers.
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (5 earlier review cycles)
|
|
The security item was right, and it was against my documentation rather than the diff. Fixed in The gap. I confirmed it before changing anything, against the real file backend with the directory chmod'd to deny unlink: The fix splits the filter rather than widening the tolerance. The clear path uses a new Proof is no longer in-memory. Both new tests drive the production
Both fail with the suppression restored: They skip on Windows and under root, where directory permissions do not block unlink. Two items I have not resolved, because they are yours and not a proof problem. They are now called out in the PR body:
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. |
|
Tightened the logout wording in It said logout "always clears both" stores, which reads as a guarantee that holds even for a backend that cannot be opened, and used "revoking", which reads as server-side revocation. Neither is accurate. It now states the actual contract: logout removes the local cache and does not revoke anything at the service, so an already-issued token stays valid there until it expires; it covers the stores it can reach, tolerating a backend that will not open (nothing there to clear) and reporting an error when a reachable backend refuses deletion. That was the last item on the list that wasn't a maintainer decision. What remains is yours: whether to offer the file-storage tradeoff at all, and whether to accept the stricter logout result for existing OS-first users. Both are spelled out in the PR body, and I'm happy to scope the stricter contract to the pinned path only if you'd rather not change behavior for current users. @clawsweeper re-review |
|
🦞👀 Re-review progress:
|
|
🦞🧹 I asked ClawSweeper to review this item again. |
Add make build and make install for local source development, with a configurable per-user destination, directory creation, and macOS signing and verification after replacement. Document that Keychain authorization can still be required after rebuilding. Exercise changed-content reinstallations into paths containing spaces in Linux CI. Validated with the full Go 1.26.8 suite, 85.5% core coverage, lint, real macOS installations and signature checks, exact-head CI, and clean independent review. The originally reported exit-137 failure was not reproduced by a synthetic overwrite on this host. Release-note credit is collected in the companion notes PR. Credential-storage policy remains separate in #82. Co-authored-by: Omar Shahine <10343873+omarshahine@users.noreply.github.com>
|
Two updates: the logout half is split out, and I went looking for the cause of the Keychain re-prompt rather than the workaround. SplitThe logout-contract work is now #84, rebased onto current
That leaves this PR as one question — the file backend — instead of a bundle. The re-prompt looks like a signing artifact, not a Keychain limitationThis PR's premise is that a Keychain item's ACL is bound to the code identity, so rebuilding invalidates it. That is right, and it's worth naming what sets the identity, because #77 ad-hoc signs with An ad-hoc signature's designated requirement is a literal cdhash pin: And that cdhash moves on every build — not only when the source changes: Go does not produce byte-identical output across rebuilds here, so an ad-hoc That points at a narrower fix than changing where tokens are stored: sign local builds with a stable identity, falling back to ad-hoc when none is configured. Released builds already go through GoReleaser with a real identity, so they should not hit this at all — which is worth confirming, because if it holds, this PR trades the shipped product's credential boundary for a local-development artifact. A second, independent option: bound the Keychain call with a timeout. The failure being described is an indefinite block, and a timeout turns that into a fast, honest error, which is what a scheduler actually needs. That is useful whichever backend is in play. What I have not provenI verified the cdhash instability directly, but not that a certificate-signed binary's ACL survives a rebuild in practice. This host has no code-signing identity, and creating one plus triggering a real consent prompt is not something I can do safely here. So treat the mechanism as demonstrated and the remedy as untested — worth a quick check on a machine with a Developer ID before rewriting this PR around it. If the stable-identity fix does hold, this PR may not be needed at all. If it does not, the tradeoff here is still yours to accept or reject, and #84 is unaffected either way. |
|
One additional finding for the storage decision: the published macOS build already excludes the native Keychain backend.
The proposed selector therefore affects Cgo-enabled source builds on macOS; it does not fix a Keychain prompt in the currently published macOS binary. This also means the README's unconditional statement that release installations default to native Keychain storage needs care in any future storage-policy change. The independent logout bug is now repaired and verified in #84, including read-only filesystem failures that the original error filter still hid. File-only selection remains a separate maintainer security-policy decision. No storage-policy change or closure was made in this pass. |
Rebased onto current main and reduced to the storage selector alone. The logout contract that shared this branch landed separately in steipete#84, so what remains is one question rather than a bundle. Corrects the premise as well. The pinned keyring library registers its macOS Keychain backend under a "darwin && cgo" build constraint and releases are built with CGO_ENABLED=0, so released binaries already select the file backend on macOS with no configuration. The earlier README text claimed the OS keyring was the default there, which is wrong. This option is for a cgo-enabled source build (go install, or make install on a Mac with a C toolchain), where the Keychain backend is present and a rebuild invalidates the item's ACL, leaving an unattended host blocked on a consent dialog nobody can answer. The pin is a flag consulted only by Save and Load, via primaryOpener(). Clear() always reaches both real backends, so pinning where tokens are stored never narrows what revoking them covers. Regression coverage asserts exactly that, at the unit level and against the production client: seed the primary backend, pin to file, log out, unpin, and require a fresh client to re-authenticate rather than put the revoked token on the wire. Restoring the aliasing bug fails both with "logout left a usable session in the primary backend" and "a fresh client sent the revoked token after logout across a backend switch". README documents the tradeoff it actually makes: the file backend's password is a fixed public constant, so filesystem permissions carry the boundary rather than Keychain code identity. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CtVNrHHZRwkbZhk34UxLi8
64eab78 to
4796401
Compare
|
Your CGO finding was correct and it changed this PR rather than just annotating it. I verified all three parts before rewriting: So the README claim I wrote was wrong, and wrong in the direction that mattered: it told release users the OS keyring held their token when the Keychain backend was never compiled into their binary. That text is replaced, not patched, and the PR is retitled to match what the option is actually for. Rebased onto The security question is narrower than it looked: this does not move release users from Keychain protection to filesystem protection, because they were never on Keychain protection. It adds an opt-in for cgo source builds that currently have it. One argument against merging this at all, which I would rather surface than bury: the re-prompt's cause looks like the ad-hoc signature from Happy to close this in favor of the signing fix if you think that is the better shape. It is a real question and you have better context on it than I do. |
|
The standing review above describes @clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. |
|
Correction to my previous comment: it was wrong. ClawSweeper had already reviewed the rebuilt branch at 07:18 UTC, against The current verdict is accurate and needs nothing further from me: no blocking code defect, the compatibility concern resolved by #84 landing separately, and the remaining items reduced to the storage-policy decision plus the open question of whether stable-identity signing is the better remedy. That matches what I raised above. |
|
Closing this. @steipete your CGO finding settled it, and it settled it against the PR. Once it was clear that released binaries never compile in the Keychain backend, the audience for this option narrowed to people who build from source and run unattended and can't use a release build. For my own case the honest answer turned out to be simpler: install from the Homebrew tap instead of That leaves no reason to ask you to accept a weaker credential boundary for a problem the shipped product does not have. What came out of this is already merged and worth more than the selector was:
If the source-build prompt ever needs a real fix, the better shape is the one this discussion surfaced: sign local builds with a stable identity so the Keychain ACL survives a rebuild, rather than moving the token somewhere the ACL doesn't apply. Worth a timeout on the token load too, so an unattended host gets a fast error instead of an indefinite block. Neither needs this PR. Branch stays at |
Rebased onto current
mainand reduced to the storage selector alone. One commit, one question.What changed since the last review
The logout contract that shared this branch landed separately in #84, so it is gone from here. What remains is the selector.
More importantly, @steipete's finding was right and it corrected this PR's premise. I verified all three parts independently:
CGO_ENABLED=0.goreleaser.yaml:15, coveringdarwin_amd64anddarwin_arm64keychain.go:1—//go:build darwin && cgoThe old body and README were wrong. They said the OS keyring is the default on macOS and that this option changes where release users' tokens live. It doesn't. Release users are already on the file backend, with no configuration and no opt-in. That text is replaced rather than patched.
What this option is actually for
A cgo-enabled source build:
go install, ormake installon a Mac with a C toolchain. There the Keychain backend is present, a rebuild invalidates the item's ACL, and an unattended host blocks on a consent dialog nobody can answer.That narrows the security question considerably. This does not move release users from Keychain protection to filesystem protection, because they were never on Keychain protection. It adds an opt-in for source builds that currently have it.
The tradeoff, unchanged
The file backend's password is a fixed constant compiled into the binary, not a user secret, so filesystem permissions carry the boundary. Where an OS keyring backend is compiled in it remains the default and the stronger option. The README says this plainly.
Logout is not narrowed
The pin is a flag consulted only by
SaveandLoad, viaprimaryOpener().Clear()always reaches both real backends. Covered at two levels:TestClearRevokesPrimaryWhenFileBackendPinned— seed primary, pin, clear, unpin, require the primary entry to be gone.TestFreshClientDoesNotSendTokenRevokedWhilePinnedToFile— the same sequence against the production client, requiring a fresh client to re-authenticate rather than put the revoked token on the wire. Asserts non-vacuity first: before logout, the cached token really is sent.TestPinRoutesSaveAndLoadToFileBackend— the pin never opens the OS keyring for Save or Load.Restoring the aliasing bug fails both guards:
Worth weighing against this PR
A separate finding says the re-prompt's cause is the ad-hoc signature from
make install: an ad-hoc designated requirement is a literal cdhash pin, and Go does not build reproducibly, so every rebuild invalidates the ACL. If that is the cause, signing local builds with a stable identity fixes it closer to the root, and this selector is a workaround. That remedy is untested — it needs a machine with a Developer ID.Raising it because it argues against merging this as-is. I am happy to close in favor of the signing fix, or to keep this as a verified escape hatch while that is proven out. Your call.
Verification
go build ./...,go vet ./...,gofmt -lcleango test ./...— all packages passdb84b93; conflict with main resolved by dropping what fix(tokencache): report incomplete logout cleanup #84 already landed🤖 Generated with Claude Code
https://claude.ai/code/session_01CtVNrHHZRwkbZhk34UxLi8