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

credstore/probe.go

@@ -0,0 +1,58 @@
+package credstore
+
+import (
+	"context"
+	"time"
+
+	"github.com/zalando/go-keyring"
+)
+
+// probeKeyring checks system keyring availability. Overridable in tests.
+var probeKeyring = probe
+
+// The probe entry lives in this package's own service namespace —
+// probeServicePrefix plus the caller's service — publicly documented on
+// StoreOptions.ProbeTimeout as reserved by credstore, so probing never
+// touches the caller's real service and a colliding consumer would have to
+// deliberately adopt this package's declared namespace. Within it, the key
+// is deliberately deterministic, not random: go-keyring has no list API, so
+// an entry leaked by an abandoned probe (a timed-out probe whose blocked Set
+// 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.
+const (
+	probeServicePrefix = "credstore.probe."
+	probeKey           = "__probe__"
+)
+
+// probeService derives the reserved namespace the probe entry lives in.
+func probeService(serviceName string) string {
+	return probeServicePrefix + serviceName
+}
+
+// probe writes and removes a throwaway keyring entry to check availability.
+// A zero or negative timeout probes unbounded, matching historical behavior.
+// A positive timeout bounds the probe; on platforms where the probe runs a
+// child process (darwin), the child is killed when the timeout expires.
+func probe(serviceName string, timeout time.Duration) error {
+	service := probeService(serviceName)
+	if timeout <= 0 {
+		return probeDirect(service, probeKey)
+	}
+
+	ctx, cancel := context.WithTimeout(context.Background(), timeout)
+	defer cancel()
+	return probeBounded(ctx, service, probeKey)
+}
+
+// probeDirect probes via go-keyring, which has no cancellation path.
+func probeDirect(serviceName, key string) error {
+	if err := keyring.Set(serviceName, key, "probe"); err != nil {
+		return err
+	}
+	_ = keyring.Delete(serviceName, key)
+	return nil
+}

credstore/probe_darwin.go

@@ -0,0 +1,76 @@
+package credstore
+
+import (
+	"context"
+	"encoding/base64"
+	"fmt"
+	"os/exec"
+	"regexp"
+	"strings"
+	"time"
+)
+
+// securityPath is the macOS keychain tool go-keyring shells out to.
+// Variable so tests can substitute a stub.
+var securityPath = "/usr/bin/security"
+
+// probeCleanupTimeout bounds removal of the probe entry. Cleanup runs
+// synchronously under its own budget rather than the probe's: a successful
+// add may have consumed the probe deadline, and a delete skipped or killed
+// by the exhausted probe context would leave a stray probe entry in the
+// keychain. Synchronous because Go does not wait for goroutines at process
+// exit — an unjoined cleanup in a short-lived CLI would leak the entry. The
+// tail is theoretical in practice: cleanup only runs when the keychain
+// proved responsive milliseconds earlier. StoreOptions.ProbeTimeout
+// documents this additive bound.
+const probeCleanupTimeout = 5 * time.Second
+
+// 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
+// cancellation path: on a locked keychain with no TTY or GUI it blocks
+// forever and the child cannot be reclaimed from outside.
+func probeBounded(ctx context.Context, serviceName, key string) error {
+	// go-keyring base64-encodes every password it stores; mirror that so
+	// probe success genuinely predicts go-keyring usability.
+	password := "go-keyring-base64:" + base64.StdEncoding.EncodeToString([]byte("probe"))
+	command := fmt.Sprintf("add-generic-password -U -s %s -a %s -w %s\n",
+		quoteSecurityArg(serviceName), quoteSecurityArg(key), quoteSecurityArg(password))
+
+	cmd := exec.CommandContext(ctx, securityPath, "-i")
+	cmd.Stdin = strings.NewReader(command)
+	if err := cmd.Run(); err != nil {
+		if ctx.Err() != nil {
+			return ctx.Err()
+		}
+		return err
+	}
+
+	afterProbeAdd()
+
+	// Best-effort by design: the add already proved availability, and a
+	// failed delete must not fail the probe.
+	cleanupCtx, cancel := context.WithTimeout(context.Background(), probeCleanupTimeout)
+	defer cancel()
+	_ = exec.CommandContext(cleanupCtx, securityPath, "delete-generic-password", "-s", serviceName, "-a", key).Run()
+	return nil
+}
+
+// afterProbeAdd runs between the probe's add and its cleanup delete. No-op in
+// production; tests use it to expire the probe context in that window and pin
+// cleanup's independence from it.
+var afterProbeAdd = func() {}
+
+var securityArgUnsafe = regexp.MustCompile(`[^\w@%+=:,./-]`)
+
+// quoteSecurityArg mirrors go-keyring's internal shellescape.Quote so the
+// probe entry is written exactly as go-keyring would write it.
+func quoteSecurityArg(s string) string {
+	if s == "" {
+		return "''"
+	}
+	if securityArgUnsafe.MatchString(s) {
+		return "'" + strings.ReplaceAll(s, "'", `'"'"'`) + "'"
+	}
+	return s
+}

