Do not deny logins in isMFARequired (#7739)

* Do not deny logins in `isMFARequired`

This changes `isMFARequired` to return `false` rather than an error
(e.g. not found) when a user is not authorized to access a particular
resource. This defers the authorization denial until such time as the
client actually attempts to connect.

Before Teleport v6.2, clients would immediately attempt to connect to
nodes, and if denied by RBAC rules, would print an "access denied"
error.

As of Teleport v6.2, clients first call `isMFARequired` and, if
unauthorized, abort the connection and print a "node not found"
error. As no connection is ever attempted, no audit log entry is
generated.

Fixes #7652

* Remove dead code

* Ensure users are still asked for MFA if multiple nodes match.

* Remove unnecessary debug log

* Update lib/auth/auth.go

Co-authored-by: Brian Joerger <[email protected]>

* Improve clarity per review feedback

* Fix improper error log

Co-authored-by: Brian Joerger <[email protected]>

lib/auth/auth.go

@@ -2318,7 +2318,6 @@ func (a *Server) isMFARequired(ctx context.Context, checker services.AccessCheck
 	var noMFAAccessErr, notFoundErr error
 	switch t := req.Target.(type) {
 	case *proto.IsMFARequiredRequest_Node:
-		notFoundErr = trace.NotFound("node %q not found", t.Node)
 		if t.Node.Node == "" {
 			return nil, trace.BadParameter("empty Node field")
 		}
@@ -2358,7 +2357,9 @@ func (a *Server) isMFARequired(ctx context.Context, checker services.AccessCheck
 		// If at least one node requires MFA, we'll catch it.
 		for _, n := range matches {
 			err := checker.CheckAccessToServer(t.Node.Login, n, services.AccessMFAParams{AlwaysRequired: false, Verified: false})
-			if err != nil {
+
+			// Ignore other errors; they'll be caught on the real access attempt.
+			if err != nil && errors.Is(err, services.ErrSessionMFARequired) {
 				noMFAAccessErr = err
 				break
 			}
@@ -2421,17 +2422,14 @@ func (a *Server) isMFARequired(ctx context.Context, checker services.AccessCheck
 	// Errors other than ErrSessionMFARequired mean something else is wrong,
 	// most likely access denied.
 	if !errors.Is(noMFAAccessErr, services.ErrSessionMFARequired) {
-		if trace.IsAccessDenied(noMFAAccessErr) {
-			log.Infof("Access to resource denied: %v", noMFAAccessErr)
-			// notFoundErr should always be set by this point, but check it
-			// just in case.
-			if notFoundErr == nil {
-				notFoundErr = trace.NotFound("target resource not found")
-			}
-			// Mask access denied errors to prevent resource name oracles.
-			return nil, trace.Wrap(notFoundErr)
+		if !trace.IsAccessDenied(noMFAAccessErr) {
+			log.WithError(noMFAAccessErr).Warn("Could not determine MFA access")
 		}
-		return nil, trace.Wrap(noMFAAccessErr)
+
+		// Mask the access denied errors by returning false to prevent resource
+		// name oracles. Auth will be denied (and generate an audit log entry)
+		// when the client attempts to connect.
+		return &proto.IsMFARequiredResponse{Required: false}, nil
 	}
 	// If we reach here, the error from AccessChecker was
 	// ErrSessionMFARequired.

lib/auth/grpcserver_test.go

@@ -39,6 +39,7 @@ import (
 	apidefaults "github.com/gravitational/teleport/api/defaults"
 	"github.com/gravitational/teleport/api/metadata"
 	"github.com/gravitational/teleport/api/types"
+	apiutils "github.com/gravitational/teleport/api/utils"
 	"github.com/gravitational/teleport/api/utils/sshutils"
 	"github.com/gravitational/teleport/lib/auth/mocku2f"
 	"github.com/gravitational/teleport/lib/auth/u2f"
@@ -929,6 +930,95 @@ func TestIsMFARequired(t *testing.T) {
 	}
 }
 
+func TestIsMFARequiredUnauthorized(t *testing.T) {
+	ctx := context.Background()
+	srv := newTestTLSServer(t)
+
+	// Enable MFA support.
+	authPref, err := types.NewAuthPreference(types.AuthPreferenceSpecV2{
+		Type:         constants.Local,
+		SecondFactor: constants.SecondFactorOptional,
+		U2F: &types.U2F{
+			AppID:  "teleport",
+			Facets: []string{"teleport"},
+		},
+	})
+	require.NoError(t, err)
+	err = srv.Auth().SetAuthPreference(ctx, authPref)
+	require.NoError(t, err)
+
+	// Register an SSH node.
+	node1 := &types.ServerV2{
+		Kind:    types.KindNode,
+		Version: types.V2,
+		Metadata: types.Metadata{
+			Name:      "node1",
+			Namespace: apidefaults.Namespace,
+			Labels:    map[string]string{"a": "b"},
+		},
+		Spec: types.ServerSpecV2{
+			Hostname: "node1",
+			Addr:     "localhost:3022",
+		},
+	}
+	_, err = srv.Auth().UpsertNode(ctx, node1)
+	require.NoError(t, err)
+
+	// Register another SSH node with a duplicate hostname.
+	node2 := &types.ServerV2{
+		Kind:    types.KindNode,
+		Version: types.V2,
+		Metadata: types.Metadata{
+			Name:      "node2",
+			Namespace: apidefaults.Namespace,
+			Labels:    map[string]string{"a": "c"},
+		},
+		Spec: types.ServerSpecV2{
+			Hostname: "node1",
+			Addr:     "localhost:3022",
+		},
+	}
+	_, err = srv.Auth().UpsertNode(ctx, node2)
+	require.NoError(t, err)
+
+	user, role, err := CreateUserAndRole(srv.Auth(), "alice", []string{"alice"})
+	require.NoError(t, err)
+
+	// Require MFA.
+	roleOpt := role.GetOptions()
+	roleOpt.RequireSessionMFA = true
+	role.SetOptions(roleOpt)
+	role.SetNodeLabels(types.Allow, map[string]apiutils.Strings{"a": []string{"c"}})
+	err = srv.Auth().UpsertRole(ctx, role)
+	require.NoError(t, err)
+
+	cl, err := srv.NewClient(TestUser(user.GetName()))
+	require.NoError(t, err)
+
+	// Call the endpoint for an authorized login. The user is only authorized
+	// for the 2nd node, but should still be asked for MFA.
+	resp, err := cl.IsMFARequired(ctx, &proto.IsMFARequiredRequest{
+		Target: &proto.IsMFARequiredRequest_Node{Node: &proto.NodeLogin{
+			Login: "alice",
+			Node:  "node1",
+		}},
+	})
+	require.NoError(t, err)
+	require.True(t, resp.Required)
+
+	// Call the endpoint for an unauthorized login.
+	resp, err = cl.IsMFARequired(ctx, &proto.IsMFARequiredRequest{
+		Target: &proto.IsMFARequiredRequest_Node{Node: &proto.NodeLogin{
+			Login: "bob",
+			Node:  "node1",
+		}},
+	})
+
+	// When unauthorized, expect a silent `false`.
+	require.NoError(t, err)
+	require.False(t, resp.Required)
+}
+
 func TestDeleteLastMFADevice(t *testing.T) {
 	ctx := context.Background()
 	srv := newTestTLSServer(t)