From 06d7c4a0d06035d2094c33744fbfa7b315429cb5 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Thu, 27 Aug 2026 20:15:56 -0700 Subject: [PATCH 1/2] credstore: don't mistake a lost probe-write race for an unavailable keyring MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every invocation probes keyring availability by writing and deleting one fixed-name entry (credstore.probe. / __probe__). Two concurrent invocations therefore churn the same keychain item, and on darwin `security add-generic-password -U` is find-then-create inside the security tool: when a peer's delete lands between the find (miss) and the create, the create fails with errSecDuplicateItem (-25299) even though the keychain is perfectly healthy. The losing process then silently fell back to file storage, and a machine whose credentials live in the keychain got "credentials not found for profile:" (auth_required) for profiles that are fully authenticated — intermittently, only under concurrency. Observed in the wild: a long-running connector polling `basecamp` every 15s alongside a second connector failed ~5% of ticks with auth_required while every interactive (serial) invocation succeeded. Two concurrent probe loops against a real keychain reproduce it at 99/200 lost probes; with this change, 0/400. A probe answer that proves the keychain responsive must count as availability, not grounds for the file fallback: - Bounded darwin probe: capture `security` output and treat errSecDuplicateItem as success, then clean up as usual. Matched by the ABI-stable OSStatus (-25299), not the prose message. - Unbounded probe (go-keyring returns a bare exit error with no output): disambiguate a failed Set with a Get. The entry present (peer's probe) or cleanly absent (ErrNotFound, peer cleaned up) proves the keyring answers; only failing both write and read reports unavailability. Real credential entries are immune to this interleaving — they are never delete/add churned, so -U reliably takes its update path. Only the probe's write-then-delete pattern creates the race. --- credstore/probe.go | 42 ++++++++++++++++++++--- credstore/probe_darwin.go | 19 +++++++++-- credstore/probe_darwin_test.go | 41 ++++++++++++++++++++++ credstore/probe_test.go | 62 ++++++++++++++++++++++++++++++++++ 4 files changed, 157 insertions(+), 7 deletions(-) diff --git a/credstore/probe.go b/credstore/probe.go index 6b1d6ec..b4d6386 100644 --- a/credstore/probe.go +++ b/credstore/probe.go @@ -2,6 +2,7 @@ package credstore import ( "context" + "errors" "time" "github.com/zalando/go-keyring" @@ -20,14 +21,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 failed write against evidence of a responsive keyring +// (a duplicate-item error, or a read the keyring answers) 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 +67,24 @@ 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 disambiguate with a read: a keyring that +// answers Get, with the entry present (the peer's probe) or cleanly absent +// (ErrNotFound — the peer already cleaned up), is demonstrably responsive +// and usable. Only a keyring that fails both the write and the read is +// reported unavailable. func probeDirect(serviceName, key string) error { - if err := keyring.Set(serviceName, key, "probe"); err != nil { + if err := keyringSet(serviceName, key, "probe"); err != nil { + if _, getErr := keyringGet(serviceName, key); getErr == nil || errors.Is(getErr, keyring.ErrNotFound) { + _ = keyringDelete(serviceName, key) + return 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..5edd343 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,63 @@ 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. A +// keyring that answers the disambiguating read — entry present or cleanly +// absent — is available. +func TestProbeDirectWriteRaceDisambiguatedByRead(t *testing.T) { + setErr := errors.New("exit status 45") + + t.Run("peer's probe entry still 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 whichever probe entry won") + }) + + t.Run("peer already cleaned up", func(t *testing.T) { + stubKeyringOps(t, + func(_, _, _ string) error { return setErr }, + func(_, _ string) (string, error) { return "", keyring.ErrNotFound }) + + assert.NoError(t, probeDirect("credstore.probe.svc", probeKey)) + }) + + 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) +} From c8f60999f36fe9beddffea0753265b60687d9fc2 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Thu, 27 Aug 2026 20:20:25 -0700 Subject: [PATCH 2/2] credstore: require write evidence when recovering a lost probe race MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review caught a hole in the unbounded path: recovering on a clean ErrNotFound read meant a read-only keyring — writes rejected, probe entry absent — would be classified available, and every later Save would fail instead of using the documented file fallback. Recovery now demands fresh write evidence: retry the Set once (the churned entry has settled, so a healthy keyring accepts it — present means -U updates in place, absent means a plain create lands), and if the retry also loses, accept only a Get that finds the entry — a same-uid peer's completed write, which is what sustained concurrent-probe churn looks like. Failing both writes with no peer entry reports the original write error. Concurrent tight-loop validation against a real keychain stays at 0/200 failures. --- credstore/probe.go | 39 ++++++++++++++++++++++++--------------- credstore/probe_test.go | 34 ++++++++++++++++++++++++++-------- 2 files changed, 50 insertions(+), 23 deletions(-) diff --git a/credstore/probe.go b/credstore/probe.go index b4d6386..e196fd1 100644 --- a/credstore/probe.go +++ b/credstore/probe.go @@ -2,7 +2,6 @@ package credstore import ( "context" - "errors" "time" "github.com/zalando/go-keyring" @@ -28,10 +27,10 @@ var probeKeyring = probe // 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 failed write against evidence of a responsive keyring -// (a duplicate-item error, or a read the keyring answers) as availability, -// not as grounds for the file fallback — see probeDirect and the darwin -// probeBounded. +// 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__" @@ -72,18 +71,28 @@ func probe(serviceName string, timeout time.Duration) error { // 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 disambiguate with a read: a keyring that -// answers Get, with the entry present (the peer's probe) or cleanly absent -// (ErrNotFound — the peer already cleaned up), is demonstrably responsive -// and usable. Only a keyring that fails both the write and the read is -// reported unavailable. +// 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 := keyringSet(serviceName, key, "probe"); err != nil { - if _, getErr := keyringGet(serviceName, key); getErr == nil || errors.Is(getErr, keyring.ErrNotFound) { - _ = keyringDelete(serviceName, key) - return nil + 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 + } } - return err } _ = keyringDelete(serviceName, key) return nil diff --git a/credstore/probe_test.go b/credstore/probe_test.go index 5edd343..1c37c21 100644 --- a/credstore/probe_test.go +++ b/credstore/probe_test.go @@ -40,27 +40,45 @@ func stubKeyringOps(t *testing.T, set func(string, string, string) error, get fu // 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. A -// keyring that answers the disambiguating read — entry present or cleanly -// absent — is available. -func TestProbeDirectWriteRaceDisambiguatedByRead(t *testing.T) { +// 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("peer's probe entry still present", func(t *testing.T) { + 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 whichever probe entry won") + assert.True(t, *deleted, "cleanup should remove the peer's entry") }) - t.Run("peer already cleaned up", func(t *testing.T) { + 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.NoError(t, probeDirect("credstore.probe.svc", probeKey)) + assert.ErrorIs(t, probeDirect("credstore.probe.svc", probeKey), setErr) }) t.Run("keyring fails the read too: genuinely unavailable", func(t *testing.T) {