From dc3865f8abff38abb2a071883d932f5864d40631 Mon Sep 17 00:00:00 2001
From: Alano Terblanche <18033717+Benehiko@users.noreply.github.com>
Date: Tue, 13 Oct 2026 16:58:37 +0200
Subject: [PATCH 1/4] fix(keychain): reject empty secrets on Linux and Windows

Save and Upsert on the Linux secret service and the Windows credential manager now return ErrEmptySecret for an empty value, matching macOS. On Linux the check runs before connecting to the secret service, so an empty value never triggers an unlock prompt.

Signed-off-by: Alano Terblanche <18033717+Benehiko@users.noreply.github.com>
---
 store/keychain/keychain.go            |  4 ++--
 store/keychain/keychain_linux.go      | 15 +++++++------
 store/keychain/keychain_linux_test.go | 16 ++++++++++++++
 store/keychain/keychain_test.go       | 31 +++++++++++++++++++++++++++
 store/keychain/keychain_windows.go    |  3 +++
 store/store.go                        | 12 ++++++-----
 6 files changed, 68 insertions(+), 13 deletions(-)

diff --git a/store/keychain/keychain.go b/store/keychain/keychain.go
index 4e89c24b..8ec8a2d9 100644
--- a/store/keychain/keychain.go
+++ b/store/keychain/keychain.go
@@ -49,8 +49,8 @@ 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.
+// one, so every platform refuses it instead of reporting a success that changed
+// nothing.
 var ErrEmptySecret = errors.New("keychain secret value must not be empty")
 
 type (
diff --git a/store/keychain/keychain_linux.go b/store/keychain/keychain_linux.go
index bfee6b7b..eb48e4e0 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 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..90085231 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{}), ErrEmptySecret)
+	require.ErrorIs(t, ks.Upsert(t.Context(), id, &emptySecret{value: []byte{}}), 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..3bdf542b 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), ErrEmptySecret)
+			require.ErrorIs(t, ks.Upsert(t.Context(), id, secret), 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..faa7102a 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, ErrEmptySecret
+	}
 
 	encoder := unicode.UTF16(unicode.LittleEndian, unicode.IgnoreBOM).NewEncoder()
 	blob, _, err := transform.Bytes(encoder, data)
diff --git a/store/store.go b/store/store.go
index b23481ff..ba6bdebe 100644
--- a/store/store.go
+++ b/store/store.go
@@ -86,16 +86,18 @@ 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
+	// The keychain stores reject 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.
 	//
+	// The keychain stores reject an empty secret value with
+	// keychain.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].
 	//

From ebd5c171075eafd56116f5a555215cf6cdad223c Mon Sep 17 00:00:00 2001
From: Alano Terblanche <18033717+Benehiko@users.noreply.github.com>
Date: Tue, 13 Oct 2026 17:00:56 +0200
Subject: [PATCH 2/4] fix(posixage): reject empty secrets

Move ErrEmptySecret to the store package so every store can return it, keeping keychain.ErrEmptySecret as an alias. posixage Save and Upsert now return it for an empty value before taking the lock or prompting for encryption keys.

Signed-off-by: Alano Terblanche <18033717+Benehiko@users.noreply.github.com>
---
 store/errors.go              | 12 +++++++++-
 store/keychain/keychain.go   |  7 ++----
 store/posixage/store.go      | 14 ++++++++----
 store/posixage/store_test.go | 44 ++++++++++++++++++++++++++++++++++++
 store/store.go               |  6 ++---
 5 files changed, 68 insertions(+), 15 deletions(-)

diff --git a/store/errors.go b/store/errors.go
index 5b02f07b..00dbc5de 100644
--- a/store/errors.go
+++ b/store/errors.go
@@ -14,6 +14,16 @@
 
 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. The macOS Keychain ignores an empty value on update and keeps the old
