diff --git a/credstore/probe.go b/credstore/probe.go index 6b1d6ec..e196fd1 100644 --- a/credstore/probe.go +++ b/credstore/probe.go @@ -20,14 +20,31 @@ var probeKeyring = probe // completes after the process exits, or a darwin cleanup cut short) would be // permanently unfindable under a random name. Under a fixed name, the next // probe's Set overwrites the leftover and its Delete removes it — leaks -// self-heal on the following run. Concurrent probes sharing the name are -// harmless: Set results are unaffected, and the losing Delete just fails, -// which is ignored. +// self-heal on the following run. +// +// The fixed name makes concurrent probes race on one shared item. The +// losing Delete just fails, which is ignored — but on darwin the losing Set +// can fail too: `security add-generic-password -U` is find-then-create +// inside the security tool, so a peer's delete/add interleaving surfaces +// errSecDuplicateItem even though the keychain is healthy. Every probe +// therefore treats a lost write race backed by write evidence — a +// duplicate-item error, a successful retry, or the peer's completed write — +// as availability, not as grounds for the file fallback; see probeDirect +// and the darwin probeBounded. const ( probeServicePrefix = "credstore.probe." probeKey = "__probe__" ) +// keyring operations, extracted as vars so tests can exercise probeDirect's +// write-race disambiguation (go-keyring's mock cannot fail Set while +// answering Get). +var ( + keyringSet = keyring.Set + keyringGet = keyring.Get + keyringDelete = keyring.Delete +) + // probeService derives the reserved namespace the probe entry lives in. func probeService(serviceName string) string { return probeServicePrefix + serviceName @@ -49,10 +66,34 @@ func probe(serviceName string, timeout time.Duration) error { } // probeDirect probes via go-keyring, which has no cancellation path. +// +// A failed Set is not yet an unavailable keyring: concurrent probes share +// one fixed-name entry, and on darwin a peer's delete/add interleaving makes +// the write lose with errSecDuplicateItem (see the probeKey comment). The +// write error alone cannot be classified — go-keyring returns a bare exit +// error with no output — so recovery demands fresh evidence the keyring +// accepts writes, never a mere read answer (a read-only keyring cleanly +// misses a Get of the absent probe entry, and reporting it available would +// break every later Save). Two forms of write evidence qualify: +// +// - An immediate retry of the Set succeeds — the contended entry has +// settled (present, so darwin's -U updates in place; absent, so a plain +// create lands) and this process demonstrably wrote. +// - The retry also loses, but Get finds the entry — a peer process of the +// same uid completed exactly this write moments ago, which is what +// sustained churn from concurrent probes looks like. +// +// A keyring that fails both writes and cannot show a peer's is reported +// unavailable with the original write error. func probeDirect(serviceName, key string) error { - if err := keyring.Set(serviceName, key, "probe"); err != nil { - return err + err := keyringSet(serviceName, key, "probe") + if err != nil { + if retryErr := keyringSet(serviceName, key, "probe"); retryErr != nil { + if _, getErr := keyringGet(serviceName, key); getErr != nil { + return err + } + } } - _ = keyring.Delete(serviceName, key) + _ = keyringDelete(serviceName, key) return nil } diff --git a/credstore/probe_darwin.go b/credstore/probe_darwin.go index d687676..1cbd39e 100644 --- a/credstore/probe_darwin.go +++ b/credstore/probe_darwin.go @@ -25,6 +25,12 @@ var securityPath = "/usr/bin/security" // documents this additive bound. const probeCleanupTimeout = 5 * time.Second +// errSecDuplicateItem marks a `security` failure that proves the keychain is +// alive: the OSStatus for "item already exists", printed by `security -i` as +// "add-generic-password: returned -25299". Matched numerically — the code is +// ABI-stable where the prose message is not. +const errSecDuplicateItem = "-25299" + // probeBounded mirrors go-keyring's darwin Set — `security -i` fed an // add-generic-password command over stdin — via exec.CommandContext so the // child is killed when ctx expires. go-keyring's own exec has no @@ -39,11 +45,20 @@ func probeBounded(ctx context.Context, serviceName, key string) error { cmd := exec.CommandContext(ctx, securityPath, "-i") cmd.Stdin = strings.NewReader(command) - if err := cmd.Run(); err != nil { + out, err := cmd.CombinedOutput() + if err != nil { if ctx.Err() != nil { return ctx.Err() } - return err + // errSecDuplicateItem is availability, not failure: add -U is + // find-then-create inside `security`, and a concurrent probe's + // delete/add on the shared fixed-name entry can interleave so the + // create loses to a duplicate (see the probeKey comment). The + // keychain answered — it is responsive and usable. Fall through to + // cleanup, which removes whichever entry won. + if !strings.Contains(string(out), errSecDuplicateItem) { + return err + } } afterProbeAdd() diff --git a/credstore/probe_darwin_test.go b/credstore/probe_darwin_test.go index 3792b14..1f7ee49 100644 --- a/credstore/probe_darwin_test.go +++ b/credstore/probe_darwin_test.go @@ -133,6 +133,47 @@ func TestProbeBoundedCleanupSurvivesProbeExpiry(t *testing.T) { requireCleanupDelete(t, argsFile, "test", "__probe_expiry") } +// Regression: two concurrent CLI invocations probe the same fixed-name +// entry, and `add-generic-password -U` is find-then-create inside +// `security` — a peer's delete/add interleaving makes the losing add fail +// with errSecDuplicateItem (-25299) on a perfectly healthy keychain. That +// answer proves availability; treating it as failure silently degraded the +// loser to the file fallback, which reports "credentials not found" for +// profiles whose tokens sit in the keychain. +func TestProbeBoundedDuplicateItemMeansAvailable(t *testing.T) { + argsFile := filepath.Join(stubDir(t), "args") + // The add (`security -i`) loses the duplicate race; the cleanup delete + // succeeds, removing whichever probe entry won. + stubSecurity(t, "#!/bin/sh\nAF="+shQuote(argsFile)+"\necho \"$@\" >> \"$AF\"\n"+ + "if [ \"$1\" = -i ]; then\ncat > /dev/null\necho 'add-generic-password: returned -25299'\n"+ + "echo 'security: SecKeychainItemCreateFromContent (): The specified item already exists in the keychain.' >&2\nexit 45\nfi\nexit 0\n") + + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + + require.NoError(t, probeBounded(ctx, "test", "__probe_dup")) + requireCleanupDelete(t, argsFile, "test", "__probe_dup") +} + +// Any other add failure still reports the keyring unavailable, and cleanup +// is not attempted. +func TestProbeBoundedNonDuplicateFailureStillFails(t *testing.T) { + argsFile := filepath.Join(stubDir(t), "args") + stubSecurity(t, "#!/bin/sh\nAF="+shQuote(argsFile)+"\necho \"$@\" >> \"$AF\"\ncat > /dev/null\n"+ + "echo 'security: SecKeychainItemCreateFromContent (): User interaction is not allowed.' >&2\nexit 36\n") + + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + + require.Error(t, probeBounded(ctx, "test", "__probe_fail")) + + raw, err := os.ReadFile(argsFile) + require.NoError(t, err) + lines := strings.Split(strings.TrimSpace(string(raw)), "\n") + require.Len(t, lines, 1, "failed probe must not attempt cleanup") + assert.Equal(t, "-i", lines[0]) +} + func TestQuoteSecurityArg(t *testing.T) { assert.Equal(t, "basecamp", quoteSecurityArg("basecamp")) assert.Equal(t, "''", quoteSecurityArg("")) diff --git a/credstore/probe_test.go b/credstore/probe_test.go index 1bec2d3..1c37c21 100644 --- a/credstore/probe_test.go +++ b/credstore/probe_test.go @@ -1,9 +1,11 @@ package credstore import ( + "errors" "testing" "github.com/stretchr/testify/assert" + "github.com/zalando/go-keyring" ) // Pins the leak-containment contract on every platform, not just darwin: a @@ -18,3 +20,81 @@ func TestProbeContractIsDeterministicAndReserved(t *testing.T) { assert.Equal(t, "credstore.probe.svc", probeService("svc")) assert.Equal(t, "__probe__", probeKey) } + +// stubKeyringOps replaces probeDirect's keyring operations for one test. +// go-keyring's mock cannot fail Set while answering Get, which is exactly +// the shape of the concurrent-probe write race. +func stubKeyringOps(t *testing.T, set func(string, string, string) error, get func(string, string) (string, error)) (deleted *bool) { + t.Helper() + deleted = new(bool) + restoreSet, restoreGet, restoreDelete := keyringSet, keyringGet, keyringDelete + keyringSet = set + keyringGet = get + keyringDelete = func(service, key string) error { + *deleted = true + return nil + } + t.Cleanup(func() { keyringSet, keyringGet, keyringDelete = restoreSet, restoreGet, restoreDelete }) + return deleted +} + +// Regression: concurrent probes share one fixed-name entry, and a losing +// write (darwin surfaces errSecDuplicateItem through go-keyring as a bare +// exit error) must not demote a healthy keyring to the file fallback. +// Recovery requires fresh write evidence — this process's retry landing, or +// a peer's completed write sitting in the keyring — never a bare read +// answer, which a read-only keyring could also give. +func TestProbeDirectWriteRaceRecovery(t *testing.T) { + setErr := errors.New("exit status 45") + + t.Run("retry lands once the churned entry settles", func(t *testing.T) { + calls := 0 + deleted := stubKeyringOps(t, + func(_, _, _ string) error { + calls++ + if calls == 1 { + return setErr + } + return nil + }, + func(_, _ string) (string, error) { t.Fatal("no read needed when the retry lands"); return "", nil }) + + assert.NoError(t, probeDirect("credstore.probe.svc", probeKey)) + assert.Equal(t, 2, calls) + assert.True(t, *deleted, "cleanup should remove the retried entry") + }) + + t.Run("retry loses too but the peer's write is present", func(t *testing.T) { + deleted := stubKeyringOps(t, + func(_, _, _ string) error { return setErr }, + func(_, _ string) (string, error) { return "probe", nil }) + + assert.NoError(t, probeDirect("credstore.probe.svc", probeKey)) + assert.True(t, *deleted, "cleanup should remove the peer's entry") + }) + + t.Run("read-only keyring: writes fail, probe entry absent", func(t *testing.T) { + stubKeyringOps(t, + func(_, _, _ string) error { return setErr }, + func(_, _ string) (string, error) { return "", keyring.ErrNotFound }) + + assert.ErrorIs(t, probeDirect("credstore.probe.svc", probeKey), setErr) + }) + + t.Run("keyring fails the read too: genuinely unavailable", func(t *testing.T) { + stubKeyringOps(t, + func(_, _, _ string) error { return setErr }, + func(_, _ string) (string, error) { return "", errors.New("no keyring provider") }) + + assert.ErrorIs(t, probeDirect("credstore.probe.svc", probeKey), setErr) + }) +} + +func TestProbeDirectSuccessCleansUp(t *testing.T) { + deleted := stubKeyringOps(t, + func(_, _, _ string) error { return nil }, + func(_, _ string) (string, error) { t.Fatal("no read needed when the write succeeds"); return "", nil }) + + assert.NoError(t, probeDirect("credstore.probe.svc", probeKey)) + assert.True(t, *deleted) +}