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