diff --git a/credstore/probe.go b/credstore/probe.go index e196fd1..4729ccb 100644 --- a/credstore/probe.go +++ b/credstore/probe.go @@ -2,6 +2,10 @@ package credstore import ( "context" + "errors" + "fmt" + "os" + "sync/atomic" "time" "github.com/zalando/go-keyring" @@ -14,34 +18,39 @@ var probeKeyring = probe // 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. +// deliberately adopt this package's declared namespace. // -// 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. +// Within that namespace the account is per probe: probeKeyPrefix, the pid, +// and an in-process sequence number. Concurrent probes must never share a +// keychain item, because on darwin `security add-generic-password -U` is +// find-then-create inside the security tool, and a peer's delete/add +// landing in that window fails the add with errSecDuplicateItem on a +// perfectly healthy keychain — twenty concurrent probes of one shared item +// lost 190 of 200. Distinct pids separate processes; the sequence number +// separates probes within a process, including one still running after its +// bounded wait gave up (non-darwin abandons the worker goroutine) from any +// probe started later. No two probes ever touch the same entry. +// +// The account is still deterministic rather than 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 +// the pid-and-sequence name, the next process to reuse that pid overwrites +// the leftover with its own same-numbered probe — the first, in practice, +// since a process probes once — and removes it. Leaks still self-heal, on +// pid reuse instead of on the very next run. const ( probeServicePrefix = "credstore.probe." - probeKey = "__probe__" + probeKeyPrefix = "__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). +// probeSeq numbers this process's probes so no two share an account. +var probeSeq atomic.Uint64 + +// keyring operations, extracted as vars so tests can observe the entry +// probeDirect writes and removes without a live keyring. var ( keyringSet = keyring.Set - keyringGet = keyring.Get keyringDelete = keyring.Delete ) @@ -50,49 +59,39 @@ func probeService(serviceName string) string { return probeServicePrefix + serviceName } +// probeKey derives a fresh probe account for this process. +func probeKey() string { + return fmt.Sprintf("%s%d.%d", probeKeyPrefix, os.Getpid(), probeSeq.Add(1)) +} + // 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. +// child process (darwin), the child is killed when the timeout expires. A +// probe that hits the bound reports the timeout by name, since that reason +// reaches users through Store.FallbackWarning and Load errors. func probe(serviceName string, timeout time.Duration) error { - service := probeService(serviceName) + service, key := probeService(serviceName), probeKey() if timeout <= 0 { - return probeDirect(service, probeKey) + return probeDirect(service, key) } ctx, cancel := context.WithTimeout(context.Background(), timeout) defer cancel() - return probeBounded(ctx, service, probeKey) + err := probeBounded(ctx, service, key) + if errors.Is(err, context.DeadlineExceeded) { + return fmt.Errorf("keyring probe timed out after %s: %w", timeout, err) + } + return err } -// 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. +// probeDirect probes via go-keyring, which has no cancellation path. Its +// failure is named by the platform (keyringError) so the unbounded path's +// reason reads as well as the bounded path's: on darwin go-keyring returns +// a bare "exit status N" with the security tool's diagnostic discarded. func probeDirect(serviceName, key string) error { - 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 - } - } + if err := keyringSet(serviceName, key, "probe"); err != nil { + return keyringError(err) } _ = keyringDelete(serviceName, key) return nil diff --git a/credstore/probe_darwin.go b/credstore/probe_darwin.go index 1cbd39e..1d12096 100644 --- a/credstore/probe_darwin.go +++ b/credstore/probe_darwin.go @@ -3,6 +3,7 @@ package credstore import ( "context" "encoding/base64" + "errors" "fmt" "os/exec" "regexp" @@ -25,12 +26,6 @@ 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 @@ -50,15 +45,7 @@ func probeBounded(ctx context.Context, serviceName, key string) error { if ctx.Err() != nil { return ctx.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 - } + return securityError(out, err) } afterProbeAdd() @@ -76,6 +63,51 @@ func probeBounded(ctx context.Context, serviceName, key string) error { // cleanup's independence from it. var afterProbeAdd = func() {} +// securityError folds the security tool's diagnostic into its exit error. +// A bare "exit status 36" tells nobody why the keychain was unavailable; +// the tool's own line ("User interaction is not allowed.") does, and that +// reason reaches users through Store.FallbackWarning and Load errors. +func securityError(out []byte, err error) error { + diagnostic := strings.Join(strings.Fields(string(out)), " ") + if diagnostic == "" { + return err + } + return fmt.Errorf("%s (%w)", diagnostic, err) +} + +// securityExitReasons names the keychain failures go-keyring's darwin Set +// can surface, by exit status. go-keyring returns cmd.Wait()'s bare "exit +// status N" and discards security's own diagnostic line, and the unbounded +// probe — every session with a terminal — goes through go-keyring, so +// without this an interactive user's fallback read "exit status 36" where +// the bounded (headless) probe's reads "User interaction is not allowed." +// security exits with the low byte of the SecBase.h OSStatus, so the codes +// are stable; the text is what `security error ` prints. +var securityExitReasons = map[int]string{ + 36: "User interaction is not allowed.", // errSecInteractionNotAllowed (-25308) + 37: "A default keychain could not be found.", // errSecNoDefaultKeychain (-25307) + 45: "The specified item already exists in the keychain.", // errSecDuplicateItem (-25299) + 50: "The specified keychain could not be found.", // errSecNoSuchKeychain (-25294) + 51: "The user name or passphrase you entered is not correct.", // errSecAuthFailed (-25293) + 52: "This keychain cannot be modified.", // errSecReadOnly (-25292) + 53: "No keychain is available. You may need to restart your computer.", // errSecNotAvailable (-25291) + 128: "User canceled the operation.", // errSecUserCanceled (-128) +} + +// keyringError folds the security tool's reason into a go-keyring exit +// error, in the same shape securityError gives the bounded path. Any other +// error — an exit status with no keychain meaning, or no exit at all — +// passes through unchanged rather than being given an invented reason. +func keyringError(err error) error { + var exit *exec.ExitError + if errors.As(err, &exit) { + if reason, ok := securityExitReasons[exit.ExitCode()]; ok { + return fmt.Errorf("%s (%w)", reason, err) + } + } + return err +} + var securityArgUnsafe = regexp.MustCompile(`[^\w@%+=:,./-]`) // quoteSecurityArg mirrors go-keyring's internal shellescape.Quote so the diff --git a/credstore/probe_darwin_test.go b/credstore/probe_darwin_test.go index 1f7ee49..c71cdca 100644 --- a/credstore/probe_darwin_test.go +++ b/credstore/probe_darwin_test.go @@ -3,7 +3,9 @@ package credstore import ( "context" "os" + "os/exec" "path/filepath" + "regexp" "strconv" "strings" "syscall" @@ -108,12 +110,31 @@ func TestProbeBoundedSuccess(t *testing.T) { } // 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) { +// a credential in the caller's real service, whatever its name — and under +// its own per-probe account, so concurrent probes never contend for one +// keychain item (see probeKey). +func TestProbeUsesIsolatedPerProbeEntry(t *testing.T) { argsFile := argsStub(t) require.NoError(t, probe("svc", 5*time.Second)) - requireCleanupDelete(t, argsFile, probeServicePrefix+"svc", probeKey) + + 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.Regexp(t, "^delete-generic-password -s "+regexp.QuoteMeta(probeServicePrefix+"svc")+" -a "+probeKeyPattern()[1:], lines[1]) +} + +// A probe that hits its bound must say so: the reason reaches users through +// the fallback warning and Load errors, where a bare "context deadline +// exceeded" explains nothing. +func TestProbeTimeoutIsNamed(t *testing.T) { + stubSecurity(t, "#!/bin/sh\nexec sleep 60\n") + + err := probe("svc", 20*time.Millisecond) + assert.ErrorIs(t, err, context.DeadlineExceeded) + assert.ErrorContains(t, err, "keyring probe timed out after 20ms") } // Regression: the probe deadline expiring immediately after a successful add @@ -133,31 +154,10 @@ 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) { +// A failed add reports the keyring unavailable with the security tool's own +// diagnostic folded in — that reason is what users see when the store +// explains its fallback — and cleanup is not attempted. +func TestProbeBoundedFailureCarriesDiagnostic(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") @@ -165,7 +165,10 @@ func TestProbeBoundedNonDuplicateFailureStillFails(t *testing.T) { ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) defer cancel() - require.Error(t, probeBounded(ctx, "test", "__probe_fail")) + err := probeBounded(ctx, "test", "__probe_fail") + require.Error(t, err) + assert.ErrorContains(t, err, "User interaction is not allowed.") + assert.ErrorContains(t, err, "exit status 36") raw, err := os.ReadFile(argsFile) require.NoError(t, err) @@ -174,6 +177,31 @@ func TestProbeBoundedNonDuplicateFailureStillFails(t *testing.T) { assert.Equal(t, "-i", lines[0]) } +// Regression: go-keyring's darwin Set returns cmd.Wait()'s bare "exit +// status 36" with security's diagnostic discarded, so the unbounded probe — +// the interactive path — explained its fallback with a number where the +// bounded probe gave the reason. The exit status must be named the same way. +func TestProbeDirectNamesSecurityExitStatus(t *testing.T) { + exit36 := exec.Command("/bin/sh", "-c", "exit 36").Run() + require.Error(t, exit36) + recordKeyringOps(t, exit36) + + err := probe("svc", 0) + assert.ErrorIs(t, err, exit36) + assert.EqualError(t, err, "User interaction is not allowed. (exit status 36)") +} + +// An exit status with no keychain meaning passes through unchanged rather +// than being given an invented reason. (A non-exit error is covered by +// TestProbeDirectFailureSkipsCleanup.) +func TestProbeDirectPassesUnknownExitStatusThrough(t *testing.T) { + exit3 := exec.Command("/bin/sh", "-c", "exit 3").Run() + require.Error(t, exit3) + recordKeyringOps(t, exit3) + + assert.Same(t, exit3, probe("svc", 0)) +} + func TestQuoteSecurityArg(t *testing.T) { assert.Equal(t, "basecamp", quoteSecurityArg("basecamp")) assert.Equal(t, "''", quoteSecurityArg("")) diff --git a/credstore/probe_other.go b/credstore/probe_other.go index 6d63e80..1dd09ad 100644 --- a/credstore/probe_other.go +++ b/credstore/probe_other.go @@ -8,9 +8,9 @@ import "context" // 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). +// probe entry if the process exits before Delete runs; the pid-derived +// probeKey makes that self-healing — the next probe from a process reusing +// that pid 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) }() @@ -22,3 +22,7 @@ func probeBounded(ctx context.Context, serviceName, key string) error { return ctx.Err() } } + +// keyringError passes a go-keyring failure through: non-darwin backends run +// in-process and their errors already name the failure. +func keyringError(err error) error { return err } diff --git a/credstore/probe_test.go b/credstore/probe_test.go index 1c37c21..a612e4c 100644 --- a/credstore/probe_test.go +++ b/credstore/probe_test.go @@ -2,99 +2,74 @@ package credstore import ( "errors" + "os" + "strconv" "testing" "github.com/stretchr/testify/assert" - "github.com/zalando/go-keyring" ) -// 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) { +// Pins the probe-entry contract on every platform, not just darwin. +// +// Per probe: concurrent probes must never share a keychain item — on darwin +// `add-generic-password -U` is find-then-create, and a peer's delete/add +// landing in that window fails the add with errSecDuplicateItem on a +// healthy keychain, silently demoting the loser to the file fallback. The +// pid separates processes; the sequence separates probes within one, +// including a timed-out probe's abandoned worker from a later probe. +// +// Deterministic, not random: a probe abandoned mid-flight can leave at most +// one entry — this account, reserved service — which the next process +// reusing the pid overwrites and removes. The names are publicly documented +// on StoreOptions.ProbeTimeout. +func TestProbeContractIsPerProbeAndReserved(t *testing.T) { assert.Equal(t, "credstore.probe.svc", probeService("svc")) - assert.Equal(t, "__probe__", probeKey) + + first, second := probeKey(), probeKey() + assert.Regexp(t, probeKeyPattern(), first) + assert.Regexp(t, probeKeyPattern(), second) + assert.NotEqual(t, first, second, "each probe gets its own account") +} + +// probeKeyPattern matches any probe account this process derives. +func probeKeyPattern() string { + return `^__probe__\.` + strconv.Itoa(os.Getpid()) + `\.\d+$` } -// 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) { +// recordKeyringOps replaces probeDirect's keyring operations for one test, +// recording the entry each touched. +func recordKeyringOps(t *testing.T, setErr error) (set, deleted *[2]string) { t.Helper() - deleted = new(bool) - restoreSet, restoreGet, restoreDelete := keyringSet, keyringGet, keyringDelete - keyringSet = set - keyringGet = get + set, deleted = new([2]string), new([2]string) + restoreSet, restoreDelete := keyringSet, keyringDelete + keyringSet = func(service, key, _ string) error { + *set = [2]string{service, key} + return setErr + } keyringDelete = func(service, key string) error { - *deleted = true + *deleted = [2]string{service, key} return nil } - t.Cleanup(func() { keyringSet, keyringGet, keyringDelete = restoreSet, restoreGet, restoreDelete }) - return deleted + t.Cleanup(func() { keyringSet, keyringDelete = restoreSet, restoreDelete }) + return set, 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") }) +// The unbounded probe goes through go-keyring rather than the security +// binary, so it must derive the same per-probe entry as the bounded one: a +// fixed account on either path would put that path back in the race. +func TestProbeDirectUsesPerProbeEntry(t *testing.T) { + set, deleted := recordKeyringOps(t, nil) - assert.ErrorIs(t, probeDirect("credstore.probe.svc", probeKey), setErr) - }) + assert.NoError(t, probe("svc", 0)) + assert.Equal(t, "credstore.probe.svc", set[0]) + assert.Regexp(t, probeKeyPattern(), set[1]) + assert.Equal(t, *set, *deleted, "cleanup should remove exactly the entry the probe wrote") } -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 }) +func TestProbeDirectFailureSkipsCleanup(t *testing.T) { + setErr := errors.New("no keyring provider") + _, deleted := recordKeyringOps(t, setErr) - assert.NoError(t, probeDirect("credstore.probe.svc", probeKey)) - assert.True(t, *deleted) + assert.ErrorIs(t, probe("svc", 0), setErr) + assert.Zero(t, *deleted, "a failed write has nothing to clean up") } diff --git a/credstore/store.go b/credstore/store.go index dd2556f..a8af293 100644 --- a/credstore/store.go +++ b/credstore/store.go @@ -3,7 +3,6 @@ package credstore import ( "fmt" "os" - "path/filepath" "time" "github.com/zalando/go-keyring" @@ -29,9 +28,11 @@ type StoreOptions struct { // 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. + // keyring service "credstore.probe." (account + // "__probe__..") — a namespace reserved by this package — never + // under ServiceName itself, so a probe cannot touch real credentials. + // The account is unique per probe, so concurrent invocations never + // contend for one keychain item. // // On darwin, removal of the throwaway probe entry runs synchronously // after a successful probe with a short budget of its own, so worst-case @@ -56,10 +57,15 @@ type StoreOptions struct { // Store handles credential storage with keyring preference and file fallback. type Store struct { - serviceName string - useKeyring bool - fallbackDir string - fallbackWarning string + serviceName string + useKeyring bool + fallbackDir string + + // probeErr is why the keyring probe failed when the store fell back to + // file storage against the caller's wishes. Nil when the keyring is in + // use and when file storage was requested (ForceFile, DisableEnvVar): + // a requested fallback is not a degradation and warrants no warning. + probeErr error } // NewStore creates a credential store. It probes the system keyring @@ -69,22 +75,33 @@ func NewStore(opts StoreOptions) *Store { return &Store{serviceName: opts.ServiceName, useKeyring: false, fallbackDir: opts.FallbackDir} } - if probeKeyring(opts.ServiceName, opts.ProbeTimeout) == nil { - return &Store{serviceName: opts.ServiceName, useKeyring: true, fallbackDir: opts.FallbackDir} - } - + err := probeKeyring(opts.ServiceName, opts.ProbeTimeout) return &Store{ - serviceName: opts.ServiceName, - useKeyring: false, - fallbackDir: opts.FallbackDir, - fallbackWarning: fmt.Sprintf("system keyring unavailable, credentials stored in plaintext at %s", filepath.Join(opts.FallbackDir, "credentials.json")), + serviceName: opts.ServiceName, + useKeyring: err == nil, + fallbackDir: opts.FallbackDir, + probeErr: err, } } +// ProbeError returns why the keyring probe failed and the store fell back +// to file storage, or nil when the keyring is in use or file storage was +// requested outright. +func (s *Store) ProbeError() error { + return s.probeErr +} + // FallbackWarning returns a warning message if the store fell back to file -// storage, or empty string if using keyring. +// storage because the keyring probe failed, or empty string otherwise. The +// message names the probe failure so a fallback is never silent about its +// cause. Callers decide where to surface it; surface it on reads as well as +// writes, since a fallback read may return a stale file left over from an +// earlier fallback rather than the credentials the keyring holds. func (s *Store) FallbackWarning() string { - return s.fallbackWarning + if s.probeErr == nil { + return "" + } + return fmt.Sprintf("system keyring unavailable (%v), credentials stored in plaintext at %s", s.probeErr, s.credentialsPath()) } func (s *Store) key(name string) string { @@ -92,6 +109,11 @@ func (s *Store) key(name string) string { } // Load retrieves credentials for the given key. +// +// On the file fallback, a miss is reported together with the keyring probe +// failure that caused the fallback: "credentials not found" alone reads as +// "log in again" when the truth may be that the credentials sit safely in +// the keyring and only this process could not reach it. func (s *Store) Load(key string) ([]byte, error) { if s.useKeyring { data, err := keyring.Get(s.serviceName, s.key(key)) @@ -100,7 +122,12 @@ func (s *Store) Load(key string) ([]byte, error) { } return []byte(data), nil } - return s.loadFromFile(key) + + data, err := s.loadFromFile(key) + if err != nil && s.probeErr != nil { + return nil, fmt.Errorf("%w: system keyring unavailable (%w), fell back to %s", err, s.probeErr, s.credentialsPath()) + } + return data, err } // Save stores credentials for the given key. diff --git a/credstore/store_test.go b/credstore/store_test.go index bb9a833..0940c6d 100644 --- a/credstore/store_test.go +++ b/credstore/store_test.go @@ -2,6 +2,7 @@ package credstore import ( "context" + "errors" "os" "path/filepath" "testing" @@ -100,6 +101,27 @@ func TestForceFileSkipsProbe(t *testing.T) { assert.JSONEq(t, `{"token":"abc123"}`, string(data)) } +// A healthy probe keeps the keyring: the store uses it, reports no probe +// error, and has nothing to warn about. Zero-value options probe unbounded +// — the timeout reaches the probe as zero — which is the documented contract +// for tests that mock the keyring (see StoreOptions.ProbeTimeout). +func TestHealthyProbeUsesKeyring(t *testing.T) { + probed := false + stubProbe(t, func(serviceName string, timeout time.Duration) error { + probed = true + assert.Equal(t, "test", serviceName) + assert.Zero(t, timeout) + return nil + }) + + store := NewStore(StoreOptions{ServiceName: "test", FallbackDir: t.TempDir()}) + + assert.True(t, probed) + assert.True(t, store.UsingKeyring()) + assert.NoError(t, store.ProbeError()) + assert.Empty(t, store.FallbackWarning()) +} + func TestProbeTimeoutFallsBackToFile(t *testing.T) { dir := t.TempDir() stubProbe(t, func(serviceName string, timeout time.Duration) error { @@ -115,38 +137,62 @@ func TestProbeTimeoutFallsBackToFile(t *testing.T) { }) assert.False(t, store.UsingKeyring()) - assert.Contains(t, store.FallbackWarning(), "system keyring unavailable") + assert.ErrorIs(t, store.ProbeError(), context.DeadlineExceeded) + assert.Contains(t, store.FallbackWarning(), "system keyring unavailable (context deadline exceeded)") } -func TestZeroValueOptionsProbeUnbounded(t *testing.T) { +// Regression: a probe failure silently demoted reads to the plaintext file, +// and a miss there reported "credentials not found" — indistinguishable from +// never having logged in, when the credentials sat safely in the keyring +// this process merely failed to reach. The store must keep the probe error +// and name it wherever the fallback shows: the warning and Load's error. +func TestProbeFailureIsReportedOnLoad(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 - }) + probeErr := errors.New("User interaction is not allowed. (exit status 36)") + stubProbe(t, func(string, time.Duration) error { return probeErr }) + + store := NewStore(StoreOptions{ServiceName: "test", FallbackDir: dir}) + credentialsPath := filepath.Join(dir, "credentials.json") + + assert.Same(t, probeErr, store.ProbeError()) + assert.Equal(t, "system keyring unavailable ("+probeErr.Error()+"), credentials stored in plaintext at "+credentialsPath, + store.FallbackWarning()) + + _, err := store.Load("profile:work") + require.Error(t, err) + assert.ErrorIs(t, err, probeErr) + assert.ErrorContains(t, err, "credentials not found for profile:work") + assert.ErrorContains(t, err, "system keyring unavailable ("+probeErr.Error()+")") + assert.ErrorContains(t, err, "fell back to "+credentialsPath) +} - store := NewStore(StoreOptions{ - ServiceName: "test", - FallbackDir: dir, - }) +// The file fallback keeps working after a failed probe — it is the only +// storage on hosts with no keyring at all — and a hit there is not an error. +func TestProbeFailureStillReadsFallbackFile(t *testing.T) { + dir := t.TempDir() + stubProbe(t, func(string, time.Duration) error { return errors.New("no keyring") }) - assert.True(t, probed) - assert.True(t, store.UsingKeyring()) - assert.Empty(t, store.FallbackWarning()) + store := NewStore(StoreOptions{ServiceName: "test", FallbackDir: dir}) + + 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 TestLoadNonexistent(t *testing.T) { +// File storage the caller asked for is not a fallback: no probe ran, so +// there is no probe error to report and nothing to warn about. +func TestRequestedFileStorageReportsNoProbeFailure(t *testing.T) { dir := t.TempDir() - t.Setenv("TEST_NO_KEYRING", "1") - - store := NewStore(StoreOptions{ - ServiceName: "test", - DisableEnvVar: "TEST_NO_KEYRING", - FallbackDir: dir, + stubProbe(t, func(string, time.Duration) error { + t.Error("probe should not run when file storage is requested") + return nil }) - _, err := store.Load("nonexistent") - assert.Error(t, err) + store := NewStore(StoreOptions{ServiceName: "test", ForceFile: true, FallbackDir: dir}) + + assert.NoError(t, store.ProbeError()) + assert.Empty(t, store.FallbackWarning()) + _, err := store.Load("mykey") + assert.EqualError(t, err, "credentials not found for mykey") }