From 51a3cc4a6a10df4c00e77f64fbf21a0d413feef6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Johannes=20Gro=C3=9Fmann?= Date: Thu, 15 Oct 2026 15:28:43 +0200 Subject: [PATCH] refactor(keychain): open secret service sessions under the caller's context MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit OpenSession ran the D-Bus OpenSession call in a goroutine and raced it against time.After in a select. godbus already does this work: CallWithContext finalizes the pending call with ctx.Err() when the context is done and drops a late reply, so the goroutine, both channels and the select were redundant. They also leaked: the channels were unbuffered, so once the timeout branch returned the goroutine blocked forever on its send. OpenSession now takes a ctx, derives context.WithTimeout from it with the existing session-open timeout (10 s default, so a caller without a deadline still cannot hang on an unresponsive daemon) and issues a single CallWithContext. openSessionRaw and sessionOpenResponse are inlined away and the named results dropped. The timeout error changes from "timed out after 10s" to the wrapped context.DeadlineExceeded. Every Linux store operation passes its ctx through, consistent with CreateItem, DeleteItem and Unlock. An operation given an already-cancelled context therefore fails immediately instead of succeeding whenever no prompt was needed. The filter_credentials test cleanup relied on the old behavior: it deleted with t.Context(), which is already cancelled inside t.Cleanup. It now uses context.Background() like every other cleanup in the file. Signed-off-by: Johannes Großmann --- .../secretservice/secretservice.go | 57 ++++--------------- .../secretservice/secretservice_test.go | 4 +- store/keychain/keychain_linux.go | 12 ++-- store/keychain/keychain_linux_test.go | 4 +- store/keychain/keychain_test.go | 2 +- 5 files changed, 22 insertions(+), 57 deletions(-) diff --git a/store/keychain/internal/go-keychain/secretservice/secretservice.go b/store/keychain/internal/go-keychain/secretservice/secretservice.go index 84c3704b..138ce7c7 100644 --- a/store/keychain/internal/go-keychain/secretservice/secretservice.go +++ b/store/keychain/internal/go-keychain/secretservice/secretservice.go @@ -299,29 +299,9 @@ func (s *SecretService) IsLocked(collection dbus.ObjectPath) (bool, error) { return locked, nil } -type sessionOpenResponse struct { - algorithmOutput dbus.Variant - path dbus.ObjectPath -} - -func (s *SecretService) openSessionRaw(mode AuthenticationMode, sessionAlgorithmInput dbus.Variant) (resp sessionOpenResponse, err error) { - err = s.ServiceObj(). - Call("org.freedesktop.Secret.Service.OpenSession", NilFlags, mode, sessionAlgorithmInput). - Store(&resp.algorithmOutput, &resp.path) - if err != nil { - return sessionOpenResponse{}, fmt.Errorf("failed to open secretservice session: %w", err) - } - return resp, nil -} - -// OpenSession -func (s *SecretService) OpenSession(mode AuthenticationMode) (session *Session, err error) { +func (s *SecretService) OpenSession(ctx context.Context, mode AuthenticationMode) (*Session, error) { + session := &Session{Mode: mode} var sessionAlgorithmInput dbus.Variant - - session = new(Session) - - session.Mode = mode - switch mode { case AuthenticationInsecurePlain: sessionAlgorithmInput = dbus.MakeVariant("") @@ -338,31 +318,16 @@ func (s *SecretService) OpenSession(mode AuthenticationMode) (session *Session, return nil, fmt.Errorf("unknown authentication mode %v", mode) } - sessionOpenCh := make(chan sessionOpenResponse) - errCh := make(chan error) - go func() { - resp, err := s.openSessionRaw(mode, sessionAlgorithmInput) - if err != nil { - errCh <- err - } else { - sessionOpenCh <- resp - } - }() - + callCtx, cancel := context.WithTimeout(ctx, s.sessionOpenTimeout) + defer cancel() var sessionAlgorithmOutput dbus.Variant - // NOTE: If the timeout case is reached, the above goroutine is leaked. - // This is not terrible because D-Bus calls have an internal 2-mintue - // timeout, so the goroutine will finish eventually. If two OpenSessions - // are called at the saime time, they'll be on different channels so - // they won't interfere with each other. - select { - case resp := <-sessionOpenCh: - sessionAlgorithmOutput = resp.algorithmOutput - session.Path = resp.path - case err := <-errCh: - return nil, err - case <-time.After(s.sessionOpenTimeout): - return nil, fmt.Errorf("timed out after %s", s.sessionOpenTimeout) + if err := s.ServiceObj(). + CallWithContext(callCtx, "org.freedesktop.Secret.Service.OpenSession", NilFlags, mode, sessionAlgorithmInput). + Store(&sessionAlgorithmOutput, &session.Path); err != nil { + if errors.Is(err, context.DeadlineExceeded) && ctx.Err() == nil { + return nil, fmt.Errorf("failed to open secretservice session: timed out after %s: %w", s.sessionOpenTimeout, err) + } + return nil, fmt.Errorf("failed to open secretservice session: %w", err) } switch mode { diff --git a/store/keychain/internal/go-keychain/secretservice/secretservice_test.go b/store/keychain/internal/go-keychain/secretservice/secretservice_test.go index a9899ea2..11349e87 100644 --- a/store/keychain/internal/go-keychain/secretservice/secretservice_test.go +++ b/store/keychain/internal/go-keychain/secretservice/secretservice_test.go @@ -25,7 +25,7 @@ func TestKeyringDH(t *testing.T) { func testKeyring(t *testing.T, mode AuthenticationMode) { srv, err := NewService(t.Context()) require.NoError(t, err) - session, err := srv.OpenSession(mode) + session, err := srv.OpenSession(t.Context(), mode) require.NoError(t, err) defer srv.CloseSession(session) @@ -62,7 +62,7 @@ func testKeyring(t *testing.T, mode AuthenticationMode) { func TestGetAll(t *testing.T) { srv, err := NewService(t.Context()) require.NoError(t, err) - session, err := srv.OpenSession(AuthenticationDHAES) + session, err := srv.OpenSession(t.Context(), AuthenticationDHAES) require.NoError(t, err) defer srv.CloseSession(session) diff --git a/store/keychain/keychain_linux.go b/store/keychain/keychain_linux.go index d070ad82..d7999dbe 100644 --- a/store/keychain/keychain_linux.go +++ b/store/keychain/keychain_linux.go @@ -58,7 +58,7 @@ 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) + OpenSession(ctx context.Context, mode kc.AuthenticationMode) (*kc.Session, error) CloseSession(session *kc.Session) Unlock(ctx context.Context, items []dbus.ObjectPath) error SearchCollection(collection dbus.ObjectPath, attributes kc.Attributes) ([]dbus.ObjectPath, error) @@ -373,7 +373,7 @@ func (k *keychainStore[T]) Delete(ctx context.Context, id store.ID) error { // LIFO order, the session is closed first and the connection last. defer func() { _ = service.Close() }() - session, err := service.OpenSession(kc.AuthenticationDHAES) + session, err := service.OpenSession(ctx, kc.AuthenticationDHAES) if err != nil { return err } @@ -416,7 +416,7 @@ func (k *keychainStore[T]) Get(ctx context.Context, id store.ID) (store.Secret, // LIFO order, the session is closed first and the connection last. defer func() { _ = service.Close() }() - session, err := service.OpenSession(kc.AuthenticationDHAES) + session, err := service.OpenSession(ctx, kc.AuthenticationDHAES) if err != nil { return nil, err } @@ -482,7 +482,7 @@ func (k *keychainStore[T]) GetAllMetadata(ctx context.Context) (map[store.ID]sto // LIFO order, the session is closed first and the connection last. defer func() { _ = service.Close() }() - session, err := service.OpenSession(kc.AuthenticationDHAES) + session, err := service.OpenSession(ctx, kc.AuthenticationDHAES) if err != nil { return nil, err } @@ -556,7 +556,7 @@ func (k *keychainStore[T]) Save(ctx context.Context, id store.ID, secret store.S // LIFO order, the session is closed first and the connection last. defer func() { _ = service.Close() }() - session, err := service.OpenSession(kc.AuthenticationDHAES) + session, err := service.OpenSession(ctx, kc.AuthenticationDHAES) if err != nil { return err } @@ -677,7 +677,7 @@ func (k *keychainStore[T]) Filter(ctx context.Context, pattern store.Pattern) (m // LIFO order, the session is closed first and the connection last. defer func() { _ = service.Close() }() - session, err := service.OpenSession(kc.AuthenticationDHAES) + session, err := service.OpenSession(ctx, kc.AuthenticationDHAES) if err != nil { return nil, err } diff --git a/store/keychain/keychain_linux_test.go b/store/keychain/keychain_linux_test.go index 0f6d0527..c1fdebb2 100644 --- a/store/keychain/keychain_linux_test.go +++ b/store/keychain/keychain_linux_test.go @@ -111,7 +111,7 @@ func (f *fakeService) Collections() ([]dbus.ObjectPath, error) { func (f *fakeService) ReadAlias(string) (dbus.ObjectPath, error) { return loginKeychainObjectPath, nil } func (f *fakeService) IsLocked(dbus.ObjectPath) (bool, error) { return f.locked, nil } -func (f *fakeService) OpenSession(kc.AuthenticationMode) (*kc.Session, error) { +func (f *fakeService) OpenSession(context.Context, kc.AuthenticationMode) (*kc.Session, error) { // plain mode so Session.NewSecret works without a negotiated AES key, which // lets the Save path run end-to-end against the fake. return &kc.Session{Mode: kc.AuthenticationInsecurePlain}, nil @@ -634,7 +634,7 @@ func seedRealDuplicates(t *testing.T, serviceGroup, serviceName string, id store require.NoError(t, err) defer func() { _ = svc.Close() }() - session, err := svc.OpenSession(kc.AuthenticationDHAES) + session, err := svc.OpenSession(context.Background(), kc.AuthenticationDHAES) require.NoError(t, err) defer svc.CloseSession(session) diff --git a/store/keychain/keychain_test.go b/store/keychain/keychain_test.go index 4c1a2fd6..8d42f112 100644 --- a/store/keychain/keychain_test.go +++ b/store/keychain/keychain_test.go @@ -224,7 +224,7 @@ func TestKeychain(t *testing.T) { t.Cleanup(func() { for id := range moreCreds { - assert.NoError(t, ks.Delete(t.Context(), id)) + assert.NoError(t, ks.Delete(context.Background(), id)) } })