Make MFA token redemption atomic to prevent multiple sessions

Resolves #16770

The MFA login token redemption path (`POST /api/latest/fleet/sessions`)
read the one-time verification token with a non-locking `SELECT` on the
read replica, then created a session and deleted the token in a
*separate* transaction without verifying the token was still present.
Concurrent requests carrying the same token each passed the `SELECT` and
each minted a distinct session, breaking the single-use guarantee.

`SessionByMFAToken` now consumes the token and creates the session
inside a single transaction:

- The token row is locked with `SELECT ... FOR UPDATE`, then deleted,
and the delete's rows-affected count is confirmed non-zero before the
session is created.
- Concurrent redemptions serialize on the row lock; the loser re-reads
after the winner commits the delete, finds no row, and aborts before
creating a session.
- The user is still loaded *before* the transaction, so a
concurrently-deleted user or a transient read error leaves the token
intact for retry (preserving the pre-fix atomicity behavior).

---------

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Juan Fernandez <juan@fleetdm.com>

changes/16770-mfa-token-race

@@ -0,0 +1 @@
+* Made MFA login token redemption atomic so a single one-time token can no longer be used to create more than one session under concurrent requests.

server/datastore/mysql/sessions.go

@@ -31,6 +31,9 @@ func (ds *Datastore) SessionByMFAToken(ctx context.Context, token string, sessio
 		return nil, nil, err
 	}
 
+	// Load the user before consuming the token: if this fails (e.g. the user was
+	// concurrently deleted or a transient read error occurs) the token is left
+	// intact so the login link can be retried, matching the pre-fix behavior.
 	user, err := ds.UserByID(ctx, userID)
 	if err != nil {
 		return nil, nil, err
@@ -38,12 +41,35 @@ func (ds *Datastore) SessionByMFAToken(ctx context.Context, token string, sessio
 
 	var session *fleet.Session
 	err = ds.withTx(ctx, func(tx sqlx.ExtContext) error {
-		if session, err = ds.makeSessionInTransaction(ctx, tx, user.ID, sessionKeySize); err != nil {
-			return err
+		// Lock the token row and re-check its validity so that concurrent
+		// redemptions of the same one-time token are serialized. The loser of the
+		// race blocks here, re-reads after the winner commits its delete, finds no
+		// row, and aborts before creating a session.
+		var lockedUserID uint
+		err := sqlx.GetContext(
+			ctx,
+			tx,
+			&lockedUserID,
+			"SELECT user_id FROM verification_tokens WHERE token = ? AND created_at >= NOW() - INTERVAL ? SECOND FOR UPDATE",
+			token,
+			fleet.MFALinkTTL.Seconds(),
+		)
+		if err != nil {
+			if errors.Is(err, sql.ErrNoRows) {
+				return ctxerr.Wrap(ctx, notFound("Verification Token"))
+			}
+			return ctxerr.Wrap(ctx, err, "selecting verification token")
 		}
 
-		// only delete token once we've successfully consumed it
-		if _, err = tx.ExecContext(ctx, "DELETE FROM verification_tokens WHERE token = ?", token); err != nil {
+		if lockedUserID != user.ID {
+			return ctxerr.Wrap(ctx, notFound("Verification Token"))
+		}
+
+		if _, err := tx.ExecContext(ctx, "DELETE FROM verification_tokens WHERE token = ?", token); err != nil {
+			return ctxerr.Wrap(ctx, err, "deleting verification token")
+		}
+
+		if session, err = ds.makeSessionInTransaction(ctx, tx, lockedUserID, sessionKeySize); err != nil {
 			return err
 		}
 

server/datastore/mysql/sessions_test.go

@@ -2,6 +2,7 @@ package mysql
 
 import (
 	"context"
+	"sync"
 	"testing"
 	"time"
 
@@ -78,6 +79,63 @@ func testMFA(t *testing.T, ds *Datastore) {
 	require.Error(t, err)
 	require.Nil(t, mfaUser)
 	require.Nil(t, session)
+
+	// concurrent redemptions of the same token must only ever mint one session
+	sessionsBefore, err := ds.ListSessionsForUser(context.Background(), user.ID)
+	require.NoError(t, err)
+
+	token, err = ds.NewMFAToken(context.Background(), user.ID)
+	require.NoError(t, err)
+	require.NotEmpty(t, token)
+
+	const concurrentRedemptions = 8
+	var (
+		wg         sync.WaitGroup
+		mu         sync.Mutex
+		successes  int
+		lastErr    error
+		successKey string
+	)
+	wg.Add(concurrentRedemptions)
+	for range concurrentRedemptions {
+		go func() {
+			defer wg.Done()
+			s, _, err := ds.SessionByMFAToken(context.Background(), token, 8)
+			mu.Lock()
+			defer mu.Unlock()
+			if err != nil {
+				lastErr = err
+				return
+			}
+			successes++
+			if s != nil {
+				successKey = s.Key
+			}
+		}()
+	}
+	wg.Wait()
+
+	require.Equal(t, 1, successes, "exactly one concurrent redemption should succeed")
+	require.Error(t, lastErr, "losing redemptions should return an error")
+
+	// the token must be consumed and exactly one new session created for the user
+	sessionsAfter, err := ds.ListSessionsForUser(context.Background(), user.ID)
+	require.NoError(t, err)
+	require.Len(t, sessionsAfter, len(sessionsBefore)+1)
+	require.Contains(t, sessionKeys(sessionsAfter), successKey)
+
+	session, mfaUser, err = ds.SessionByMFAToken(context.Background(), token, 8)
+	require.Error(t, err)
+	require.Nil(t, mfaUser)
+	require.Nil(t, session)
+}
+
+func sessionKeys(sessions []*fleet.Session) []string {
+	keys := make([]string, 0, len(sessions))
+	for _, s := range sessions {
+		keys = append(keys, s.Key)
+	}
+	return keys
 }
 
 func testSessionsGetters(t *testing.T, ds *Datastore) {