diff --git a/store/keychain/internal/go-keychain/secretservice/secretservice.go b/store/keychain/internal/go-keychain/secretservice/secretservice.go index c95fa7b6..81cc2bf2 100644 --- a/store/keychain/internal/go-keychain/secretservice/secretservice.go +++ b/store/keychain/internal/go-keychain/secretservice/secretservice.go @@ -110,7 +110,15 @@ func NewService(ctx context.Context) (*SecretService, error) { } signalCh := make(chan *dbus.Signal, 16) conn.Signal(signalCh) - _ = conn.AddMatchSignal(dbus.WithMatchOption("org.freedesktop.Secret.Prompt", "Completed")) + // Without this subscription the bus never delivers Prompt.Completed and + // PromptAndWait times out on every real (non-null) prompt. + if err := conn.AddMatchSignal( + dbus.WithMatchInterface("org.freedesktop.Secret.Prompt"), + dbus.WithMatchMember("Completed"), + ); err != nil { + _ = conn.Close() + return nil, fmt.Errorf("failed to subscribe to prompt completion signals: %w", err) + } return &SecretService{conn: conn, signalCh: signalCh, sessionOpenTimeout: DefaultSessionOpenTimeout}, nil } diff --git a/store/keychain/keychain_linux.go b/store/keychain/keychain_linux.go index 161968c9..cbf67e7e 100644 --- a/store/keychain/keychain_linux.go +++ b/store/keychain/keychain_linux.go @@ -277,8 +277,9 @@ const ( // 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. +// withRelockRetry runs a collection or item 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 @@ -291,6 +292,11 @@ var sleepFn = time.Sleep // between the check and the call, so we react to the authoritative signal — the // operation's own locked error — by unlocking again and retrying. // +// itemPaths are unlocked alongside collectionPath. The spec allows an item to +// be locked independently of its collection (e.g. KeePassXC's per-item access +// confirmation), so operations that target a known item must pass its path; +// omit it only when none exists yet (CreateItem). +// // 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 @@ -298,13 +304,14 @@ 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) error { +func withRelockRetry(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([]dbus.ObjectPath{collectionPath}); unlockErr != nil { + if unlockErr := service.Unlock(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 @@ -368,7 +375,7 @@ func (k *keychainStore[T]) Delete(ctx context.Context, id store.ID) error { return withRelockRetry(service, objectPath, func() error { return service.DeleteItem(items[0]) - }) + }, items[0]) } func (k *keychainStore[T]) Get(ctx context.Context, id store.ID) (store.Secret, error) { @@ -426,7 +433,7 @@ func (k *keychainStore[T]) Get(ctx context.Context, id store.ID) (store.Secret, var getErr error value, getErr = service.GetSecret(items[0], *session) return getErr - }) + }, items[0]) if err != nil { return nil, err } @@ -594,7 +601,7 @@ func (k *keychainStore[T]) Save(ctx context.Context, id store.ID, secret store.S primary := items[0] if err := withRelockRetry(service, objectPath, func() error { return service.SetItemSecret(primary, sessSecret) - }); err != nil { + }, primary); err != nil { return err } _ = service.SetItemAttributes(primary, attributes) @@ -605,7 +612,7 @@ func (k *keychainStore[T]) Save(ctx context.Context, id store.ID, secret store.S // exists to drain (see withRelockRetry and issue #446). _ = withRelockRetry(service, objectPath, func() error { return service.DeleteItem(dup) - }) + }, dup) } return nil @@ -631,7 +638,7 @@ func (k *keychainStore[T]) loadSecret( var getErr error value, getErr = svc.GetSecret(itemPath, *session) return getErr - }) + }, itemPath) if err != nil { return nil, err } diff --git a/store/keychain/keychain_linux_test.go b/store/keychain/keychain_linux_test.go index 6cc8c589..774688b8 100644 --- a/store/keychain/keychain_linux_test.go +++ b/store/keychain/keychain_linux_test.go @@ -74,6 +74,8 @@ type fakeService struct { unlockCalls int unlockErr error + lastUnlockPaths []dbus.ObjectPath + // availableErr, when set, is returned by Available so a test can drive the // eager-probe failure paths in New. The zero value (nil) reports the backend // as available, so every existing test that constructs a store via @@ -110,8 +112,9 @@ func (f *fakeService) OpenSession(kc.AuthenticationMode) (*kc.Session, error) { return &kc.Session{Mode: kc.AuthenticationInsecurePlain}, nil } func (f *fakeService) CloseSession(*kc.Session) {} -func (f *fakeService) Unlock([]dbus.ObjectPath) error { +func (f *fakeService) Unlock(items []dbus.ObjectPath) error { f.unlockCalls++ + f.lastUnlockPaths = items return f.unlockErr } @@ -333,6 +336,8 @@ func TestKeychainSaveRetriesWhenSetSecretRelocks(t *testing.T) { assert.Equal(t, []dbus.ObjectPath{"/item/a"}, fake.setSecretItems, "the secret must be written in place once the relock clears") assert.Equal(t, 2, fake.unlockCalls, "exactly one Unlock per relock retry") + assert.Contains(t, fake.lastUnlockPaths, dbus.ObjectPath("/item/a"), + "the retry must unlock the item itself, not only its collection") } // TestKeychainSaveCollapseRetriesWhenDeleteRelocks is the unit-level counterpart @@ -354,6 +359,8 @@ func TestKeychainSaveCollapseRetriesWhenDeleteRelocks(t *testing.T) { "the duplicate must be collapsed once the relock clears") assert.Equal(t, 3, fake.deleteCalls, "two locked failures then one success") assert.Equal(t, 2, fake.unlockCalls, "exactly one Unlock per relock retry") + assert.Contains(t, fake.lastUnlockPaths, dbus.ObjectPath("/item/b"), + "the retry must unlock the duplicate item itself, not only its collection") } // TestKeychainSaveStopsRetryingAfterMaxRelocks asserts the retry is bounded: a @@ -389,6 +396,8 @@ func TestKeychainGetRetriesWhenCollectionRelocks(t *testing.T) { assert.Equal(t, 3, fake.getSecretCalls, "two locked failures then one success") assert.Equal(t, 2, fake.unlockCalls, "exactly one Unlock per relock retry") + assert.Contains(t, fake.lastUnlockPaths, dbus.ObjectPath("/org/freedesktop/secrets/collection/login/1"), + "the retry must unlock the item itself, not only its collection") } // TestKeychainFilterRetriesWhenCollectionRelocks covers the read path reached