credstore: don't mistake a lost probe-write race for an unavailable keyring

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
 }

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()

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 (<default>): 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 (<default>): 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(""))

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)
+}