+// one, so every store refuses it instead of reporting a success that changed
+// nothing.
+var ErrEmptySecret = errors.New("secret value must not be empty")
diff --git a/store/keychain/keychain.go b/store/keychain/keychain.go
index 8ec8a2d9..be8e0684 100644
--- a/store/keychain/keychain.go
+++ b/store/keychain/keychain.go
@@ -47,11 +47,8 @@ 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 every platform refuses it instead of reporting a success that changed
-// nothing.
-var ErrEmptySecret = errors.New("keychain secret value must not be empty")
+// ErrEmptySecret is [store.ErrEmptySecret].
+var ErrEmptySecret = store.ErrEmptySecret
 
 type (
 	Option            interface{ apply(any) error }
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 ba6bdebe..355d9dfa 100644
--- a/store/store.go
+++ b/store/store.go
@@ -86,13 +86,11 @@ type Store interface {
 	GetAllMetadata(ctx context.Context) (map[ID]Secret, error)
 	// Save persists credentials from the store.
 	//
-	// The keychain stores reject 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.
 	//
-	// The keychain stores reject an empty secret value with
-	// keychain.ErrEmptySecret.
+	// 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. Another store instance or

From e08927e54f0361a2e8e410b6f53447393e4d9eda Mon Sep 17 00:00:00 2001
From: Alano Terblanche <18033717+Benehiko@users.noreply.github.com>
Date: Tue, 13 Oct 2026 17:02:18 +0200
Subject: [PATCH 3/4] docs(store): shorten ErrEmptySecret godoc

Signed-off-by: Alano Terblanche <18033717+Benehiko@users.noreply.github.com>
---
 store/errors.go | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/store/errors.go b/store/errors.go
index 00dbc5de..1eb885ee 100644
--- a/store/errors.go
+++ b/store/errors.go
@@ -23,7 +23,5 @@ import (
 var ErrCredentialNotFound = secrets.ErrNotFound
 
 // 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 every store refuses it instead of reporting a success that changed
-// nothing.
+// empty.
 var ErrEmptySecret = errors.New("secret value must not be empty")

From fadc4c405e42f65244d33b4e9ac43691de1d3f5d Mon Sep 17 00:00:00 2001
From: Alano Terblanche <18033717+Benehiko@users.noreply.github.com>
Date: Tue, 13 Oct 2026 17:03:34 +0200
Subject: [PATCH 4/4] refactor(keychain): use store.ErrEmptySecret directly

Signed-off-by: Alano Terblanche <18033717+Benehiko@users.noreply.github.com>
---
 store/keychain/keychain.go             | 3 ---
 store/keychain/keychain_darwin.go      | 6 +++---
 store/keychain/keychain_darwin_test.go | 8 ++++----
 store/keychain/keychain_linux.go       | 2 +-
 store/keychain/keychain_linux_test.go  | 4 ++--
 store/keychain/keychain_test.go        | 4 ++--
 store/keychain/keychain_windows.go     | 2 +-
 7 files changed, 13 insertions(+), 16 deletions(-)

diff --git a/store/keychain/keychain.go b/store/keychain/keychain.go
index be8e0684..0cf21c67 100644
--- a/store/keychain/keychain.go
+++ b/store/keychain/keychain.go
@@ -47,9 +47,6 @@ var ErrCollectionLocked = errors.New("keychain collection is locked")
 // existing item in place.
 var ErrDuplicateItem = errors.New("keychain item already exists")
 
-// ErrEmptySecret is [store.ErrEmptySecret].
-var ErrEmptySecret = store.ErrEmptySecret
-
 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 eb48e4e0..d070ad82 100644
--- a/store/keychain/keychain_linux.go
+++ b/store/keychain/keychain_linux.go
@@ -544,7 +544,7 @@ func (k *keychainStore[T]) Save(ctx context.Context, id store.ID, secret store.S
 	}
 	defer clear(value)
 	if len(value) == 0 {
-		return ErrEmptySecret
+		return store.ErrEmptySecret
 	}
 
 	service, err := operationService(ctx)
diff --git a/store/keychain/keychain_linux_test.go b/store/keychain/keychain_linux_test.go
index 90085231..6881c19a 100644
--- a/store/keychain/keychain_linux_test.go
+++ b/store/keychain/keychain_linux_test.go
@@ -292,8 +292,8 @@ func TestKeychainSaveRejectsEmptySecret(t *testing.T) {
 	fake.opened.Store(0)
 	id := store.MustParseID("com.test.test/test/empty")
 
-	require.ErrorIs(t, ks.Save(t.Context(), id, &emptySecret{}), ErrEmptySecret)
-	require.ErrorIs(t, ks.Upsert(t.Context(), id, &emptySecret{value: []byte{}}), ErrEmptySecret)
+	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)
diff --git a/store/keychain/keychain_test.go b/store/keychain/keychain_test.go
index 3bdf542b..4c1a2fd6 100644
--- a/store/keychain/keychain_test.go
+++ b/store/keychain/keychain_test.go
@@ -375,8 +375,8 @@ func TestKeychain(t *testing.T) {
 		})
 
 		for _, secret := range []store.Secret{&emptySecret{}, &emptySecret{value: []byte{}}} {
-			require.ErrorIs(t, ks.Save(t.Context(), id, secret), ErrEmptySecret)
-			require.ErrorIs(t, ks.Upsert(t.Context(), id, secret), ErrEmptySecret)
+			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)
diff --git a/store/keychain/keychain_windows.go b/store/keychain/keychain_windows.go
index faa7102a..7233c17a 100644
--- a/store/keychain/keychain_windows.go
+++ b/store/keychain/keychain_windows.go
@@ -62,7 +62,7 @@ func encodeSecret(secret store.Secret) ([]byte, error) {
 	}
 	defer clear(data)
 	if len(data) == 0 {
-		return nil, ErrEmptySecret
+		return nil, store.ErrEmptySecret
 	}
 
 	encoder := unicode.UTF16(unicode.LittleEndian, unicode.IgnoreBOM).NewEncoder()