From 6291546901f14ff5a51e4cfb43ac21b7a0d9cc63 Mon Sep 17 00:00:00 2001 From: Alano Terblanche <18033717+Benehiko@users.noreply.github.com> Date: Fri, 6 Mar 2026 12:15:49 +0100 Subject: [PATCH 1/3] feat: upsert credential support On macOS the credentials cannot be stored if one already exists. But we didn't want to clobber the credentials on Save. A new `Upsert` function is added to clobber credentials purposefully. macOS is the only operating system that requires this to be atomic, but others might require it too. Signed-off-by: Alano Terblanche <18033717+Benehiko@users.noreply.github.com> --- plugins/pass/teststore/teststore.go | 18 ++++++++++++++++++ store/keychain/keychain_darwin.go | 17 +++++++++++++++++ store/keychain/keychain_linux.go | 4 ++++ store/keychain/keychain_windows.go | 4 ++++ store/mocks/mock_store.go | 4 ++++ store/posixage/store.go | 4 ++++ store/store.go | 5 +++++ 7 files changed, 56 insertions(+) diff --git a/plugins/pass/teststore/teststore.go b/plugins/pass/teststore/teststore.go index f8a2a4e8..84be5da8 100644 --- a/plugins/pass/teststore/teststore.go +++ b/plugins/pass/teststore/teststore.go @@ -33,6 +33,7 @@ type MockStore struct { errDelete error errGet error errFilter error + errUpsert error store map[store.ID]store.Secret } @@ -74,6 +75,12 @@ func WithStoreDeleteErr(err error) Option { } } +func WithStoreUpsertErr(err error) Option { + return func(m *MockStore) { + m.errUpsert = err + } +} + func WithStore(store map[store.ID]store.Secret) Option { return func(m *MockStore) { m.store = store @@ -127,6 +134,17 @@ func (m *MockStore) Save(_ context.Context, id store.ID, secret store.Secret) er return nil } +func (m *MockStore) Upsert(_ context.Context, id store.ID, secret store.Secret) error { + m.lock.Lock() + defer m.lock.Unlock() + if m.errUpsert != nil { + return m.errUpsert + } + + m.store[id] = secret + return nil +} + func (m *MockStore) Filter(_ context.Context, pattern store.Pattern) (map[store.ID]store.Secret, error) { m.lock.Lock() defer m.lock.Unlock() diff --git a/store/keychain/keychain_darwin.go b/store/keychain/keychain_darwin.go index 4e7070df..d59779be 100644 --- a/store/keychain/keychain_darwin.go +++ b/store/keychain/keychain_darwin.go @@ -19,6 +19,7 @@ import ( "errors" "fmt" "maps" + "sync" kc "github.com/docker/secrets-engine/store/keychain/internal/go-keychain" @@ -31,6 +32,7 @@ var ( ) type keychainStore[T store.Secret] struct { + mu sync.Mutex serviceGroup string serviceName string factory store.Factory[T] @@ -198,6 +200,21 @@ func (k *keychainStore[T]) Save(_ context.Context, id store.ID, secret store.Sec return mapKeychainError(kc.AddItem(item)) } +// Upsert atomically replaces a credential in the macOS Keychain. +// +// 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 { + k.mu.Lock() + defer k.mu.Unlock() + + if err := k.Delete(ctx, id); err != nil { + return err + } + return k.Save(ctx, id, secret) +} + func (k *keychainStore[T]) Filter(ctx context.Context, pattern store.Pattern) (map[store.ID]store.Secret, error) { // Note: Filter on macOS cannot filter by generic attributes and thus we // cannot split the ID and store it in the keychain as parts for later diff --git a/store/keychain/keychain_linux.go b/store/keychain/keychain_linux.go index d4a5118b..11397ea7 100644 --- a/store/keychain/keychain_linux.go +++ b/store/keychain/keychain_linux.go @@ -339,6 +339,10 @@ func (k *keychainStore[T]) Save(_ context.Context, id store.ID, secret store.Sec return nil } +func (k *keychainStore[T]) Upsert(ctx context.Context, id store.ID, secret store.Secret) error { + return k.Save(ctx, id, secret) +} + //gocyclo:ignore func (k *keychainStore[T]) Filter(ctx context.Context, pattern store.Pattern) (map[store.ID]store.Secret, error) { service, err := kc.NewService() diff --git a/store/keychain/keychain_windows.go b/store/keychain/keychain_windows.go index 7c9e9e8c..4642334a 100644 --- a/store/keychain/keychain_windows.go +++ b/store/keychain/keychain_windows.go @@ -336,6 +336,10 @@ func (k *keychainStore[T]) Save(_ context.Context, id store.ID, secret store.Sec return mapWindowsCredentialError(g.Write()) } +func (k *keychainStore[T]) Upsert(ctx context.Context, id store.ID, secret store.Secret) error { + return k.Save(ctx, id, secret) +} + func (k *keychainStore[T]) Filter(ctx context.Context, pattern store.Pattern) (map[store.ID]store.Secret, error) { // Note: there is no notion of a filter on Windows inside the wincred API. // It has no way to even filter on known attributes. diff --git a/store/mocks/mock_store.go b/store/mocks/mock_store.go index f1bd969a..5e473c1f 100644 --- a/store/mocks/mock_store.go +++ b/store/mocks/mock_store.go @@ -70,6 +70,10 @@ func (m *MockStore) Save(_ context.Context, id store.ID, secret store.Secret) er return nil } +func (m *MockStore) Upsert(ctx context.Context, id store.ID, secret store.Secret) error { + return m.Save(ctx, id, secret) +} + func (m *MockStore) Filter(_ context.Context, pattern store.Pattern) (map[store.ID]store.Secret, error) { m.lock.Lock() defer m.lock.Unlock() diff --git a/store/posixage/store.go b/store/posixage/store.go index bead1de3..fc5d27c4 100644 --- a/store/posixage/store.go +++ b/store/posixage/store.go @@ -376,6 +376,10 @@ func (f *fileStore[T]) Save(ctx context.Context, id store.ID, s store.Secret) er return secretfile.Persist(id, f.filesystem, metadata, secrets) } +func (f *fileStore[T]) Upsert(ctx context.Context, id store.ID, s store.Secret) error { + return f.Save(ctx, id, s) +} + type config struct { logger logging.Logger registeredDecryptionFunc []promptCaller diff --git a/store/store.go b/store/store.go index 1381171e..bafca00b 100644 --- a/store/store.go +++ b/store/store.go @@ -84,6 +84,11 @@ type Store interface { GetAllMetadata(ctx context.Context) (map[ID]Secret, error) // 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. + Upsert(ctx context.Context, id ID, secret Secret) error // Filter returns a map of secrets based on a [Pattern]. // // Secrets returned will have both [Secret.SetMetadata] and [Secret.Unmarshal] From 87814e7e534efd39cc0782ed98289e610b0bffdf Mon Sep 17 00:00:00 2001 From: Alano Terblanche <18033717+Benehiko@users.noreply.github.com> Date: Fri, 6 Mar 2026 14:16:32 +0100 Subject: [PATCH 2/3] test: darwin keychain upsert Signed-off-by: Alano Terblanche <18033717+Benehiko@users.noreply.github.com> --- store/keychain/keychain_darwin_test.go | 78 ++++++++++++++++++++++++++ 1 file changed, 78 insertions(+) diff --git a/store/keychain/keychain_darwin_test.go b/store/keychain/keychain_darwin_test.go index 89e7d86c..4f5b59f2 100644 --- a/store/keychain/keychain_darwin_test.go +++ b/store/keychain/keychain_darwin_test.go @@ -25,6 +25,8 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + kc "github.com/docker/secrets-engine/store/keychain/internal/go-keychain" + "github.com/docker/secrets-engine/store" "github.com/docker/secrets-engine/store/mocks" ) @@ -150,6 +152,82 @@ func TestMacosKeychain(t *testing.T) { }) } +func TestUpsert(t *testing.T) { + var ( + serviceName = uuid.NewString() + serviceGroup = "com.test.testing" + ) + ks := keychainStore[*mocks.MockCredential]{ + serviceGroup: serviceGroup, + serviceName: serviceName, + factory: func(_ context.Context, _ store.ID) *mocks.MockCredential { + return &mocks.MockCredential{} + }, + } + + t.Run("inserts when credential does not exist", func(t *testing.T) { + id := store.MustParseID(serviceGroup + "/" + serviceName + "/" + uuid.NewString()) + t.Cleanup(func() { + assert.NoError(t, ks.Delete(t.Context(), id)) + }) + + secret := &mocks.MockCredential{ + Username: "alice", + Password: "alice-password", + } + require.NoError(t, ks.Upsert(t.Context(), id, secret)) + + got, err := ks.Get(t.Context(), id) + require.NoError(t, err) + actual := got.(*mocks.MockCredential) + actual.Attributes = nil + assert.Equal(t, secret.Username, actual.Username) + assert.Equal(t, secret.Password, actual.Password) + }) + + t.Run("overwrites an existing credential", func(t *testing.T) { + id := store.MustParseID(serviceGroup + "/" + serviceName + "/" + uuid.NewString()) + t.Cleanup(func() { + assert.NoError(t, ks.Delete(t.Context(), id)) + }) + + original := &mocks.MockCredential{ + Username: "bob", + Password: "original-password", + } + require.NoError(t, ks.Save(t.Context(), id, original)) + + updated := &mocks.MockCredential{ + Username: "bob", + Password: "updated-password", + } + require.NoError(t, ks.Upsert(t.Context(), id, updated)) + + got, err := ks.Get(t.Context(), id) + require.NoError(t, err) + actual := got.(*mocks.MockCredential) + actual.Attributes = nil + assert.Equal(t, updated.Username, actual.Username) + assert.Equal(t, updated.Password, actual.Password) + }) + + 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() { + assert.NoError(t, ks.Delete(t.Context(), id)) + }) + + secret := &mocks.MockCredential{ + Username: "charlie", + Password: "charlie-password", + } + require.NoError(t, ks.Save(t.Context(), id, secret)) + + err := ks.Save(t.Context(), id, secret) + require.ErrorIs(t, err, kc.ErrorDuplicateItem) + }) +} + func TestConvertAttributes(t *testing.T) { t.Run("can convert attributes map into map of any", func(t *testing.T) { attributes := map[string]any{ From cf49a6d74c05fd35f3ff11e0ee85e474ec65637b Mon Sep 17 00:00:00 2001 From: Alano Terblanche <18033717+Benehiko@users.noreply.github.com> Date: Fri, 6 Mar 2026 14:30:51 +0100 Subject: [PATCH 3/3] test: keychain upsert Signed-off-by: Alano Terblanche <18033717+Benehiko@users.noreply.github.com> --- store/keychain/keychain_test.go | 46 ++++++++++++++++++++ store/posixage/store_test.go | 74 +++++++++++++++++++++++++++++++++ 2 files changed, 120 insertions(+) diff --git a/store/keychain/keychain_test.go b/store/keychain/keychain_test.go index 027ee8b2..ec859d47 100644 --- a/store/keychain/keychain_test.go +++ b/store/keychain/keychain_test.go @@ -366,6 +366,52 @@ func TestKeychain(t *testing.T) { require.ErrorContains(t, err, "i am failing on purpose") }) + t.Run("upsert inserts when credential does not exist", func(t *testing.T) { + ks := setupKeychain(t, nil) + id := store.MustParseID("com.test.test/test/upsert-insert") + creds := &mocks.MockCredential{ + Username: "alice", + Password: "alice-password", + } + t.Cleanup(func() { + require.NoError(t, ks.Delete(context.Background(), id)) + }) + require.NoError(t, ks.Upsert(t.Context(), id, creds)) + + secret, err := ks.Get(t.Context(), id) + require.NoError(t, err) + actual := secret.(*mocks.MockCredential) + actual.Attributes = nil + assert.Equal(t, creds.Username, actual.Username) + assert.Equal(t, creds.Password, actual.Password) + }) + + t.Run("upsert overwrites an existing credential", func(t *testing.T) { + ks := setupKeychain(t, nil) + id := store.MustParseID("com.test.test/test/upsert-overwrite") + original := &mocks.MockCredential{ + Username: "bob", + Password: "original-password", + } + t.Cleanup(func() { + require.NoError(t, ks.Delete(context.Background(), id)) + }) + require.NoError(t, ks.Save(t.Context(), id, original)) + + updated := &mocks.MockCredential{ + Username: "bob", + Password: "updated-password", + } + require.NoError(t, ks.Upsert(t.Context(), id, updated)) + + secret, err := ks.Get(t.Context(), id) + require.NoError(t, err) + actual := secret.(*mocks.MockCredential) + actual.Attributes = nil + assert.Equal(t, updated.Username, actual.Username) + assert.Equal(t, updated.Password, actual.Password) + }) + t.Run("set metadata error on getAllMetadata", func(t *testing.T) { kc := setupKeychain(t, func(_ context.Context, _ store.ID) store.Secret { return &mustUnmarshalError{} diff --git a/store/posixage/store_test.go b/store/posixage/store_test.go index e9319524..f5921df5 100644 --- a/store/posixage/store_test.go +++ b/store/posixage/store_test.go @@ -656,6 +656,80 @@ func TestPOSIXAge(t *testing.T) { assert.Equal(t, secret, storeSecret) }) + t.Run("upsert inserts when credential does not exist", func(t *testing.T) { + root, err := os.OpenRoot(t.TempDir()) + require.NoError(t, err) + t.Cleanup(func() { + assert.NoError(t, root.Close()) + }) + + masterKey := uuid.NewString() + s, err := New(root, + func(_ context.Context, _ store.ID) *mocks.MockCredential { + return &mocks.MockCredential{} + }, + WithLogger(&testLogger{t}), + WithEncryptionCallbackFunc[EncryptionPassword](func(_ context.Context) ([]byte, error) { + return []byte(masterKey), nil + }), + WithDecryptionCallbackFunc[DecryptionPassword](func(_ context.Context) ([]byte, error) { + return []byte(masterKey), nil + }), + ) + require.NoError(t, err) + + secret := &mocks.MockCredential{ + Username: uuid.NewString(), + Password: uuid.NewString(), + } + id := secrets.MustParseID("test/something/" + uuid.NewString()) + require.NoError(t, s.Upsert(t.Context(), id, secret)) + + storeSecret, err := s.Get(t.Context(), id) + require.NoError(t, err) + assert.EqualValues(t, secret, storeSecret) + }) + + t.Run("upsert overwrites an existing credential", func(t *testing.T) { + root, err := os.OpenRoot(t.TempDir()) + require.NoError(t, err) + t.Cleanup(func() { + assert.NoError(t, root.Close()) + }) + + masterKey := uuid.NewString() + s, err := New(root, + func(_ context.Context, _ store.ID) *mocks.MockCredential { + return &mocks.MockCredential{} + }, + WithLogger(&testLogger{t}), + WithEncryptionCallbackFunc[EncryptionPassword](func(_ context.Context) ([]byte, error) { + return []byte(masterKey), nil + }), + WithDecryptionCallbackFunc[DecryptionPassword](func(_ context.Context) ([]byte, error) { + return []byte(masterKey), nil + }), + ) + require.NoError(t, err) + + original := &mocks.MockCredential{ + Username: uuid.NewString(), + Password: uuid.NewString(), + } + id := secrets.MustParseID("test/something/" + uuid.NewString()) + require.NoError(t, s.Save(t.Context(), id, original)) + + updated := &mocks.MockCredential{ + Username: uuid.NewString(), + Password: uuid.NewString(), + } + require.NoError(t, s.Upsert(t.Context(), id, updated)) + + storeSecret, err := s.Get(t.Context(), id) + require.NoError(t, err) + assert.EqualValues(t, updated, storeSecret) + }) + t.Run("an error on encryption callbackFunc is propagated on save", func(t *testing.T) { root, err := os.OpenRoot(t.TempDir()) require.NoError(t, err)