From d8b400004ce3f30f51d080bc22c5c192424c546d Mon Sep 17 00:00:00 2001 From: Alano Terblanche <18033717+Benehiko@users.noreply.github.com> Date: Fri, 19 Jun 2026 16:00:36 +0200 Subject: [PATCH 1/2] fix(keychain): update Secret Service items in place on Linux to stop duplicate accumulation On Linux, keychainStore.Save copied the secret's volatile Metadata() into the searchable Secret Service attributes and relied on CreateItem(ReplaceBehaviorReplace) to overwrite the prior item. Both gnome-keyring and kwalletd select the replace target by matching the full supplied attribute set, so any change in the volatile metadata (e.g. the Docker Hub OAuth credential's rotating JWT claims) defeats the match and a brand-new item is created on every save -- duplicates pile up without bound, and each stale item keeps a cleartext copy of the old claims. Fix Save to update in place: search by the stable identity triple {service:group, service:name, id} only, then either create when absent or rewrite the first match's secret/attributes/label in place and collapse any pre-existing duplicates. The item's object path is preserved, so the secret is never momentarily absent and no duplicate is minted. The observable store contract is unchanged: Save still returns nil iff the secret is stored (refreshing attributes/label and collapsing leftovers are best-effort). This is backend-agnostic: the attribute-match behaviour is shared by gnome-keyring and kwalletd, and macOS/Windows key items on a stable identifier so are unaffected. Add SetItemSecret/SetItemAttributes/SetItemLabel to the vendored secretservice library (thin org.freedesktop.Secret.Item wrappers) to enable the in-place update. Refs: docker/secrets-engine-private#446 Co-Authored-By: Claude Opus 4.8 (1M context) --- .../secretservice/secretservice.go | 34 +++++++++++++++++ store/keychain/keychain_linux.go | 37 ++++++++++++++++++- 2 files changed, 69 insertions(+), 2 deletions(-) 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..d1fe4aed 100644 --- a/store/keychain/keychain_linux.go +++ b/store/keychain/keychain_linux.go @@ -65,6 +65,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 } @@ -385,13 +388,43 @@ 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) + _, err = service.CreateItem(objectPath, properties, sessSecret, kc.ReplaceBehaviorReplace) + return err + } + + // 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 := service.SetItemSecret(primary, sessSecret); err != nil { + return err + } + _ = service.SetItemAttributes(primary, attributes) + _ = service.SetItemLabel(primary, label) + for _, dup := range items[1:] { + _ = service.DeleteItem(dup) + } + return nil } From cff4885f2b068898920bbc6c8cd8968f68a4c9c6 Mon Sep 17 00:00:00 2001 From: Alano Terblanche <18033717+Benehiko@users.noreply.github.com> Date: Fri, 19 Jun 2026 16:00:39 +0200 Subject: [PATCH 2/2] test(keychain): cover create, in-place collapse, and real-keychain dedup convergence Fake-backed unit tests for the create vs in-place-collapse branches, plus real-keychain integration tests (run under the gnome-keyring harness) that seed a duplicate backlog and assert a single Save collapses it to one item, and that repeated saves with changing metadata never accumulate. The integration assertions poll the daemon via require.EventuallyWithT until the expected item count is reached, absorbing the lag between the store deleting duplicates and an independent connection observing it without masking a genuine failure to converge. The dedup tests use their own service group/name (com.test.dedup/dedup) so a leaked credential can never reach TestKeychain, and ensureUnlocked polls IsLocked after Unlock to close the first-unlock race on the direct-to-daemon seed and purge paths. Refs: docker/secrets-engine-private#446 Co-Authored-By: Claude Opus 4.8 (1M context) --- store/keychain/keychain_linux_test.go | 278 +++++++++++++++++++++++++- 1 file changed, 275 insertions(+), 3 deletions(-) diff --git a/store/keychain/keychain_linux_test.go b/store/keychain/keychain_linux_test.go index d6ab98be..94f5aca9 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,6 +44,13 @@ 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 + setSecretItems []dbus.ObjectPath + deletedItems []dbus.ObjectPath + opened atomic.Int64 closed atomic.Int64 } @@ -50,7 +61,9 @@ func (f *fakeService) Collections() ([]dbus.ObjectPath, error) { 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 } @@ -59,11 +72,22 @@ func (f *fakeService) SearchCollection(dbus.ObjectPath, kc.Attributes) ([]dbus.O } func (f *fakeService) CreateItem(dbus.ObjectPath, map[string]dbus.Variant, kc.Secret, kc.ReplaceBehavior) (dbus.ObjectPath, error) { - return "", nil + f.createCalls++ + return "/created", nil +} + +func (f *fakeService) DeleteItem(item dbus.ObjectPath) error { + 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.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 @@ -116,6 +140,254 @@ 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") +} + +// 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) + + _, err = svc.CreateItem(collection, kc.NewSecretProperties(label, attrs), sessSecret, kc.ReplaceBehaviorDoNotReplace) + 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, 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")