credstore: forced file-only construction and a bounded keyring probe

Merged
JE
3mo ago
+424-16

The hang

NewStore probes keyring availability eagerly with keyring.Set. On darwin, zalando/go-keyring v0.2.8 implements Set by exec'ing /usr/bin/security -i and calling Wait() 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 hung security child. 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.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. Previously the only way to force file mode was mutating the process environment before construction. Matching DisableEnvVar, an explicit opt-out carries no fallback warning.

Layer 2: StoreOptions.ProbeTimeout

Bounds the availability probe. On darwin, the bounded probe mirrors go-keyring's Set exactly — security -i fed add-generic-password -U -s ... -a ... -w ... over stdin, with the same argument escaping and go-keyring-base64: password encoding — but through exec.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 ProbeTimeout keeps 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 use ForceFile as its fallback path).

Tests

  • ForceFile constructs without invoking the probe (probe seam stubbed to fail the test if called)
  • Probe timeout falls back to file mode with the fallback warning
  • Zero-value options still probe unbounded (today's behavior)
  • Darwin: a stub security that records its PID and hangs is killed when the context expires (verified via kill(pid, 0) returning ESRCH); success path records the mirrored add/delete invocations; escaping matches go-keyring's shellescape

gofmt, go vet, golangci-lint run (0 issues), go test -race ./... all green; GOOS=linux/GOOS=windows builds verified.

Fixes #55