fix: Verify MFA device locks during authentication

lib/auth/accountrecovery_test.go

@@ -1317,7 +1317,7 @@ func TestGetAccountRecoveryCodes(t *testing.T) {
 
 func triggerLoginLock(t *testing.T, srv *Server, username string) {
 	for i := 1; i <= defaults.MaxLoginAttempts; i++ {
-		_, _, err := srv.authenticateUser(context.Background(), AuthenticateUserRequest{
+		_, _, _, err := srv.authenticateUser(context.Background(), AuthenticateUserRequest{
 			Username: username,
 			OTP:      &OTPCreds{},
 		})

lib/auth/auth_test.go

@@ -61,6 +61,7 @@ import (
 	"github.com/gravitational/teleport/lib/auth/keystore"
 	"github.com/gravitational/teleport/lib/auth/native"
 	"github.com/gravitational/teleport/lib/auth/testauthority"
+	wantypes "github.com/gravitational/teleport/lib/auth/webauthntypes"
 	"github.com/gravitational/teleport/lib/authz"
 	"github.com/gravitational/teleport/lib/backend"
 	"github.com/gravitational/teleport/lib/backend/lite"
@@ -522,6 +523,130 @@ func TestAuthenticateSSHUser(t *testing.T) {
 	require.Error(t, err)
 }
 
+func TestAuthenticateUser_mfaDeviceLocked(t *testing.T) {
+	t.Parallel()
+
+	testServer := newTestTLSServer(t)
+	authServer := testServer.Auth()
+
+	ctx := context.Background()
+	const user = "llama"
+	const pass = "supersecret!!1!one"
+
+	// Configure auth preferences.
+	authPref, err := authServer.GetAuthPreference(ctx)
+	require.NoError(t, err, "GetAuthPreference")
+	authPref.SetSecondFactor(constants.SecondFactorOptional) // good enough
+	authPref.SetWebauthn(&types.Webauthn{
+		RPID: "localhost",
+	})
+	require.NoError(t,
+		authServer.SetAuthPreference(ctx, authPref),
+		"SetAuthPreference")
+
+	// Prepare user, password and MFA device.
+	_, _, err = CreateUserAndRole(authServer, user, []string{user}, nil /* allowRules */)
+	require.NoError(t, err, "CreateUserAndRole")
+	require.NoError(t,
+		authServer.UpsertPassword(user, []byte(pass)),
+		"UpsertPassword")
+
+	userClient, err := testServer.NewClient(TestUser(user))
+	require.NoError(t, err, "NewClient")
+
+	// OTP devices would work for this test too.
+	dev1, err := RegisterTestDevice(ctx, userClient, "dev1", proto.DeviceType_DEVICE_TYPE_WEBAUTHN, nil /* authenticator */)
+	require.NoError(t, err, "RegisterTestDevice")
+	dev2, err := RegisterTestDevice(ctx, userClient, "dev2", proto.DeviceType_DEVICE_TYPE_WEBAUTHN, dev1 /* authenticator */)
+	require.NoError(t, err, "RegisterTestDevice")
+
+	// Prepare an SSH public key for testing.
+	privKey, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader)
+	require.NoError(t, err, "GenerateKey")
+	signer, err := ssh.NewSignerFromSigner(privKey)
+	require.NoError(t, err, "NewSignerFromSigner")
+	pubKey := ssh.MarshalAuthorizedKey(signer.PublicKey())
+
+	// Users initially authenticate via Proxy, as there isn't a userClient before
+	// authn.
+	proxyClient, err := testServer.NewClient(TestBuiltin(types.RoleProxy))
+	require.NoError(t, err, "NewClient")
+
+	authenticateSSH := func(dev *TestDevice) (*SSHLoginResponse, error) {
+		chal, err := proxyClient.CreateAuthenticateChallenge(ctx, &proto.CreateAuthenticateChallengeRequest{
+			Request: &proto.CreateAuthenticateChallengeRequest_UserCredentials{
+				UserCredentials: &proto.UserCredentials{
+					Username: user,
+					Password: []byte(pass),
+				},
+			},
+		})
+		if err != nil {
+			return nil, fmt.Errorf("create challenge: %w", err)
+		}
+
+		chalResp, err := dev.SolveAuthn(chal)
+		if err != nil {
+			return nil, fmt.Errorf("solve challenge: %w", err)
+		}
+
+		return proxyClient.AuthenticateSSHUser(ctx, AuthenticateSSHRequest{
+			AuthenticateUserRequest: AuthenticateUserRequest{
+				Username:  user,
+				PublicKey: pubKey,
+				Pass: &PassCreds{
+					Password: []byte(pass),
+				},
+				Webauthn: wantypes.CredentialAssertionResponseFromProto(chalResp.GetWebauthn()),
+			},
+			TTL: 1 * time.Hour,
+		})
+	}
+
+	// Lock dev1.
+	const lockMessage = "device locked for testing"
+	lock, err := types.NewLock("dev1-lock", types.LockSpecV2{
+		Target: types.LockTarget{
+			MFADevice: dev1.MFA.Id,
+		},
+		Message: lockMessage,
+	})
+	require.NoError(t, err, "NewLock")
+	require.NoError(t,
+		userClient.UpsertLock(ctx, lock),
+		"UpsertLock")
+
+	t.Run("locked device", func(t *testing.T) {
+		_, err := authenticateSSH(dev1)
+		assert.ErrorContains(t, err, lockMessage)
+	})
+
+	t.Run("unlocked device", func(t *testing.T) {
+		_, err := authenticateSSH(dev2)
+		assert.NoError(t, err, "authenticateSSH failed unexpectedly")
+	})
+
+	t.Run("locked device password change", func(t *testing.T) {
+		chal, err := userClient.CreateAuthenticateChallenge(ctx, &proto.CreateAuthenticateChallengeRequest{
+			Request: &proto.CreateAuthenticateChallengeRequest_ContextUser{},
+		})
+		require.NoError(t, err, "CreateAuthenticateChallenge")
+
+		// dev1 is still locked.
+		chalResp, err := dev1.SolveAuthn(chal)
+		require.NoError(t, err, "SolveAuthn")
+
+		assert.ErrorContains(t,
+			userClient.ChangePassword(ctx, &proto.ChangePasswordRequest{
+				User:        user,
+				OldPassword: []byte(pass),
+				NewPassword: []byte("evenmoresecret!!1!ONE"),
+				Webauthn:    chalResp.GetWebauthn(),
+			}),
+			lockMessage)
+	})
+}
+
 func TestUserLock(t *testing.T) {
 	t.Parallel()
 	s := newAuthSuite(t)

lib/auth/methods.go

@@ -119,7 +119,7 @@ type SessionCreds struct {
 func (a *Server) AuthenticateUser(ctx context.Context, req AuthenticateUserRequest) (services.UserState, services.AccessChecker, error) {
 	username := req.Username
 
-	mfaDev, actualUsername, err := a.authenticateUser(ctx, req)
+	verifyMFALocks, mfaDev, actualUsername, err := a.authenticateUser(ctx, req)
 	if err != nil {
 		// Log event after authentication failure
 		if err := a.emitAuthAuditEvent(ctx, authAuditProps{
@@ -167,6 +167,23 @@ func (a *Server) AuthenticateUser(ctx context.Context, req AuthenticateUserReque
 		return nil, nil, trace.Wrap(err)
 	}
 
+	// Verify if the MFA device is locked.
+	if err := verifyMFALocks(verifyMFADeviceLocksParams{
+		Checker: checker,
+	}); err != nil {
+		// Log MFA lock failure as an authn failure.
+		if err := a.emitAuthAuditEvent(ctx, authAuditProps{
+			username:       req.Username,
+			clientMetadata: req.ClientMetadata,
+			mfaDevice:      mfaDev,
+			checker:        checker,
+			authErr:        err,
+		}); err != nil {
+			log.WithError(err).Warn("Failed to emit login event.")
+		}
+		return nil, nil, trace.Wrap(err)
+	}
+
 	// Log event after authentication success
 	if err := a.emitAuthAuditEvent(ctx, authAuditProps{
 		username:       username,
@@ -260,14 +277,77 @@ func IsInvalidLocalCredentialError(err error) bool {
 	return errors.Is(err, invalidUserPassError) || errors.Is(err, invalidUserPass2FError)
 }
 
+type verifyMFADeviceLocksParams struct {
+	// Checker used to verify locks.
+	// Optional, created via a [UserState] fetch if nil.
+	Checker services.AccessChecker
+
+	// ClusterLockingMode used to verify locks.
+	// Optional, acquired from [Server.GetAuthPreference] if nil.
+	ClusterLockingMode constants.LockingMode
+}
+
 // authenticateUser authenticates a user through various methods (password, MFA,
 // passwordless)
-// Returns the device used to authenticate (if applicable) and the username.
-func (a *Server) authenticateUser(ctx context.Context, req AuthenticateUserRequest) (*types.MFADevice, string, error) {
+//
+// Returns a callback to verify MFA device locks, the MFA device used to
+// authenticate (if applicable), and the authenticated user name.
+//
+// Callers MUST call the verifyLocks callback.
+func (a *Server) authenticateUser(
+	ctx context.Context,
+	req AuthenticateUserRequest,
+) (verifyLocks func(verifyMFADeviceLocksParams) error, mfaDev *types.MFADevice, user string, err error) {
+	mfaDev, user, err = a.authenticateUserInternal(ctx, req)
+	if err != nil || mfaDev == nil {
+		return func(verifyMFADeviceLocksParams) error { return nil }, mfaDev, user, trace.Wrap(err)
+	}
+
+	verifyLocks = func(p verifyMFADeviceLocksParams) error {
+		if p.Checker == nil {
+			userState, err := a.GetUserOrLoginState(ctx, user)
+			if err != nil {
+				return trace.Wrap(err)
+			}
+			accessInfo := services.AccessInfoFromUserState(userState)
+			clusterName, err := a.GetClusterName()
+			if err != nil {
+				return trace.Wrap(err)
+			}
+			checker, err := services.NewAccessChecker(accessInfo, clusterName.GetClusterName(), a)
+			if err != nil {
+				return trace.Wrap(err)
+			}
+			p.Checker = checker
+		}
+
+		if p.ClusterLockingMode == "" {
+			authPref, err := a.GetAuthPreference(ctx)
+			if err != nil {
+				return trace.Wrap(err)
+			}
+			p.ClusterLockingMode = authPref.GetLockingMode()
+		}
+
+		// The MFA device needs to be explicitly verified, as it won't be verified
+		// as part of certificate issuance in various scenarios (password change,
+		// non-session certificates, etc)
+		return a.verifyLocksForUserCerts(verifyLocksForUserCertsReq{
+			checker:     p.Checker,
+			defaultMode: p.ClusterLockingMode,
+			username:    user,
+			mfaVerified: mfaDev.Id,
+		})
+	}
+	return verifyLocks, mfaDev, user, nil
+}
+
+// Do not use this method directly, use authenticateUser instead.
+func (a *Server) authenticateUserInternal(ctx context.Context, req AuthenticateUserRequest) (mfaDev *types.MFADevice, user string, err error) {
 	if err := req.CheckAndSetDefaults(); err != nil {
 		return nil, "", trace.Wrap(err)
 	}
-	user := req.Username
+	user = req.Username
 	passwordless := user == ""
 
 	// Only one path if passwordless, other variants shouldn't see an empty user.
@@ -299,8 +379,8 @@ func (a *Server) authenticateUser(ctx context.Context, req AuthenticateUserReque
 					Webauthn: wantypes.CredentialAssertionResponseToProto(req.Webauthn),
 				},
 			}
-			dev, _, err := a.ValidateMFAAuthResponse(ctx, mfaResponse, user, passwordless)
-			return dev, trace.Wrap(err)
+			mfaDev, _, err := a.ValidateMFAAuthResponse(ctx, mfaResponse, user, passwordless)
+			return mfaDev, trace.Wrap(err)
 		}
 		authErr = authenticateWebauthnError
 	case req.OTP != nil:
@@ -316,10 +396,9 @@ func (a *Server) authenticateUser(ctx context.Context, req AuthenticateUserReque
 		authErr = invalidUserPass2FError
 	}
 	if authenticateFn != nil {
-		var dev *types.MFADevice
 		err := a.WithUserLock(ctx, user, func() error {
 			var err error
-			dev, err = authenticateFn()
+			mfaDev, err = authenticateFn()
 			return err
 		})
 		switch {
@@ -330,13 +409,13 @@ func (a *Server) authenticateUser(ctx context.Context, req AuthenticateUserReque
 			}
 
 			return nil, "", trace.Wrap(authErr)
-		case dev == nil:
+		case mfaDev == nil:
 			log.Debugf(
 				"MFA authentication returned nil device (Webauthn = %v, TOTP = %v, Headless = %v): %v.",
 				req.Webauthn != nil, req.OTP != nil, req.HeadlessAuthenticationID != "", err)
 			return nil, "", trace.Wrap(authErr)
 		default:
-			return dev, user, nil
+			return mfaDev, user, nil
 		}
 	}
 

lib/auth/password.go

@@ -140,7 +140,12 @@ func (a *Server) ChangePassword(ctx context.Context, req *proto.ChangePasswordRe
 			Token:    req.SecondFactorToken,
 		}
 	}
-	if _, _, err := a.authenticateUser(ctx, authReq); err != nil {
+	verifyMFALocks, _, _, err := a.authenticateUser(ctx, authReq)
+	if err != nil {
+		return trace.Wrap(err)
+	}
+	// Verify if the MFA device used is locked.
+	if err := verifyMFALocks(verifyMFADeviceLocksParams{}); err != nil {
 		return trace.Wrap(err)
 	}
 

lib/auth/password_test.go

@@ -31,6 +31,7 @@ import (
 	"github.com/pquerna/otp/totp"
 	"github.com/stretchr/testify/require"
 
+	"github.com/gravitational/teleport"
 	"github.com/gravitational/teleport/api/client/proto"
 	"github.com/gravitational/teleport/api/constants"
 	"github.com/gravitational/teleport/api/types"
@@ -89,6 +90,16 @@ func setupPasswordSuite(t *testing.T) *passwordSuite {
 	err = s.a.SetClusterName(clusterName)
 	require.NoError(t, err)
 
+	// set lock watcher
+	lockWatcher, err := services.NewLockWatcher(ctx, services.LockWatcherConfig{
+		ResourceWatcherConfig: services.ResourceWatcherConfig{
+			Component: teleport.ComponentAuth,
+			Client:    s.a,
+		},
+	})
+	require.NoError(t, err, "NewLockWatcher")
+	s.a.SetLockWatcher(lockWatcher)
+
 	// set static tokens
 	staticTokens, err := types.NewStaticTokens(types.StaticTokensSpecV2{
 		StaticTokens: []types.ProvisionTokenV1{},