From a191855ca324e3054619c8d589539e355feef3ab Mon Sep 17 00:00:00 2001 From: Alano Terblanche <18033717+Benehiko@users.noreply.github.com> Date: Wed, 24 Jun 2026 16:14:56 +0200 Subject: [PATCH] fix(keychain): retry the Linux read path when the collection relocks MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #557 made the write path relock-aware but left the reads bare. Get and Filter (via loadSecret) call GetSecret directly, which hits the same gnome-keyring relock race the write path did: a prior operation's closing D-Bus connection can relock the collection asynchronously — after the unlock but before the read runs — surfacing an intermittent org.freedesktop.Secret.Error.IsLocked error. Wrap both GetSecret call sites in the withRelockRetry helper already on main, so a relock is unlocked and retried rather than failing the read. loadSecret now takes the enclosing collectionPath so it can re-unlock. Tests (fake-backed): TestKeychainGetRetriesWhenCollectionRelocks and TestKeychainFilterRetriesWhenCollectionRelocks. Verified green against real gnome-keyring on the ubuntu-24 and fedora-43 CI harnesses; lint clean. Co-Authored-By: Claude Opus 4.8 (1M context) --- store/keychain/keychain_linux.go | 28 ++++++++++-- store/keychain/keychain_linux_test.go | 66 ++++++++++++++++++++++++++- 2 files changed, 87 insertions(+), 7 deletions(-) diff --git a/store/keychain/keychain_linux.go b/store/keychain/keychain_linux.go index 1126fe66..e1da7489 100644 --- a/store/keychain/keychain_linux.go +++ b/store/keychain/keychain_linux.go @@ -329,7 +329,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 } @@ -520,9 +525,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 } @@ -612,7 +630,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 } diff --git a/store/keychain/keychain_linux_test.go b/store/keychain/keychain_linux_test.go index d547e001..2c20ef1a 100644 --- a/store/keychain/keychain_linux_test.go +++ b/store/keychain/keychain_linux_test.go @@ -44,12 +44,18 @@ type fakeService struct { // "credential not found" path. items []dbus.ObjectPath + // attributes is returned verbatim by GetAttributes. Filter only descends into + // loadSecret (and therefore the wrapped GetSecret) for items whose attributes + // carry a parseable "id"; leave nil for paths that do not inspect attributes. + attributes kc.Attributes + // recorded write operations, for assertions in the Save tests. Not // concurrency-safe: the tests that read them drive a single sequential // operation through the fake. createCalls int setSecretCalls int deleteCalls int + getSecretCalls int setSecretItems []dbus.ObjectPath deletedItems []dbus.ObjectPath @@ -63,6 +69,7 @@ type fakeService struct { createItemLockedErrs int setSecretLockedErrs int deleteItemLockedErrs int + getSecretLockedErrs int unlockCalls int unlockErr error @@ -112,8 +119,21 @@ func (f *fakeService) DeleteItem(item dbus.ObjectPath) error { f.deletedItems = append(f.deletedItems, item) return nil } -func (f *fakeService) GetAttributes(dbus.ObjectPath) (kc.Attributes, error) { return nil, nil } -func (f *fakeService) GetSecret(dbus.ObjectPath, kc.Session) ([]byte, error) { return nil, nil } + +func (f *fakeService) GetAttributes(dbus.ObjectPath) (kc.Attributes, error) { + return f.attributes, nil +} + +func (f *fakeService) GetSecret(dbus.ObjectPath, kc.Session) ([]byte, error) { + f.getSecretCalls++ + if f.getSecretCalls <= f.getSecretLockedErrs { + return nil, lockedErr("get secret") + } + // A value MockCredential.Unmarshal can parse ("username:password"), so the + // Get/Filter read path completes once the simulated relock clears. + return []byte("bob:bob-password"), nil +} + func (f *fakeService) SetItemSecret(item dbus.ObjectPath, _ kc.Secret) error { f.setSecretCalls++ if f.setSecretCalls <= f.setSecretLockedErrs { @@ -299,6 +319,48 @@ func TestKeychainSaveStopsRetryingAfterMaxRelocks(t *testing.T) { assert.Equal(t, maxRelockRetries+1, fake.createCalls, "initial attempt plus the bounded retries") } +// 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. +func TestKeychainGetRetriesWhenCollectionRelocks(t *testing.T) { + stubRelockSleep(t) + fake := &fakeService{items: []dbus.ObjectPath{"/org/freedesktop/secrets/collection/login/1"}} + fake.getSecretLockedErrs = 2 + 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, 3, fake.getSecretCalls, "two locked failures then one success") + assert.Equal(t, 2, fake.unlockCalls, "exactly one Unlock per relock retry") +} + +// TestKeychainFilterRetriesWhenCollectionRelocks covers the read path reached +// through Filter, which loads each matched secret via loadSecret. That GetSecret +// is wrapped in withRelockRetry too, so a collection that relocks mid-read must be +// unlocked and retried rather than failing the whole Filter. +func TestKeychainFilterRetriesWhenCollectionRelocks(t *testing.T) { + stubRelockSleep(t) + fake := &fakeService{ + items: []dbus.ObjectPath{"/org/freedesktop/secrets/collection/login/1"}, + // Filter only descends into loadSecret for items whose attributes carry a + // parseable id; without it the item is skipped and GetSecret never runs. + attributes: kc.Attributes{"id": "com.test.test/test/bob"}, + } + fake.getSecretLockedErrs = 2 + withFakeService(t, fake) + + ks := setupKeychain(t, nil) + creds, err := ks.Filter(t.Context(), store.MustParsePattern("**")) + require.NoError(t, err) + require.Len(t, creds, 1) + + assert.Equal(t, 3, fake.getSecretCalls, "two locked failures then one success") + assert.Equal(t, 2, fake.unlockCalls, "exactly one Unlock per relock retry") +} + // The real-keychain dedup tests use their own service group/name so their items // are namespace-isolated from TestKeychain (which shares com.test.test/test). // GetAllMetadata/Filter search by {service:group, service:name}, so a leaked