From 35beb5ecf0362c18f2f495caf9306fb15b7c037a Mon Sep 17 00:00:00 2001 From: Christian Dupuis Date: Fri, 9 Oct 2026 15:36:37 +0200 Subject: [PATCH] fix(keychain): update existing items in place on Upsert 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 --- store/keychain/keychain_darwin.go | 55 +++++++++++++++++++++----- store/keychain/keychain_darwin_test.go | 38 ++++++++++++++++++ store/store.go | 5 +-- 3 files changed, 85 insertions(+), 13 deletions(-) diff --git a/store/keychain/keychain_darwin.go b/store/keychain/keychain_darwin.go index 744da1de..759c5ba3 100644 --- a/store/keychain/keychain_darwin.go +++ b/store/keychain/keychain_darwin.go @@ -192,34 +192,69 @@ func (k *keychainStore[T]) Save(_ context.Context, id store.ID, secret store.Sec // it is a user-friendly name for the item, which is displayed in the keychain UI. // https://developer.apple.com/documentation/security/ksecattrlabel item.SetLabel(k.itemLabel(id.String())) + item.SetGenericMetadata(k.genericMetadata(id, secret)) + return mapKeychainError(kc.AddItem(item)) +} + +func (k *keychainStore[T]) genericMetadata(id store.ID, secret store.Secret) map[string]any { metadata := make(map[string]string) maps.Copy(metadata, secret.Metadata()) safelySetMetadata(k.serviceGroup, k.serviceName, metadata) safelySetID(id, metadata) - metadataAny := make(map[string]any) + metadataAny := make(map[string]any, len(metadata)) for k, v := range metadata { metadataAny[k] = v } - item.SetGenericMetadata(metadataAny) - - return mapKeychainError(kc.AddItem(item)) + return metadataAny } -// Upsert atomically replaces a credential in the macOS Keychain. +// Upsert updates an existing credential in place and adds it when it does +// not exist yet. // -// The macOS Keychain does not allow overwriting an existing item via AddItem, -// so Upsert holds a mutex and performs a Delete followed by a Save to ensure -// no concurrent Upsert can interleave between the two operations. +// Updating in place keeps the item's access control and partition lists, so +// applications the user allowed to read the item stay allowed. Deleting and +// re-adding it would reset those lists to the calling binary alone. func (k *keychainStore[T]) Upsert(ctx context.Context, id store.ID, secret store.Secret) error { k.mu.Lock() defer k.mu.Unlock() - if err := k.Delete(ctx, id); err != nil { + err := k.update(id, secret) + if !errors.Is(err, store.ErrCredentialNotFound) { return err } - return k.Save(ctx, id, secret) + err = k.Save(ctx, id, secret) + if errors.Is(err, ErrDuplicateItem) { + // Another process added the item after our update missed it. + return k.update(id, secret) + } + return err +} + +func (k *keychainStore[T]) update(id store.ID, secret store.Secret) error { + data, err := secret.Marshal() + if err != nil { + return err + } + defer clear(data) + + // The query names only the attributes that identify the item; the match + // and return options of newKeychainItem belong to SecItemCopyMatching. + query := kc.NewItem() + query.SetSecClass(kc.SecClassGenericPassword) + query.SetService(k.serviceName) + query.SetAccessGroup(k.serviceGroup) + query.SetAccount(id.String()) + if k.useDataProtectionKeychain { + query.SetUseDataProtectionKeychain(kc.UseDataProtectionKeychainYes) + } + + changes := kc.NewItem() + changes.SetData(data) + changes.SetGenericMetadata(k.genericMetadata(id, secret)) + + return mapKeychainError(kc.UpdateItem(query, changes)) } func (k *keychainStore[T]) Filter(ctx context.Context, pattern store.Pattern) (map[store.ID]store.Secret, error) { diff --git a/store/keychain/keychain_darwin_test.go b/store/keychain/keychain_darwin_test.go index 027b8366..9b76b155 100644 --- a/store/keychain/keychain_darwin_test.go +++ b/store/keychain/keychain_darwin_test.go @@ -20,6 +20,7 @@ import ( "bytes" "context" "testing" + "time" "github.com/google/uuid" "github.com/stretchr/testify/assert" @@ -209,6 +210,43 @@ func TestUpsert(t *testing.T) { assert.Equal(t, updated.Password, actual.Password) }) + t.Run("updates an existing credential in place", func(t *testing.T) { + id := store.MustParseID(serviceGroup + "/" + serviceName + "/" + uuid.NewString()) + t.Cleanup(func() { + assert.NoError(t, ks.Delete(t.Context(), id)) + }) + + require.NoError(t, ks.Save(t.Context(), id, &mocks.MockCredential{ + Username: "dana", + Password: "original-password", + Attributes: map[string]string{"expiry": "1"}, + })) + before, err := getItemWithData(id.String(), &ks) + require.NoError(t, err) + + // Keychain timestamps have one-second resolution. + time.Sleep(1100 * time.Millisecond) + + updated := &mocks.MockCredential{ + Username: "dana", + Password: "updated-password", + Attributes: map[string]string{"expiry": "2"}, + } + require.NoError(t, ks.Upsert(t.Context(), id, updated)) + + after, err := getItemWithData(id.String(), &ks) + require.NoError(t, err) + assert.Equal(t, before.CreationDate, after.CreationDate, "the item must be updated, not deleted and re-added") + assert.True(t, after.ModificationDate.After(before.ModificationDate)) + assert.Equal(t, before.Label, after.Label) + + got, err := ks.Get(t.Context(), id) + require.NoError(t, err) + actual := got.(*mocks.MockCredential) + assert.Equal(t, updated.Password, actual.Password) + assert.Equal(t, map[string]string{"expiry": "2"}, actual.Metadata()) + }) + t.Run("save returns duplicate item error when credential already exists", func(t *testing.T) { id := store.MustParseID(serviceGroup + "/" + serviceName + "/" + uuid.NewString()) t.Cleanup(func() { diff --git a/store/store.go b/store/store.go index 211bf505..33010f53 100644 --- a/store/store.go +++ b/store/store.go @@ -87,9 +87,8 @@ type Store interface { // Save persists credentials from the store. Save(ctx context.Context, id ID, secret Secret) error // Upsert atomically replaces an existing credential or inserts a new one. - // On stores that do not support overwriting (e.g. macOS Keychain), it - // deletes the existing credential and then saves the new one under a mutex - // to ensure the two operations are not interleaved. + // On the macOS Keychain an existing item is updated in place, keeping the + // applications the user allowed to access it. Upsert(ctx context.Context, id ID, secret Secret) error // Filter returns a map of secrets based on a [Pattern]. //