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 } 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")