diff --git a/store/keychain/internal/go-keychain/secretservice/secretservice.go b/store/keychain/internal/go-keychain/secretservice/secretservice.go index af4579c3..6fac01c9 100644 --- a/store/keychain/internal/go-keychain/secretservice/secretservice.go +++ b/store/keychain/internal/go-keychain/secretservice/secretservice.go @@ -436,6 +436,40 @@ func (s *SecretService) PromptAndWait(prompt dbus.ObjectPath) (paths *dbus.Varia } } +// SetItemSecret replaces an existing item's secret value in place via +// org.freedesktop.Secret.Item.SetSecret. The secret must already be encoded for +// the session it references (see [Session.NewSecret]). SetSecret takes a single +// Secret argument (D-Bus signature (oayays)) and returns no values, so there is +// no prompt path: the item's collection must be unlocked first (use [Unlock]), +// otherwise the call fails. +func (s *SecretService) SetItemSecret(item dbus.ObjectPath, secret Secret) error { + if err := s.Obj(item).Call("org.freedesktop.Secret.Item.SetSecret", NilFlags, secret).Store(); err != nil { + return fmt.Errorf("failed to set item secret: %w", err) + } + return nil +} + +// SetItemAttributes replaces an existing item's lookup attributes in place by +// setting the read-write org.freedesktop.Secret.Item.Attributes property +// (type a{ss}) through org.freedesktop.DBus.Properties.Set. The collection must +// be unlocked. +func (s *SecretService) SetItemAttributes(item dbus.ObjectPath, attributes Attributes) error { + if err := s.Obj(item).SetProperty("org.freedesktop.Secret.Item.Attributes", dbus.MakeVariant(map[string]string(attributes))); err != nil { + return fmt.Errorf("failed to set item attributes: %w", err) + } + return nil +} + +// SetItemLabel replaces an existing item's displayable label in place by setting +// the read-write org.freedesktop.Secret.Item.Label property (type s) through +// org.freedesktop.DBus.Properties.Set. The collection must be unlocked. +func (s *SecretService) SetItemLabel(item dbus.ObjectPath, label string) error { + if err := s.Obj(item).SetProperty("org.freedesktop.Secret.Item.Label", dbus.MakeVariant(label)); err != nil { + return fmt.Errorf("failed to set item label: %w", err) + } + return nil +} + // NewSecretProperties func NewSecretProperties(label string, attributes map[string]string) map[string]dbus.Variant { return map[string]dbus.Variant{ diff --git a/store/keychain/keychain_linux.go b/store/keychain/keychain_linux.go index 9d1498ca..1126fe66 100644 --- a/store/keychain/keychain_linux.go +++ b/store/keychain/keychain_linux.go @@ -23,6 +23,7 @@ import ( "fmt" "maps" "slices" + "time" dbus "github.com/godbus/dbus/v5" @@ -65,6 +66,9 @@ type secretService interface { DeleteItem(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 + SetItemAttributes(item dbus.ObjectPath, attributes kc.Attributes) error + SetItemLabel(item dbus.ObjectPath, label string) error Close() error } @@ -145,6 +149,81 @@ func isCollectionUnlocked(collectionPath dbus.ObjectPath, service secretService) return errCollectionLocked } +// 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 @@ -195,7 +274,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) { @@ -385,13 +466,52 @@ func (k *keychainStore[T]) Save(_ context.Context, id store.ID, secret store.Sec safelySetID(id, attributes) label := k.itemLabel(id.String()) - properties := kc.NewSecretProperties(label, attributes) - _, err = service.CreateItem(objectPath, properties, sessSecret, kc.ReplaceBehaviorReplace) + // Find existing items for this identity by the stable triple only + // {service:group, service:name, id}, never the volatile metadata, so a + // changed metadata value can never hide a previously-stored item. This is + // what makes the in-place update below reliable and stops the duplicate + // accumulation described in issue #446. + ident := make(map[string]string) + safelySetMetadata(k.serviceGroup, k.serviceName, ident) + safelySetID(id, ident) + + items, err := service.SearchCollection(objectPath, ident) if err != nil { return err } + // 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 createErr + }) + } + + // Update the first match in place. Its object path is preserved, so the + // secret is never momentarily absent and no duplicate is minted. Writing the + // secret value IS the operation, so only its failure fails Save; refreshing + // 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 { + return service.SetItemSecret(primary, sessSecret) + }); err != nil { + return err + } + _ = service.SetItemAttributes(primary, attributes) + _ = service.SetItemLabel(primary, label) + for _, dup := range items[1:] { + // 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) + }) + } + return nil } diff --git a/store/keychain/keychain_linux_test.go b/store/keychain/keychain_linux_test.go index d6ab98be..d547e001 100644 --- a/store/keychain/keychain_linux_test.go +++ b/store/keychain/keychain_linux_test.go @@ -17,8 +17,11 @@ package keychain import ( + "context" + "fmt" "sync/atomic" "testing" + "time" dbus "github.com/godbus/dbus/v5" "github.com/stretchr/testify/assert" @@ -26,6 +29,7 @@ import ( "github.com/docker/secrets-engine/store" kc "github.com/docker/secrets-engine/store/keychain/internal/go-keychain/secretservice" + "github.com/docker/secrets-engine/store/mocks" ) // fakeService is a pure in-memory [secretService]. It never talks to a real @@ -40,30 +44,86 @@ type fakeService struct { // "credential not found" path. items []dbus.ObjectPath + // 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 + setSecretItems []dbus.ObjectPath + deletedItems []dbus.ObjectPath + + // {createItem,setSecret,deleteItem}LockedErrs is how many leading calls of + // each kind fail with the secret service "collection is locked" D-Bus error + // before one succeeds, simulating a collection that relocks underneath the + // store (see withRelockRetry). The error is wrapped exactly as the real + // service wraps it, so the tests exercise the errors.As-through-wrap path + // isLockedDBusError depends on. unlockCalls counts the re-unlocks the retry + // issues; unlockErr, when set, makes Unlock fail (e.g. a dismissed prompt). + createItemLockedErrs int + setSecretLockedErrs int + deleteItemLockedErrs int + unlockCalls int + unlockErr error + opened atomic.Int64 closed atomic.Int64 } +// lockedErr mirrors how the real SecretService wraps a locked-collection D-Bus +// error (see secretservice.go), so isLockedDBusError must unwrap to detect it. +func lockedErr(op string) error { + return fmt.Errorf("failed to %s: %w", op, dbus.Error{Name: secretServiceIsLockedError}) +} + 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) OpenSession(kc.AuthenticationMode) (*kc.Session, error) { - return &kc.Session{}, nil + // 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([]dbus.ObjectPath) error { return nil } +func (f *fakeService) CloseSession(*kc.Session) {} +func (f *fakeService) Unlock([]dbus.ObjectPath) error { + f.unlockCalls++ + return f.unlockErr +} + func (f *fakeService) SearchCollection(dbus.ObjectPath, kc.Attributes) ([]dbus.ObjectPath, error) { return f.items, nil } func (f *fakeService) CreateItem(dbus.ObjectPath, map[string]dbus.Variant, kc.Secret, kc.ReplaceBehavior) (dbus.ObjectPath, error) { - return "", nil + f.createCalls++ + if f.createCalls <= f.createItemLockedErrs { + return "", lockedErr("create item") + } + return "/created", nil +} + +func (f *fakeService) DeleteItem(item dbus.ObjectPath) error { + f.deleteCalls++ + if f.deleteCalls <= f.deleteItemLockedErrs { + return lockedErr("delete item") + } + f.deletedItems = append(f.deletedItems, item) + return nil } -func (f *fakeService) DeleteItem(dbus.ObjectPath) error { 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) SetItemSecret(item dbus.ObjectPath, _ kc.Secret) error { + f.setSecretCalls++ + if f.setSecretCalls <= f.setSecretLockedErrs { + return lockedErr("set item secret") + } + f.setSecretItems = append(f.setSecretItems, item) + return nil +} +func (f *fakeService) SetItemAttributes(dbus.ObjectPath, kc.Attributes) error { return nil } +func (f *fakeService) SetItemLabel(dbus.ObjectPath, string) error { return nil } func (f *fakeService) Close() error { f.closed.Add(1) return nil @@ -81,6 +141,16 @@ func withFakeService(t *testing.T, fake *fakeService) { } } +// stubRelockSleep replaces the relock backoff sleep with a no-op so retry tests +// run without real delays, restoring it on cleanup. It mutates the package-level +// sleepFn var, so tests using it must not run in parallel. +func stubRelockSleep(t *testing.T) { + t.Helper() + orig := sleepFn + t.Cleanup(func() { sleepFn = orig }) + sleepFn = func(time.Duration) {} +} + // TestKeychainGetNotFound exercises the full Get path against the fake — open, // resolve collection, search — and asserts an empty search maps to // ErrCredentialNotFound, all without a live keyring. @@ -116,6 +186,336 @@ func TestKeychainClosesEveryConnection(t *testing.T) { assert.Equal(t, opened, closed, "every opened connection must be closed") } +// TestKeychainSaveCreatesWhenAbsent asserts Save mints a new item only when the +// identity has no existing item, and performs no in-place update or cleanup. +func TestKeychainSaveCreatesWhenAbsent(t *testing.T) { + fake := &fakeService{} // no items -> create path + withFakeService(t, fake) + + ks := setupKeychain(t, nil) + id := store.MustParseID("com.test.test/test/new-user") + creds := &mocks.MockCredential{Username: "alice", Password: "alice-password"} + + require.NoError(t, ks.Save(t.Context(), id, creds)) + + assert.Equal(t, 1, fake.createCalls, "must CreateItem when no existing item") + assert.Empty(t, fake.setSecretItems, "no in-place update when creating") + assert.Empty(t, fake.deletedItems, "nothing to collapse") +} + +// TestKeychainSaveCollapsesDuplicatesInPlace is the issue #446 regression test: +// when several items already share one stable identity (the accumulated +// duplicates), Save must update the first match in place — never minting a new +// item — and drain the remaining duplicates, leaving exactly one. +func TestKeychainSaveCollapsesDuplicatesInPlace(t *testing.T) { + fake := &fakeService{ + items: []dbus.ObjectPath{"/item/a", "/item/b", "/item/c"}, + } + withFakeService(t, fake) + + ks := setupKeychain(t, nil) + id := store.MustParseID("com.test.test/test/bob") + creds := &mocks.MockCredential{Username: "bob", Password: "bob-password"} + + require.NoError(t, ks.Save(t.Context(), id, creds)) + + assert.Zero(t, fake.createCalls, "must not CreateItem when an item already exists") + assert.Equal(t, []dbus.ObjectPath{"/item/a"}, fake.setSecretItems, + "secret must be rewritten on the first match in place") + assert.ElementsMatch(t, []dbus.ObjectPath{"/item/b", "/item/c"}, fake.deletedItems, + "the remaining duplicates must be collapsed, leaving only the first match") +} + +// TestKeychainSaveRetriesWhenCreateRelocks covers the create path: gnome-keyring +// can relock the collection between the unlock and the CreateItem, so a fresh +// Save must react to the locked error by unlocking and retrying rather than +// failing. +func TestKeychainSaveRetriesWhenCreateRelocks(t *testing.T) { + stubRelockSleep(t) + fake := &fakeService{} // no items -> create path + fake.createItemLockedErrs = 2 + withFakeService(t, fake) + + ks := setupKeychain(t, nil) + require.NoError(t, ks.Save(t.Context(), store.MustParseID("com.test.test/test/bob"), + &mocks.MockCredential{Username: "bob", Password: "bob-password"})) + + assert.Equal(t, 3, fake.createCalls, "two locked failures then one success") + assert.Equal(t, 2, fake.unlockCalls, "exactly one Unlock per relock retry") +} + +// TestKeychainSaveRetriesWhenSetSecretRelocks covers the in-place update path: +// the SetItemSecret that rewrites the surviving item must survive a relock. +func TestKeychainSaveRetriesWhenSetSecretRelocks(t *testing.T) { + stubRelockSleep(t) + fake := &fakeService{items: []dbus.ObjectPath{"/item/a"}} + fake.setSecretLockedErrs = 2 + withFakeService(t, fake) + + ks := setupKeychain(t, nil) + require.NoError(t, ks.Save(t.Context(), store.MustParseID("com.test.test/test/bob"), + &mocks.MockCredential{Username: "bob", Password: "bob-password"})) + + 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") +} + +// TestKeychainSaveCollapseRetriesWhenDeleteRelocks is the unit-level counterpart +// of the real-keyring backlog test: collapsing a duplicate must drain it even if +// the collection relocks mid-delete. The collapse delete is best-effort, but a +// silently swallowed locked error would leave the duplicate behind — the exact +// #446 symptom — so it is still relock-aware. +func TestKeychainSaveCollapseRetriesWhenDeleteRelocks(t *testing.T) { + stubRelockSleep(t) + fake := &fakeService{items: []dbus.ObjectPath{"/item/a", "/item/b"}} + fake.deleteItemLockedErrs = 2 + withFakeService(t, fake) + + ks := setupKeychain(t, nil) + require.NoError(t, ks.Save(t.Context(), store.MustParseID("com.test.test/test/bob"), + &mocks.MockCredential{Username: "bob", Password: "bob-password"})) + + assert.Equal(t, []dbus.ObjectPath{"/item/b"}, fake.deletedItems, + "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") +} + +// TestKeychainSaveStopsRetryingAfterMaxRelocks asserts the retry is bounded: a +// persistently locked collection surfaces the locked error to the caller instead +// of looping forever. +func TestKeychainSaveStopsRetryingAfterMaxRelocks(t *testing.T) { + stubRelockSleep(t) + fake := &fakeService{} // no items -> create path + fake.createItemLockedErrs = 1 << 30 // never recovers + 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.True(t, isLockedDBusError(err), "the persistent locked error must reach the caller") + assert.Equal(t, maxRelockRetries+1, fake.createCalls, "initial attempt plus the bounded retries") +} + +// 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 +// dedup item can never show up in — and break — the shared suite. +const ( + dedupServiceGroup = "com.test.dedup" + dedupServiceName = "dedup" +) + +// ensureUnlocked unlocks the collection and waits until the daemon actually +// reports it unlocked. The freedesktop Unlock call can return before the +// collection is fully unlocked, so a CreateItem/DeleteItem issued immediately +// after can still fail with "locked collection" — polling IsLocked closes that +// race. (The store's own Save/Get/Delete avoid it only because the collection +// 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.Eventually(t, func() bool { + locked, err := svc.IsLocked(collection) + return err == nil && !locked + }, 5*time.Second, 100*time.Millisecond, "collection did not unlock") +} + +// searchRealItems queries a live Secret Service for every item sharing id's +// stable identity triple {service:group, service:name, id}. The store API keys +// results by ID and so would collapse duplicates into one logical entry — +// counting the physical items requires going under it, straight to the daemon. +// +// It opens its own short-lived connection (mirroring how the store dials a fresh +// connection per operation) so it observes exactly what an independent client +// would see. Object paths it returns stay valid after the connection closes. +// +// It returns its error rather than failing the test so it is safe to call from a +// [require.Eventually] condition, which runs on a separate goroutine where the +// require.* helpers must not be used. +func searchRealItems(serviceGroup, serviceName string, id store.ID) ([]dbus.ObjectPath, error) { + svc, err := kc.NewService() + if err != nil { + return nil, err + } + defer func() { _ = svc.Close() }() + + collection, err := getDefaultCollection(svc) + if err != nil { + return nil, err + } + + attrs := map[string]string{} + safelySetMetadata(serviceGroup, serviceName, attrs) + safelySetID(id, attrs) + + return svc.SearchCollection(collection, attrs) +} + +// findRealItems is the test-goroutine wrapper around [searchRealItems] that fails +// the test on error. +func findRealItems(t *testing.T, serviceGroup, serviceName string, id store.ID) []dbus.ObjectPath { + t.Helper() + items, err := searchRealItems(serviceGroup, serviceName, id) + require.NoError(t, err) + return items +} + +// requireItemCount polls the live Secret Service until exactly want items remain +// for id, failing after a short timeout. Polling absorbs any lag between the +// store deleting duplicates and an independent connection observing it; on +// timeout EventuallyWithT reports the last observed count (and any search +// error), so a genuine failure to converge is still caught and diagnosable. +func requireItemCount(t *testing.T, serviceGroup, serviceName string, id store.ID, want int, msg string) { + t.Helper() + require.EventuallyWithT(t, func(c *assert.CollectT) { + items, err := searchRealItems(serviceGroup, serviceName, id) + assert.NoError(c, err) + assert.Len(c, items, want, msg) + }, 10*time.Second, 200*time.Millisecond) +} + +// seedRealDuplicates creates n separate Secret Service items that all share id's +// stable identity triple but carry a distinct volatile attribute each — exactly +// how issue #446 accumulates duplicates: the daemon's replace match fails on the +// differing volatile attributes, so every save mints a fresh item. +func seedRealDuplicates(t *testing.T, serviceGroup, serviceName string, id store.ID, n int) { + t.Helper() + svc, err := kc.NewService() + require.NoError(t, err) + defer func() { _ = svc.Close() }() + + session, err := svc.OpenSession(kc.AuthenticationDHAES) + require.NoError(t, err) + defer svc.CloseSession(session) + + collection, err := getDefaultCollection(svc) + require.NoError(t, err) + + // Talking to the daemon directly skips the unlock the store does internally, + // and gnome-keyring reports even the passwordless 'login' collection as + // locked until then. + ensureUnlocked(t, svc, collection) + + label := serviceGroup + ":" + serviceName + ":" + id.String() + for i := range n { + sessSecret, err := session.NewSecret(fmt.Appendf(nil, "seed-user:seed-pass-%d", i)) + require.NoError(t, err) + + attrs := map[string]string{ + // volatile: distinct per item, so each CreateItem adds a new one + // rather than replacing — the duplicate-accumulation pattern. + "nonce": fmt.Sprintf("seed-%d", i), + } + safelySetMetadata(serviceGroup, serviceName, attrs) + safelySetID(id, attrs) + + // Seed directly against the daemon, but stay relock-aware: a prior op's + // 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) + return createErr + }) + require.NoError(t, err) + } +} + +// purgeRealItems removes every item for id, draining any leftover duplicates so +// the test cannot leak state. It unlocks the collection first (DeleteItem fails +// on a locked collection) and deletes all matches, asserting success so a silent +// cleanup failure surfaces as a leak rather than corrupting a later test. +func purgeRealItems(t *testing.T, serviceGroup, serviceName string, id store.ID) { + t.Helper() + svc, err := kc.NewService() + require.NoError(t, err) + defer func() { _ = svc.Close() }() + + collection, err := getDefaultCollection(svc) + require.NoError(t, err) + ensureUnlocked(t, svc, collection) + + attrs := map[string]string{} + safelySetMetadata(serviceGroup, serviceName, attrs) + safelySetID(id, attrs) + 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) + })) + } +} + +// TestKeychainCollapsesExistingDuplicates is the issue #446 backlog test against +// a real Secret Service: given several duplicate items already stored under one +// identity, a single Save must update one item in place and delete the rest, +// leaving exactly one item holding the latest secret. +func TestKeychainCollapsesExistingDuplicates(t *testing.T) { + id := store.MustParseID(dedupServiceGroup + "/" + dedupServiceName + "/backlog") + t.Cleanup(func() { purgeRealItems(t, dedupServiceGroup, dedupServiceName, id) }) + + // Pre-existing backlog: three duplicate items for one identity. + seedRealDuplicates(t, dedupServiceGroup, dedupServiceName, id, 3) + require.Len(t, findRealItems(t, dedupServiceGroup, dedupServiceName, id), 3, "precondition: three duplicates seeded") + + ks, err := New(dedupServiceGroup, dedupServiceName, func(_ context.Context, _ store.ID) store.Secret { + return &mocks.MockCredential{} + }) + require.NoError(t, err) + + require.NoError(t, ks.Save(t.Context(), id, &mocks.MockCredential{ + Username: "backlog-user", + Password: "final-password", + })) + + requireItemCount(t, dedupServiceGroup, dedupServiceName, id, 1, + "a single Save must collapse the duplicate backlog to one item") + + got, err := ks.Get(t.Context(), id) + require.NoError(t, err) + assert.Equal(t, "final-password", got.(*mocks.MockCredential).Password, + "the surviving item must hold the latest secret") +} + +// TestKeychainSaveDoesNotAccumulate is the forward-looking issue #446 test +// against a real Secret Service: saving the same identity repeatedly with +// metadata that changes on every save (mimicking volatile JWT claims) must keep +// exactly one item, never minting duplicates. +func TestKeychainSaveDoesNotAccumulate(t *testing.T) { + id := store.MustParseID(dedupServiceGroup + "/" + dedupServiceName + "/no-accumulate") + t.Cleanup(func() { purgeRealItems(t, dedupServiceGroup, dedupServiceName, id) }) + + ks, err := New(dedupServiceGroup, dedupServiceName, func(_ context.Context, _ store.ID) store.Secret { + return &mocks.MockCredential{} + }) + require.NoError(t, err) + + const saves = 5 + for i := range saves { + require.NoError(t, ks.Save(t.Context(), id, &mocks.MockCredential{ + Username: "no-accumulate-user", + Password: fmt.Sprintf("password-%d", i), + Attributes: map[string]string{ + "nonce": fmt.Sprintf("%d", i), // volatile: differs every save + }, + })) + } + + requireItemCount(t, dedupServiceGroup, dedupServiceName, id, 1, + "saving with changing metadata must not accumulate duplicate items") + + got, err := ks.Get(t.Context(), id) + require.NoError(t, err) + actual := got.(*mocks.MockCredential) + assert.Equal(t, fmt.Sprintf("password-%d", saves-1), actual.Password) + assert.Equal(t, fmt.Sprintf("%d", saves-1), actual.Attributes["nonce"], + "the surviving item's metadata must be refreshed in place") +} + func TestResolveDefaultCollection(t *testing.T) { const customCollection = dbus.ObjectPath("/org/freedesktop/secrets/collection/custom")