fix(store): reject empty secrets on Linux, Windows and posixage

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")

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

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)

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)

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

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

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{}

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)

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

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")
+}

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].
 	//