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

store/keychain/keychain.go

@@ -47,6 +47,12 @@ var ErrCollectionLocked = errors.New("keychain collection is locked")
 // existing item in place.
 var ErrDuplicateItem = errors.New("keychain item already exists")
 
+// ErrEmptySecret is returned by Save and Upsert when the secret's value is
+// empty. The macOS Keychain ignores an empty value on update and keeps the old
+// one, so both refuse it instead of reporting a success that changed nothing.
+// macOS-only.
+var ErrEmptySecret = errors.New("keychain secret value must not be empty")
+
 type (
 	Option            interface{ apply(any) error }
 	optionFunc[K any] func(K) error

store/keychain/keychain_darwin.go

@@ -186,40 +186,83 @@ func (k *keychainStore[T]) Save(_ context.Context, id store.ID, secret store.Sec
 		return err
 	}
 	defer clear(data)
+	if len(data) == 0 {
+		return ErrEmptySecret
+	}
+	return k.add(id, secret, data)
+}
+
+func (k *keychainStore[T]) add(id store.ID, secret store.Secret, data []byte) error {
 	item := newKeychainItem(id.String(), k)
 	item.SetData(data)
 	// only creation of a secret needs the label attribute.
 	// 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.
+//
+// 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.
 //
-// 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.
-func (k *keychainStore[T]) Upsert(ctx context.Context, id store.ID, secret store.Secret) error {
+// An empty secret value returns [ErrEmptySecret] without touching the
+// keychain. Another store instance or process can still write the same item
+// concurrently, in which case Upsert returns [ErrDuplicateItem].
+func (k *keychainStore[T]) Upsert(_ context.Context, id store.ID, secret store.Secret) error {
 	k.mu.Lock()
 	defer k.mu.Unlock()
 
-	if err := k.Delete(ctx, id); err != nil {
+	data, err := secret.Marshal()
+	if err != nil {
+		return err
+	}
+	defer clear(data)
+	if len(data) == 0 {
+		return ErrEmptySecret
+	}
+
+	err = k.update(id, secret, data)
+	if !errors.Is(err, store.ErrCredentialNotFound) {
 		return err
 	}
-	return k.Save(ctx, id, secret)
+	return k.add(id, secret, data)
+}
+
+func (k *keychainStore[T]) update(id store.ID, secret store.Secret, data []byte) error {
+	// We aren't using newKeychainItem here because we only want to identify the item, not read it.
+	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

@@ -26,6 +26,7 @@ import (
 	"github.com/stretchr/testify/require"
 
 	"github.com/docker/secrets-engine/store"
+	kc "github.com/docker/secrets-engine/store/keychain/internal/go-keychain"
 	"github.com/docker/secrets-engine/store/mocks"
 )
 
@@ -209,6 +210,111 @@ 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"},
+		}))
+		query := kc.NewItem()
+		query.SetSecClass(kc.SecClassGenericPassword)
+		query.SetService(serviceName)
+		query.SetAccessGroup(serviceGroup)
+		query.SetAccount(id.String())
+		marker := kc.NewItem()
+		marker.SetComment("set-outside-the-store")
+		require.NoError(t, kc.UpdateItem(query, marker))
+
+		before, err := getItemWithData(id.String(), &ks)
+		require.NoError(t, err)
+		require.Equal(t, "set-outside-the-store", before.Comment)
+
+		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, "set-outside-the-store", after.Comment, "the item must be updated, not deleted and re-added")
+		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("inserts a secret whose Marshal returns its backing buffer", func(t *testing.T) {
+		bs := keychainStore[*bufferSecret]{
+			serviceGroup: serviceGroup,
+			serviceName:  serviceName,
+			factory: func(_ context.Context, _ store.ID) *bufferSecret {
+				return &bufferSecret{}
+			},
+		}
+		id := store.MustParseID(serviceGroup + "/" + serviceName + "/" + uuid.NewString())
+		t.Cleanup(func() {
+			assert.NoError(t, bs.Delete(t.Context(), id))
+		})
+
+		require.NoError(t, bs.Upsert(t.Context(), id, &bufferSecret{value: []byte("review-test-value")}))
+
+		got, err := bs.Get(t.Context(), id)
+		require.NoError(t, err)
+		assert.Equal(t, []byte("review-test-value"), got.(*bufferSecret).value)
+	})
+
+	t.Run("rejects an empty secret value and keeps the existing one", func(t *testing.T) {
+		bs := keychainStore[*bufferSecret]{
+			serviceGroup: serviceGroup,
+			serviceName:  serviceName,
+			factory: func(_ context.Context, _ store.ID) *bufferSecret {
+				return &bufferSecret{}
+			},
+		}
+		id := store.MustParseID(serviceGroup + "/" + serviceName + "/" + uuid.NewString())
+		t.Cleanup(func() {
+			assert.NoError(t, bs.Delete(t.Context(), id))
+		})
+
+		require.NoError(t, bs.Save(t.Context(), id, &bufferSecret{value: []byte("review-old-value")}))
+		require.ErrorIs(t, bs.Upsert(t.Context(), id, &bufferSecret{}), ErrEmptySecret)
+		require.ErrorIs(t, bs.Upsert(t.Context(), id, &bufferSecret{value: []byte{}}), ErrEmptySecret)
+
+		got, err := bs.Get(t.Context(), id)
+		require.NoError(t, err)
+		assert.Equal(t, []byte("review-old-value"), got.(*bufferSecret).value)
+	})
+
+	t.Run("save rejects an empty secret value", func(t *testing.T) {
+		bs := keychainStore[*bufferSecret]{
+			serviceGroup: serviceGroup,
+			serviceName:  serviceName,
+			factory: func(_ context.Context, _ store.ID) *bufferSecret {
+				return &bufferSecret{}
+			},
+		}
+		id := store.MustParseID(serviceGroup + "/" + serviceName + "/" + uuid.NewString())
+		t.Cleanup(func() {
+			assert.NoError(t, bs.Delete(t.Context(), id))
+		})
+
+		require.ErrorIs(t, bs.Save(t.Context(), id, &bufferSecret{}), ErrEmptySecret)
+		require.ErrorIs(t, bs.Save(t.Context(), id, &bufferSecret{value: []byte{}}), ErrEmptySecret)
+
+		_, err := bs.Get(t.Context(), id)
+		require.ErrorIs(t, err, store.ErrCredentialNotFound)
+	})
+
 	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() {
@@ -226,6 +332,33 @@ func TestUpsert(t *testing.T) {
 	})
 }
 
