Skip to content

fix(keychain): retry collection ops when gnome-keyring relocks #560

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
Benehiko wants to merge 2 commits into from
+394 −49
Closed

fix(keychain): retry collection ops when gnome-keyring relocks #560

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
74471c1
fix(keychain): retry collection ops when gnome-keyring relocks
Benehiko Jun 24, 2026
8d7ca4e
fix(keychain): satisfy lll and apply relock-retry review fixups
Benehiko Jun 24, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
169 changes: 127 additions & 42 deletions store/keychain/keychain_linux.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import (
"fmt"
"maps"
"slices"
"time"

dbus "github.com/godbus/dbus/v5"

Expand Down Expand Up @@ -145,6 +146,97 @@ func isCollectionUnlocked(collectionPath dbus.ObjectPath, service secretService)
return errCollectionLocked
}

// ensureUnlocked unlocks the collection if it currently reports as locked. It is
// the cheap, best-effort preamble run before any collection operation; the
// authoritative guard against a collection that relocks underneath us is
// [withRelockRetry], since the lock state can change between this check and the
// operation itself (see that function for why).
func ensureUnlocked(collectionPath dbus.ObjectPath, service secretService) error {
err := isCollectionUnlocked(collectionPath, service)
if err != nil && !errors.Is(err, errCollectionLocked) {
return err
}
if errors.Is(err, errCollectionLocked) {
return service.Unlock([]dbus.ObjectPath{collectionPath})
}
return nil
}

// secretServiceIsLockedError is the D-Bus error name the secret service returns
// when a mutating call (e.g. CreateItem) targets a locked collection.
//
// https://specifications.freedesktop.org/secret-service-spec/latest/errors.html
const secretServiceIsLockedError = "org.freedesktop.Secret.Error.IsLocked"

// isLockedDBusError reports whether err is the secret service's "collection is
// locked" D-Bus error. The lock state is matched on the structured D-Bus error
// name rather than the human-readable message so it is stable across backends
// and locales.
func isLockedDBusError(err error) bool {
var dbusErr dbus.Error
return errors.As(err, &dbusErr) && dbusErr.Name == secretServiceIsLockedError
}

// Relock retry tuning. An operation that hits a relocked collection is retried
// with exponential backoff: the relock is a brief race that settles on its own,
// and spacing the attempts out avoids hammering the secret service (or, on a
// password-protected keyring, re-issuing Unlock fast enough to spam the user
// with authentication prompts).
//
// relockRetryMaxDelay caps the backoff growth; with the current
// maxRelockRetries the slept delays are 20,40,80,160,320ms (the cap only takes
// effect once maxRelockRetries reaches 6, where the sixth delay would otherwise
// be 640ms).
const (
maxRelockRetries = 5
relockRetryBaseDelay = 20 * time.Millisecond
relockRetryMaxDelay = 500 * time.Millisecond
)

// sleepFn is the sleep seam used by the relock backoff so tests can exercise the
// retry loop without real delays. It is a package-level var with no
// synchronisation, so tests that swap it must not run in parallel.
var sleepFn = time.Sleep

// withRelockRetry runs a collection operation, retrying it with exponential
// backoff when the secret service rejects it because the collection is locked.
//
// The store dials a fresh D-Bus connection for every operation and closes it on
// return. gnome-keyring scopes an unlock to the session that performed it, so
// when a previous operation's connection closes the daemon relocks the
// collection — and that relock can land asynchronously in the middle of a later
// operation, after we have already observed the collection as unlocked but
// before the call against the collection runs. The result is an intermittent
// "Cannot create an item in a locked collection" error even though we unlocked
// moments earlier. IsLocked cannot guard against this because the state changes
// between the check and the call, so we react to the authoritative signal — the
// operation's own locked error — by unlocking again and retrying.
//
// In the common case this is the passwordless auto-unlock path (e.g. the
// PAM-unlocked login keyring), where Unlock returns the null prompt and asks
// the user for nothing. withRelockRetry cannot itself prove the keyring is
// passwordless, so on a password-protected keyring a retry could surface an
// 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) error {
err := op()
delay := relockRetryBaseDelay
for attempt := 0; attempt < maxRelockRetries && isLockedDBusError(err); attempt++ {
sleepFn(delay)
delay = min(delay*2, relockRetryMaxDelay)
if unlockErr := service.Unlock([]dbus.ObjectPath{collectionPath}); 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)
}
err = op()
}
return err
}

