Repository navigation
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>
| return err | ||
| } | ||
| return k.Save(ctx, id, secret) | ||
| err = k.Save(ctx, id, secret) |
There was a problem hiding this comment.
[P1] Preserve the marshaled payload across the update/add attempts
When the item does not exist, k.update still calls secret.Marshal() and runs defer clear(data) before returning ErrCredentialNotFound. PassValue.Marshal() in plugins/pass/store/store.go returns its backing buffer, so this subsequent Save marshals an already-zeroed value and successfully stores zeros. The duplicate-item retry has the same buffer-lifetime problem.
I reproduced this with Upsert of a new PassValue containing review-test-value: reading it back returns 17 zero bytes. The same check passes on the parent commit. MockCredential.Marshal() allocates fresh bytes on each call, which is why the current insertion test passes.
Please marshal once for the whole Upsert, reuse that payload for update/add/retry, and clear it only after the operation finishes. Add a regression case using a secret whose Marshal returns its backing buffer.
| } | ||
|
|
||
| changes := kc.NewItem() | ||
| changes.SetData(data) |
There was a problem hiding this comment.
[P2] Write an explicit empty payload when Marshal returns nil
kc.Item.SetData(nil) removes kSecValueData from the attributes dictionary. In an update, omitting that attribute leaves the existing secret unchanged, so an empty replacement silently becomes a metadata-only update. This affects the real PassValue: its Unmarshal produces a nil backing slice for empty input.
I reproduced this by saving review-old-value and then upserting passstore.NewPassValue(nil): Upsert returns nil, but Get still returns review-old-value. The parent commit correctly returns an empty value.
Please normalize a nil marshaled payload to a non-nil empty slice before calling SetData, and add a regression test that replaces a non-empty secret with an empty one.
|
|
||
| if err := k.Delete(ctx, id); err != nil { | ||
| err := k.update(id, secret) | ||
| if !errors.Is(err, store.ErrCredentialNotFound) { |
There was a problem hiding this comment.
If the upsert fails (i.e. due to another processing deleting a credential) then we should simply return with the error. A retry can always happen by the caller. Being too smart here might actually open up more problems in future and backtracking this change would simply be devestating for other tools relying on this library.
| // The query names only the attributes that identify the item; the match | ||
| // and return options of newKeychainItem belong to SecItemCopyMatching. |
There was a problem hiding this comment.
| // The query names only the attributes that identify the item; the match | |
| // and return options of newKeychainItem belong to SecItemCopyMatching. | |
| // We aren't using newKeychainItem here because we only want to identify the item, not read it. |
2 participants
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. If another process adds the item between the two calls,Upsertretries the update.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
Save,Delete, and the item format are unchanged.Testing
TestUpsertsubtest "updates an existing credential in place". It checks that the item's creation date and label are unchanged, that its modification date moved forward, and thatGetreturns the new secret and metadata. It fails against the previous delete-and-add implementation.make keychain-unit-testspasses, including with-race;gofmtandgo vetare clean.AI usage
Cursor (Claude) wrote most of this change and the test, after diagnosing the repeated prompts in Docker Sandboxes. I reviewed the diff and ran the tests locally on macOS.