Improve performance of MFA ceremony

api/proto/teleport/legacy/client/proto/authservice.proto

@@ -324,6 +324,11 @@ message UserCertsRequest {
   // An optional field, that when provided, the response will be validated
   // and the ID of the validated MFA device will be stored in the certificate.
   MFAAuthenticateResponse MFAResponse = 18 [(gogoproto.jsontag) = "mfa_response,omitempty"];
+
+  // SSHLogin is the OS Login for the SSH session that the certificate will be used for.
+  // This login is used when performing RBAC checks to determine if MFA is required
+  // to access the resource.
+  string SSHLogin = 19;
 }
 
 // RouteToDatabase combines parameters for database service routing information.
@@ -1067,6 +1072,20 @@ enum DeviceUsage {
   DEVICE_USAGE_PASSWORDLESS = 2;
 }
 
+// MFARequired indicates if MFA is required to access a
+// resource.
+enum MFARequired {
+  // Indicates the client/server are either old and don't support
+  // checking if MFA is required during the ceremony or that there
+  // was a catastrophic error checking RBAC to determine if completing
+  // an MFA ceremony will grant access to a resource.
+  MFA_REQUIRED_UNSPECIFIED = 0;
+  // Completing an MFA ceremony will grant access to a resource.
+  MFA_REQUIRED_YES = 1;
+  // Completing an MFA ceremony will not grant access to a resource.
+  MFA_REQUIRED_NO = 2;
+}
+
 // MFAAuthenticateChallenge is a challenge for all MFA devices registered for a
 // user.
 message MFAAuthenticateChallenge {
@@ -1081,6 +1100,11 @@ message MFAAuthenticateChallenge {
   // credentials for the ceremony (one for each U2F or Webauthn device
   // registered by the user).
   webauthn.CredentialAssertion WebauthnChallenge = 3;
+  // MFRequired indicates whether proceeding with the MFA ceremony will
+  // grant access to the resource. If `MFA_REQUIRED_NO` is returned by the
+  // server then the stream will be terminated to prevent a fruitless MFA ceremony from
+  // proceeding.
+  MFARequired MFARequired = 4;
 }
 
 // MFAAuthenticateResponse is a response to MFAAuthenticateChallenge using one

integration/integration_test.go

@@ -40,6 +40,7 @@ import (
 	"strconv"
 	"strings"
 	"sync"
+	"sync/atomic"
 	"testing"
 	"time"
 
@@ -1280,26 +1281,19 @@ func testEscapeSequenceNoTrigger(t *testing.T, terminal *Terminal, sess <-chan e
 }
 
 type localAddr struct {
-	mu   sync.Mutex
-	addr net.Addr
+	addr atomic.Pointer[net.Addr]
 }
 
 func (a *localAddr) set(addr net.Addr) {
-	a.mu.Lock()
-	defer a.mu.Unlock()
-
-	a.addr = addr
+	a.addr.CompareAndSwap(nil, &addr)
 }
 
 func (a *localAddr) get() net.Addr {
-	a.mu.Lock()
-	defer a.mu.Unlock()
-
-	if a.addr == nil {
-		return &utils.NetAddr{}
+	addr := a.addr.Load()
+	if addr != nil {
+		return *addr
 	}
-
-	return a.addr
+	return &utils.NetAddr{}
 }
 
 // testIPPropagation makes sure that we can correctly propagate initial client IP observed by proxy.
@@ -1413,7 +1407,7 @@ func testIPPropagation(t *testing.T, suite *integrationTestSuite) {
 		tc.Stdout = person
 		tc.Stdin = person
 
-		local := &localAddr{}
+		local := localAddr{}
 
 		tc.Config.DialOpts = []grpc.DialOption{
 			grpc.WithContextDialer(func(ctx context.Context, s string) (net.Conn, error) {
@@ -1459,7 +1453,7 @@ func testIPPropagation(t *testing.T, suite *integrationTestSuite) {
 		})
 		require.NoError(t, err)
 
-		local := &localAddr{}
+		local := localAddr{}
 
 		tc.Config.DialOpts = []grpc.DialOption{
 			grpc.WithContextDialer(func(ctx context.Context, s string) (net.Conn, error) {
@@ -2117,7 +2111,7 @@ func testEnvironmentVariables(t *testing.T, suite *integrationTestSuite) {
 }
 
 // TestInvalidLogins validates that you can't login with invalid login or
-// with invalid 'site' parameter
+// with invalid 'cluster' parameter
 func testInvalidLogins(t *testing.T, suite *integrationTestSuite) {
 	tr := utils.NewTracer(utils.ThisFunction()).Start()
 	defer tr.Stop()
@@ -2139,8 +2133,7 @@ func testInvalidLogins(t *testing.T, suite *integrationTestSuite) {
 	require.NoError(t, err)
 
 	err = tc.SSH(context.Background(), cmd, false)
-	require.True(t, trace.IsNotFound(err))
-	require.Contains(t, err.Error(), `cluster "wrong-site" is not found`)
+	require.ErrorIs(t, err, trace.NotFound("failed to dial target host\n\tcluster \"wrong-site\" is not found"))
 }
 
 // TestTwoClustersTunnel creates two teleport clusters: "a" and "b" and creates a
@@ -2994,12 +2987,10 @@ func trustedClusters(t *testing.T, suite *integrationTestSuite, test trustedClus
 	})
 	require.True(t, trace.IsNotFound(err))
 
-	// ListNodes expect labels as a value of host
-	tc.Host = ""
+	// check that we can list resources for the cluster.
 	servers, err := tc.ListNodesWithFilters(ctx)
 	require.NoError(t, err)
 	require.Len(t, servers, 2)
-	tc.Host = Loopback
 
 	// check that remote cluster has been provisioned
 	remoteClusters, err := main.Process.GetAuthServer().GetRemoteClusters()

lib/auth/auth.go

@@ -4571,23 +4571,17 @@ func (a *Server) isMFARequired(ctx context.Context, checker services.AccessCheck
 		if t.Node.Login == "" {
 			return nil, trace.BadParameter("empty Login field")
 		}
+
 		// Find the target node and check whether MFA is required.
-		nodes, err := a.GetNodes(ctx, apidefaults.Namespace)
+		matches, err := client.GetResourcesWithFilters(ctx, a, proto.ListResourcesRequest{
+			ResourceType:   types.KindNode,
+			Namespace:      apidefaults.Namespace,
+			SearchKeywords: []string{t.Node.Node},
+		})
 		if err != nil {
 			return nil, trace.Wrap(err)
 		}
-		var matches []types.Server
-		for _, n := range nodes {
-			// Get the server address without port number.
-			addr, _, err := net.SplitHostPort(n.GetAddr())
-			if err != nil {
-				addr = n.GetAddr()
-			}
-			// Match NodeName to UUID, hostname or self-reported server address.
-			if n.GetName() == t.Node.Node || n.GetHostname() == t.Node.Node || addr == t.Node.Node {
-				matches = append(matches, n)
-			}
-		}
+
 		if len(matches) == 0 {
 			// If t.Node.Node is not a known registered node, it may be an
 			// unregistered host running OpenSSH with a certificate created via
@@ -4600,10 +4594,25 @@ func (a *Server) isMFARequired(ctx context.Context, checker services.AccessCheck
 			// connection.
 			return &proto.IsMFARequiredResponse{Required: false}, nil
 		}
+
 		// Check RBAC against all matching nodes and return the first error.
 		// If at least one node requires MFA, we'll catch it.
 		for _, n := range matches {
-			err := checker.CheckAccess(
+			srv, ok := n.(types.Server)
+			if !ok {
+				continue
+			}
+			// Get the server address without port number.
+			addr, _, err := net.SplitHostPort(srv.GetAddr())
+			if err != nil {
+				addr = srv.GetAddr()
+			}
+			// Filter out any matches on labels before checking access
+			if n.GetName() != t.Node.Node && srv.GetHostname() != t.Node.Node && addr != t.Node.Node {
+				continue
+			}
+
+			err = checker.CheckAccess(
 				n,
 				services.AccessState{},
 				services.NewLoginMatcher(t.Node.Login),

lib/auth/grpcserver.go

@@ -2437,9 +2437,26 @@ func (g *GRPCServer) GenerateUserSingleUseCerts(stream proto.AuthService_Generat
 		return trace.Wrap(err)
 	}
 
+	mfaRequired := proto.MFARequired_MFA_REQUIRED_UNSPECIFIED
+	if required, err := isMFARequiredForSingleUseCertRequest(ctx, actx, initReq); err == nil {
+		// If MFA is not required to gain access to the resource then let the client
+		// know and abort the ceremony.
+		if !required {
+			return trace.Wrap(stream.Send(&proto.UserSingleUseCertsResponse{
+				Response: &proto.UserSingleUseCertsResponse_MFAChallenge{
+					MFAChallenge: &proto.MFAAuthenticateChallenge{
+						MFARequired: proto.MFARequired_MFA_REQUIRED_NO,
+					},
+				},
+			}))
+		}
+
+		mfaRequired = proto.MFARequired_MFA_REQUIRED_YES
+	}
+
 	// 2. send MFAChallenge
 	// 3. receive and validate MFAResponse
-	mfaDev, err := userSingleUseCertsAuthChallenge(actx, stream)
+	mfaDev, err := userSingleUseCertsAuthChallenge(actx, stream, mfaRequired)
 	if err != nil {
 		g.Entry.Debugf("Failed to perform single-use cert challenge: %v", err)
 		return trace.Wrap(err)
@@ -2508,14 +2525,46 @@ func validateUserSingleUseCertRequest(ctx context.Context, actx *grpcContext, re
 	return nil
 }
 
+// isMFARequiredForSingleUseCertRequest validates that mfa is actually required for
+// the target of the single-use user cert.
+func isMFARequiredForSingleUseCertRequest(ctx context.Context, actx *grpcContext, req *proto.UserCertsRequest) (bool, error) {
+	mfaReq := &proto.IsMFARequiredRequest{}
+
+	switch req.Usage {
+	case proto.UserCertsRequest_SSH:
+		// An old or non-conforming client did not provide a login which means rbac
+		// won't be able to accurately determine if mfa is required.
+		if req.SSHLogin == "" {
+			return false, trace.BadParameter("no ssh login provided")
+		}
+
+		mfaReq.Target = &proto.IsMFARequiredRequest_Node{Node: &proto.NodeLogin{Node: req.NodeName, Login: req.SSHLogin}}
+	case proto.UserCertsRequest_Kubernetes:
+		mfaReq.Target = &proto.IsMFARequiredRequest_KubernetesCluster{KubernetesCluster: req.KubernetesCluster}
+	case proto.UserCertsRequest_Database:
+		mfaReq.Target = &proto.IsMFARequiredRequest_Database{Database: &req.RouteToDatabase}
+	case proto.UserCertsRequest_WindowsDesktop:
+		mfaReq.Target = &proto.IsMFARequiredRequest_WindowsDesktop{WindowsDesktop: &req.RouteToWindowsDesktop}
+	default:
+		return false, trace.BadParameter("unknown certificate Usage %q", req.Usage)
+	}
+
+	resp, err := actx.IsMFARequired(ctx, mfaReq)
+	if err != nil {
+		return false, trace.Wrap(err)
+	}
+
+	return resp.Required, nil
+}
+
 // isDBLocalProxyTunnelCertReq returns whether a cert request is for
 // a database cert and the requester is a local proxy tunnel.
 func isDBLocalProxyTunnelCertReq(req *proto.UserCertsRequest) bool {
 	return req.Usage == proto.UserCertsRequest_Database &&
 		req.RequesterName == proto.UserCertsRequest_TSH_DB_LOCAL_PROXY_TUNNEL
 }
 
-func userSingleUseCertsAuthChallenge(gctx *grpcContext, stream proto.AuthService_GenerateUserSingleUseCertsServer) (*types.MFADevice, error) {
+func userSingleUseCertsAuthChallenge(gctx *grpcContext, stream proto.AuthService_GenerateUserSingleUseCertsServer, mfaRequired proto.MFARequired) (*types.MFADevice, error) {
 	ctx := stream.Context()
 	auth := gctx.authServer
 	user := gctx.User.GetName()
@@ -2528,6 +2577,9 @@ func userSingleUseCertsAuthChallenge(gctx *grpcContext, stream proto.AuthService
 	if challenge.TOTP == nil && challenge.WebauthnChallenge == nil {
 		return nil, trace.AccessDenied("MFA is required to access this resource but user has no MFA devices; use 'tsh mfa add' to register MFA devices")
 	}
+
+	challenge.MFARequired = mfaRequired
+
 	if err := stream.Send(&proto.UserSingleUseCertsResponse{
 		Response: &proto.UserSingleUseCertsResponse_MFAChallenge{MFAChallenge: challenge},
 	}); err != nil {