refactor(keychain): introduce secretService interface and in-memory fake

store/keychain/internal/go-keychain/secretservice/secretservice.go

@@ -107,6 +107,53 @@ func (s *SecretService) Obj(path dbus.ObjectPath) dbus.BusObject {
 	return s.conn.Object(SecretServiceInterface, path)
 }
 
+// dbus interface members used by the high-level helpers below.
+//
+// https://specifications.freedesktop.org/secret-service-spec/latest/index.html
+const (
+	collectionsProperty  = "org.freedesktop.Secret.Service.Collections"
+	readAliasMethod      = "org.freedesktop.Secret.Service.ReadAlias"
+	collectionLockedProp = "org.freedesktop.Secret.Collection.Locked"
+)
+
+// Collections returns the object paths of every collection known to the secret
+// service.
+func (s *SecretService) Collections() ([]dbus.ObjectPath, error) {
+	variant, err := s.ServiceObj().GetProperty(collectionsProperty)
+	if err != nil {
+		return nil, err
+	}
+	collections, ok := variant.Value().([]dbus.ObjectPath)
+	if !ok {
+		return nil, errors.New("could not list keychain collections")
+	}
+	return collections, nil
+}
+
+// ReadAlias resolves an alias (e.g. "default") to the collection object path it
+// points at. The secret service returns the null path "/" when the alias is not
+// assigned to any collection.
+func (s *SecretService) ReadAlias(alias string) (dbus.ObjectPath, error) {
+	var path dbus.ObjectPath
+	if err := s.ServiceObj().Call(readAliasMethod, NilFlags, alias).Store(&path); err != nil {
+		return "", err
+	}
+	return path, nil
+}
+
+// IsLocked reports whether the given collection is currently locked.
+func (s *SecretService) IsLocked(collection dbus.ObjectPath) (bool, error) {
+	variant, err := s.Obj(collection).GetProperty(collectionLockedProp)
+	if err != nil {
+		return false, err
+	}
+	locked, ok := variant.Value().(bool)
+	if !ok {
+		return false, errors.New("unexpected type for collection locked property")
+	}
+	return locked, nil
+}
+
 type sessionOpenResponse struct {
 	algorithmOutput dbus.Variant
 	path            dbus.ObjectPath

store/keychain/keychain_linux.go

@@ -45,22 +45,35 @@ const (
 	//
 	// https://specifications.freedesktop.org/secret-service-spec/latest/org.freedesktop.Secret.Service.html#org.freedesktop.Secret.Service.ReadAlias
 	nullObjectPath = dbus.ObjectPath("/")
+)
 
-	// used to list all available collections on the secret service API
-	//
-	// https://specifications.freedesktop.org/secret-service-spec/latest/org.freedesktop.Secret.Service.html
-	secretServiceCollectionProperty = "org.freedesktop.Secret.Service.Collections"
+// secretService is the subset of [kc.SecretService] the keychain store depends
+// on. It exists so the store can be unit tested against a fake implementation
+// without a live secret service over dbus — see the fake in the linux tests.
+//
+// Every method maps to a method on [kc.SecretService]; none expose a
+// dbus.BusObject, so a fake never needs to talk to the bus.
+type secretService interface {
+	Collections() ([]dbus.ObjectPath, error)
+	ReadAlias(alias string) (dbus.ObjectPath, error)
+	IsLocked(collection dbus.ObjectPath) (bool, error)
+	OpenSession(mode kc.AuthenticationMode) (*kc.Session, error)
+	CloseSession(session *kc.Session)
+	Unlock(items []dbus.ObjectPath) error
+	SearchCollection(collection dbus.ObjectPath, attributes kc.Attributes) ([]dbus.ObjectPath, error)
+	CreateItem(collection dbus.ObjectPath, properties map[string]dbus.Variant, secret kc.Secret, replaceBehavior kc.ReplaceBehavior) (dbus.ObjectPath, error)
+	DeleteItem(item dbus.ObjectPath) error
+	GetAttributes(item dbus.ObjectPath) (kc.Attributes, error)
+	GetSecret(item dbus.ObjectPath, session kc.Session) ([]byte, error)
+	Close() error
+}
 
-	// used to get the dbus object path of an aliased collection
-	// An common alias would be 'default'
-	// https://specifications.freedesktop.org/secret-service-spec/latest/org.freedesktop.Secret.Service.html
-	secretServiceGetAliasObjectPath = "org.freedesktop.Secret.Service.ReadAlias"
+// the concrete secret service must satisfy the interface the store depends on.
+var _ secretService = (*kc.SecretService)(nil)
 
-	// used to check if the collection is locked
-	//
-	// https://specifications.freedesktop.org/secret-service-spec/latest/org.freedesktop.Secret.Collection.html
-	secretServiceIsCollectionLockedProperty = "org.freedesktop.Secret.Collection.Locked"
-)
+// newService dials a fresh secret service. It is a package var so tests can
+// substitute a fake; production always returns a real [kc.SecretService].
+var newService = func() (secretService, error) { return kc.NewService() }
 
 // getDefaultCollection gets the secret service collection dbus object path.
 //
