pam/adapter: fix authTracker race with a generation counter (#1449)

Closes #1424

Author: adombeck

internal/brokers/broker.go

@@ -43,6 +43,12 @@ type Broker struct {
 	ongoingUserRequests   map[string]string
 	ongoingUserRequestsMu *sync.Mutex
 
+	// isAuthMu serialises IsAuthenticated calls per session. A new call for
+	// the same session must wait until the previous one — including any broker-
+	// side cancellation and cleanup — has fully returned.
+	isAuthMu   map[string]*sync.Mutex
+	isAuthMuMu *sync.Mutex
+
 	brokerer brokerer
 }
 
@@ -83,6 +89,8 @@ func newBroker(ctx context.Context, configFile string, bus *dbus.Conn) (b Broker
 		layoutValidatorsMu:    &sync.Mutex{},
 		ongoingUserRequests:   make(map[string]string),
 		ongoingUserRequestsMu: &sync.Mutex{},
+		isAuthMu:              make(map[string]*sync.Mutex),
+		isAuthMuMu:            &sync.Mutex{},
 	}, nil
 }
 
@@ -142,6 +150,24 @@ func (b Broker) SelectAuthenticationMode(ctx context.Context, sessionID, authent
 func (b Broker) IsAuthenticated(ctx context.Context, sessionID, authenticationData string) (access string, data string, err error) {
 	sessionID = b.parseSessionID(sessionID)
 
+	// Serialise concurrent IsAuthenticated calls for the same session.
+	// A previous call may still be in-flight on the broker side after the
+	// client-side context was cancelled; we must wait for it to fully finish
+	// (including the broker's own cleanup) before sending a new request.
+	// Because broker.IsAuthenticated already does <-done in the ctx.Done()
+	// branch, this mutex is only released after the broker goroutine has
+	// exited — so the second call is guaranteed to see a clean broker state.
+	b.isAuthMuMu.Lock()
+	mu, ok := b.isAuthMu[sessionID]
+	if !ok {
+		mu = &sync.Mutex{}
+		b.isAuthMu[sessionID] = mu
+	}
+	b.isAuthMuMu.Unlock()
+
+	mu.Lock()
+	defer mu.Unlock()
+
 	// monitor ctx in goroutine to call cancel
 	done := make(chan struct{})
 	go func() {
@@ -230,6 +256,10 @@ func (b Broker) endSession(ctx context.Context, sessionID string) (err error) {
 	defer b.ongoingUserRequestsMu.Unlock()
 	delete(b.ongoingUserRequests, sessionID)
 
+	b.isAuthMuMu.Lock()
+	delete(b.isAuthMu, sessionID)
+	b.isAuthMuMu.Unlock()
+
 	return b.brokerer.EndSession(ctx, sessionID)
 }
 

internal/brokers/broker_test.go

@@ -223,20 +223,20 @@ func TestIsAuthenticated(t *testing.T) {
 		"No_error_when_broker_returns_userinfo_with_mismatching_username":  {sessionID: "ia_info_mismatching_user_name"},
 
 		// broker errors
-		"Error_when_authenticating":                                           {sessionID: "ia_error"},
-		"Error_on_empty_data_even_if_granted":                                 {sessionID: "ia_empty_data"},
-		"Error_when_broker_returns_invalid_data":                              {sessionID: "ia_invalid_data"},
-		"Error_when_broker_returns_invalid_access":                            {sessionID: "ia_invalid_access"},
-		"Error_when_broker_returns_invalid_userinfo":                          {sessionID: "ia_invalid_userinfo"},
-		"Error_when_broker_returns_userinfo_with_empty_username":              {sessionID: "ia_info_empty_user_name"},
-		"Error_when_broker_returns_userinfo_with_empty_group_name":            {sessionID: "ia_info_empty_group_name"},
-		"Error_when_broker_returns_userinfo_with_invalid_homedir":             {sessionID: "ia_info_invalid_home"},
-		"Error_when_broker_returns_userinfo_with_invalid_shell":               {sessionID: "ia_info_invalid_shell"},
-		"Error_when_broker_returns_invalid_data_on_auth.Next":                 {sessionID: "ia_next_with_invalid_data"},
-		"Error_when_broker_returns_data_on_auth.Cancelled":                    {sessionID: "ia_cancelled_with_data"},
-		"Error_when_broker_returns_no_data_on_auth.Denied":                    {sessionID: "ia_denied_without_data"},
-		"Error_when_broker_returns_no_data_on_auth.Retry":                     {sessionID: "ia_retry_without_data"},
-		"Error_when_calling_IsAuthenticated_a_second_time_without_cancelling": {sessionID: "ia_second_call", secondCall: true, cancelFirstCall: true},
+		"Error_when_authenticating":                                      {sessionID: "ia_error"},
+		"Error_on_empty_data_even_if_granted":                            {sessionID: "ia_empty_data"},
+		"Error_when_broker_returns_invalid_data":                         {sessionID: "ia_invalid_data"},
+		"Error_when_broker_returns_invalid_access":                       {sessionID: "ia_invalid_access"},
+		"Error_when_broker_returns_invalid_userinfo":                     {sessionID: "ia_invalid_userinfo"},
+		"Error_when_broker_returns_userinfo_with_empty_username":         {sessionID: "ia_info_empty_user_name"},
+		"Error_when_broker_returns_userinfo_with_empty_group_name":       {sessionID: "ia_info_empty_group_name"},
+		"Error_when_broker_returns_userinfo_with_invalid_homedir":        {sessionID: "ia_info_invalid_home"},
+		"Error_when_broker_returns_userinfo_with_invalid_shell":          {sessionID: "ia_info_invalid_shell"},
+		"Error_when_broker_returns_invalid_data_on_auth.Next":            {sessionID: "ia_next_with_invalid_data"},
+		"Error_when_broker_returns_data_on_auth.Cancelled":               {sessionID: "ia_cancelled_with_data"},
+		"Error_when_broker_returns_no_data_on_auth.Denied":               {sessionID: "ia_denied_without_data"},
+		"Error_when_broker_returns_no_data_on_auth.Retry":                {sessionID: "ia_retry_without_data"},
+		"Successfully_authenticate_after_second_call_without_cancelling": {sessionID: "ia_second_call", secondCall: true, cancelFirstCall: true},
 	}
 	for name, tc := range tests {
 		t.Run(name, func(t *testing.T) {

…ticated_a_second_time_without_cancelling …ate_after_second_call_without_cancellinginternal/brokers/testdata/golden/TestIsAuthenticated/Error_when_calling_IsAuthenticated_a_second_time_without_cancelling renamed to internal/brokers/testdata/golden/TestIsAuthenticated/Successfully_authenticate_after_second_call_without_cancelling internal/brokers/testdata/golden/TestIsAuthenticated/Error_when_calling_IsAuthenticated_a_second_time_without_cancelling renamed to internal/brokers/testdata/golden/TestIsAuthenticated/Successfully_authenticate_after_second_call_without_cancelling

@@ -3,6 +3,6 @@ FIRST CALL:
 	data: {"Name":"ia_second_call@example.com","UID":0,"Gecos":"gecos for ia_second_call@example.com","Dir":"/home/ia_second_call@example.com","Shell":"/bin/sh/ia_second_call@example.com","Groups":[{"Name":"group-ia_second_call@example.com","GID":null,"UGID":"ugid-ia_second_call@example.com"}]}
 	err: <nil>
 SECOND CALL:
-	access: 
-	data: 
-	err: broker "TestIsAuthenticated": IsAuthenticated already running for session "TestIsAuthenticated/Error_when_calling_IsAuthenticated_a_second_time_without_cancelling_separator_ia_second_call"
+	access: granted
+	data: {"Name":"ia_second_call@example.com","UID":0,"Gecos":"gecos for ia_second_call@example.com","Dir":"/home/ia_second_call@example.com","Shell":"/bin/sh/ia_second_call@example.com","Groups":[{"Name":"group-ia_second_call@example.com","GID":null,"UGID":"ugid-ia_second_call@example.com"}]}
+	err: <nil>

internal/services/pam/pam_test.go

@@ -451,12 +451,12 @@ func TestIsAuthenticated(t *testing.T) {
 		"Error_when_user_is_locked":     {username: "locked@example.com", existingDB: "cache-with-locked-user.db"},
 
 		// broker errors
-		"Error_when_authenticating":                         {username: "ia_error@example.com"},
-		"Error_on_empty_data_even_if_granted":               {username: "ia_empty_data@example.com"},
-		"Error_when_broker_returns_invalid_access":          {username: "ia_invalid_access@example.com"},
-		"Error_when_broker_returns_invalid_data":            {username: "ia_invalid_data@example.com"},
-		"Error_when_broker_returns_invalid_userinfo":        {username: "ia_invalid_userinfo@example.com"},
-		"Error_when_calling_second_time_without_cancelling": {username: "ia_second_call@example.com", secondCall: true},
+		"Error_when_authenticating":                                              {username: "ia_error@example.com"},
+		"Error_on_empty_data_even_if_granted":                                    {username: "ia_empty_data@example.com"},
+		"Error_when_broker_returns_invalid_access":                               {username: "ia_invalid_access@example.com"},
+		"Error_when_broker_returns_invalid_data":                                 {username: "ia_invalid_data@example.com"},
+		"Error_when_broker_returns_invalid_userinfo":                             {username: "ia_invalid_userinfo@example.com"},
+		"Successfully_authenticate_after_calling_second_time_without_cancelling": {username: "ia_second_call@example.com", secondCall: true},
 
 		// local group error
 		"Error_on_updating_local_groups_with_unexisting_file": {username: "success_with_local_groups@example.com", localGroupsFile: "does_not_exists.group"},

internal/services/pam/testdata/golden/TestIsAuthenticated/Error_when_calling_second_time_without_cancelling/IsAuthenticated

@@ -0,0 +1,8 @@
+FIRST CALL:
+	access: granted
+	msg: 
+	err: <nil>
+SECOND CALL:
+	access: granted
+	msg: 
+	err: <nil>

internal/services/pam/testdata/golden/TestIsAuthenticated/Successfully_authenticate_after_calling_second_time_without_cancelling/IsAuthenticated

@@ -55,11 +55,12 @@ func sendIsAuthenticated(ctx context.Context, client authd.PAMClient, sessionID
 			if st := status.Convert(err); st.Code() == codes.Canceled {
 				// Note that this error is only the client-side error, so being here doesn't
 				// mean the cancellation on broker side is fully completed.
-
-				// Wait for the cancellation requests to have been delivered and actually handled.
-				// The multiplier can be increased to avoid that we return the cancelled event too
-				// early, but it implies slowing down the UI responses.
-				<-time.After(cancellationWait * 3)
+				// We still wait briefly so that the CancelIsAuthenticated D-Bus call (sent
+				// by the broker layer when ctx is cancelled) has time to arrive at the broker
+				// after the IsAuthenticated call — not before it.  Serialisation of
+				// back-to-back IsAuthenticated calls for the same session is handled
+				// server-side, so we no longer need a longer delay here.
+				<-time.After(cancellationWait)
 
 				return isAuthenticatedResultReceived{
 					access: auth.Cancelled,
@@ -138,9 +139,18 @@ type authenticationModel struct {
 	errorMsg string
 }
 
+// authTracker serialises IsAuthenticated calls and supports cancellation.
+//
+// At most one authentication goroutine is in flight at a time. A goroutine
+// that arrives while another is running blocks until the running one finishes.
+// cancelAndWait() cancels any in-flight goroutine and bumps the generation
+// counter so that any goroutine that is waiting (or has just been woken) knows
+// it has been superseded and must abort without making the RPC.
 type authTracker struct {
-	cancelFunc func()
-	cond       *sync.Cond
+	mu         sync.Mutex
+	generation uint64        // incremented by cancelAndWait; goroutines abort if theirs is stale
+	cancelFunc func()        // cancels the context of the current in-flight RPC; nil when idle
+	done       chan struct{} // closed by the goroutine when it exits; nil when idle
 }
 
 // startAuthentication signals that the authentication model can start
@@ -174,7 +184,7 @@ func newAuthenticationModel(client authd.PAMClient, clientType PamClientType, mo
 		client:      client,
 		clientType:  clientType,
 		mode:        mode,
-		authTracker: &authTracker{cond: sync.NewCond(&sync.Mutex{})},
+		authTracker: &authTracker{},
 	}
 }
 
@@ -293,7 +303,12 @@ func (m authenticationModel) Update(msg tea.Msg) (authModel authenticationModel,
 		clientType := m.clientType
 		currentLayout := m.currentLayout
 		return m, func() tea.Msg {
-			authTracker.waitAndStart(cancelFunc)
+			// waitForSlot blocks until no other auth is in flight, then registers
+			// us as the active goroutine for this generation.  It returns false
+			// if we have been superseded by a cancelAndWait() call and must abort.
+			if !authTracker.waitForSlot(cancelFunc) {
+				return nil
+			}
 
 			secret, hasSecret := msg.item.(*authd.IARequest_AuthenticationData_Secret)
 			if hasSecret && clientType == Gdm && currentLayout == layouts.NewPassword {
@@ -332,7 +347,7 @@ func (m authenticationModel) Update(msg tea.Msg) (authModel authenticationModel,
 			if msg.access != auth.Next && msg.access != auth.Retry {
 				m.currentModel = nil
 			}
-			m.authTracker.reset()
+			m.authTracker.finish()
 		}()
 
 		var authMsg string
@@ -550,43 +565,65 @@ func (authData *isAuthenticatedRequestedSend) encryptSecretIfPresent(publicKey *
 	return &secret.Secret, nil
 }
 
-// wait waits for the current authentication to be completed.
-func (at *authTracker) wait() {
-	at.cond.L.Lock()
-	defer at.cond.L.Unlock()
-
-	for at.cancelFunc != nil {
-		at.cond.Wait()
+// waitForSlot blocks until no other authentication goroutine is in flight,
+// then registers itself as the active goroutine.  It returns true when the
+// caller should proceed with the RPC, or false when a cancelAndWait() call
+// has superseded this goroutine and the caller must abort.
+//
+// The generation counter is the key to correctness: cancelAndWait() bumps it
+// under the lock before signalling, so any goroutine that wakes up afterwards
+// will see a stale generation and abort — with no window for a race.
+func (at *authTracker) waitForSlot(cancelFunc func()) bool {
+	at.mu.Lock()
+	gen := at.generation
+	// Wait until the previous auth goroutine has finished.
+	for at.done != nil {
+		done := at.done
+		at.mu.Unlock()
+		<-done
+		at.mu.Lock()
+	}
+	// If cancelAndWait() was called while we were waiting (or before we even
+	// started), our generation is stale — abort without making any RPC.
+	if at.generation != gen {
+		at.mu.Unlock()
+		return false
 	}
+	// Register ourselves as the active goroutine.
+	at.cancelFunc = cancelFunc
+	at.done = make(chan struct{})
+	at.mu.Unlock()
+	return true
 }
 
-// waitAndStart waits for the current authentication to be completed and
-// marks the authentication as in progress.
-func (at *authTracker) waitAndStart(cancelFunc func()) {
-	at.cond.L.Lock()
-	defer at.cond.L.Unlock()
-
-	for at.cancelFunc != nil {
-		at.cond.Wait()
+// finish marks the active authentication goroutine as done.  It must be called
+// by every goroutine that received true from waitForSlot, regardless of whether
+// the RPC was actually made (e.g. even for newPasswordCheck detours).
+func (at *authTracker) finish() {
+	at.mu.Lock()
+	done := at.done
+	at.cancelFunc = nil
+	at.done = nil
+	at.mu.Unlock()
+	if done != nil {
+		close(done)
 	}
-
-	at.cancelFunc = cancelFunc
 }
 
+// cancelAndWait cancels the in-flight authentication (if any) and waits for
+// its goroutine to exit.  After it returns, any goroutine currently blocked in
+// waitForSlot will also abort, because the generation counter was bumped.
 func (at *authTracker) cancelAndWait() {
-	at.cond.L.Lock()
+	at.mu.Lock()
+	at.generation++
 	cancelFunc := at.cancelFunc
-	at.cond.L.Unlock()
-	if cancelFunc == nil {
-		return
-	}
-	cancelFunc()
-	at.wait()
-}
+	done := at.done
+	at.mu.Unlock()
 
-func (at *authTracker) reset() {
-	at.cond.L.Lock()
-	defer at.cond.L.Unlock()
-	at.cancelFunc = nil
-	at.cond.Signal()
+	if cancelFunc != nil {
+		cancelFunc()
+	}
+	if done != nil {
+		<-done
+	}
 }