diff --git a/.github/workflows/keychain.yml b/.github/workflows/keychain.yml index bb8bd0c1..3a0ec9e7 100644 --- a/.github/workflows/keychain.yml +++ b/.github/workflows/keychain.yml @@ -21,6 +21,7 @@ jobs: subtest: - fedora-43-gnome-keyring - ubuntu-24-gnome-keyring + - ubuntu-24-gnome-keyring-locked # disabled kdewallet tests since it prompts for a password in a # headless environment... need to still fix this # - fedora-43-kdewallet diff --git a/store/Dockerfile b/store/Dockerfile index 892ce5d3..345f8f61 100644 --- a/store/Dockerfile +++ b/store/Dockerfile @@ -61,6 +61,15 @@ RUN --mount=type=bind,target=. \ --mount=type=cache,target=/root/.cache/go-build \ bash -c "set -euxo pipefail; /app/store/scripts/gnome-keyring" +FROM ubuntu24 AS ubuntu-24-gnome-keyring-locked +ENV CGO_ENABLED=0 +USER user +WORKDIR /app +RUN --mount=type=bind,target=. \ + --mount=type=cache,target=/go/pkg/mod \ + --mount=type=cache,target=/root/.cache/go-build \ + bash -c "set -euxo pipefail; bash /app/store/scripts/gnome-keyring-locked" + FROM ubuntu24 AS ubuntu-24-kdewallet ENV CGO_ENABLED=0 USER user diff --git a/store/docker-bake.hcl b/store/docker-bake.hcl index ca409c27..4440645e 100644 --- a/store/docker-bake.hcl +++ b/store/docker-bake.hcl @@ -19,7 +19,8 @@ group "default" { # it just prompts anyway... # "fedora_43_kdewallet", # "ubuntu_24_kdewallet", - "ubuntu_24_gnome_keyring" + "ubuntu_24_gnome_keyring", + "ubuntu_24_gnome_keyring_locked" ] } @@ -62,3 +63,12 @@ target "ubuntu_24_gnome_keyring" { GO_VERSION = GO_VERSION } } + +target "ubuntu_24_gnome_keyring_locked" { + dockerfile = "store/Dockerfile" + target = "ubuntu-24-gnome-keyring-locked" + context = "." + args = { + GO_VERSION = GO_VERSION + } +} diff --git a/store/docs/keychain/decision-logs.md b/store/docs/keychain/decision-logs.md index 00b52c24..abac81aa 100644 --- a/store/docs/keychain/decision-logs.md +++ b/store/docs/keychain/decision-logs.md @@ -135,3 +135,54 @@ every store operation (not only the probe) benefits. thread it into the dial (via `operationService`) like the other operations. --- + +2026-08-25 Locked collections fail fast with ErrCollectionLocked; unlock prompt bounded + +On headless Linux hosts with SSH key-only logins, PAM has no password to +auto-unlock the login keyring, so the collection is locked after every +keyring-daemon restart. The store reacted to a locked collection by calling +Service.Unlock and waiting on the prompt. The failures surfaced as opaque +strings ("failed to prompt: prompt dismissed", "prompt timed out") that +callers could not classify, and the wait was a hardcoded 30 seconds. + +Decisions: + +- One exported sentinel, `ErrCollectionLocked`, declared in the cross-platform + `keychain.go` like `ErrKeychainUnavailable` and `ErrNoDefaultCollection`. + Every path that fails because the collection stayed locked wraps it: the + up-front unlock in `ensureCollectionUnlocked` (which names the collection), + the re-unlock in `withRelockRetry`, and a collection still locked after the + bounded retries. The prompt failure is kept as the wrapped cause. No + exported prompt-dismissed or prompt-timeout sentinels; those stay unexported + causes, the same pattern as `errSessionBusUnavailable`. +- The prompt wait is bounded by the operation's context. `PromptAndWait`, + `Unlock`, `LockItems`, `CreateItem` and `DeleteItem` now take a ctx. Store + operations pass their original ctx, not the `context.WithoutCancel` + connection ctx, so a caller deadline bounds the wait for the user while + in-flight D-Bus calls stay protected from teardown. The internal 30s cap + remains as an upper bound and is created once outside the receive loop; + previously any unrelated bus signal reset it. A null prompt returns before + the ctx check, so cleanup calls with cancelled contexts still work on + passwordless keyrings. +- Nothing else was added. Rejected: prompter-presence probes (the + `org.gnome.keyring.SystemPrompter` name is activatable but unstartable on + headless hosts with gcr installed, and KWallet/KeePassXC never own it, so a + probe misclassifies in both directions), password callbacks and + master-password unlock via the gnome-only + `InternalUnsupportedGuiltRiddenInterface`, TTY prompting in the library, and + a lock check in `New` (lock state is per-operation and mutable). The caller + detects `ErrCollectionLocked` with `errors.Is` and owns the remediation + message. A locked collection must not be treated as unavailable; falling + back to another store would split credentials across stores. +- Validated live on an Ubuntu 24.04 VM: a headless locked operation fails in + about 15ms with "prompt dismissed" (gnome-keyring dismisses immediately when + no prompter can start), and an answered prompt on a display still works. To + prove the caller's deadline is honored, a test probe with a deliberately + short 2 second context deadline aborted an unanswered prompt at 2 seconds. + Nothing changes for callers without a deadline: they still get the 30 second + cap, and gnome-keyring itself never times a prompt out. A new + `ubuntu-24-gnome-keyring-locked` CI target runs + `TestKeychainLiveLockedCollection` against a password-protected keyring with + the collection locked. + +--- diff --git a/store/docs/keychain/design.md b/store/docs/keychain/design.md index b095e223..d77bc5b1 100644 --- a/store/docs/keychain/design.md +++ b/store/docs/keychain/design.md @@ -92,3 +92,44 @@ collection exists, so a reachable-but-uninitialized keyring still passes `New` and surfaces `ErrNoDefaultCollection` lazily on the first operation, as before. On macOS and Windows the check is a no-op (`New` never returns `ErrKeychainUnavailable` there). + +### Locked collections and the bounded unlock prompt + +A reachable backend can still hold a locked collection. This is the default +state on headless hosts with SSH key-only logins: PAM has no password to +auto-unlock the login keyring, so it is locked after every keyring-daemon +restart. + +Every store operation checks the lock state up front +(`ensureCollectionUnlocked`) and, when locked, issues a Secret Service +`Unlock`. On a passwordless keyring this completes silently via the null +prompt. On a password-protected keyring it opens the backend's unlock prompt. +The prompt wait is bounded twice: + +- by the operation's `ctx`. This is the caller's original context, not the + `context.WithoutCancel` connection context, so a caller deadline bounds the + wait for the user while in-flight D-Bus calls stay protected from teardown. +- by an internal 30 second cap (`promptTimeout`), so a prompt nobody can + answer cannot block an operation forever. + +When the unlock fails (prompt dismissed, timed out, or ctx expired), the +operation returns an error wrapping the exported `ErrCollectionLocked` +sentinel, naming the collection and keeping the prompt failure as the cause. +The same wrapping applies in the relock-retry loop (`withRelockRetry`) and +when a collection is still locked after the bounded retries. + +Validated live on Ubuntu 24.04 with gnome-keyring: on a headless host the +unlock prompt does not hang. gnome-keyring dismisses it within milliseconds +when no prompter can start, so the locked error surfaces in about 15ms. The +30 second cap and the ctx bound cover the remaining case of a live prompter +with nobody answering. + +Deliberately not built (see the decision log): prompter-presence probes (the +`org.gnome.keyring.SystemPrompter` name is activatable but unstartable on +headless hosts with gcr installed, and KWallet/KeePassXC never own it, so a +probe misclassifies in both directions), password callbacks, master-password +unlock via `InternalUnsupportedGuiltRiddenInterface`, and TTY prompting in +the library. The library reports the locked state; the caller owns the +remediation message. A locked collection must not be treated as unavailable; +it still holds the user's credentials, and falling back to another store +would split credentials across stores. diff --git a/store/keychain/README.md b/store/keychain/README.md index 680d1e9b..16b6e0df 100644 --- a/store/keychain/README.md +++ b/store/keychain/README.md @@ -64,6 +64,36 @@ daemon whether the Secret Service is registered and never touches your stored secrets. On macOS and Windows the check is a no-op (and `ctx` is unused). See [../docs/keychain/design.md](../docs/keychain/design.md) for details. +### Locked collections (Linux) + +A reachable keychain can still hold a locked collection. This is the default +state on headless Linux hosts with SSH key-only logins: PAM has no password to +auto-unlock the login keyring, so it is locked after every keyring-daemon +restart. + +A store operation that finds the collection locked asks the Secret Service to +unlock it. On a passwordless keyring this succeeds silently. On a +password-protected keyring it opens the backend's unlock prompt. If the prompt +is dismissed (gnome-keyring does this immediately when no prompter can be +shown), times out, or the operation's context expires, the operation fails +with an error matching `keychain.ErrCollectionLocked`: + +```go +_, err := st.Get(ctx, id) +if errors.Is(err, keychain.ErrCollectionLocked) { + // The collection still holds the user's credentials. Tell the user how + // to unlock it, for example by logging in to the desktop session or + // running gnome-keyring-daemon --unlock. Do not fall back to another + // store; that would split credentials across stores. +} +``` + +The operation's `ctx` bounds the prompt wait, so a caller can set its own +deadline; an internal 30 second cap always applies. Unavailable means there is +no keychain to use, so fall back. Locked means the keychain and credentials +exist but need the user's help, so surface the remediation and do not fall +back. + ### Secrets The `keychain` assumes that any secret stored would conform to the `store.Secret` diff --git a/store/keychain/internal/go-keychain/secretservice/secretservice.go b/store/keychain/internal/go-keychain/secretservice/secretservice.go index 81cc2bf2..5a416e9e 100644 --- a/store/keychain/internal/go-keychain/secretservice/secretservice.go +++ b/store/keychain/internal/go-keychain/secretservice/secretservice.go @@ -412,8 +412,14 @@ const ReplaceBehaviorDoNotReplace = 0 // ReplaceBehaviorReplace const ReplaceBehaviorReplace = 1 -// CreateItem -func (s *SecretService) CreateItem(collection dbus.ObjectPath, properties map[string]dbus.Variant, secret Secret, replaceBehavior ReplaceBehavior) (item dbus.ObjectPath, err error) { +// CreateItem creates an item in the collection; ctx bounds the prompt wait. +func (s *SecretService) CreateItem( + ctx context.Context, + collection dbus.ObjectPath, + properties map[string]dbus.Variant, + secret Secret, + replaceBehavior ReplaceBehavior, +) (item dbus.ObjectPath, err error) { var replace bool switch replaceBehavior { case ReplaceBehaviorDoNotReplace: @@ -431,15 +437,15 @@ func (s *SecretService) CreateItem(collection dbus.ObjectPath, properties map[st if err != nil { return "", fmt.Errorf("failed to create item: %w", err) } - _, err = s.PromptAndWait(prompt) + _, err = s.PromptAndWait(ctx, prompt) if err != nil { return "", err } return item, nil } -// DeleteItem -func (s *SecretService) DeleteItem(item dbus.ObjectPath) (err error) { +// DeleteItem deletes an item; ctx bounds the prompt wait. +func (s *SecretService) DeleteItem(ctx context.Context, item dbus.ObjectPath) (err error) { var prompt dbus.ObjectPath err = s.Obj(item). Call("org.freedesktop.Secret.Item.Delete", NilFlags). @@ -447,7 +453,7 @@ func (s *SecretService) DeleteItem(item dbus.ObjectPath) (err error) { if err != nil { return fmt.Errorf("failed to delete item: %w", err) } - _, err = s.PromptAndWait(prompt) + _, err = s.PromptAndWait(ctx, prompt) if err != nil { return err } @@ -501,8 +507,8 @@ func (s *SecretService) GetSecret(item dbus.ObjectPath, session Session) (secret // NullPrompt const NullPrompt = "/" -// Unlock -func (s *SecretService) Unlock(items []dbus.ObjectPath) (err error) { +// Unlock unlocks the given collections or items; ctx bounds the prompt wait. +func (s *SecretService) Unlock(ctx context.Context, items []dbus.ObjectPath) (err error) { var dummy []dbus.ObjectPath var prompt dbus.ObjectPath err = s.ServiceObj(). @@ -511,15 +517,15 @@ func (s *SecretService) Unlock(items []dbus.ObjectPath) (err error) { if err != nil { return fmt.Errorf("failed to unlock items: %w", err) } - _, err = s.PromptAndWait(prompt) + _, err = s.PromptAndWait(ctx, prompt) if err != nil { return fmt.Errorf("failed to prompt: %w", err) } return nil } -// LockItems -func (s *SecretService) LockItems(items []dbus.ObjectPath) (err error) { +// LockItems locks the given collections or items; ctx bounds the prompt wait. +func (s *SecretService) LockItems(ctx context.Context, items []dbus.ObjectPath) (err error) { var dummy []dbus.ObjectPath var prompt dbus.ObjectPath err = s.ServiceObj(). @@ -528,7 +534,7 @@ func (s *SecretService) LockItems(items []dbus.ObjectPath) (err error) { if err != nil { return fmt.Errorf("failed to lock items: %w", err) } - _, err = s.PromptAndWait(prompt) + _, err = s.PromptAndWait(ctx, prompt) if err != nil { return fmt.Errorf("failed to prompt: %w", err) } @@ -545,8 +551,15 @@ func (p PromptDismissedError) Error() string { return p.err.Error() } +// promptTimeout caps how long PromptAndWait waits for a prompt to complete. +const promptTimeout = 30 * time.Second + +// PromptAndWait displays the prompt and blocks until it completes, is +// dismissed, ctx is done, or promptTimeout elapses. A NullPrompt returns +// immediately. +// // PromptAndWait is NOT thread-safe. -func (s *SecretService) PromptAndWait(prompt dbus.ObjectPath) (paths *dbus.Variant, err error) { +func (s *SecretService) PromptAndWait(ctx context.Context, prompt dbus.ObjectPath) (paths *dbus.Variant, err error) { if prompt == NullPrompt { return nil, nil } @@ -554,6 +567,8 @@ func (s *SecretService) PromptAndWait(prompt dbus.ObjectPath) (paths *dbus.Varia if call.Err != nil { return nil, fmt.Errorf("failed to prompt: %w", call.Err) } + // created once, outside the loop, so unrelated signals cannot reset it + timeout := time.After(promptTimeout) for { var result PromptCompletedResult select { @@ -575,7 +590,9 @@ func (s *SecretService) PromptAndWait(prompt dbus.ObjectPath) (paths *dbus.Varia return nil, PromptDismissedError{errors.New("prompt dismissed")} } return &result.Paths, nil - case <-time.After(30 * time.Second): + case <-ctx.Done(): + return nil, fmt.Errorf("prompt aborted: %w", ctx.Err()) + case <-timeout: return nil, errors.New("prompt timed out") } } diff --git a/store/keychain/internal/go-keychain/secretservice/secretservice_test.go b/store/keychain/internal/go-keychain/secretservice/secretservice_test.go index 634d2999..a9899ea2 100644 --- a/store/keychain/internal/go-keychain/secretservice/secretservice_test.go +++ b/store/keychain/internal/go-keychain/secretservice/secretservice_test.go @@ -38,10 +38,10 @@ func testKeyring(t *testing.T, mode AuthenticationMode) { secret, err := session.NewSecret([]byte("secret")) require.NoError(t, err) - err = srv.Unlock([]dbus.ObjectPath{collection}) + err = srv.Unlock(t.Context(), []dbus.ObjectPath{collection}) require.NoError(t, err) - _, err = srv.CreateItem(collection, NewSecretProperties("testlabel", map[string]string{"foo": "bar"}), secret, ReplaceBehaviorReplace) + _, err = srv.CreateItem(t.Context(), collection, NewSecretProperties("testlabel", map[string]string{"foo": "bar"}), secret, ReplaceBehaviorReplace) require.NoError(t, err) items, err = srv.SearchCollection(collection, map[string]string{"foo": "bar"}) @@ -52,10 +52,10 @@ func testKeyring(t *testing.T, mode AuthenticationMode) { require.NoError(t, err) require.Equal(t, secretPlaintext, []byte("secret")) - err = srv.DeleteItem(gotItem) + err = srv.DeleteItem(t.Context(), gotItem) require.NoError(t, err) - err = srv.LockItems([]dbus.ObjectPath{collection}) + err = srv.LockItems(t.Context(), []dbus.ObjectPath{collection}) require.NoError(t, err) } @@ -71,16 +71,16 @@ func TestGetAll(t *testing.T) { secret, err := session.NewSecret([]byte("secret")) require.NoError(t, err) - err = srv.Unlock([]dbus.ObjectPath{collection}) + err = srv.Unlock(t.Context(), []dbus.ObjectPath{collection}) require.NoError(t, err) - item, err := srv.CreateItem(collection, NewSecretProperties("testlabel", map[string]string{"username": "testuser"}), secret, ReplaceBehaviorReplace) + item, err := srv.CreateItem(t.Context(), collection, NewSecretProperties("testlabel", map[string]string{"username": "testuser"}), secret, ReplaceBehaviorReplace) require.NoError(t, err) attrs, err := srv.GetAttributes(item) require.NoError(t, err) require.Equal(t, attrs["username"], "testuser") - err = srv.DeleteItem(item) + err = srv.DeleteItem(t.Context(), item) require.NoError(t, err) } diff --git a/store/keychain/keychain.go b/store/keychain/keychain.go index 7fe48bf9..9f33378f 100644 --- a/store/keychain/keychain.go +++ b/store/keychain/keychain.go @@ -71,6 +71,19 @@ var ErrNoDefaultCollection = errors.New("no default keychain collection availabl // ErrNoDefaultCollection lazily on the first operation, exactly as before. var ErrKeychainUnavailable = errors.New("keychain backend unavailable") +// ErrCollectionLocked is returned by store operations when the keychain +// collection is locked and could not be unlocked: the unlock prompt was +// dismissed (gnome-keyring does this immediately when no prompter can be +// shown, e.g. on a headless host), timed out, or was aborted by the +// operation's context. Detect it with [errors.Is]. +// +// Unlike [ErrKeychainUnavailable], the collection exists and still holds the +// user's credentials. Tell the user how to unlock it. +// +// It is declared in the cross-platform file so callers can reference it +// without build tags; it only matches on Linux. +var ErrCollectionLocked = errors.New("keychain collection is locked") + type ( Option interface{ apply(any) error } optionFunc[K any] func(K) error diff --git a/store/keychain/keychain_linux.go b/store/keychain/keychain_linux.go index cbf67e7e..bfee6b7b 100644 --- a/store/keychain/keychain_linux.go +++ b/store/keychain/keychain_linux.go @@ -60,10 +60,10 @@ type secretService interface { IsLocked(collection dbus.ObjectPath) (bool, error) OpenSession(mode kc.AuthenticationMode) (*kc.Session, error) CloseSession(session *kc.Session) - Unlock(items []dbus.ObjectPath) error + Unlock(ctx context.Context, items []dbus.ObjectPath) error SearchCollection(collection dbus.ObjectPath, attributes kc.Attributes) ([]dbus.ObjectPath, error) - CreateItem(collection dbus.ObjectPath, properties map[string]dbus.Variant, secret kc.Secret, replaceBehavior kc.ReplaceBehavior) (dbus.ObjectPath, error) - DeleteItem(item dbus.ObjectPath) error + CreateItem(ctx context.Context, collection dbus.ObjectPath, properties map[string]dbus.Variant, secret kc.Secret, replaceBehavior kc.ReplaceBehavior) (dbus.ObjectPath, error) + DeleteItem(ctx context.Context, item dbus.ObjectPath) error GetAttributes(item dbus.ObjectPath) (kc.Attributes, error) GetSecret(item dbus.ObjectPath, session kc.Session) ([]byte, error) SetItemSecret(item dbus.ObjectPath, secret kc.Secret) error @@ -224,11 +224,9 @@ func resolveDefaultCollection(collections []dbus.ObjectPath, aliasPath dbus.Obje return aliasPath, nil } -var errCollectionLocked = errors.New("collection is locked") - // isCollectionUnlocked verifies if the collection is unlocked. // -// It returns the errCollectionLocked error by default if the collection is locked. +// It returns [ErrCollectionLocked] by default if the collection is locked. // On any other error, it returns the underlying error instead. func isCollectionUnlocked(collectionPath dbus.ObjectPath, service secretService) error { locked, err := service.IsLocked(collectionPath) @@ -238,7 +236,37 @@ func isCollectionUnlocked(collectionPath dbus.ObjectPath, service secretService) if !locked { return nil } - return errCollectionLocked + return ErrCollectionLocked +} + +// lockedError wraps cause under [ErrCollectionLocked], naming the collection. +func lockedError(collectionPath dbus.ObjectPath, cause error) error { + return fmt.Errorf("%w: could not unlock collection %q: %w", ErrCollectionLocked, collectionPath, cause) +} + +// ensureCollectionUnlocked unlocks the collection if it is locked. On a +// passwordless keyring the unlock completes silently via the null prompt; on a +// password-protected keyring it opens the backend's unlock prompt. +// +// ctx bounds the prompt wait. It is the caller's original operation context, +// not the detached connection context from [operationService]: waiting on the +// user is bounded by the caller. A null prompt ignores ctx, so cleanup calls +// with a cancelled ctx still work on passwordless keyrings. +// +// A failed unlock (prompt dismissed, timed out, or ctx expired) wraps +// [ErrCollectionLocked]. +func ensureCollectionUnlocked(ctx context.Context, service secretService, collectionPath dbus.ObjectPath) error { + err := isCollectionUnlocked(collectionPath, service) + if err == nil { + return nil + } + if !errors.Is(err, ErrCollectionLocked) { + return err + } + if err := service.Unlock(ctx, []dbus.ObjectPath{collectionPath}); err != nil { + return lockedError(collectionPath, err) + } + return nil } // secretServiceIsLockedError is the D-Bus error name the secret service returns @@ -304,22 +332,28 @@ var sleepFn = time.Sleep // authentication prompt; the bounded retry count and backoff keep that to a // handful of spaced-out prompts at worst, and a dismissed prompt makes Unlock // return an error that aborts the loop immediately rather than re-prompting. -func withRelockRetry(service secretService, collectionPath dbus.ObjectPath, op func() error, itemPaths ...dbus.ObjectPath) error { +// +// ctx bounds each retry's unlock-prompt wait. Unlock failures, and a +// collection still locked after the retries, wrap [ErrCollectionLocked]. +func withRelockRetry(ctx context.Context, service secretService, collectionPath dbus.ObjectPath, op func() error, itemPaths ...dbus.ObjectPath) error { err := op() delay := relockRetryBaseDelay unlockPaths := append([]dbus.ObjectPath{collectionPath}, itemPaths...) for attempt := 0; attempt < maxRelockRetries && isLockedDBusError(err); attempt++ { sleepFn(delay) delay = min(delay*2, relockRetryMaxDelay) - if unlockErr := service.Unlock(unlockPaths); unlockErr != nil { + if unlockErr := service.Unlock(ctx, unlockPaths); unlockErr != nil { // Surface why the retry stopped while preserving errors.Is on the // underlying Unlock error (e.g. a dismissed prompt). The original // locked error is intentionally dropped: the failed unlock is the // actionable cause once we have decided to stop retrying. - return fmt.Errorf("unlock after relock: %w", unlockErr) + return lockedError(collectionPath, fmt.Errorf("unlock after relock: %w", unlockErr)) } err = op() } + if isLockedDBusError(err) { + return lockedError(collectionPath, err) + } return err } @@ -350,15 +384,9 @@ func (k *keychainStore[T]) Delete(ctx context.Context, id store.ID) error { return err } - err = isCollectionUnlocked(objectPath, service) - if err != nil && !errors.Is(err, errCollectionLocked) { + if err := ensureCollectionUnlocked(ctx, service, objectPath); err != nil { return err } - if errors.Is(err, errCollectionLocked) { - if err := service.Unlock([]dbus.ObjectPath{objectPath}); err != nil { - return err - } - } attributes := make(map[string]string) safelySetMetadata(k.serviceGroup, k.serviceName, attributes) @@ -373,8 +401,8 @@ func (k *keychainStore[T]) Delete(ctx context.Context, id store.ID) error { return nil } - return withRelockRetry(service, objectPath, func() error { - return service.DeleteItem(items[0]) + return withRelockRetry(ctx, service, objectPath, func() error { + return service.DeleteItem(ctx, items[0]) }, items[0]) } @@ -399,15 +427,9 @@ func (k *keychainStore[T]) Get(ctx context.Context, id store.ID) (store.Secret, return nil, err } - err = isCollectionUnlocked(objectPath, service) - if err != nil && !errors.Is(err, errCollectionLocked) { + if err := ensureCollectionUnlocked(ctx, service, objectPath); err != nil { return nil, err } - if errors.Is(err, errCollectionLocked) { - if err := service.Unlock([]dbus.ObjectPath{objectPath}); err != nil { - return nil, err - } - } searchMetadata := make(map[string]string) safelySetMetadata(k.serviceGroup, k.serviceName, searchMetadata) @@ -429,7 +451,7 @@ func (k *keychainStore[T]) Get(ctx context.Context, id store.ID) (store.Secret, safelyCleanMetadata(attributes) var value []byte - err = withRelockRetry(service, objectPath, func() error { + err = withRelockRetry(ctx, service, objectPath, func() error { var getErr error value, getErr = service.GetSecret(items[0], *session) return getErr @@ -471,15 +493,9 @@ func (k *keychainStore[T]) GetAllMetadata(ctx context.Context) (map[store.ID]sto return nil, err } - err = isCollectionUnlocked(objectPath, service) - if err != nil && !errors.Is(err, errCollectionLocked) { + if err := ensureCollectionUnlocked(ctx, service, objectPath); err != nil { return nil, err } - if errors.Is(err, errCollectionLocked) { - if err := service.Unlock([]dbus.ObjectPath{objectPath}); err != nil { - return nil, err - } - } searchMetadata := make(map[string]string) safelySetMetadata(k.serviceGroup, k.serviceName, searchMetadata) @@ -542,15 +558,9 @@ func (k *keychainStore[T]) Save(ctx context.Context, id store.ID, secret store.S return err } - err = isCollectionUnlocked(objectPath, service) - if err != nil && !errors.Is(err, errCollectionLocked) { + if err := ensureCollectionUnlocked(ctx, service, objectPath); err != nil { return err } - if errors.Is(err, errCollectionLocked) { - if err := service.Unlock([]dbus.ObjectPath{objectPath}); err != nil { - return err - } - } value, err := secret.Marshal() if err != nil { @@ -587,8 +597,8 @@ func (k *keychainStore[T]) Save(ctx context.Context, id store.ID, secret store.S // Nothing stored yet: create a fresh item. if len(items) == 0 { properties := kc.NewSecretProperties(label, attributes) - return withRelockRetry(service, objectPath, func() error { - _, createErr := service.CreateItem(objectPath, properties, sessSecret, kc.ReplaceBehaviorReplace) + return withRelockRetry(ctx, service, objectPath, func() error { + _, createErr := service.CreateItem(ctx, objectPath, properties, sessSecret, kc.ReplaceBehaviorReplace) return createErr }) } @@ -599,7 +609,7 @@ func (k *keychainStore[T]) Save(ctx context.Context, id store.ID, secret store.S // the attributes and label and collapsing any pre-existing duplicates are // best-effort (the secret is already stored) and must not flip the result. primary := items[0] - if err := withRelockRetry(service, objectPath, func() error { + if err := withRelockRetry(ctx, service, objectPath, func() error { return service.SetItemSecret(primary, sessSecret) }, primary); err != nil { return err @@ -610,8 +620,8 @@ func (k *keychainStore[T]) Save(ctx context.Context, id store.ID, secret store.S // Best-effort, but still relock-aware: a collection that relocks // mid-collapse would otherwise leave the duplicates the whole feature // exists to drain (see withRelockRetry and issue #446). - _ = withRelockRetry(service, objectPath, func() error { - return service.DeleteItem(dup) + _ = withRelockRetry(ctx, service, objectPath, func() error { + return service.DeleteItem(ctx, dup) }, dup) } @@ -634,7 +644,7 @@ func (k *keychainStore[T]) loadSecret( attributes map[string]string, ) (store.Secret, error) { var value []byte - err := withRelockRetry(svc, collectionPath, func() error { + err := withRelockRetry(ctx, svc, collectionPath, func() error { var getErr error value, getErr = svc.GetSecret(itemPath, *session) return getErr @@ -675,15 +685,9 @@ func (k *keychainStore[T]) Filter(ctx context.Context, pattern store.Pattern) (m return nil, err } - err = isCollectionUnlocked(objectPath, service) - if err != nil && !errors.Is(err, errCollectionLocked) { + if err := ensureCollectionUnlocked(ctx, service, objectPath); err != nil { return nil, err } - if errors.Is(err, errCollectionLocked) { - if err := service.Unlock([]dbus.ObjectPath{objectPath}); err != nil { - return nil, err - } - } attributes := make(map[string]string) // add our pattern to the attributes so we can match against items that diff --git a/store/keychain/keychain_linux_test.go b/store/keychain/keychain_linux_test.go index 774688b8..76ec41d5 100644 --- a/store/keychain/keychain_linux_test.go +++ b/store/keychain/keychain_linux_test.go @@ -20,6 +20,7 @@ import ( "context" "errors" "fmt" + "os" "sync/atomic" "testing" "time" @@ -74,6 +75,9 @@ type fakeService struct { unlockCalls int unlockErr error + // locked is returned by IsLocked. + locked bool + lastUnlockPaths []dbus.ObjectPath // availableErr, when set, is returned by Available so a test can drive the @@ -105,14 +109,14 @@ func (f *fakeService) Collections() ([]dbus.ObjectPath, error) { return []dbus.ObjectPath{loginKeychainObjectPath}, nil } func (f *fakeService) ReadAlias(string) (dbus.ObjectPath, error) { return loginKeychainObjectPath, nil } -func (f *fakeService) IsLocked(dbus.ObjectPath) (bool, error) { return false, nil } +func (f *fakeService) IsLocked(dbus.ObjectPath) (bool, error) { return f.locked, nil } func (f *fakeService) OpenSession(kc.AuthenticationMode) (*kc.Session, error) { // plain mode so Session.NewSecret works without a negotiated AES key, which // lets the Save path run end-to-end against the fake. return &kc.Session{Mode: kc.AuthenticationInsecurePlain}, nil } func (f *fakeService) CloseSession(*kc.Session) {} -func (f *fakeService) Unlock(items []dbus.ObjectPath) error { +func (f *fakeService) Unlock(_ context.Context, items []dbus.ObjectPath) error { f.unlockCalls++ f.lastUnlockPaths = items return f.unlockErr @@ -122,7 +126,7 @@ func (f *fakeService) SearchCollection(dbus.ObjectPath, kc.Attributes) ([]dbus.O return f.items, nil } -func (f *fakeService) CreateItem(dbus.ObjectPath, map[string]dbus.Variant, kc.Secret, kc.ReplaceBehavior) (dbus.ObjectPath, error) { +func (f *fakeService) CreateItem(context.Context, dbus.ObjectPath, map[string]dbus.Variant, kc.Secret, kc.ReplaceBehavior) (dbus.ObjectPath, error) { f.createCalls++ if f.createCalls <= f.createItemLockedErrs { return "", lockedErr("create item") @@ -130,7 +134,7 @@ func (f *fakeService) CreateItem(dbus.ObjectPath, map[string]dbus.Variant, kc.Se return "/created", nil } -func (f *fakeService) DeleteItem(item dbus.ObjectPath) error { +func (f *fakeService) DeleteItem(_ context.Context, item dbus.ObjectPath) error { f.deleteCalls++ if f.deleteCalls <= f.deleteItemLockedErrs { return lockedErr("delete item") @@ -377,9 +381,81 @@ func TestKeychainSaveStopsRetryingAfterMaxRelocks(t *testing.T) { &mocks.MockCredential{Username: "bob", Password: "bob-password"}) require.Error(t, err) assert.True(t, isLockedDBusError(err), "the persistent locked error must reach the caller") + assert.ErrorIs(t, err, ErrCollectionLocked, + "a collection still locked after the bounded retries must be detectable via the exported sentinel") assert.Equal(t, maxRelockRetries+1, fake.createCalls, "initial attempt plus the bounded retries") } +func TestKeychainLockedCollectionSurfacesErrCollectionLocked(t *testing.T) { + ops := map[string]func(store.Store) error{ + "get": func(ks store.Store) error { + _, err := ks.Get(t.Context(), store.MustParseID("com.test.test/test/bob")) + return err + }, + "save": func(ks store.Store) error { + return ks.Save(t.Context(), store.MustParseID("com.test.test/test/bob"), + &mocks.MockCredential{Username: "bob", Password: "bob-password"}) + }, + "delete": func(ks store.Store) error { + return ks.Delete(t.Context(), store.MustParseID("com.test.test/test/bob")) + }, + "get all metadata": func(ks store.Store) error { + _, err := ks.GetAllMetadata(t.Context()) + return err + }, + "filter": func(ks store.Store) error { + _, err := ks.Filter(t.Context(), store.MustParsePattern("**")) + return err + }, + } + for name, op := range ops { + t.Run(name, func(t *testing.T) { + fake := &fakeService{locked: true} + fake.unlockErr = errors.New("failed to prompt: prompt dismissed") + withFakeService(t, fake) + + err := op(setupKeychain(t, nil)) + require.Error(t, err) + assert.ErrorIs(t, err, ErrCollectionLocked) + assert.ErrorContains(t, err, string(loginKeychainObjectPath), + "the error must name the locked collection") + assert.ErrorContains(t, err, "prompt dismissed", + "the unlock failure cause must be preserved") + assert.Equal(t, 1, fake.unlockCalls, "exactly one unlock attempt before failing fast") + }) + } +} + +func TestKeychainLockedCollectionUnlocksAndProceeds(t *testing.T) { + fake := &fakeService{ + locked: true, + items: []dbus.ObjectPath{"/org/freedesktop/secrets/collection/login/1"}, + } + withFakeService(t, fake) + + ks := setupKeychain(t, nil) + secret, err := ks.Get(t.Context(), store.MustParseID("com.test.test/test/bob")) + require.NoError(t, err) + require.NotNil(t, secret) + assert.Equal(t, 1, fake.unlockCalls, "the locked collection must be unlocked before the read") +} + +func TestKeychainRelockRetryUnlockFailureWrapsErrCollectionLocked(t *testing.T) { + stubRelockSleep(t) + fake := &fakeService{items: []dbus.ObjectPath{"/item/a"}} + fake.setSecretLockedErrs = 1 << 30 // never recovers + fake.unlockErr = errors.New("failed to prompt: prompt dismissed") + withFakeService(t, fake) + + ks := setupKeychain(t, nil) + err := ks.Save(t.Context(), store.MustParseID("com.test.test/test/bob"), + &mocks.MockCredential{Username: "bob", Password: "bob-password"}) + require.Error(t, err) + assert.ErrorIs(t, err, ErrCollectionLocked) + assert.ErrorContains(t, err, "unlock after relock") + assert.Equal(t, 1, fake.unlockCalls, "a failed unlock must abort the retry loop immediately") +} + // TestKeychainGetRetriesWhenCollectionRelocks covers the read path: GetSecret can // hit org.freedesktop.Secret.Error.IsLocked if the collection relocks between the // unlock and the read, so Get wraps it in withRelockRetry too. @@ -467,7 +543,7 @@ const ( // stays unlocked once any earlier operation has unlocked it.) func ensureUnlocked(t *testing.T, svc *kc.SecretService, collection dbus.ObjectPath) { t.Helper() - require.NoError(t, svc.Unlock([]dbus.ObjectPath{collection})) + require.NoError(t, svc.Unlock(context.Background(), []dbus.ObjectPath{collection})) require.Eventually(t, func() bool { locked, err := svc.IsLocked(collection) return err == nil && !locked @@ -570,8 +646,8 @@ func seedRealDuplicates(t *testing.T, serviceGroup, serviceName string, id store // closing connection can relock the collection between the unlock above // and this create (see withRelockRetry), which would otherwise fail the // seed with "Cannot create an item in a locked collection". - err = withRelockRetry(svc, collection, func() error { - _, createErr := svc.CreateItem(collection, kc.NewSecretProperties(label, attrs), sessSecret, kc.ReplaceBehaviorDoNotReplace) + err = withRelockRetry(context.Background(), svc, collection, func() error { + _, createErr := svc.CreateItem(context.Background(), collection, kc.NewSecretProperties(label, attrs), sessSecret, kc.ReplaceBehaviorDoNotReplace) return createErr }) require.NoError(t, err) @@ -601,8 +677,8 @@ func purgeRealItems(t *testing.T, serviceGroup, serviceName string, id store.ID) items, err := svc.SearchCollection(collection, attrs) require.NoError(t, err) for _, item := range items { - require.NoError(t, withRelockRetry(svc, collection, func() error { - return svc.DeleteItem(item) + require.NoError(t, withRelockRetry(context.Background(), svc, collection, func() error { + return svc.DeleteItem(context.Background(), item) })) } } @@ -673,6 +749,36 @@ func TestKeychainSaveDoesNotAccumulate(t *testing.T) { "the surviving item's metadata must be refreshed in place") } +// Needs a live password-protected keyring (see scripts/gnome-keyring-locked); +// on a passwordless keyring the unlock succeeds and the operation passes. +func TestKeychainLiveLockedCollection(t *testing.T) { + if os.Getenv("TEST_KEYCHAIN_LOCKED_COLLECTION") == "" { + t.Skip("TEST_KEYCHAIN_LOCKED_COLLECTION not set; needs a live password-protected keyring") + } + + svc, err := kc.NewService(context.Background()) + require.NoError(t, err) + defer func() { _ = svc.Close() }() + + collection, err := getDefaultCollection(svc) + require.NoError(t, err) + require.NoError(t, svc.LockItems(context.Background(), []dbus.ObjectPath{collection})) + locked, err := svc.IsLocked(collection) + require.NoError(t, err) + require.True(t, locked, "collection must be locked; is the keyring password-protected?") + + ks := setupKeychain(t, nil) + start := time.Now() + _, err = ks.Get(t.Context(), store.MustParseID("com.test.test/test/bob")) + elapsed := time.Since(start) + + require.Error(t, err) + assert.ErrorIs(t, err, ErrCollectionLocked) + assert.ErrorContains(t, err, string(collection), "the error must name the locked collection") + assert.Less(t, elapsed, 15*time.Second, + "a locked collection must fail fast, not sit out the full prompt timeout") +} + // TestNewProbeSucceeds asserts the eager availability probe passes for a // reachable backend: New returns a usable store, dialing exactly one connection // and closing it (honoring the leak contract). diff --git a/store/scripts/gnome-keyring-locked b/store/scripts/gnome-keyring-locked new file mode 100755 index 00000000..8a7e9789 --- /dev/null +++ b/store/scripts/gnome-keyring-locked @@ -0,0 +1,91 @@ +#!/bin/bash + +# Locked-collection variant of the gnome-keyring harness (see ./gnome-keyring). +# +# Instead of seeding a passwordless 'login' keyring, it creates a +# PASSWORD-PROTECTED one the way PAM does (gnome-keyring-daemon --login reads +# the password from stdin), so locking the collection makes it genuinely +# require the password to unlock. The container has no display and no prompter, +# so the unlock prompt can never be answered — exactly the headless situation +# hit by downstream consumers — and the gated live test asserts store operations +# fail fast with ErrCollectionLocked instead of hanging. + +set -euxo pipefail + +if test -z $(command -v gnome-keyring-daemon); then + echo "gnome-keyring-daemon is not installed" + exit 1 +fi + +if test -z $(command -v dbus-daemon); then + echo "dbus-daemon is not installed" + exit 1 +fi + +mkdir -p ~/.local/share/keyrings + +# Start D-Bus session (dbus must be installed) +export DBUS_SESSION_BUS_ADDRESS=$(dbus-daemon --session --print-address --fork) + +# PAM-style login mode: read the password from stdin and create the +# password-protected 'login' keyring (unlocked for now; the test locks it). +# The daemon prints GNOME_KEYRING_CONTROL=