Skip to content

Commit 92a26ca

Browse files
Benehikoclaude
andcommitted
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) <noreply@anthropic.com>
1 parent 5d056a7 commit 92a26ca

2 files changed

Lines changed: 69 additions & 2 deletions

File tree

store/keychain/internal/go-keychain/secretservice/secretservice.go

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -436,6 +436,40 @@ func (s *SecretService) PromptAndWait(prompt dbus.ObjectPath) (paths *dbus.Varia
436436
}
437437
}
438438

439+
// SetItemSecret replaces an existing item's secret value in place via
440+
// org.freedesktop.Secret.Item.SetSecret. The secret must already be encoded for
441+
// the session it references (see [Session.NewSecret]). SetSecret takes a single
442+
// Secret argument (D-Bus signature (oayays)) and returns no values, so there is
443+
// no prompt path: the item's collection must be unlocked first (use [Unlock]),
444+
// otherwise the call fails.
445+
func (s *SecretService) SetItemSecret(item dbus.ObjectPath, secret Secret) error {
446+
if err := s.Obj(item).Call("org.freedesktop.Secret.Item.SetSecret", NilFlags, secret).Store(); err != nil {
447+
return fmt.Errorf("failed to set item secret: %w", err)
448+
}
449+
return nil
450+
}
451+
452+
// SetItemAttributes replaces an existing item's lookup attributes in place by
453+
// setting the read-write org.freedesktop.Secret.Item.Attributes property
454+
// (type a{ss}) through org.freedesktop.DBus.Properties.Set. The collection must
455+
// be unlocked.
456+
func (s *SecretService) SetItemAttributes(item dbus.ObjectPath, attributes Attributes) error {
457+
if err := s.Obj(item).SetProperty("org.freedesktop.Secret.Item.Attributes", dbus.MakeVariant(map[string]string(attributes))); err != nil {
458+
return fmt.Errorf("failed to set item attributes: %w", err)
459+
}
460+
return nil
461+
}
462+
463+
// SetItemLabel replaces an existing item's displayable label in place by setting
464+
// the read-write org.freedesktop.Secret.Item.Label property (type s) through
465+
// org.freedesktop.DBus.Properties.Set. The collection must be unlocked.
466+
func (s *SecretService) SetItemLabel(item dbus.ObjectPath, label string) error {
467+
if err := s.Obj(item).SetProperty("org.freedesktop.Secret.Item.Label", dbus.MakeVariant(label)); err != nil {
468+
return fmt.Errorf("failed to set item label: %w", err)
469+
}
470+
return nil
471+
}
472+
439473
// NewSecretProperties
440474
func NewSecretProperties(label string, attributes map[string]string) map[string]dbus.Variant {
441475
return map[string]dbus.Variant{

store/keychain/keychain_linux.go

Lines changed: 35 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -65,6 +65,9 @@ type secretService interface {
6565
DeleteItem(item dbus.ObjectPath) error
6666
GetAttributes(item dbus.ObjectPath) (kc.Attributes, error)
6767
GetSecret(item dbus.ObjectPath, session kc.Session) ([]byte, error)
68+
SetItemSecret(item dbus.ObjectPath, secret kc.Secret) error
69+
SetItemAttributes(item dbus.ObjectPath, attributes kc.Attributes) error
70+
SetItemLabel(item dbus.ObjectPath, label string) error
6871
Close() error
6972
}
7073

@@ -385,13 +388,43 @@ func (k *keychainStore[T]) Save(_ context.Context, id store.ID, secret store.Sec
385388
safelySetID(id, attributes)
386389

387390
label := k.itemLabel(id.String())
388-
properties := kc.NewSecretProperties(label, attributes)
389391

390-
_, err = service.CreateItem(objectPath, properties, sessSecret, kc.ReplaceBehaviorReplace)
392+
// Find existing items for this identity by the stable triple only
393+
// {service:group, service:name, id}, never the volatile metadata, so a
394+
// changed metadata value can never hide a previously-stored item. This is
395+
// what makes the in-place update below reliable and stops the duplicate
396+
// accumulation described in issue #446.
397+
ident := make(map[string]string)
398+
safelySetMetadata(k.serviceGroup, k.serviceName, ident)
399+
safelySetID(id, ident)
400+
401+
items, err := service.SearchCollection(objectPath, ident)
391402
if err != nil {
392403
return err
393404
}
394405

406+
// Nothing stored yet: create a fresh item.
407+
if len(items) == 0 {
408+
properties := kc.NewSecretProperties(label, attributes)
409+
_, err = service.CreateItem(objectPath, properties, sessSecret, kc.ReplaceBehaviorReplace)
410+
return err
411+
}
412+
413+
// Update the first match in place. Its object path is preserved, so the
414+
// secret is never momentarily absent and no duplicate is minted. Writing the
415+
// secret value IS the operation, so only its failure fails Save; refreshing
416+
// the attributes and label and collapsing any pre-existing duplicates are
417+
// best-effort (the secret is already stored) and must not flip the result.
418+
primary := items[0]
419+
if err := service.SetItemSecret(primary, sessSecret); err != nil {
420+
return err
421+
}
422+
_ = service.SetItemAttributes(primary, attributes)
423+
_ = service.SetItemLabel(primary, label)
424+
for _, dup := range items[1:] {
425+
_ = service.DeleteItem(dup)
426+
}
427+
395428
return nil
396429
}
397430

0 commit comments

Comments
 (0)