Repository navigation
fix(keychain): update existing items in place on Upsert - #687
Merged
Merged
Conversation
Upsert deleted the item and added it again. SecItemAdd gives the new item an access control list that trusts only the calling binary, so every other application the user had allowed to read the item gets a keychain prompt again after each write. Update the item in place with SecItemUpdate, which keeps its access control and partition lists, and add it only when it does not exist yet. Signed-off-by: Christian Dupuis <cd@docker.com>
Benehiko
reviewed
Oct 9, 2026
Marshal the secret once and reuse it for the update and the add, so a secret whose Marshal returns its backing buffer is not stored as zeros. Drop the update retry after a duplicate-item error and return the error to the caller. Reject empty secret values in Upsert with ErrEmptySecret, since the keychain ignores an empty value on update. Signed-off-by: Alano Terblanche <18033717+Benehiko@users.noreply.github.com>
Save now returns ErrEmptySecret for an empty value, matching Upsert. The in-place update test checks that a comment set outside the store survives Upsert instead of comparing timestamps, so it no longer sleeps. Document ErrEmptySecret and the cross-process ErrDuplicateItem case on Save and Upsert. Signed-off-by: Alano Terblanche <18033717+Benehiko@users.noreply.github.com>
joe0BAB
reviewed
Oct 13, 2026
| return err | ||
| } | ||
| defer clear(data) | ||
| if len(data) == 0 { |
Collaborator
There was a problem hiding this comment.
I don't think this is consistent cross platform, no?
joe0BAB
approved these changes
Oct 13, 2026
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
3 participants
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.
Problem
On macOS,
keychainStore.Upsertdeleted the item and added it again.SecItemAddgives the new item an access control list that trusts only the calling binary and resets its partition list. Any other application the user had clicked "Always Allow" for loses access and gets a keychain prompt again after every write.Docker Sandboxes hits this on every Docker Hub token refresh:
sbxrewrites thesandboxes-authitem throughUpsert, and the desktop app is prompted for the keychain password again.Change
Upsertnow updates the existing item in place withSecItemUpdate, which keeps its access control and partition lists. It adds the item withSecItemAddonly when the update finds no match. Upsert is still atomic within a store. Another store instance or process can still write the same item concurrently, in which caseUpsertreturnsErrDuplicateItem.The update query names only the identifying attributes (class, service, access group, account, and data-protection keychain). The new data and the generic metadata are the only changes. The label and other attributes stay as they are, and
Deleteand the item format are unchanged.Upsertmarshals the secret once and uses the same bytes for the update and the add, clearing them only when it is done. A secret whoseMarshalreturns its own buffer is no longer stored as zeros.SaveandUpsertreject an empty secret value with the newErrEmptySecret. The keychain ignores an empty value on update and keeps the old secret, so an emptyUpsertwould otherwise succeed without changing anything;Saverejects it too so the two behave the same. This only applies to the macOS Keychain.Related: #693 makes
PassValue.Marshalreturn a copy, since every store backend clears the bytesMarshalreturns.Testing
TestUpsertsubtest "updates an existing credential in place". AfterSave, it sets a comment on the keychain item directly, outside the store. AfterUpsert, it checks that the comment and label are unchanged and thatGetreturns the new secret and metadata. Deleting and re-adding the item drops the comment, so the test fails against the previous delete-and-add implementation. It does not depend on timestamps or sleeps.TestUpsertsubtests using a secret whoseMarshalreturns its backing buffer: inserting a new secret stores its real value, upserting an empty value over an existing one returnsErrEmptySecretand keeps the old value, and saving an empty value returnsErrEmptySecretand stores nothing.go test ./...instore/passes on macOS;gofmtandgo vetare clean.AI usage
Cursor (Claude) wrote most of the original change and the first test, after diagnosing the repeated prompts in Docker Sandboxes. The review follow-up commits (single marshal, no retry,
ErrEmptySecret, the sleep-free test, and the docs) and this updated description were written by Claude (Anthropic) through the alki coding agent, at the direction of @Benehiko.