fix(keychain): update existing items in place on Upsert

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) {

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() {

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].
 	//