+type bufferSecret struct {
+	value    []byte
+	metadata map[string]string
+}
+
+func (b *bufferSecret) Marshal() ([]byte, error) {
+	return b.value, nil
+}
+
+func (b *bufferSecret) Unmarshal(data []byte) error {
+	if len(data) == 0 {
+		b.value = nil
+		return nil
+	}
+	b.value = append([]byte(nil), data...)
+	return nil
+}
+
+func (b *bufferSecret) Metadata() map[string]string {
+	return b.metadata
+}
+
+func (b *bufferSecret) SetMetadata(md map[string]string) error {
+	b.metadata = md
+	return nil
+}
+
 func TestConvertAttributes(t *testing.T) {
 	t.Run("can convert attributes map into map of any", func(t *testing.T) {
 		attributes := map[string]any{

store/store.go

@@ -85,11 +85,17 @@ type Store interface {
 	// Experience.
 	GetAllMetadata(ctx context.Context) (map[ID]Secret, error)
 	// Save persists credentials from the store.
+	//
+	// The macOS Keychain rejects an empty secret value with
+	// keychain.ErrEmptySecret.
 	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. The macOS Keychain rejects
+	// an empty secret value with keychain.ErrEmptySecret. Another store
+	// instance or process can still write the same item concurrently, in which
+	// case Upsert returns keychain.ErrDuplicateItem.
 	Upsert(ctx context.Context, id ID, secret Secret) error
 	// Filter returns a map of secrets based on a [Pattern].
 	//