@@ -70,24 +83,17 @@ const (
 // As a fallback it queries the secret service for the default collection.
 // It is possible that the host does not have a collection set up, in that case
 // the only option is to error.
-func getDefaultCollection(service *kc.SecretService) (dbus.ObjectPath, error) {
-	variant, err := service.ServiceObj().GetProperty(secretServiceCollectionProperty)
+func getDefaultCollection(service secretService) (dbus.ObjectPath, error) {
+	collections, err := service.Collections()
 	if err != nil {
 		return "", err
 	}
-	collections, ok := variant.Value().([]dbus.ObjectPath)
-	if !ok {
-		return "", errors.New("could not list keychain collections")
-	}
 	// choose the 'login' collection if it exists
 	if slices.Contains(collections, loginKeychainObjectPath) {
 		return loginKeychainObjectPath, nil
 	}
 	// we need to fallback to the default collection
-	var defaultKeychainObjectPath dbus.ObjectPath
-	err = service.ServiceObj().
-		Call(secretServiceGetAliasObjectPath, 0, "default").
-		Store(&defaultKeychainObjectPath)
+	defaultKeychainObjectPath, err := service.ReadAlias("default")
 	if err != nil {
 		return "", err
 	}
@@ -128,12 +134,12 @@ var errCollectionLocked = errors.New("collection is locked")
 //
 // It returns the errCollectionLocked error by default if the collection is locked.
 // On any other error, it returns the underlying error instead.
-func isCollectionUnlocked(collectionPath dbus.ObjectPath, service *kc.SecretService) error {
-	variant, err := service.Obj(collectionPath).GetProperty(secretServiceIsCollectionLockedProperty)
+func isCollectionUnlocked(collectionPath dbus.ObjectPath, service secretService) error {
+	locked, err := service.IsLocked(collectionPath)
 	if err != nil {
 		return err
 	}
-	if locked, ok := variant.Value().(bool); ok && !locked {
+	if !locked {
 		return nil
 	}
 	return errCollectionLocked
@@ -146,7 +152,7 @@ type keychainStore[T store.Secret] struct {
 }
 
 func (k *keychainStore[T]) Delete(_ context.Context, id store.ID) error {
-	service, err := kc.NewService()
+	service, err := newService()
 	if err != nil {
 		return err
 	}
@@ -193,7 +199,7 @@ func (k *keychainStore[T]) Delete(_ context.Context, id store.ID) error {
 }
 
 func (k *keychainStore[T]) Get(ctx context.Context, id store.ID) (store.Secret, error) {
-	service, err := kc.NewService()
+	service, err := newService()
 	if err != nil {
 		return nil, err
 	}
@@ -260,7 +266,7 @@ func (k *keychainStore[T]) Get(ctx context.Context, id store.ID) (store.Secret,
 }
 
 func (k *keychainStore[T]) GetAllMetadata(ctx context.Context) (map[store.ID]store.Secret, error) {
-	service, err := kc.NewService()
+	service, err := newService()
 	if err != nil {
 		return nil, err
 	}
@@ -332,7 +338,7 @@ func (k *keychainStore[T]) GetAllMetadata(ctx context.Context) (map[store.ID]sto
 }
 
 func (k *keychainStore[T]) Save(_ context.Context, id store.ID, secret store.Secret) error {
-	service, err := kc.NewService()
+	service, err := newService()
 	if err != nil {
 		return err
 	}
@@ -395,7 +401,7 @@ func (k *keychainStore[T]) Upsert(ctx context.Context, id store.ID, secret store
 
 // loadSecret fetches the raw secret value for itemPath, zeroes it after use,
 // and returns a fully populated Secret.
-func (k *keychainStore[T]) loadSecret(ctx context.Context, id store.ID, svc *kc.SecretService, itemPath dbus.ObjectPath, session *kc.Session, attributes map[string]string) (store.Secret, error) {
+func (k *keychainStore[T]) loadSecret(ctx context.Context, id store.ID, svc secretService, itemPath dbus.ObjectPath, session *kc.Session, attributes map[string]string) (store.Secret, error) {
 	value, err := svc.GetSecret(itemPath, *session)
 	if err != nil {
 		return nil, err
@@ -413,7 +419,7 @@ func (k *keychainStore[T]) loadSecret(ctx context.Context, id store.ID, svc *kc.
 
 //gocyclo:ignore
 func (k *keychainStore[T]) Filter(ctx context.Context, pattern store.Pattern) (map[store.ID]store.Secret, error) {
-	service, err := kc.NewService()
+	service, err := newService()
 	if err != nil {
 		return nil, err
 	}

store/keychain/keychain_linux_test.go

@@ -17,13 +17,105 @@
 package keychain
 
 import (
+	"sync/atomic"
 	"testing"
 
 	dbus "github.com/godbus/dbus/v5"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
+
+	"github.com/docker/secrets-engine/store"
+	kc "github.com/docker/secrets-engine/store/keychain/internal/go-keychain/secretservice"
 )
 
+// fakeService is a pure in-memory [secretService]. It never talks to a real
+// secret service over dbus, so tests that drive the store through it are
+// deterministic and need no keyring on the host.
+//
+// It records how many connections were opened and closed so a test can assert
+// the store balances every open with a Close. The counters are atomic so the
+// fake is safe to share across concurrent operations.
+type fakeService struct {
+	// items is returned verbatim by SearchCollection; leave empty to drive the
+	// "credential not found" path.
+	items []dbus.ObjectPath
+
+	opened atomic.Int64
+	closed atomic.Int64
+}
+
+func (f *fakeService) Collections() ([]dbus.ObjectPath, error) {
+	return []dbus.ObjectPath{loginKeychainObjectPath}, nil
+}
+func (f *fakeService) ReadAlias(string) (dbus.ObjectPath, error) { return loginKeychainObjectPath, nil }
+func (f *fakeService) IsLocked(dbus.ObjectPath) (bool, error)    { return false, nil }
+func (f *fakeService) OpenSession(kc.AuthenticationMode) (*kc.Session, error) {
+	return &kc.Session{}, nil
+}
+func (f *fakeService) CloseSession(*kc.Session)       {}
+func (f *fakeService) Unlock([]dbus.ObjectPath) error { return nil }
+func (f *fakeService) SearchCollection(dbus.ObjectPath, kc.Attributes) ([]dbus.ObjectPath, error) {
+	return f.items, nil
+}
+
+func (f *fakeService) CreateItem(dbus.ObjectPath, map[string]dbus.Variant, kc.Secret, kc.ReplaceBehavior) (dbus.ObjectPath, error) {
+	return "", nil
+}
+func (f *fakeService) DeleteItem(dbus.ObjectPath) error                      { return nil }
+func (f *fakeService) GetAttributes(dbus.ObjectPath) (kc.Attributes, error)  { return nil, nil }
+func (f *fakeService) GetSecret(dbus.ObjectPath, kc.Session) ([]byte, error) { return nil, nil }
+func (f *fakeService) Close() error {
+	f.closed.Add(1)
+	return nil
+}
+
+// withFakeService swaps the package newService seam for one that hands out the
+// given fake (counting each open) and restores the original on cleanup.
+func withFakeService(t *testing.T, fake *fakeService) {
+	t.Helper()
+	orig := newService
+	t.Cleanup(func() { newService = orig })
+	newService = func() (secretService, error) {
+		fake.opened.Add(1)
+		return fake, nil
+	}
+}
+
+// TestKeychainGetNotFound exercises the full Get path against the fake — open,
+// resolve collection, search — and asserts an empty search maps to
+// ErrCredentialNotFound, all without a live keyring.
+func TestKeychainGetNotFound(t *testing.T) {
+	fake := &fakeService{} // no items -> not found
+	withFakeService(t, fake)
+
+	ks := setupKeychain(t, nil)
+	_, err := ks.Get(t.Context(), store.MustParseID("com.test.test/test/missing"))
+	assert.ErrorIs(t, err, store.ErrCredentialNotFound)
+}
+
+// TestKeychainClosesEveryConnection is a deterministic regression test for the
+// D-Bus connection leak: each keychain operation dials a fresh connection via
+// newService and must Close it. Driving the store through a fake lets us assert
+// the contract directly — every opened connection is closed — instead of
+// counting host file descriptors.
+func TestKeychainClosesEveryConnection(t *testing.T) {
+	fake := &fakeService{} // no items -> Get returns ErrCredentialNotFound
+	withFakeService(t, fake)
+
+	ks := setupKeychain(t, nil)
+	missing := store.MustParseID("com.test.test/test/missing")
+
+	const iterations = 30
+	for range iterations {
+		_, err := ks.Get(t.Context(), missing)
+		require.ErrorIs(t, err, store.ErrCredentialNotFound)
+	}
+
+	opened, closed := fake.opened.Load(), fake.closed.Load()
+	require.Equal(t, int64(iterations), opened)
+	assert.Equal(t, opened, closed, "every opened connection must be closed")
+}
+
 func TestResolveDefaultCollection(t *testing.T) {
 	const customCollection = dbus.ObjectPath("/org/freedesktop/secrets/collection/custom")