diff --git a/README.md b/README.md index 92c1431..97d74bc 100644 --- a/README.md +++ b/README.md @@ -23,6 +23,15 @@ This repo is the shared Go toolkit that powers all three CLIs. It provides reusa | `profile` | `github.com/basecamp/cli/profile` | Named environment profiles for targeting different accounts or environments | | `surface` | `github.com/basecamp/cli/surface` | CLI surface snapshot and compatibility diffing for Cobra command trees | +### Credential-store deadlines + +`credstore.StoreOptions` has two independent, opt-in deadlines, both unbounded +by default: `ProbeTimeout` bounds initialization (a failed probe selects the +file fallback), and `OperationTimeout` bounds keyring operations after a +successful probe (a timeout is an error, never a fallback to plaintext). See +their doc comments in [`credstore/store.go`](credstore/store.go) for the full +contract. + ## Seed templates The `seed/` directory contains templates for bootstrapping a new 37signals Go CLI. Templates include project scaffolding for auth, commands, output formatting, distribution, skills, and CI. diff --git a/credstore/operation.go b/credstore/operation.go new file mode 100644 index 0000000..432689d --- /dev/null +++ b/credstore/operation.go @@ -0,0 +1,73 @@ +package credstore + +import ( + "context" + "fmt" + "time" +) + +// keyringOperation bounds both the wait behind another operation and the call +// itself. go-keyring has no cancellation API: after a timeout its worker may +// still finish. Refusing subsequent operations avoids overlapping an unknown +// write/delete and bounds abandoned workers to one per store. File storage is +// never substituted for a previously working keyring. +func (s *Store) keyringOperation(operation string, fn func() ([]byte, error)) ([]byte, error) { + if s.operationTimeout <= 0 { + return fn() + } + ctx, cancel := context.WithTimeout(context.Background(), s.operationTimeout) + defer cancel() + select { + case s.operationGate <- struct{}{}: + defer func() { <-s.operationGate }() + case <-ctx.Done(): + return nil, queueTimeoutError(operation, s.operationTimeout, ctx.Err()) + } + if s.operationErr != nil { + return nil, fmt.Errorf("keyring unavailable after an earlier timeout: %w", s.operationErr) + } + if err := operationDeadlineError(ctx); err != nil { + return nil, queueTimeoutError(operation, s.operationTimeout, err) + } + + type result struct { + data []byte + err error + } + done := make(chan result, 1) + go func() { + data, err := fn() + done <- result{data, err} + }() + select { + case r := <-done: + return s.keyringOperationResult(ctx, operation, r.data, r.err) + case <-ctx.Done(): + return s.keyringOperationResult(ctx, operation, nil, ctx.Err()) + } +} + +// keyringOperationResult is called with operationGate held, after dispatching +// the provider call. Deadline expiry wins even when the result was selected first. +func (s *Store) keyringOperationResult(ctx context.Context, operation string, data []byte, providerErr error) ([]byte, error) { + if err := operationDeadlineError(ctx); err != nil { + s.operationErr = fmt.Errorf("keyring %s timed out after %s (the operation may have completed or may still complete): %w", operation, s.operationTimeout, err) + return nil, s.operationErr + } + return data, providerErr +} + +func operationDeadlineError(ctx context.Context) error { + if err := ctx.Err(); err != nil { + return err + } + // Timer delivery can lag behind the deadline when the scheduler is busy. + if deadline, ok := ctx.Deadline(); ok && !time.Now().Before(deadline) { + return context.DeadlineExceeded + } + return nil +} + +func queueTimeoutError(operation string, timeout time.Duration, err error) error { + return fmt.Errorf("keyring %s timed out after %s waiting for another operation (not attempted): %w", operation, timeout, err) +} diff --git a/credstore/operation_test.go b/credstore/operation_test.go new file mode 100644 index 0000000..da7b3d4 --- /dev/null +++ b/credstore/operation_test.go @@ -0,0 +1,305 @@ +package credstore + +import ( + "context" + "errors" + "os" + "sync" + "sync/atomic" + "testing" + "testing/synctest" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func swapKeyringOperations(t *testing.T, get func(string, string) (string, error), set func(string, string, string) error, del func(string, string) error) { + t.Helper() + oldGet, oldSet, oldDelete := keyringGet, keyringSet, keyringDelete + keyringGet, keyringSet, keyringDelete = get, set, del + t.Cleanup(func() { keyringGet, keyringSet, keyringDelete = oldGet, oldSet, oldDelete }) +} + +// A responsive probe does not promise that the next real keyring operation +// will respond. Timing out must not serve a stale file, write plaintext, or +// start another operation alongside a write/delete whose outcome is unknown. +func TestKeyringOperationTimeout(t *testing.T) { + for _, operation := range []string{"load", "save", "delete", "migrate"} { + t.Run(operation, func(t *testing.T) { + dir := t.TempDir() + file := NewStore(StoreOptions{ForceFile: true, FallbackDir: dir}) + require.NoError(t, file.Save("work", []byte(`{"access_token":"stale-file-token"}`))) + before, err := os.ReadFile(file.credentialsPath()) + require.NoError(t, err) + + stubProbe(t, func(string, time.Duration) error { return nil }) + var calls atomic.Int32 + release, finished := make(chan struct{}), make(chan struct{}) + block := func() { + calls.Add(1) + <-release + close(finished) + } + swapKeyringOperations(t, + func(string, string) (string, error) { block(); return "keyring-token", nil }, + func(string, string, string) error { block(); return nil }, + func(string, string) error { block(); return nil }, + ) + var releaseOnce sync.Once + t.Cleanup(func() { + releaseOnce.Do(func() { close(release) }) + if calls.Load() > 0 { + <-finished + } + }) + store := NewStore(StoreOptions{ServiceName: "test", FallbackDir: dir, OperationTimeout: 40 * time.Millisecond}) + result := make(chan error, 1) + go func() { + switch operation { + case "load": + data, loadErr := store.Load("work") + if len(data) != 0 { + loadErr = errors.New("timed-out read returned credential data") + } + result <- loadErr + case "save": + result <- store.Save("work", []byte("new-token")) + case "delete": + result <- store.Delete("work") + case "migrate": + result <- store.MigrateToKeyring() + } + }() + select { + case operationErr := <-result: + require.ErrorIs(t, operationErr, context.DeadlineExceeded) + assert.ErrorContains(t, operationErr, "may still complete") + assert.NotContains(t, operationErr.Error(), "not found", "a hung keyring is not a missing login") + case <-time.After(time.Second): + releaseOnce.Do(func() { close(release) }) + <-result + t.Fatal("keyring operation ignored OperationTimeout") + } + + assert.True(t, store.UsingKeyring(), "a timed-out operation must not switch to plaintext") + assert.Empty(t, store.FallbackWarning()) + _, err = store.Load("work") + assert.ErrorIs(t, err, context.DeadlineExceeded) + assert.NotContains(t, err.Error(), "not found", "a hung keyring is not a missing login") + assert.ErrorIs(t, store.Save("work", []byte("newer-token")), context.DeadlineExceeded) + assert.ErrorIs(t, store.Delete("work"), context.DeadlineExceeded) + assert.EqualValues(t, 1, calls.Load(), "a stalled operation poisons this store, not the process") + after, err := os.ReadFile(file.credentialsPath()) + require.NoError(t, err) + assert.Equal(t, before, after, "timeout must neither change nor remove the fallback file") + + // A late success is not permission to resume writes whose order + // relative to the timed-out write/delete cannot be established. + releaseOnce.Do(func() { close(release) }) + <-finished + assert.ErrorIs(t, store.Save("work", []byte("late-token")), context.DeadlineExceeded) + assert.EqualValues(t, 1, calls.Load()) + }) + } +} + +func TestBoundedKeyringKeepsHealthyResultsAndErrors(t *testing.T) { + stubProbe(t, func(string, time.Duration) error { return nil }) + providerErr := errors.New("keyring locked") + swapKeyringOperations(t, + func(service, key string) (string, error) { + assert.Equal(t, "test", service) + assert.Equal(t, "test::work", key) + return "token", nil + }, + func(string, string, string) error { return providerErr }, + func(string, string) error { return nil }, + ) + store := NewStore(StoreOptions{ServiceName: "test", OperationTimeout: time.Second}) + data, err := store.Load("work") + require.NoError(t, err) + assert.Equal(t, "token", string(data)) + assert.ErrorIs(t, store.Save("work", data), providerErr) + assert.NoError(t, store.Delete("work"), "an ordinary provider error must not poison the store") +} + +func TestWaitingForKeyringOperationAlsoHasADeadline(t *testing.T) { + stubProbe(t, func(string, time.Duration) error { return nil }) + var calls atomic.Int32 + swapKeyringOperations(t, + func(string, string) (string, error) { calls.Add(1); return "token", nil }, + func(string, string, string) error { return nil }, + func(string, string) error { return nil }, + ) + store := NewStore(StoreOptions{ServiceName: "test", OperationTimeout: 40 * time.Millisecond}) + store.operationGate <- struct{}{} + _, err := store.Load("work") + assert.ErrorIs(t, err, context.DeadlineExceeded) + assert.ErrorContains(t, err, "waiting for another operation") + assert.NotContains(t, err.Error(), "may still complete") + assert.Zero(t, calls.Load(), "a queued call must not reach the provider after its deadline") + <-store.operationGate + data, err := store.Load("work") + require.NoError(t, err, "timing out before calling the provider must not poison the store") + assert.Equal(t, "token", string(data)) +} + +func TestZeroOperationTimeoutPreservesUnboundedCalls(t *testing.T) { + stubProbe(t, func(string, time.Duration) error { return nil }) + swapKeyringOperations(t, + func(string, string) (string, error) { return "token", nil }, + func(string, string, string) error { return nil }, + func(string, string) error { return nil }, + ) + store := NewStore(StoreOptions{ServiceName: "test"}) + data, err := store.Load("work") + require.NoError(t, err) + assert.Equal(t, "token", string(data)) + assert.NoError(t, store.Save("work", data)) + assert.NoError(t, store.Delete("work")) +} + +func TestKeyringOperationResultRejectsExpiredDeadline(t *testing.T) { + for _, operation := range []string{"read", "write", "delete"} { + for _, timerDelivered := range []bool{true, false} { + name := operation + "/timer-pending" + if timerDelivered { + name = operation + "/timer-delivered" + } + t.Run(name, func(t *testing.T) { + stubProbe(t, func(string, time.Duration) error { return nil }) + var calls atomic.Int32 + swapKeyringOperations(t, nil, + func(string, string, string) error { calls.Add(1); return nil }, nil) + store := NewStore(StoreOptions{ServiceName: "test", OperationTimeout: time.Second}) + + deadline := time.Now().Add(-time.Second) + var ctx context.Context + if timerDelivered { + var cancel context.CancelFunc + ctx, cancel = context.WithDeadline(context.Background(), deadline) + t.Cleanup(cancel) + require.ErrorIs(t, ctx.Err(), context.DeadlineExceeded) + } else { + parent, cancel := context.WithCancel(context.Background()) + t.Cleanup(cancel) + ctx = pendingDeadlineContext{Context: parent, deadline: deadline} + require.NoError(t, ctx.Err()) + } + + // Model selecting the provider's successful result after expiry, + // whether or not the deadline timer has delivered its signal yet. + store.operationGate <- struct{}{} + data, err := store.keyringOperationResult(ctx, operation, []byte("late-token"), nil) + <-store.operationGate + require.ErrorIs(t, err, context.DeadlineExceeded) + assert.Empty(t, data) + assert.ErrorContains(t, err, "may have completed") + assert.ErrorIs(t, store.Save("work", []byte("retry")), context.DeadlineExceeded) + assert.Zero(t, calls.Load(), "a late result must not permit another provider call") + assert.True(t, store.UsingKeyring()) + }) + } + } +} + +// pendingDeadlineContext models an expired deadline whose timer callback has +// not yet closed Done or set Err, as can happen when the scheduler is busy. +type pendingDeadlineContext struct { + context.Context + deadline time.Time +} + +func (c pendingDeadlineContext) Deadline() (time.Time, bool) { + return c.deadline, true +} + +func TestConcurrentKeyringCallsBehindStalledProvider(t *testing.T) { + synctest.Test(t, func(t *testing.T) { + stubProbe(t, func(string, time.Duration) error { return nil }) + file := NewStore(StoreOptions{ForceFile: true, FallbackDir: t.TempDir()}) + require.NoError(t, file.Save("work", []byte(`{"access_token":"stale-file-token"}`))) + before, err := os.ReadFile(file.credentialsPath()) + require.NoError(t, err) + + started, release := make(chan struct{}), make(chan struct{}) + var startedOnce, releaseOnce sync.Once + var calls atomic.Int32 + var providers sync.WaitGroup + block := func() { + providers.Add(1) + defer providers.Done() + calls.Add(1) + startedOnce.Do(func() { close(started) }) + <-release + } + swapKeyringOperations(t, + func(string, string) (string, error) { block(); return "keyring-token", nil }, + func(string, string, string) error { block(); return nil }, + func(string, string) error { block(); return nil }, + ) + t.Cleanup(func() { + releaseOnce.Do(func() { close(release) }) + providers.Wait() + }) + + const queued = 64 + const timeout = 40 * time.Millisecond + store := NewStore(StoreOptions{ServiceName: "test", FallbackDir: file.fallbackDir, OperationTimeout: timeout}) + type result struct { + data []byte + err error + } + results := make(chan result, queued+1) + go func() { results <- result{err: store.Save("work", []byte("new-token"))} }() + <-started + synctest.Wait() + + for i := range queued { + go func() { + var r result + switch i % 3 { + case 0: + r.data, r.err = store.Load("work") + case 1: + r.err = store.Save("work", []byte("queued-token")) + case 2: + r.err = store.Delete("work") + } + results <- r + }() + } + // All queued callers are blocked behind the actual provider call + // before fake time advances to their deadlines. + synctest.Wait() + assert.Empty(t, results) + assert.EqualValues(t, 1, calls.Load()) + time.Sleep(timeout) + synctest.Wait() + require.Len(t, results, queued+1, "every caller must return while the provider is still blocked") + for range queued + 1 { + r := <-results + assert.ErrorIs(t, r.err, context.DeadlineExceeded) + assert.Empty(t, r.data, "a queued read must not return stale fallback credentials") + } + assert.EqualValues(t, 1, calls.Load()) + assert.True(t, store.UsingKeyring()) + assert.Empty(t, store.FallbackWarning()) + + // A late successful write must not revive the store or permit any + // queued read/write/delete to reach the provider afterward. + releaseOnce.Do(func() { close(release) }) + providers.Wait() + synctest.Wait() + data, err := store.Load("work") + assert.ErrorIs(t, err, context.DeadlineExceeded) + assert.Empty(t, data) + assert.ErrorIs(t, store.Save("work", []byte("retry-token")), context.DeadlineExceeded) + assert.ErrorIs(t, store.Delete("work"), context.DeadlineExceeded) + assert.EqualValues(t, 1, calls.Load()) + after, err := os.ReadFile(file.credentialsPath()) + require.NoError(t, err) + assert.Equal(t, before, after, "the fallback file must remain untouched") + }) +} diff --git a/credstore/probe.go b/credstore/probe.go index 4729ccb..f0bb9cb 100644 --- a/credstore/probe.go +++ b/credstore/probe.go @@ -47,9 +47,10 @@ const ( // 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. +// keyring operations, extracted as vars so tests can exercise storage and +// probing without a live keyring. var ( + keyringGet = keyring.Get keyringSet = keyring.Set keyringDelete = keyring.Delete ) diff --git a/credstore/store.go b/credstore/store.go index a8af293..4807e03 100644 --- a/credstore/store.go +++ b/credstore/store.go @@ -1,11 +1,11 @@ package credstore import ( + "context" + "errors" "fmt" "os" "time" - - "github.com/zalando/go-keyring" ) // StoreOptions configures credential storage. @@ -51,6 +51,27 @@ type StoreOptions struct { // probe goes through go-keyring and honors the mock) or ForceFile. ProbeTimeout time.Duration + // OperationTimeout bounds keyring reads, writes, and deletes (including + // each MigrateToKeyring write) after a successful probe, independently of + // ProbeTimeout. Zero or negative preserves unbounded operations. File + // storage is never bounded. The deadline includes waiting behind another + // operation on the same store. + // + // A timeout returns an error wrapping context.DeadlineExceeded — never a + // transition to plaintext file storage or a read of a possibly stale + // fallback file. go-keyring cannot cancel an operation already started: + // a timed-out write or delete may still complete, so the store refuses + // all further keyring operations, even if the abandoned call later + // succeeds. This bounds abandoned operations to one per store. A new + // store or process can try again; callers should not retry an unknown + // write automatically or report the timeout as a missing login. + // + // If the deadline expires while waiting behind another operation, the + // queued call is not attempted and cannot complete later. That queue + // timeout does not itself make the store refuse further operations; + // the already-running provider call may independently time out and do so. + OperationTimeout time.Duration + // FallbackDir is the directory for file-based credential storage. FallbackDir string } @@ -61,6 +82,10 @@ type Store struct { useKeyring bool fallbackDir string + operationTimeout time.Duration + operationGate chan struct{} + operationErr error // guarded by operationGate + // 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): @@ -77,10 +102,12 @@ func NewStore(opts StoreOptions) *Store { err := probeKeyring(opts.ServiceName, opts.ProbeTimeout) return &Store{ - serviceName: opts.ServiceName, - useKeyring: err == nil, - fallbackDir: opts.FallbackDir, - probeErr: err, + serviceName: opts.ServiceName, + useKeyring: err == nil, + fallbackDir: opts.FallbackDir, + probeErr: err, + operationTimeout: opts.OperationTimeout, + operationGate: make(chan struct{}, 1), } } @@ -116,11 +143,17 @@ func (s *Store) key(name string) string { // 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)) + data, err := s.keyringOperation("read", func() ([]byte, error) { + secret, err := keyringGet(s.serviceName, s.key(key)) + return []byte(secret), err + }) + if errors.Is(err, context.DeadlineExceeded) { + return nil, err + } if err != nil { return nil, fmt.Errorf("credentials not found: %w", err) } - return []byte(data), nil + return data, nil } data, err := s.loadFromFile(key) @@ -133,7 +166,11 @@ func (s *Store) Load(key string) ([]byte, error) { // Save stores credentials for the given key. func (s *Store) Save(key string, data []byte) error { if s.useKeyring { - return keyring.Set(s.serviceName, s.key(key), string(data)) + secret := string(data) + _, err := s.keyringOperation("write", func() ([]byte, error) { + return nil, keyringSet(s.serviceName, s.key(key), secret) + }) + return err } return s.saveToFile(key, data) } @@ -141,7 +178,10 @@ func (s *Store) Save(key string, data []byte) error { // Delete removes credentials for the given key. func (s *Store) Delete(key string) error { if s.useKeyring { - return keyring.Delete(s.serviceName, s.key(key)) + _, err := s.keyringOperation("delete", func() ([]byte, error) { + return nil, keyringDelete(s.serviceName, s.key(key)) + }) + return err } return s.deleteFromFile(key) }