type keychainStore[T store.Secret] struct {
serviceGroup string
serviceName string
Expand Down Expand Up @@ -172,15 +264,9 @@ func (k *keychainStore[T]) Delete(_ context.Context, id store.ID) error {
return err
}

err = isCollectionUnlocked(objectPath, service)
if err != nil && !errors.Is(err, errCollectionLocked) {
if err := ensureUnlocked(objectPath, service); 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)
Expand All @@ -195,7 +281,9 @@ func (k *keychainStore[T]) Delete(_ context.Context, id store.ID) error {
return nil
}

return service.DeleteItem(items[0])
return withRelockRetry(service, objectPath, func() error {
return service.DeleteItem(items[0])
})
}

func (k *keychainStore[T]) Get(ctx context.Context, id store.ID) (store.Secret, error) {
Expand All @@ -219,15 +307,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 := ensureUnlocked(objectPath, service); 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)
Expand All @@ -248,7 +330,12 @@ func (k *keychainStore[T]) Get(ctx context.Context, id store.ID) (store.Secret,
}
safelyCleanMetadata(attributes)

value, err := service.GetSecret(items[0], *session)
var value []byte
err = withRelockRetry(service, objectPath, func() error {
var getErr error
value, getErr = service.GetSecret(items[0], *session)
return getErr
})
if err != nil {
return nil, err
}
Expand Down Expand Up @@ -286,15 +373,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 := ensureUnlocked(objectPath, service); 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)
Expand Down Expand Up @@ -358,15 +439,9 @@ func (k *keychainStore[T]) Save(_ context.Context, id store.ID, secret store.Sec
return err
}

err = isCollectionUnlocked(objectPath, service)
if err != nil && !errors.Is(err, errCollectionLocked) {
if err := ensureUnlocked(objectPath, service); 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 {
Expand All @@ -387,7 +462,10 @@ func (k *keychainStore[T]) Save(_ context.Context, id store.ID, secret store.Sec
label := k.itemLabel(id.String())
properties := kc.NewSecretProperties(label, attributes)

_, err = service.CreateItem(objectPath, properties, sessSecret, kc.ReplaceBehaviorReplace)
err = withRelockRetry(service, objectPath, func() error {
_, createErr := service.CreateItem(objectPath, properties, sessSecret, kc.ReplaceBehaviorReplace)
return createErr
})
if err != nil {
return err
}
Expand All @@ -400,9 +478,22 @@ func (k *keychainStore[T]) Upsert(ctx context.Context, id store.ID, secret store
}

// loadSecret fetches the raw secret value for itemPath, zeroes it after use,
// and returns a fully populated Secret.
func (k *keychainStore[T]) loadSecret(ctx context.Context, id store.ID, svc secretService, itemPath dbus.ObjectPath, session *kc.Session, attributes map[string]string) (store.Secret, error) {
value, err := svc.GetSecret(itemPath, *session)
// and returns a fully populated Secret. collectionPath is the enclosing
// collection, used to re-unlock if it relocks mid-read (see withRelockRetry).
func (k *keychainStore[T]) loadSecret(
ctx context.Context,
id store.ID,
svc secretService,
collectionPath, itemPath dbus.ObjectPath,
session *kc.Session,
attributes map[string]string,
) (store.Secret, error) {
var value []byte
err := withRelockRetry(svc, collectionPath, func() error {
var getErr error
value, getErr = svc.GetSecret(itemPath, *session)
return getErr
})
if err != nil {
return nil, err
}
Expand Down Expand Up @@ -439,15 +530,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 := ensureUnlocked(objectPath, service); 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
Expand Down Expand Up @@ -492,7 +577,7 @@ func (k *keychainStore[T]) Filter(ctx context.Context, pattern store.Pattern) (m
continue
}

secret, err := k.loadSecret(ctx, secretID, service, itemPath, session, attributes)
secret, err := k.loadSecret(ctx, secretID, service, objectPath, itemPath, session, attributes)
if err != nil {
return nil, err
}
Expand Down
Loading
Loading