fix(keychain): update Secret Service items in place on Linux to stop duplicate accumulation#557
Open
Benehiko wants to merge 2 commits into
Open
fix(keychain): update Secret Service items in place on Linux to stop duplicate accumulation#557Benehiko wants to merge 2 commits into
Benehiko wants to merge 2 commits into
Conversation
![docker-agent[bot]](https://avatars.githubusercontent.com/in/2787757?s=60&v=4){kind=small}
There was a problem hiding this comment.
Assessment: 🟢 APPROVE
The in-place update logic is well-structured and correct. The stable-triple search () reliably isolates existing items regardless of volatile metadata changes, and the LIFO defer ordering ensures the session remains valid throughout all D-Bus calls. The new SetItemSecret/SetItemAttributes/SetItemLabel wrappers are minimal, correct, and properly documented. The best-effort contract for attribute refresh and duplicate collapse is clearly stated and consistently enforced. Test coverage spans both the unit (fake-backed) and integration (real gnome-keyring) paths.
Benehiko
commented
Jun 19, 2026
Comment on lines
+418
to
+426
| 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) | ||
| } |
Member
Author
There was a problem hiding this comment.
migration step for secrets that are currently being duplicated. future writes won't duplicate
…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>
…dup 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) <noreply@anthropic.com>
Benehiko
force-pushed
the
fix/keychain-linux-dedup
branch
from
ef6cbee to
cff4885
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
On Linux,
keychainStore.Savecopied the secret's volatileMetadata()into the searchable Secret Service attributes and relied onCreateItem(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.Fixes the write side of docker/secrets-engine-private#446 and docker/sbx-releases#245.
How
Savenow updates in place instead of relying on the daemon's replace-match:{service:group, service:name, id}only — never the volatile metadata — so a changed metadata value can never hide a previously-stored item.CreateItem.SetItemSecret), then best-effort refresh its attributes/label and delete the remaining duplicates.The item's object path is preserved, so the secret is never momentarily absent and no duplicate is minted. To enable the in-place update, three thin
org.freedesktop.Secret.Itemwrappers were added to the vendoredsecretservicelibrary:SetItemSecret,SetItemAttributes,SetItemLabel.This is backend-agnostic — the attribute-match behaviour is shared by gnome-keyring and kwalletd, verified against both daemons' source. macOS and Windows key items on a stable identifier (
Account/TargetName) and are unaffected.Contract
Unchanged:
Savestill returnsniliff the secret is stored. Refreshing attributes/label and collapsing leftover duplicates are best-effort and never flip the result.Deleteis untouched.Tests
TestKeychainSaveCreatesWhenAbsent(create branch),TestKeychainSaveCollapsesDuplicatesInPlace(in-place update + collapse).TestKeychainCollapsesExistingDuplicates— seeds a 3-item duplicate backlog directly via the daemon, then asserts a singleSavecollapses it to one item holding the latest secret.TestKeychainSaveDoesNotAccumulate— five saves with changing metadata stay at exactly one item, with the surviving item's metadata refreshed in place.All pass against real gnome-keyring; lint clean (
golangci-lint v2.12.2). The kwalletd target is daemon-agnostic but remains disabled in the harness (prompts under headless).🤖 Generated with Claude Code