diff --git a/credstore/probe.go b/credstore/probe.go new file mode 100644 index 0000000..6b1d6ec --- /dev/null +++ b/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 +} diff --git a/credstore/probe_darwin.go b/credstore/probe_darwin.go new file mode 100644 index 0000000..d687676 --- /dev/null +++ b/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 +} diff --git a/credstore/probe_darwin_test.go b/credstore/probe_darwin_test.go new file mode 100644 index 0000000..3792b14 --- /dev/null +++ b/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")) +} diff --git a/credstore/probe_other.go b/credstore/probe_other.go new file mode 100644 index 0000000..6d63e80 --- /dev/null +++ b/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() + } +} diff --git a/credstore/probe_test.go b/credstore/probe_test.go new file mode 100644 index 0000000..1bec2d3 --- /dev/null +++ b/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) +} diff --git a/credstore/store.go b/credstore/store.go index fdacb0e..dd2556f 100644 --- a/credstore/store.go +++ b/credstore/store.go @@ -1,11 +1,10 @@ package credstore import ( - "crypto/rand" - "encoding/hex" "fmt" "os" "path/filepath" + "time" "github.com/zalando/go-keyring" ) @@ -15,10 +14,42 @@ type StoreOptions struct { // ServiceName is the keyring service name (e.g., "basecamp", "fizzy"). ServiceName string - // DisableEnvVar is the env var name that disables keyring (e.g., "BASECAMP_NO_KEYRING"). - // When set to any non-empty value, forces file-based storage. + // DisableEnvVar names an environment variable (e.g., "BASECAMP_NO_KEYRING"). + // When that variable is set to any non-empty value in the process + // environment, the store uses file-based storage without probing the + // keyring. An empty DisableEnvVar field disables this check entirely. DisableEnvVar string + // ForceFile forces file-based storage without probing the keyring — + // the programmatic equivalent of the DisableEnvVar environment variable + // being set. + ForceFile bool + + // ProbeTimeout bounds the keyring availability probe. Zero or negative + // means no bound, matching historical behavior. When the probe times + // out, the store falls back to file storage as if the probe had failed. + // Probing writes and removes a throwaway entry under the dedicated + // keyring service "credstore.probe." (account "__probe__") + // — a namespace reserved by this package — never under ServiceName + // itself, so a probe cannot touch real credentials. + // + // On darwin, removal of the throwaway probe entry runs synchronously + // after a successful probe with a short budget of its own, so worst-case + // construction there is ProbeTimeout plus that cleanup bound (five + // seconds; typically milliseconds, since cleanup only runs when the + // keyring just proved responsive). That cleanup is deliberately not + // detached: Go does not wait for goroutines at process exit, and a + // short-lived CLI would leak a probe entry per invocation. On other + // platforms cleanup is part of the probe itself and adds no extra time. + // + // Limitation: on darwin, a positive timeout probes via the security + // binary directly — the whole point is operating below go-keyring's + // uncancellable exec layer — so it cannot observe go-keyring's mocked + // provider (keyring.MockInit), which go-keyring does not expose. Tests + // that mock the keyring should use a zero ProbeTimeout (the unbounded + // probe goes through go-keyring and honors the mock) or ForceFile. + ProbeTimeout time.Duration + // FallbackDir is the directory for file-based credential storage. FallbackDir string } @@ -34,15 +65,11 @@ type Store struct { // NewStore creates a credential store. It probes the system keyring // and falls back to file storage if unavailable. func NewStore(opts StoreOptions) *Store { - if opts.DisableEnvVar != "" && os.Getenv(opts.DisableEnvVar) != "" { + if opts.ForceFile || (opts.DisableEnvVar != "" && os.Getenv(opts.DisableEnvVar) != "") { return &Store{serviceName: opts.ServiceName, useKeyring: false, fallbackDir: opts.FallbackDir} } - // Probe keyring with a random key to avoid collisions. - probeKey := probeKeyName() - err := keyring.Set(opts.ServiceName, probeKey, "probe") - if err == nil { - _ = keyring.Delete(opts.ServiceName, probeKey) + if probeKeyring(opts.ServiceName, opts.ProbeTimeout) == nil { return &Store{serviceName: opts.ServiceName, useKeyring: true, fallbackDir: opts.FallbackDir} } @@ -60,12 +87,6 @@ func (s *Store) FallbackWarning() string { return s.fallbackWarning } -func probeKeyName() string { - b := make([]byte, 8) - _, _ = rand.Read(b) - return "__probe_" + hex.EncodeToString(b) -} - func (s *Store) key(name string) string { return fmt.Sprintf("%s::%s", s.serviceName, name) } diff --git a/credstore/store_test.go b/credstore/store_test.go index ebdb579..bb9a833 100644 --- a/credstore/store_test.go +++ b/credstore/store_test.go @@ -1,14 +1,23 @@ package credstore import ( + "context" "os" "path/filepath" "testing" + "time" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) +func stubProbe(t *testing.T, fn func(serviceName string, timeout time.Duration) error) { + t.Helper() + restore := probeKeyring + probeKeyring = fn + t.Cleanup(func() { probeKeyring = restore }) +} + func TestFileStore(t *testing.T) { dir := t.TempDir() t.Setenv("TEST_NO_KEYRING", "1") @@ -69,6 +78,65 @@ func TestFileStoreMultipleKeys(t *testing.T) { assert.JSONEq(t, `{"b":2}`, string(d2)) } +func TestForceFileSkipsProbe(t *testing.T) { + dir := t.TempDir() + stubProbe(t, func(string, time.Duration) error { + t.Error("probe should not run when ForceFile is set") + return nil + }) + + store := NewStore(StoreOptions{ + ServiceName: "test", + ForceFile: true, + FallbackDir: dir, + }) + + assert.False(t, store.UsingKeyring()) + assert.Empty(t, store.FallbackWarning()) + + require.NoError(t, store.Save("mykey", []byte(`{"token":"abc123"}`))) + data, err := store.Load("mykey") + require.NoError(t, err) + assert.JSONEq(t, `{"token":"abc123"}`, string(data)) +} + +func TestProbeTimeoutFallsBackToFile(t *testing.T) { + dir := t.TempDir() + stubProbe(t, func(serviceName string, timeout time.Duration) error { + assert.Equal(t, "test", serviceName) + assert.Equal(t, 50*time.Millisecond, timeout) + return context.DeadlineExceeded + }) + + store := NewStore(StoreOptions{ + ServiceName: "test", + ProbeTimeout: 50 * time.Millisecond, + FallbackDir: dir, + }) + + assert.False(t, store.UsingKeyring()) + assert.Contains(t, store.FallbackWarning(), "system keyring unavailable") +} + +func TestZeroValueOptionsProbeUnbounded(t *testing.T) { + dir := t.TempDir() + probed := false + stubProbe(t, func(serviceName string, timeout time.Duration) error { + probed = true + assert.Zero(t, timeout) + return nil + }) + + store := NewStore(StoreOptions{ + ServiceName: "test", + FallbackDir: dir, + }) + + assert.True(t, probed) + assert.True(t, store.UsingKeyring()) + assert.Empty(t, store.FallbackWarning()) +} + func TestLoadNonexistent(t *testing.T) { dir := t.TempDir() t.Setenv("TEST_NO_KEYRING", "1")