Skip to content

Commit b2b5356

Browse files
BenehikoBenehiko
authored
Merge pull request #485 from docker/feat/store/upsert
feat: upsert credential support
2 parents 75256c1 + cf49a6d commit b2b5356

10 files changed

Lines changed: 254 additions & 0 deletions

File tree

‎plugins/pass/teststore/teststore.go‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,7 @@ type MockStore struct {
3333
errDelete error
3434
errGet error
3535
errFilter error
36+
errUpsert error
3637
store map[store.ID]store.Secret
3738
}
3839

@@ -74,6 +75,12 @@ func WithStoreDeleteErr(err error) Option {
7475
}
7576
}
7677

78+
func WithStoreUpsertErr(err error) Option {
79+
return func(m *MockStore) {
80+
m.errUpsert = err
81+
}
82+
}
83+
7784
func WithStore(store map[store.ID]store.Secret) Option {
7885
return func(m *MockStore) {
7986
m.store = store
@@ -127,6 +134,17 @@ func (m *MockStore) Save(_ context.Context, id store.ID, secret store.Secret) er
127134
return nil
128135
}
129136

137+
func (m *MockStore) Upsert(_ context.Context, id store.ID, secret store.Secret) error {
138+
m.lock.Lock()
139+
defer m.lock.Unlock()
140+
if m.errUpsert != nil {
141+
return m.errUpsert
142+
}
143+
144+
m.store[id] = secret
145+
return nil
146+
}
147+
130148
func (m *MockStore) Filter(_ context.Context, pattern store.Pattern) (map[store.ID]store.Secret, error) {
131149
m.lock.Lock()
132150
defer m.lock.Unlock()

‎store/keychain/keychain_darwin.go‎

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@ import (
1919
"errors"
2020
"fmt"
2121
"maps"
22+
"sync"
2223

2324
kc "github.com/docker/secrets-engine/store/keychain/internal/go-keychain"
2425

@@ -31,6 +32,7 @@ var (
3132
)
3233

3334
type keychainStore[T store.Secret] struct {
35+
mu sync.Mutex
3436
serviceGroup string
3537
serviceName string
3638
factory store.Factory[T]
@@ -198,6 +200,21 @@ func (k *keychainStore[T]) Save(_ context.Context, id store.ID, secret store.Sec
198200
return mapKeychainError(kc.AddItem(item))
199201
}
200202

203+
// Upsert atomically replaces a credential in the macOS Keychain.
204+
//
205+
// The macOS Keychain does not allow overwriting an existing item via AddItem,
206+
// so Upsert holds a mutex and performs a Delete followed by a Save to ensure
207+
// no concurrent Upsert can interleave between the two operations.
208+
func (k *keychainStore[T]) Upsert(ctx context.Context, id store.ID, secret store.Secret) error {
209+
k.mu.Lock()
210+
defer k.mu.Unlock()
211+
212+
if err := k.Delete(ctx, id); err != nil {
213+
return err
214+
}
215+
return k.Save(ctx, id, secret)
216+
}
217+
201218
func (k *keychainStore[T]) Filter(ctx context.Context, pattern store.Pattern) (map[store.ID]store.Secret, error) {
202219
// Note: Filter on macOS cannot filter by generic attributes and thus we
203220
// cannot split the ID and store it in the keychain as parts for later

‎store/keychain/keychain_darwin_test.go‎

Lines changed: 78 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,8 @@ import (
2525
"github.com/stretchr/testify/assert"
2626
"github.com/stretchr/testify/require"
2727

28+
kc "github.com/docker/secrets-engine/store/keychain/internal/go-keychain"
29+
2830
"github.com/docker/secrets-engine/store"
2931
"github.com/docker/secrets-engine/store/mocks"
3032
)
@@ -150,6 +152,82 @@ func TestMacosKeychain(t *testing.T) {
150152
})
151153
}
152154

155+
func TestUpsert(t *testing.T) {
156+
var (
157+
serviceName = uuid.NewString()
158+
serviceGroup = "com.test.testing"
159+
)
160+
ks := keychainStore[*mocks.MockCredential]{
161+
serviceGroup: serviceGroup,
162+
serviceName: serviceName,
163+
factory: func(_ context.Context, _ store.ID) *mocks.MockCredential {
164+
return &mocks.MockCredential{}
165+
},
166+
}
167+
168+
t.Run("inserts when credential does not exist", func(t *testing.T) {
169+
id := store.MustParseID(serviceGroup + "/" + serviceName + "/" + uuid.NewString())
170+
t.Cleanup(func() {
171+
assert.NoError(t, ks.Delete(t.Context(), id))
172+
})
173+
174+
secret := &mocks.MockCredential{
175+
Username: "alice",
176+
Password: "alice-password",
177+
}
178+
require.NoError(t, ks.Upsert(t.Context(), id, secret))
179+
180+
got, err := ks.Get(t.Context(), id)
181+
require.NoError(t, err)
182+
actual := got.(*mocks.MockCredential)
183+
actual.Attributes = nil
184+
assert.Equal(t, secret.Username, actual.Username)
185+
assert.Equal(t, secret.Password, actual.Password)
186+
})
187+
188+
t.Run("overwrites an existing credential", func(t *testing.T) {
189+
id := store.MustParseID(serviceGroup + "/" + serviceName + "/" + uuid.NewString())
190+
t.Cleanup(func() {
191+
assert.NoError(t, ks.Delete(t.Context(), id))
192+
})
193+
194+
original := &mocks.MockCredential{
195+
Username: "bob",
196+
Password: "original-password",
197+
}
198+
require.NoError(t, ks.Save(t.Context(), id, original))
199+
200+
updated := &mocks.MockCredential{
201+
Username: "bob",
202+
Password: "updated-password",
203+
}
204+
require.NoError(t, ks.Upsert(t.Context(), id, updated))
205+
206+
got, err := ks.Get(t.Context(), id)
207+
require.NoError(t, err)
208+
actual := got.(*mocks.MockCredential)
209+
actual.Attributes = nil
210+
assert.Equal(t, updated.Username, actual.Username)
211+
assert.Equal(t, updated.Password, actual.Password)
212+
})
213+
214+
t.Run("save returns duplicate item error when credential already exists", func(t *testing.T) {
215+
id := store.MustParseID(serviceGroup + "/" + serviceName + "/" + uuid.NewString())
216+
t.Cleanup(func() {
217+
assert.NoError(t, ks.Delete(t.Context(), id))
218+
})
219+
220+
secret := &mocks.MockCredential{
221+
Username: "charlie",
222+
Password: "charlie-password",
223+
}
224+
require.NoError(t, ks.Save(t.Context(), id, secret))
225+
226+
err := ks.Save(t.Context(), id, secret)
227+
require.ErrorIs(t, err, kc.ErrorDuplicateItem)
228+
})
229+
}
230+
153231
func TestConvertAttributes(t *testing.T) {
154232
t.Run("can convert attributes map into map of any", func(t *testing.T) {
155233
attributes := map[string]any{

‎store/keychain/keychain_linux.go‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -339,6 +339,10 @@ func (k *keychainStore[T]) Save(_ context.Context, id store.ID, secret store.Sec
339339
return nil
340340
}
341341

342+
func (k *keychainStore[T]) Upsert(ctx context.Context, id store.ID, secret store.Secret) error {
343+
return k.Save(ctx, id, secret)
344+
}
345+
342346
//gocyclo:ignore
343347
func (k *keychainStore[T]) Filter(ctx context.Context, pattern store.Pattern) (map[store.ID]store.Secret, error) {
344348
service, err := kc.NewService()

‎store/keychain/keychain_test.go‎

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -366,6 +366,52 @@ func TestKeychain(t *testing.T) {
366366
require.ErrorContains(t, err, "i am failing on purpose")
367367
})
368368

369+
t.Run("upsert inserts when credential does not exist", func(t *testing.T) {
370+
ks := setupKeychain(t, nil)
371+
id := store.MustParseID("com.test.test/test/upsert-insert")
372+
creds := &mocks.MockCredential{
373+
Username: "alice",
374+
Password: "alice-password",
375+
}
376+
t.Cleanup(func() {
377+
require.NoError(t, ks.Delete(context.Background(), id))
378+
})
379+
require.NoError(t, ks.Upsert(t.Context(), id, creds))
380+
381+
secret, err := ks.Get(t.Context(), id)
382+
require.NoError(t, err)
383+
actual := secret.(*mocks.MockCredential)
384+
actual.Attributes = nil
385+
assert.Equal(t, creds.Username, actual.Username)
386+
assert.Equal(t, creds.Password, actual.Password)
387+
})
388+
389+
t.Run("upsert overwrites an existing credential", func(t *testing.T) {
390+
ks := setupKeychain(t, nil)
391+
id := store.MustParseID("com.test.test/test/upsert-overwrite")
392+
original := &mocks.MockCredential{
393+
Username: "bob",
394+
Password: "original-password",
395+
}
396+
t.Cleanup(func() {
397+
require.NoError(t, ks.Delete(context.Background(), id))
398+
})
399+
require.NoError(t, ks.Save(t.Context(), id, original))
400+
401+
updated := &mocks.MockCredential{
402+
Username: "bob",
403+
Password: "updated-password",
404+
}
405+
require.NoError(t, ks.Upsert(t.Context(), id, updated))
406+
407+
secret, err := ks.Get(t.Context(), id)
408+
require.NoError(t, err)
409+
actual := secret.(*mocks.MockCredential)
410+
actual.Attributes = nil
411+
assert.Equal(t, updated.Username, actual.Username)
412+
assert.Equal(t, updated.Password, actual.Password)
413+
})
414+
369415
t.Run("set metadata error on getAllMetadata", func(t *testing.T) {
370416
kc := setupKeychain(t, func(_ context.Context, _ store.ID) store.Secret {
371417
return &mustUnmarshalError{}

‎store/keychain/keychain_windows.go‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -336,6 +336,10 @@ func (k *keychainStore[T]) Save(_ context.Context, id store.ID, secret store.Sec
336336
return mapWindowsCredentialError(g.Write())
337337
}
338338

339+
func (k *keychainStore[T]) Upsert(ctx context.Context, id store.ID, secret store.Secret) error {
340+
return k.Save(ctx, id, secret)
341+
}
342+
339343
func (k *keychainStore[T]) Filter(ctx context.Context, pattern store.Pattern) (map[store.ID]store.Secret, error) {
340344
// Note: there is no notion of a filter on Windows inside the wincred API.
341345
// It has no way to even filter on known attributes.

‎store/mocks/mock_store.go‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -70,6 +70,10 @@ func (m *MockStore) Save(_ context.Context, id store.ID, secret store.Secret) er
7070
return nil
7171
}
7272

73+
func (m *MockStore) Upsert(ctx context.Context, id store.ID, secret store.Secret) error {
74+
return m.Save(ctx, id, secret)
75+
}
76+
7377
func (m *MockStore) Filter(_ context.Context, pattern store.Pattern) (map[store.ID]store.Secret, error) {
7478
m.lock.Lock()
7579
defer m.lock.Unlock()

‎store/posixage/store.go‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -376,6 +376,10 @@ func (f *fileStore[T]) Save(ctx context.Context, id store.ID, s store.Secret) er
376376
return secretfile.Persist(id, f.filesystem, metadata, secrets)
377377
}
378378

379+
func (f *fileStore[T]) Upsert(ctx context.Context, id store.ID, s store.Secret) error {
380+
return f.Save(ctx, id, s)
381+
}
382+
379383
type config struct {
380384
logger logging.Logger
381385
registeredDecryptionFunc []promptCaller

‎store/posixage/store_test.go‎

Lines changed: 74 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -656,6 +656,80 @@ func TestPOSIXAge(t *testing.T) {
656656
assert.Equal(t, secret, storeSecret)
657657
})
658658

659+
t.Run("upsert inserts when credential does not exist", func(t *testing.T) {
660+
root, err := os.OpenRoot(t.TempDir())
661+
require.NoError(t, err)
662+
t.Cleanup(func() {
663+
assert.NoError(t, root.Close())
664+
})
665+
666+
masterKey := uuid.NewString()
667+
s, err := New(root,
668+
func(_ context.Context, _ store.ID) *mocks.MockCredential {
669+
return &mocks.MockCredential{}
670+
},
671+
WithLogger(&testLogger{t}),
672+
WithEncryptionCallbackFunc[EncryptionPassword](func(_ context.Context) ([]byte, error) {
673+
return []byte(masterKey), nil
674+
}),
675+
WithDecryptionCallbackFunc[DecryptionPassword](func(_ context.Context) ([]byte, error) {
676+
return []byte(masterKey), nil
677+
}),
678+
)
679+
require.NoError(t, err)
680+
681+
secret := &mocks.MockCredential{
682+
Username: uuid.NewString(),
683+
Password: uuid.NewString(),
684+
}
685+
id := secrets.MustParseID("test/something/" + uuid.NewString())
686+
require.NoError(t, s.Upsert(t.Context(), id, secret))
687+
688+
storeSecret, err := s.Get(t.Context(), id)
689+
require.NoError(t, err)
690+
assert.EqualValues(t, secret, storeSecret)
691+
})
692+
693+
t.Run("upsert overwrites an existing credential", func(t *testing.T) {
694+
root, err := os.OpenRoot(t.TempDir())
695+
require.NoError(t, err)
696+
t.Cleanup(func() {
697+
assert.NoError(t, root.Close())
698+
})
699+
700+
masterKey := uuid.NewString()
701+
s, err := New(root,
702+
func(_ context.Context, _ store.ID) *mocks.MockCredential {
703+
return &mocks.MockCredential{}
704+
},
705+
WithLogger(&testLogger{t}),
706+
WithEncryptionCallbackFunc[EncryptionPassword](func(_ context.Context) ([]byte, error) {
707+
return []byte(masterKey), nil
708+
}),
709+
WithDecryptionCallbackFunc[DecryptionPassword](func(_ context.Context) ([]byte, error) {
710+
return []byte(masterKey), nil
711+
}),
712+
)
713+
require.NoError(t, err)
714+
715+
original := &mocks.MockCredential{
716+
Username: uuid.NewString(),
717+
Password: uuid.NewString(),
718+
}
719+
id := secrets.MustParseID("test/something/" + uuid.NewString())
720+
require.NoError(t, s.Save(t.Context(), id, original))
721+
722+
updated := &mocks.MockCredential{
723+
Username: uuid.NewString(),
724+
Password: uuid.NewString(),
725+
}
726+
require.NoError(t, s.Upsert(t.Context(), id, updated))
727+
728+
storeSecret, err := s.Get(t.Context(), id)
729+
require.NoError(t, err)
730+
assert.EqualValues(t, updated, storeSecret)
731+
})
732+
659733
t.Run("an error on encryption callbackFunc is propagated on save", func(t *testing.T) {
660734
root, err := os.OpenRoot(t.TempDir())
661735
require.NoError(t, err)

‎store/store.go‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -84,6 +84,11 @@ type Store interface {
8484
GetAllMetadata(ctx context.Context) (map[ID]Secret, error)
8585
// Save persists credentials from the store.
8686
Save(ctx context.Context, id ID, secret Secret) error
87+
// Upsert atomically replaces an existing credential or inserts a new one.
88+
// On stores that do not support overwriting (e.g. macOS Keychain), it
89+
// deletes the existing credential and then saves the new one under a mutex
90+
// to ensure the two operations are not interleaved.
91+
Upsert(ctx context.Context, id ID, secret Secret) error
8792
// Filter returns a map of secrets based on a [Pattern].
8893
//
8994
// Secrets returned will have both [Secret.SetMetadata] and [Secret.Unmarshal]

0 commit comments

Comments
 (0)