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