diff --git a/store/keychain/internal/go-keychain/secretservice/secretservice.go b/store/keychain/internal/go-keychain/secretservice/secretservice.go index 4b301aed..1381785a 100644 --- a/store/keychain/internal/go-keychain/secretservice/secretservice.go +++ b/store/keychain/internal/go-keychain/secretservice/secretservice.go @@ -78,6 +78,20 @@ func NewService() (*SecretService, error) { return &SecretService{conn: conn, signalCh: signalCh, sessionOpenTimeout: DefaultSessionOpenTimeout}, nil } +// Close releases the underlying D-Bus connection and its socket file +// descriptor. Each [NewService] call dials a private session-bus connection +// (via dbus.ConnectSessionBus), so every service MUST be closed when it is no +// longer needed; otherwise the connection — and its fd — leaks for the lifetime +// of the process. Closing the connection also tears down the signal goroutine +// that NewService starts. Close is safe to call on a service whose connection +// is nil. +func (s *SecretService) Close() error { + if s == nil || s.conn == nil { + return nil + } + return s.conn.Close() +} + // SetSessionOpenTimeout func (s *SecretService) SetSessionOpenTimeout(d time.Duration) { s.sessionOpenTimeout = d diff --git a/store/keychain/keychain_linux.go b/store/keychain/keychain_linux.go index 651bfb6e..cfa89161 100644 --- a/store/keychain/keychain_linux.go +++ b/store/keychain/keychain_linux.go @@ -150,6 +150,10 @@ func (k *keychainStore[T]) Delete(_ context.Context, id store.ID) error { if err != nil { return err } + // NewService dials a fresh private session-bus connection; close it (and + // its socket fd) when we return. Deferred before CloseSession so that, by + // LIFO order, the session is closed first and the connection last. + defer func() { _ = service.Close() }() session, err := service.OpenSession(kc.AuthenticationDHAES) if err != nil { @@ -193,6 +197,10 @@ func (k *keychainStore[T]) Get(ctx context.Context, id store.ID) (store.Secret, if err != nil { return nil, err } + // NewService dials a fresh private session-bus connection; close it (and + // its socket fd) when we return. Deferred before CloseSession so that, by + // LIFO order, the session is closed first and the connection last. + defer func() { _ = service.Close() }() session, err := service.OpenSession(kc.AuthenticationDHAES) if err != nil { @@ -256,6 +264,10 @@ func (k *keychainStore[T]) GetAllMetadata(ctx context.Context) (map[store.ID]sto if err != nil { return nil, err } + // NewService dials a fresh private session-bus connection; close it (and + // its socket fd) when we return. Deferred before CloseSession so that, by + // LIFO order, the session is closed first and the connection last. + defer func() { _ = service.Close() }() session, err := service.OpenSession(kc.AuthenticationDHAES) if err != nil { @@ -324,6 +336,10 @@ func (k *keychainStore[T]) Save(_ context.Context, id store.ID, secret store.Sec if err != nil { return err } + // NewService dials a fresh private session-bus connection; close it (and + // its socket fd) when we return. Deferred before CloseSession so that, by + // LIFO order, the session is closed first and the connection last. + defer func() { _ = service.Close() }() session, err := service.OpenSession(kc.AuthenticationDHAES) if err != nil { @@ -401,6 +417,10 @@ func (k *keychainStore[T]) Filter(ctx context.Context, pattern store.Pattern) (m if err != nil { return nil, err } + // NewService dials a fresh private session-bus connection; close it (and + // its socket fd) when we return. Deferred before CloseSession so that, by + // LIFO order, the session is closed first and the connection last. + defer func() { _ = service.Close() }() session, err := service.OpenSession(kc.AuthenticationDHAES) if err != nil { diff --git a/store/keychain/keychain_linux_test.go b/store/keychain/keychain_linux_test.go index d9aff5a7..f0771ac4 100644 --- a/store/keychain/keychain_linux_test.go +++ b/store/keychain/keychain_linux_test.go @@ -17,13 +17,74 @@ package keychain import ( + "os" "testing" dbus "github.com/godbus/dbus/v5" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + + "github.com/docker/secrets-engine/store" ) +// openFDCount returns the number of open file descriptors held by the current +// process by counting entries in /proc/self/fd (Linux only). +func openFDCount(t *testing.T) int { + t.Helper() + entries, err := os.ReadDir("/proc/self/fd") + require.NoError(t, err) + return len(entries) +} + +// TestKeychainDoesNotLeakConnections is a regression test for a D-Bus +// connection leak: each keychain operation dialed a fresh session-bus +// connection (kc.NewService -> dbus.ConnectSessionBus) but only closed the +// secret-service session, never the connection itself. Every operation +// therefore leaked one socket file descriptor, eventually exhausting the +// session bus's max_connections_per_user limit on long-lived processes. +// +// It reproduces the failure shape directly: perform many lookups and assert +// the process's open-fd count stays bounded instead of growing +// one-per-operation. +// +// The lookups target a non-existent id so the test exercises the full +// connection setup (NewService -> OpenSession -> resolve collection -> +// SearchCollection) and then returns ErrCredentialNotFound *before* fetching a +// secret. That keeps the test focused on the connection lifecycle — the thing +// the fix changes — without creating, reading, or deleting shared keyring +// items (which is both stateful and prone to gnome-keyring item-lock quirks). +func TestKeychainDoesNotLeakConnections(t *testing.T) { + ks := setupKeychain(t, nil) + missing := store.MustParseID("com.test.test/test/does-not-exist") + + const iterations = 30 + + get := func() { + _, err := ks.Get(t.Context(), missing) + require.ErrorIs(t, err, store.ErrCredentialNotFound) + } + + // Warm up so any one-time, non-leaking fds (lazy runtime/dbus init) are + // already open before we take the baseline. + for range 3 { + get() + } + + before := openFDCount(t) + for range iterations { + get() + } + after := openFDCount(t) + + // A correct implementation closes every connection it opens, so the fd + // count should be flat. Allow a small slack for unrelated runtime churn; + // the leak grows the count by ~iterations, far above the threshold. + const slack = 5 + assert.LessOrEqualf(t, after-before, slack, + "open fd count grew by %d over %d lookups (before=%d after=%d): D-Bus connections are leaking", + after-before, iterations, before, after) +} + func TestResolveDefaultCollection(t *testing.T) { const customCollection = dbus.ObjectPath("/org/freedesktop/secrets/collection/custom")