diff --git a/store/keychain/keychain.go b/store/keychain/keychain.go index 0cf21c67..4e89c24b 100644 --- a/store/keychain/keychain.go +++ b/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 diff --git a/store/keychain/keychain_darwin.go b/store/keychain/keychain_darwin.go index 744da1de..6f002922 100644 --- a/store/keychain/keychain_darwin.go +++ b/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) { diff --git a/store/keychain/keychain_darwin_test.go b/store/keychain/keychain_darwin_test.go index 027b8366..ce0084fd 100644 --- a/store/keychain/keychain_darwin_test.go +++ b/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{ diff --git a/store/store.go b/store/store.go index 211bf505..b23481ff 100644 --- a/store/store.go +++ b/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]. //