diff --git a/store/errors.go b/store/errors.go index 5b02f07b..1eb885ee 100644 --- a/store/errors.go +++ b/store/errors.go @@ -14,6 +14,14 @@ package store -import "github.com/docker/secrets-engine/x/secrets" +import ( + "errors" + + "github.com/docker/secrets-engine/x/secrets" +) var ErrCredentialNotFound = secrets.ErrNotFound + +// ErrEmptySecret is returned by Save and Upsert when the secret's value is +// empty. +var ErrEmptySecret = errors.New("secret value must not be empty") diff --git a/store/keychain/keychain.go b/store/keychain/keychain.go index 4e89c24b..0cf21c67 100644 --- a/store/keychain/keychain.go +++ b/store/keychain/keychain.go @@ -47,12 +47,6 @@ 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 6f002922..ebab6b5f 100644 --- a/store/keychain/keychain_darwin.go +++ b/store/keychain/keychain_darwin.go @@ -187,7 +187,7 @@ func (k *keychainStore[T]) Save(_ context.Context, id store.ID, secret store.Sec } defer clear(data) if len(data) == 0 { - return ErrEmptySecret + return store.ErrEmptySecret } return k.add(id, secret, data) } @@ -224,7 +224,7 @@ func (k *keychainStore[T]) genericMetadata(id store.ID, secret store.Secret) map // 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 +// An empty secret value returns [store.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 { @@ -237,7 +237,7 @@ func (k *keychainStore[T]) Upsert(_ context.Context, id store.ID, secret store.S } defer clear(data) if len(data) == 0 { - return ErrEmptySecret + return store.ErrEmptySecret } err = k.update(id, secret, data) diff --git a/store/keychain/keychain_darwin_test.go b/store/keychain/keychain_darwin_test.go index ce0084fd..2b01b0fa 100644 --- a/store/keychain/keychain_darwin_test.go +++ b/store/keychain/keychain_darwin_test.go @@ -287,8 +287,8 @@ func TestUpsert(t *testing.T) { }) 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) + require.ErrorIs(t, bs.Upsert(t.Context(), id, &bufferSecret{}), store.ErrEmptySecret) + require.ErrorIs(t, bs.Upsert(t.Context(), id, &bufferSecret{value: []byte{}}), store.ErrEmptySecret) got, err := bs.Get(t.Context(), id) require.NoError(t, err) @@ -308,8 +308,8 @@ func TestUpsert(t *testing.T) { 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) + require.ErrorIs(t, bs.Save(t.Context(), id, &bufferSecret{}), store.ErrEmptySecret) + require.ErrorIs(t, bs.Save(t.Context(), id, &bufferSecret{value: []byte{}}), store.ErrEmptySecret) _, err := bs.Get(t.Context(), id) require.ErrorIs(t, err, store.ErrCredentialNotFound) diff --git a/store/keychain/keychain_linux.go b/store/keychain/keychain_linux.go index bfee6b7b..d070ad82 100644 --- a/store/keychain/keychain_linux.go +++ b/store/keychain/keychain_linux.go @@ -538,6 +538,15 @@ func (k *keychainStore[T]) GetAllMetadata(ctx context.Context) (map[store.ID]sto } func (k *keychainStore[T]) Save(ctx context.Context, id store.ID, secret store.Secret) error { + value, err := secret.Marshal() + if err != nil { + return err + } + defer clear(value) + if len(value) == 0 { + return store.ErrEmptySecret + } + service, err := operationService(ctx) if err != nil { return err @@ -562,12 +571,6 @@ func (k *keychainStore[T]) Save(ctx context.Context, id store.ID, secret store.S return err } - value, err := secret.Marshal() - if err != nil { - return err - } - defer clear(value) - sessSecret, err := session.NewSecret(value) if err != nil { return err diff --git a/store/keychain/keychain_linux_test.go b/store/keychain/keychain_linux_test.go index 76ec41d5..6881c19a 100644 --- a/store/keychain/keychain_linux_test.go +++ b/store/keychain/keychain_linux_test.go @@ -284,6 +284,22 @@ func TestKeychainSaveCreatesWhenAbsent(t *testing.T) { assert.Empty(t, fake.deletedItems, "nothing to collapse") } +func TestKeychainSaveRejectsEmptySecret(t *testing.T) { + fake := &fakeService{} + withFakeService(t, fake) + + ks := setupKeychain(t, nil) + fake.opened.Store(0) + id := store.MustParseID("com.test.test/test/empty") + + require.ErrorIs(t, ks.Save(t.Context(), id, &emptySecret{}), store.ErrEmptySecret) + require.ErrorIs(t, ks.Upsert(t.Context(), id, &emptySecret{value: []byte{}}), store.ErrEmptySecret) + + assert.Zero(t, fake.opened.Load(), "must not connect to the secret service") + assert.Zero(t, fake.createCalls) + assert.Zero(t, fake.setSecretCalls) +} + // TestKeychainSaveCollapsesDuplicatesInPlace is the issue #446 regression test: // when several items already share one stable identity (the accumulated // duplicates), Save must update the first match in place — never minting a new diff --git a/store/keychain/keychain_test.go b/store/keychain/keychain_test.go index 87969703..4c1a2fd6 100644 --- a/store/keychain/keychain_test.go +++ b/store/keychain/keychain_test.go @@ -66,6 +66,21 @@ func (m *mustUnmarshalError) Unmarshal([]byte) error { return errors.New("i am failing on purpose") } +type emptySecret struct { + attributes + value []byte +} + +var _ store.Secret = &emptySecret{} + +func (e *emptySecret) Marshal() ([]byte, error) { + return e.value, nil +} + +func (e *emptySecret) Unmarshal([]byte) error { + return nil +} + func setupKeychain(t *testing.T, secretFactory func(context.Context, store.ID) store.Secret) store.Store { t.Helper() if secretFactory == nil { @@ -352,6 +367,22 @@ func TestKeychain(t *testing.T) { require.ErrorContains(t, kc.Save(t.Context(), id, &mustMarshalError{}), "i am failing on purpose") }) + t.Run("save and upsert reject an empty secret value", func(t *testing.T) { + ks := setupKeychain(t, nil) + id := store.MustParseID("com.test.test/test/empty-secret") + t.Cleanup(func() { + require.NoError(t, ks.Delete(context.Background(), id)) + }) + + for _, secret := range []store.Secret{&emptySecret{}, &emptySecret{value: []byte{}}} { + require.ErrorIs(t, ks.Save(t.Context(), id, secret), store.ErrEmptySecret) + require.ErrorIs(t, ks.Upsert(t.Context(), id, secret), store.ErrEmptySecret) + } + + _, err := ks.Get(t.Context(), id) + require.ErrorIs(t, err, store.ErrCredentialNotFound) + }) + t.Run("unmarshal error on get", func(t *testing.T) { kc := setupKeychain(t, func(_ context.Context, _ store.ID) store.Secret { return &mustUnmarshalError{} diff --git a/store/keychain/keychain_windows.go b/store/keychain/keychain_windows.go index 81fdd800..7233c17a 100644 --- a/store/keychain/keychain_windows.go +++ b/store/keychain/keychain_windows.go @@ -61,6 +61,9 @@ func encodeSecret(secret store.Secret) ([]byte, error) { return nil, err } defer clear(data) + if len(data) == 0 { + return nil, store.ErrEmptySecret + } encoder := unicode.UTF16(unicode.LittleEndian, unicode.IgnoreBOM).NewEncoder() blob, _, err := transform.Bytes(encoder, data) diff --git a/store/posixage/store.go b/store/posixage/store.go index fa5a1aa8..c3ad890d 100644 --- a/store/posixage/store.go +++ b/store/posixage/store.go @@ -330,6 +330,15 @@ func (f *fileStore[T]) GetAllMetadata(ctx context.Context) (map[store.ID]store.S } func (f *fileStore[T]) Save(ctx context.Context, id store.ID, s store.Secret) error { + secret, err := s.Marshal() + if err != nil { + return err + } + defer clear(secret) + if len(secret) == 0 { + return store.ErrEmptySecret + } + unlock, err := f.tryLock(ctx) if err != nil { return err @@ -344,11 +353,6 @@ func (f *fileStore[T]) Save(ctx context.Context, id store.ID, s store.Secret) er return err } - secret, err := s.Marshal() - if err != nil { - return err - } - defer clear(secret) metadata := s.Metadata() var secrets []secretfile.EncryptedSecret diff --git a/store/posixage/store_test.go b/store/posixage/store_test.go index 913245cf..6d0fb5b0 100644 --- a/store/posixage/store_test.go +++ b/store/posixage/store_test.go @@ -1002,3 +1002,47 @@ func TestScryptWorkFactorMigration(t *testing.T) { }) } } + +type emptySecret struct { + value []byte +} + +func (e *emptySecret) Marshal() ([]byte, error) { return e.value, nil } +func (e *emptySecret) Unmarshal([]byte) error { return nil } +func (e *emptySecret) Metadata() map[string]string { return nil } +func (e *emptySecret) SetMetadata(map[string]string) error { return nil } + +func TestSaveRejectsEmptySecret(t *testing.T) { + root, err := os.OpenRoot(t.TempDir()) + require.NoError(t, err) + t.Cleanup(func() { + assert.NoError(t, root.Close()) + }) + + prompted := 0 + s, err := New(root, + func(_ context.Context, _ store.ID) *emptySecret { + return &emptySecret{} + }, + WithLogger(&testLogger{t}), + WithEncryptionCallbackFunc[EncryptionPassword](func(_ context.Context) ([]byte, error) { + prompted++ + return []byte("password"), nil + }), + WithDecryptionCallbackFunc[DecryptionPassword](func(_ context.Context) ([]byte, error) { + return []byte("password"), nil + }), + ) + require.NoError(t, err) + id := secrets.MustParseID("test/something/" + uuid.NewString()) + + for _, secret := range []store.Secret{&emptySecret{}, &emptySecret{value: []byte{}}} { + require.ErrorIs(t, s.Save(t.Context(), id, secret), store.ErrEmptySecret) + require.ErrorIs(t, s.Upsert(t.Context(), id, secret), store.ErrEmptySecret) + } + + assert.Zero(t, prompted, "must not prompt for encryption keys") + dirs, err := onlyDirs(root.FS()) + require.NoError(t, err) + assert.Empty(t, dirs, "must not write anything") +} diff --git a/store/store.go b/store/store.go index b23481ff..355d9dfa 100644 --- a/store/store.go +++ b/store/store.go @@ -86,16 +86,16 @@ type Store interface { 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. + // An empty secret value is rejected with [ErrEmptySecret]. Save(ctx context.Context, id ID, secret Secret) error // Upsert atomically replaces an existing credential or inserts a new one. // + // An empty secret value is rejected with [ErrEmptySecret]. + // // 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. + // applications the user allowed to access it. 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]. //