From 35beb5ecf0362c18f2f495caf9306fb15b7c037a Mon Sep 17 00:00:00 2001 From: Christian Dupuis Date: Fri, 9 Oct 2026 15:36:37 +0200 Subject: [PATCH 1/3] 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]. // From 69f8abca3739decfefb6fab22f8f9318c570163a Mon Sep 17 00:00:00 2001 From: Alano Terblanche <18033717+Benehiko@users.noreply.github.com> Date: Tue, 13 Oct 2026 15:09:42 +0200 Subject: [PATCH 2/3] fix(keychain): address review feedback on Upsert Marshal the secret once and reuse it for the update and the add, so a secret whose Marshal returns its backing buffer is not stored as zeros. Drop the update retry after a duplicate-item error and return the error to the caller. Reject empty secret values in Upsert with ErrEmptySecret, since the keychain ignores an empty value on update. Signed-off-by: Alano Terblanche <18033717+Benehiko@users.noreply.github.com> --- store/keychain/keychain.go | 2 + store/keychain/keychain_darwin.go | 31 ++++++------ store/keychain/keychain_darwin_test.go | 69 ++++++++++++++++++++++++++ 3 files changed, 87 insertions(+), 15 deletions(-) diff --git a/store/keychain/keychain.go b/store/keychain/keychain.go index 0cf21c67..4095a084 100644 --- a/store/keychain/keychain.go +++ b/store/keychain/keychain.go @@ -47,6 +47,8 @@ var ErrCollectionLocked = errors.New("keychain collection is locked") // existing item in place. var ErrDuplicateItem = errors.New("keychain item already exists") +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 759c5ba3..4e22cd34 100644 --- a/store/keychain/keychain_darwin.go +++ b/store/keychain/keychain_darwin.go @@ -186,6 +186,10 @@ func (k *keychainStore[T]) Save(_ context.Context, id store.ID, secret store.Sec return err } defer clear(data) + 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. @@ -216,31 +220,28 @@ func (k *keychainStore[T]) genericMetadata(id store.ID, secret store.Secret) map // 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 { +func (k *keychainStore[T]) Upsert(_ context.Context, id store.ID, secret store.Secret) error { k.mu.Lock() defer k.mu.Unlock() - err := k.update(id, secret) - if !errors.Is(err, store.ErrCredentialNotFound) { + data, err := secret.Marshal() + if err != nil { return err } - 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) + defer clear(data) + if len(data) == 0 { + return ErrEmptySecret } - return err -} -func (k *keychainStore[T]) update(id store.ID, secret store.Secret) error { - data, err := secret.Marshal() - if err != nil { + err = k.update(id, secret, data) + if !errors.Is(err, store.ErrCredentialNotFound) { return err } - defer clear(data) + return k.add(id, secret, data) +} - // The query names only the attributes that identify the item; the match - // and return options of newKeychainItem belong to SecItemCopyMatching. +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) diff --git a/store/keychain/keychain_darwin_test.go b/store/keychain/keychain_darwin_test.go index 9b76b155..00949a48 100644 --- a/store/keychain/keychain_darwin_test.go +++ b/store/keychain/keychain_darwin_test.go @@ -247,6 +247,48 @@ func TestUpsert(t *testing.T) { 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 returns duplicate item error when credential already exists", func(t *testing.T) { id := store.MustParseID(serviceGroup + "/" + serviceName + "/" + uuid.NewString()) t.Cleanup(func() { @@ -264,6 +306,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{ From 6057f1a12541f16956ebed9b58a0a9a41bde8449 Mon Sep 17 00:00:00 2001 From: Alano Terblanche <18033717+Benehiko@users.noreply.github.com> Date: Tue, 13 Oct 2026 16:33:53 +0200 Subject: [PATCH 3/3] fix(keychain): reject empty secrets in Save and drop test sleep Save now returns ErrEmptySecret for an empty value, matching Upsert. The in-place update test checks that a comment set outside the store survives Upsert instead of comparing timestamps, so it no longer sleeps. Document ErrEmptySecret and the cross-process ErrDuplicateItem case on Save and Upsert. Signed-off-by: Alano Terblanche <18033717+Benehiko@users.noreply.github.com> --- store/keychain/keychain.go | 4 +++ store/keychain/keychain_darwin.go | 7 +++++ store/keychain/keychain_darwin_test.go | 38 ++++++++++++++++++++++---- store/store.go | 9 +++++- 4 files changed, 51 insertions(+), 7 deletions(-) diff --git a/store/keychain/keychain.go b/store/keychain/keychain.go index 4095a084..4e89c24b 100644 --- a/store/keychain/keychain.go +++ b/store/keychain/keychain.go @@ -47,6 +47,10 @@ 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 ( diff --git a/store/keychain/keychain_darwin.go b/store/keychain/keychain_darwin.go index 4e22cd34..6f002922 100644 --- a/store/keychain/keychain_darwin.go +++ b/store/keychain/keychain_darwin.go @@ -186,6 +186,9 @@ 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) } @@ -220,6 +223,10 @@ func (k *keychainStore[T]) genericMetadata(id store.ID, secret store.Secret) map // 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. +// +// 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() diff --git a/store/keychain/keychain_darwin_test.go b/store/keychain/keychain_darwin_test.go index 00949a48..ce0084fd 100644 --- a/store/keychain/keychain_darwin_test.go +++ b/store/keychain/keychain_darwin_test.go @@ -20,13 +20,13 @@ import ( "bytes" "context" "testing" - "time" "github.com/google/uuid" "github.com/stretchr/testify/assert" "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" ) @@ -221,11 +221,18 @@ func TestUpsert(t *testing.T) { 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) - - // Keychain timestamps have one-second resolution. - time.Sleep(1100 * time.Millisecond) + require.Equal(t, "set-outside-the-store", before.Comment) updated := &mocks.MockCredential{ Username: "dana", @@ -236,8 +243,7 @@ func TestUpsert(t *testing.T) { 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, "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) @@ -289,6 +295,26 @@ func TestUpsert(t *testing.T) { 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() { diff --git a/store/store.go b/store/store.go index 33010f53..b23481ff 100644 --- a/store/store.go +++ b/store/store.go @@ -85,10 +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 the macOS Keychain an existing item is updated in place, keeping the - // applications the user allowed to access it. + // 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]. //