From e205b6f09946635740f5ca0171fdd8946179c0eb Mon Sep 17 00:00:00 2001 From: Remy Suen Date: Thu, 20 Aug 2026 09:52:28 -0400 Subject: [PATCH 1/3] fix(keychain): unlock the item itself on a Linux relock retry, not just the collection Saving or updating a credential could fail permanently with `org.freedesktop.Secret.Error.IsLocked` on Linux backends that lock an item independently of its enclosing collection. `Service.Unlock` accepts both collection and item object paths, but every retry in `withRelockRetry` (and its callers in Delete/Get/Save/loadSecret) only ever unlocked the collection, so an independently locked item stayed locked forever and the retried SetItemSecret/DeleteItem/GetSecret call kept failing with the same error. Thread the relevant item's object path into withRelockRetry wherever one is already known (Delete, Get, Save's SetItemSecret and duplicate DeleteItem, loadSecret's GetSecret) so the retry unlocks the item alongside its collection. Signed-off-by: Remy Suen Co-Authored-By: Claude Sonnet 5 --- store/keychain/keychain_linux.go | 29 ++++++++++++++++++--------- store/keychain/keychain_linux_test.go | 18 ++++++++++++++++- 2 files changed, 37 insertions(+), 10 deletions(-) diff --git a/store/keychain/keychain_linux.go b/store/keychain/keychain_linux.go index 161968c9..1f91b4b1 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,15 @@ 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 additional object paths to unlock alongside collectionPath, +// for operations that target a specific item (e.g. SetItemSecret, DeleteItem, +// GetSecret). The Secret Service spec lets an item be locked independently of +// its enclosing collection, so unlocking only the collection can leave the +// item itself locked and the retried op fails with the same IsLocked error +// forever. Callers that already know the item's object path must pass it here +// rather than relying on the collection unlock alone. Omit itemPaths for +// operations that do not yet have an item path (e.g. 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 +308,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 +379,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 +437,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 +605,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 +616,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 +642,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..87cc7433 100644 --- a/store/keychain/keychain_linux_test.go +++ b/store/keychain/keychain_linux_test.go @@ -74,6 +74,12 @@ type fakeService struct { unlockCalls int unlockErr error + // lastUnlockPaths is the argument of the most recent Unlock call, so a test + // can assert which object paths a relock retry actually unlocks — the + // collection alone is not enough when the secret service locks an item + // independently of its enclosing collection (see withRelockRetry). + 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 +116,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 +340,11 @@ 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: the "+ + "secret service can lock an item independently of the collection, in "+ + "which case unlocking only the collection leaves SetItemSecret stuck "+ + "failing with org.freedesktop.Secret.Error.IsLocked forever") } // TestKeychainSaveCollapseRetriesWhenDeleteRelocks is the unit-level counterpart @@ -354,6 +366,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 +403,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 From 1316c09b9e99fb450eef08487ed01845a0b9126e Mon Sep 17 00:00:00 2001 From: Alano Terblanche <18033717+Benehiko@users.noreply.github.com> Date: Thu, 20 Aug 2026 16:52:11 +0200 Subject: [PATCH 2/3] fix(keychain): subscribe to Prompt.Completed with a valid match rule MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit NewService added its signal match rule as a bare key=value pair with the interface name as the key. That is not a valid match rule, the bus rejects it, and the error was discarded — so the connection never subscribed to org.freedesktop.Secret.Prompt.Completed and PromptAndWait timed out on every real (non-null) prompt, even after the user confirmed it. The passwordless gnome-keyring test environment only ever produces null prompts, which is why this never surfaced there. Use interface and member match options instead, and fail NewService if the subscription cannot be established. Verified against KeePassXC's secret service with per-item access confirmation enabled: unlocking an independently locked item now completes once the access prompt is allowed, where it previously timed out after 30 seconds. Co-Authored-By: Claude Fable 5 Signed-off-by: Alano Terblanche <18033717+Benehiko@users.noreply.github.com> --- .../go-keychain/secretservice/secretservice.go | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) 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 } From 0754fd03ddf7a7b89b64ee2fe4d6a519296967c2 Mon Sep 17 00:00:00 2001 From: Alano Terblanche <18033717+Benehiko@users.noreply.github.com> Date: Thu, 20 Aug 2026 16:52:14 +0200 Subject: [PATCH 3/3] refactor(keychain): tighten comments around the relock retry Trim the itemPaths documentation to its contract and drop the test comments and assertion prose that restated it. Co-Authored-By: Claude Fable 5 Signed-off-by: Alano Terblanche <18033717+Benehiko@users.noreply.github.com> --- store/keychain/keychain_linux.go | 12 ++++-------- store/keychain/keychain_linux_test.go | 9 +-------- 2 files changed, 5 insertions(+), 16 deletions(-) diff --git a/store/keychain/keychain_linux.go b/store/keychain/keychain_linux.go index 1f91b4b1..cbf67e7e 100644 --- a/store/keychain/keychain_linux.go +++ b/store/keychain/keychain_linux.go @@ -292,14 +292,10 @@ 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 additional object paths to unlock alongside collectionPath, -// for operations that target a specific item (e.g. SetItemSecret, DeleteItem, -// GetSecret). The Secret Service spec lets an item be locked independently of -// its enclosing collection, so unlocking only the collection can leave the -// item itself locked and the retried op fails with the same IsLocked error -// forever. Callers that already know the item's object path must pass it here -// rather than relying on the collection unlock alone. Omit itemPaths for -// operations that do not yet have an item path (e.g. CreateItem). +// 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 diff --git a/store/keychain/keychain_linux_test.go b/store/keychain/keychain_linux_test.go index 87cc7433..774688b8 100644 --- a/store/keychain/keychain_linux_test.go +++ b/store/keychain/keychain_linux_test.go @@ -74,10 +74,6 @@ type fakeService struct { unlockCalls int unlockErr error - // lastUnlockPaths is the argument of the most recent Unlock call, so a test - // can assert which object paths a relock retry actually unlocks — the - // collection alone is not enough when the secret service locks an item - // independently of its enclosing collection (see withRelockRetry). lastUnlockPaths []dbus.ObjectPath // availableErr, when set, is returned by Available so a test can drive the @@ -341,10 +337,7 @@ func TestKeychainSaveRetriesWhenSetSecretRelocks(t *testing.T) { "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: the "+ - "secret service can lock an item independently of the collection, in "+ - "which case unlocking only the collection leaves SetItemSecret stuck "+ - "failing with org.freedesktop.Secret.Error.IsLocked forever") + "the retry must unlock the item itself, not only its collection") } // TestKeychainSaveCollapseRetriesWhenDeleteRelocks is the unit-level counterpart