credstore: forced file-only construction and a bounded keyring probe - #56
Conversation
NewStore probes keyring availability with keyring.Set, which on darwin execs /usr/bin/security -i and Wait()s with no cancellation path. On a locked keychain with no TTY or GUI the child blocks forever and cannot be reclaimed from outside go-keyring, so every credential-touching caller hangs unbounded (basecamp/basecamp-cli#568). Two additions to StoreOptions: * ForceFile forces file-backed storage with no keyring probe — the programmatic equivalent of DisableEnvVar, and the fallback target a caller reaches for after a probe timeout. Like DisableEnvVar, an explicit opt-out carries no fallback warning. * ProbeTimeout bounds the availability probe. Zero keeps today's unbounded probe. When set, the darwin probe mirrors go-keyring's Set (security -i fed add-generic-password over stdin, same escaping and base64 password encoding) via exec.CommandContext, so expiry kills the child instead of leaking it. Non-darwin backends run in-process with no child to reclaim, so the bound there is a plain goroutine timeout. A timed-out probe falls back to file storage with the same warning as a failed probe. Fixes #55
There was a problem hiding this comment.
Pull request overview
This PR hardens credstore.NewStore construction by adding an explicit “file-only” option and a timeout-bounded keyring availability probe (with a darwin-specific implementation that can actually terminate a hung security -i child process). This fits the repo’s goal of providing robust shared CLI infrastructure by preventing unbounded hangs during credential store initialization, especially in headless macOS environments.
Changes:
- Add
StoreOptions.ForceFileto allow programmatic file-only construction with no keyring probe. - Add
StoreOptions.ProbeTimeoutand route keyring probing through a newprobeKeyringseam. - Implement a bounded darwin probe via
exec.CommandContextagainstsecurity -i, with platform-specific probe logic and tests.
Tip
If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or run gh pr ready --undo.
Click "Ready for review" or run gh pr ready to reengage.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| credstore/store.go | Adds ForceFile/ProbeTimeout options and switches construction-time probing to probeKeyring. |
| credstore/store_test.go | Adds coverage for ForceFile (skips probe), timeout fallback behavior, and zero-value behavior. |
| credstore/probe.go | Introduces probe implementation + test seam and shared probe key generation. |
| credstore/probe_other.go | Implements bounded probing for non-darwin via goroutine + context deadline. |
| credstore/probe_darwin.go | Implements bounded macOS probe by mirroring security -i behavior under exec.CommandContext. |
| credstore/probe_darwin_test.go | Validates darwin bounded probe kills a hung child and mirrors expected invocations/escaping. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
The probe treats any non-positive timeout as unbounded (timeout <= 0), but the ProbeTimeout and probe doc comments claimed only zero did. Align both comments with the behavior.
|
macOS-specific proof at exact head b2011ec (credstore CI is ubuntu-only, so the darwin probe tests never run hosted): full credstore suite on macOS (Darwin 27.0.0, stock /usr/bin/security) with -race -count=2 — all pass, including TestProbeBoundedKillsChildOnTimeout (hung child confirmed dead via ESRCH after context expiry) and TestProbeBoundedSuccess. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (2)
credstore/probe_darwin_test.go:47
- The goroutine that waits for
pidFileloops forever with no exit condition if the stub fails to create the file (e.g., due to quoting/permissions issues). Because it doesn’t observe context cancellation, it can keep the test process alive and hanggo test. Prefer runningprobeBoundedin a goroutine and usingrequire.Eventually(or a select on ctx.Done) in the main goroutine to wait for the pid file before canceling.
ctx, cancel := context.WithCancel(context.Background())
defer cancel()
go func() {
for {
if _, err := os.Stat(pidFile); err == nil {
cancel()
return
}
time.Sleep(10 * time.Millisecond)
}
}()
err := probeBounded(ctx, "test", "__probe_timeout")
assert.ErrorIs(t, err, context.Canceled)
credstore/probe_darwin_test.go:62
- The stubbed
securityscript appends toargsFilevia an unquoted path. If the temp directory path contains spaces or shell-special characters, the redirection can fail and make this test flaky on some environments. Quote the path via a shell variable in the stub script.
argsFile := filepath.Join(t.TempDir(), "args")
stubSecurity(t, "#!/bin/sh\necho \"$@\" >> "+argsFile+"\ncat > /dev/null\nexit 0\n")
A successful add may consume the probe deadline; the delete then ran under the exhausted probe context, was skipped or killed, and the probe still returned nil — leaving a stray __probe_* entry in the keychain. Cleanup now gets a fresh bounded context (probeCleanupTimeout) and remains best-effort: the add already proved availability, so a failed delete must not fail the probe. afterProbeAdd is a test seam pinning cleanup's independence from the probe context.
The generated security stubs embedded pid/args file paths into shell source unquoted, so a space-containing TMPDIR broke the redirections and could hang the kill test's PID watcher indefinitely. Stub paths are now shell-quoted and every stub lives in a dir-with-space so the quoting is pinned on all machines; the watcher observes a deadline and fails fast on fixture breakage. Adds the cleanup-survives-expiry regression for the probe fix.
probeKeyName ignored rand.Read errors: a failed or short read yields a predictable (zeroed) key, raising the odds of colliding with an existing entry — which cleanup would then delete. The fallback keys on PID + wall clock, unique enough for a transient probe entry, behind a randRead seam so the failure path is testable.
Review carefully before merging. Consider a major version bump. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 47714d1d85
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
A slow cleanup delete under its fresh five-second budget could stretch NewStore past the caller's configured ProbeTimeout, contradicting the documented guarantee that ProbeTimeout bounds the probe. The best-effort delete now runs in a fire-and-forget goroutine: probe latency stays within ProbeTimeout, cleanup keeps its own budget, and the securityPath value is captured synchronously so the goroutine never races the test seam.
probeKeyName's comment promised protection against failed or short reads but the code only checked the error. A short read now takes the same PID + time fallback, pinned by TestProbeKeyNameShortRead.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5c551d6d75
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
A timed-out non-darwin probe abandons its goroutine; if the blocked Set later succeeds and the process exits before Delete, the probe entry leaks — and go-keyring has no list API, so under a random name the leftover is permanently unfindable. A fixed, reserved __probe__ name makes the leak self-healing: the next probe's Set overwrites it and its Delete removes it. Concurrent probes sharing the name are harmless — Set results are unaffected and the losing Delete's failure is already ignored. Supersedes the random-key machinery (rand fallback, PID+time key, per-process sequence) and its tests.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9de72abfff
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
A private constant does not enforce a name reservation: a service that already held an account named __probe__ would have its password overwritten by the probe's Set and removed by its Delete. The probe now operates in serviceName + ".probe", so it cannot touch any credential in the caller's real service, whatever its name — while keeping the deterministic key's self-healing property within the probe namespace. Also from this round: ProbeTimeout docs scope the additive cleanup bound to darwin (elsewhere cleanup is part of the probe itself), and stale __probe_* wording no longer implies the removed randomized scheme.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a91e335629
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
A bare .probe suffix on the caller's service is an ordinary global keyring name, not a reservation. The probe now writes under credstore.probe.<ServiceName>, and the ProbeTimeout docs publicly declare that namespace as reserved by this package — a colliding consumer would have to deliberately adopt the declared namespace, at which point the deterministic key's self-healing semantics are exactly what they'd share.
The deterministic-name containment for abandoned probes (a timed-out non-darwin probe whose blocked Set completes after process exit leaves at most one known entry, overwritten and removed by the next probe) was only exercised by darwin-local tests — invisible to ubuntu CI. A pure derivation helper and a platform-independent test now pin the reserved service and account names; changing either would orphan entries written by earlier releases.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 87e7a24f3a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
The child-process probe exists precisely to operate beneath go-keyring's uncancellable exec layer, so it cannot observe the mocked provider — go-keyring exports no way to detect MockInit. State the limitation on ProbeTimeout with the test guidance: mocked-keyring tests use a zero timeout (probeDirect honors the mock) or ForceFile.
The hang
NewStoreprobes keyring availability eagerly withkeyring.Set. On darwin, zalando/go-keyring v0.2.8 implementsSetby exec'ing/usr/bin/security -iand callingWait()with no cancellation path (keyring_darwin.go:76). On a locked keychain with no TTY or GUI (headless CI, ssh), that child blocks forever. A timeout wrapper in a consumer can only abandon the goroutine — the exec handle lives inside go-keyring, so nothing outside it can kill the hungsecuritychild. basecamp/basecamp-cli#578 made construction lazy on the consumer side, but credential-touching commands still block unbounded. go-keyring has no context-aware API at v0.2.8 (its latest), so the bounded probe has to live here.Layer 1:
StoreOptions.ForceFileForces file-backed storage with no keyring probe — the programmatic equivalent of
DisableEnvVar, and the fallback target a caller reaches for after a probe timeout. Previously the only way to force file mode was mutating the process environment before construction. MatchingDisableEnvVar, an explicit opt-out carries no fallback warning.Layer 2:
StoreOptions.ProbeTimeoutBounds the availability probe. On darwin, the bounded probe mirrors go-keyring's
Setexactly —security -ifedadd-generic-password -U -s ... -a ... -w ...over stdin, with the same argument escaping andgo-keyring-base64:password encoding — but throughexec.CommandContext, so context expiry kills the child instead of leaking it. Probe success therefore genuinely predicts go-keyring usability for subsequent operations. Non-darwin backends (dbus secret service, Windows credential manager) run in-process with no child to reclaim, so the bound there is a plain goroutine timeout. A timed-out probe falls back to file storage with the same warning a failed probe produces today.Compatibility choice: zero
ProbeTimeoutkeeps today's unbounded probe rather than defaulting to a bound. Existing callers see no behavior change from a dependency bump; consumers opt in explicitly (basecamp-cli will set a timeout and useForceFileas its fallback path).Tests
ForceFileconstructs without invoking the probe (probe seam stubbed to fail the test if called)securitythat records its PID and hangs is killed when the context expires (verified viakill(pid, 0)returningESRCH); success path records the mirroredadd/deleteinvocations; escaping matches go-keyring's shellescapegofmt,go vet,golangci-lint run(0 issues),go test -race ./...all green;GOOS=linux/GOOS=windowsbuilds verified.Fixes #55