credstore/probe_darwin_test.go

@@ -0,0 +1,141 @@
+package credstore
+
+import (
+	"context"
+	"os"
+	"path/filepath"
+	"strconv"
+	"strings"
+	"syscall"
+	"testing"
+	"time"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+func stubSecurity(t *testing.T, script string) {
+	t.Helper()
+	stub := filepath.Join(t.TempDir(), "security")
+	require.NoError(t, os.WriteFile(stub, []byte(script), 0755))
+	restore := securityPath
+	securityPath = stub
+	t.Cleanup(func() { securityPath = restore })
+}
+
+// stubDir returns a scratch directory whose path contains a space, so any
+// stub script that embeds a path without quoting fails loudly here rather
+// than only on machines with space-containing TMPDIRs.
+func stubDir(t *testing.T) string {
+	t.Helper()
+	dir := filepath.Join(t.TempDir(), "dir with space")
+	require.NoError(t, os.Mkdir(dir, 0755))
+	return dir
+}
+
+// shQuote single-quotes s for safe embedding in generated stub scripts.
+func shQuote(s string) string {
+	return "'" + strings.ReplaceAll(s, "'", `'"'"'`) + "'"
+}
+
+func TestProbeBoundedKillsChildOnTimeout(t *testing.T) {
+	pidFile := filepath.Join(stubDir(t), "pid")
+	// exec replaces the shell so the recorded PID is the hung process itself,
+	// mirroring a `security -i` blocked on a locked keychain.
+	stubSecurity(t, "#!/bin/sh\nPF="+shQuote(pidFile)+"\necho $$ > \"$PF.tmp\" && mv \"$PF.tmp\" \"$PF\"\nexec sleep 60\n")
+
+	// Expire the context once the child is demonstrably hung (PID recorded),
+	// so the test exercises exactly the timeout path without racing startup.
+	// The watcher observes a deadline: a broken fixture cancels anyway and
+	// fails fast on the missing PID file instead of hanging the test.
+	ctx, cancel := context.WithCancel(context.Background())
+	defer cancel()
+	deadline := time.Now().Add(5 * time.Second)
+	go func() {
+		for time.Now().Before(deadline) {
+			if _, err := os.Stat(pidFile); err == nil {
+				break
+			}
+			time.Sleep(10 * time.Millisecond)
+		}
+		cancel()
+	}()
+
+	err := probeBounded(ctx, "test", "__probe_timeout")
+	assert.ErrorIs(t, err, context.Canceled)
+
+	raw, err := os.ReadFile(pidFile)
+	require.NoError(t, err)
+	pid, err := strconv.Atoi(strings.TrimSpace(string(raw)))
+	require.NoError(t, err)
+
+	assert.Eventually(t, func() bool {
+		return syscall.Kill(pid, 0) == syscall.ESRCH
+	}, 5*time.Second, 10*time.Millisecond, "hung child should be killed when the context expires")
+}
+
+// argsStub emits a stub security that records each invocation's argv and
+// consumes stdin, mirroring a healthy keychain.
+func argsStub(t *testing.T) (argsFile string) {
+	t.Helper()
+	argsFile = filepath.Join(stubDir(t), "args")
+	stubSecurity(t, "#!/bin/sh\nAF="+shQuote(argsFile)+"\necho \"$@\" >> \"$AF\"\ncat > /dev/null\nexit 0\n")
+	return argsFile
+}
+
+// requireCleanupDelete asserts the stub recorded the add followed by the
+// cleanup delete. Both are synchronous: cleanup is deliberately not detached
+// (see probeCleanupTimeout), so it has completed by the time probeBounded
+// returns.
+func requireCleanupDelete(t *testing.T, argsFile, service, key string) {
+	t.Helper()
+	raw, err := os.ReadFile(argsFile)
+	require.NoError(t, err)
+	lines := strings.Split(strings.TrimSpace(string(raw)), "\n")
+	require.Len(t, lines, 2, "probe should add then delete the probe entry")
+	assert.Equal(t, "-i", lines[0])
+	assert.Equal(t, "delete-generic-password -s "+service+" -a "+key, lines[1])
+}
+
+func TestProbeBoundedSuccess(t *testing.T) {
+	argsFile := argsStub(t)
+
+	ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
+	defer cancel()
+
+	require.NoError(t, probeBounded(ctx, "test", "__probe_ok"))
+	requireCleanupDelete(t, argsFile, "test", "__probe_ok")
+}
+
+// The probe must operate in its own service namespace so it can never touch
+// a credential in the caller's real service, whatever its name.
+func TestProbeUsesIsolatedNamespace(t *testing.T) {
+	argsFile := argsStub(t)
+
+	require.NoError(t, probe("svc", 5*time.Second))
+	requireCleanupDelete(t, argsFile, probeServicePrefix+"svc", probeKey)
+}
+
+// Regression: the probe deadline expiring immediately after a successful add
+// must not skip or kill the cleanup delete — cleanup runs synchronously
+// under its own budget (the documented additive bound), so no stray probe
+// entry is left in the keychain.
+func TestProbeBoundedCleanupSurvivesProbeExpiry(t *testing.T) {
+	argsFile := argsStub(t)
+
+	ctx, cancel := context.WithCancel(context.Background())
+	defer cancel()
+	restore := afterProbeAdd
+	afterProbeAdd = cancel // probe context is dead before cleanup starts
+	t.Cleanup(func() { afterProbeAdd = restore })
+
+	require.NoError(t, probeBounded(ctx, "test", "__probe_expiry"))
+	requireCleanupDelete(t, argsFile, "test", "__probe_expiry")
+}
+
+func TestQuoteSecurityArg(t *testing.T) {
+	assert.Equal(t, "basecamp", quoteSecurityArg("basecamp"))
+	assert.Equal(t, "''", quoteSecurityArg(""))
+	assert.Equal(t, "'my service'", quoteSecurityArg("my service"))
+	assert.Equal(t, `'it'"'"'s'`, quoteSecurityArg("it's"))
+}

credstore/probe_other.go

@@ -0,0 +1,24 @@
+//go:build !darwin
+
+package credstore
+
+import "context"
+
+// probeBounded runs the go-keyring probe with a deadline. Non-darwin
+// backends (dbus secret service, Windows credential manager) run in-process,
+// so timing out abandons at most a goroutine — there is no child process to
+// reclaim. An abandoned probe whose blocked Set later succeeds can leak the
+// probe entry if the process exits before Delete runs; the deterministic
+// probeKey makes that self-healing — the next probe overwrites and removes
+// the leftover (see probeKey).
+func probeBounded(ctx context.Context, serviceName, key string) error {
+	done := make(chan error, 1)
+	go func() { done <- probeDirect(serviceName, key) }()
+
+	select {
+	case err := <-done:
+		return err
+	case <-ctx.Done():
+		return ctx.Err()
+	}
+}

credstore/probe_test.go

@@ -0,0 +1,20 @@
+package credstore
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+)
+
+// Pins the leak-containment contract on every platform, not just darwin: a
+// probe abandoned mid-flight (a timed-out non-darwin probe whose blocked Set
+// completes after process exit, or a darwin cleanup cut short) can leave at
+// most the one known entry — deterministic account, reserved service — which
+// the next probe's Set overwrites and Delete removes. The names below are
+// publicly documented on StoreOptions.ProbeTimeout; changing either breaks
+// self-healing across versions and orphans entries written by earlier
+// releases.
+func TestProbeContractIsDeterministicAndReserved(t *testing.T) {
+	assert.Equal(t, "credstore.probe.svc", probeService("svc"))
+	assert.Equal(t, "__probe__", probeKey)
+}