always require MFA when joining a session if at least one role requir…

…es session MFA

* extend role 'join_sessions' so MFA can be required at the role level when joining sessions

* add test cases

* update operator CRDs

* remove change to 'join_sessions', instead require MFA if any roles require it

* add comment

* address feedback

* add additional test cases

* add authhandler test

* fix role from test case not getting used in test

lib/auth/auth.go

@@ -2586,8 +2586,8 @@ func generateCert(ctx context.Context, a *Server, req certRequest, caType types.
 		CertificateType: events.CertificateTypeUser,
 		Identity:        &eventIdentity,
 		ClientMetadata: apievents.ClientMetadata{
-			//TODO(greedy52) currently only user-agent from GRPC clients are
-			//fetched. Need to propagate user-agent from HTTP calls.
+			// TODO(greedy52) currently only user-agent from GRPC clients are
+			// fetched. Need to propagate user-agent from HTTP calls.
 			UserAgent: trimUserAgent(metadata.UserAgentFromContext(ctx)),
 		},
 	}); err != nil {
@@ -4178,6 +4178,7 @@ func (a *Server) DeleteNamespace(namespace string) error {
 	}
 	return a.Services.DeleteNamespace(namespace)
 }
+
 func (a *Server) CreateAccessRequest(ctx context.Context, req types.AccessRequest, identity tlsca.Identity) error {
 	_, err := a.CreateAccessRequestV2(ctx, req, identity)
 	return trace.Wrap(err)
@@ -5266,6 +5267,21 @@ func (a *Server) isMFARequired(ctx context.Context, checker services.AccessCheck
 			return nil, trace.BadParameter("empty Login field")
 		}
 
+		// state.MFARequired is "per-role", so if the user is joining
+		// a session, MFA is required no matter what node they are
+		// connecting to. We don't preform an RBAC check like we do
+		// below when users are starting a session to selectively
+		// require MFA because we don't know what session the user
+		// is joining, nor do we know what role allowed the session
+		// creator to start the session that is attempting to be joined.
+		// We need this info to be able to selectively skip MFA in
+		// this case.
+		if t.Node.Login == teleport.SSHSessionJoinPrincipal {
+			return &proto.IsMFARequiredResponse{
+				Required: true,
+			}, nil
+		}
+
 		// Find the target node and check whether MFA is required.
 		matches, err := client.GetResourcesWithFilters(ctx, a, proto.ListResourcesRequest{
 			ResourceType:   types.KindNode,

lib/auth/auth_login_test.go

@@ -20,10 +20,12 @@ import (
 	"testing"
 	"time"
 
+	"github.com/google/uuid"
 	"github.com/gravitational/trace"
 	"github.com/stretchr/testify/assert"
 	"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"
@@ -339,6 +341,203 @@ func TestCreateAuthenticateChallenge_WithRecoveryStartToken(t *testing.T) {
 	}
 }
 
+func TestCreateAuthenticateChallenge_mfaVerification(t *testing.T) {
+	t.Parallel()
+
+	testServer := newTestTLSServer(t)
+	ctx := context.Background()
+
+	adminClient, err := testServer.NewClient(TestBuiltin(types.RoleAdmin))
+	require.NoError(t, err, "NewClient(types.RoleAdmin)")
+
+	// Register a couple of SSH nodes.
+	registerNode := func(node, env string) error {
+		_, err := adminClient.UpsertNode(ctx, &types.ServerV2{
+			Kind:    types.KindNode,
+			Version: types.V2,
+			Metadata: types.Metadata{
+				Name: uuid.NewString(),
+				Labels: map[string]string{
+					"env": env,
+				},
+			},
+			Spec: types.ServerSpecV2{
+				Hostname: node,
+			},
+		})
+		return err
+	}
+	const devNode = "node1"
+	const prodNode = "node2"
+	require.NoError(t, registerNode(devNode, "dev"), "registerNode(%q)", devNode)
+	require.NoError(t, registerNode(prodNode, "prod"), "registerNode(%q)", prodNode)
+
+	// Create an MFA required role for "prod" nodes.
+	prodRole, err := types.NewRole("prod_access", types.RoleSpecV6{
+		Options: types.RoleOptions{
+			RequireMFAType: types.RequireMFAType_SESSION,
+		},
+		Allow: types.RoleConditions{
+			Logins: []string{"{{internal.logins}}"},
+			NodeLabels: types.Labels{
+				"env": []string{"prod"},
+			},
+		},
+	})
+	require.NoError(t, err, "NewRole(prod)")
+	err = adminClient.UpsertRole(ctx, prodRole)
+	require.NoError(t, err, "UpsertRole(%q)", prodRole.GetName())
+
+	// Create a role that requires MFA when joining sessions
+	joinMFARole, err := types.NewRole("mfa_session_join", types.RoleSpecV6{
+		Options: types.RoleOptions{
+			RequireMFAType: types.RequireMFAType_SESSION,
+		},
+		Allow: types.RoleConditions{
+			Logins: []string{"{{internal.logins}}"},
+			NodeLabels: types.Labels{
+				"env": []string{"*"},
+			},
+			JoinSessions: []*types.SessionJoinPolicy{
+				{
+					Name:  "session_join",
+					Roles: []string{"access"},
+					Kinds: []string{string(types.SSHSessionKind)},
+					Modes: []string{string(types.SessionPeerMode)},
+				},
+			},
+		},
+	})
+	require.NoError(t, err, "NewRole(joinMFA)")
+	err = adminClient.UpsertRole(ctx, joinMFARole)
+	require.NoError(t, err, "UpsertRole(%q)", joinMFARole.GetName())
+
+	// Create a role that doesn't require MFA when joining sessions
+	joinNoMFARole, err := types.NewRole("no_mfa_session_join", types.RoleSpecV6{
+		Allow: types.RoleConditions{
+			Logins: []string{"{{internal.logins}}"},
+			NodeLabels: types.Labels{
+				"env": []string{"*"},
+			},
+			JoinSessions: []*types.SessionJoinPolicy{
+				{
+					Name:  "session_join",
+					Roles: []string{"access"},
+					Kinds: []string{string(types.SSHSessionKind)},
+					Modes: []string{string(types.SessionPeerMode)},
+				},
+			},
+		},
+	})
+	require.NoError(t, err, "NewRole(joinNoMFA)")
+	err = adminClient.UpsertRole(ctx, joinNoMFARole)
+	require.NoError(t, err, "UpsertRole(%q)", joinNoMFARole.GetName())
+
+	const normalLogin = "llama"
+	createUser := func(role types.Role) *Client {
+		// Create a user with MFA devices...
+		userCreds, err := createUserWithSecondFactors(testServer)
+		require.NoError(t, err, "createUserWithSecondFactors")
+		username := userCreds.username
+
+		// ...and assign the user a sane unix login, plus the specified role.
+		user, err := adminClient.GetUser(username, false /* withSecrets */)
+		require.NoError(t, err, "GetUser(%q)", username)
+
+		user.SetLogins(append(user.GetLogins(), normalLogin))
+		user.AddRole(role.GetName())
+		err = adminClient.UpdateUser(ctx, user.(*types.UserV2))
+		require.NoError(t, err, "UpdateUser(%q)", username)
+
+		userClient, err := testServer.NewClient(TestUser(username))
+		require.NoError(t, err, "NewClient(%q)", username)
+
+		return userClient
+	}
+
+	prodAccessClient := createUser(prodRole)
+	joinMFAClient := createUser(joinMFARole)
+	joinNoMFAClient := createUser(joinNoMFARole)
+
+	createReqForNode := func(node, login string) *proto.IsMFARequiredRequest {
+		return &proto.IsMFARequiredRequest{
+			Target: &proto.IsMFARequiredRequest_Node{
+				Node: &proto.NodeLogin{
+					Node:  node,
+					Login: login,
+				},
+			},
+		}
+	}
+
+	tests := []struct {
+		name            string
+		userClient      *Client
+		req             *proto.IsMFARequiredRequest
+		wantMFARequired bool
+	}{
+		{
+			name:            "MFA not required to start session, no challenges issued",
+			userClient:      prodAccessClient,
+			req:             createReqForNode(devNode, normalLogin),
+			wantMFARequired: false,
+		},
+		{
+			name:            "MFA required to start session",
+			userClient:      prodAccessClient,
+			req:             createReqForNode(prodNode, normalLogin),
+			wantMFARequired: true,
+		},
+		{
+			name:            "MFA required to join session on prod node (prod role)",
+			userClient:      prodAccessClient,
+			req:             createReqForNode(prodNode, teleport.SSHSessionJoinPrincipal),
+			wantMFARequired: true,
+		},
+		{
+			name:            "MFA required to join session on dev node (prod role)",
+			userClient:      prodAccessClient,
+			req:             createReqForNode(devNode, teleport.SSHSessionJoinPrincipal),
+			wantMFARequired: true,
+		},
+		{
+			name:            "MFA required to join session on prod node (join MFA role)",
+			userClient:      joinMFAClient,
+			req:             createReqForNode(prodNode, teleport.SSHSessionJoinPrincipal),
+			wantMFARequired: true,
+		},
+		{
+			name:            "MFA required to join session dev node (join MFA role)",
+			userClient:      joinMFAClient,
+			req:             createReqForNode(prodNode, teleport.SSHSessionJoinPrincipal),
+			wantMFARequired: true,
+		},
+		{
+			name:            "MFA not required to join session, no challenges issued on dev node (join no MFA role)",
+			userClient:      joinNoMFAClient,
+			req:             createReqForNode(devNode, teleport.SSHSessionJoinPrincipal),
+			wantMFARequired: false,
+		},
+		{
+			name:            "MFA not required to join session, no challenges issued on prod node (join no MFA role)",
+			userClient:      joinNoMFAClient,
+			req:             createReqForNode(prodNode, teleport.SSHSessionJoinPrincipal),
+			wantMFARequired: false,
+		},
+	}
+	for _, test := range tests {
+		test := test
+		t.Run(test.name, func(t *testing.T) {
+			t.Parallel()
+
+			resp, err := test.userClient.IsMFARequired(ctx, test.req)
+			require.NoError(t, err, "CreateAuthenticateChallenge")
+
+			assert.Equal(t, test.wantMFARequired, resp.Required, "resp.MFARequired mismatch")
+		})
+	}
+}
+
 func TestCreateRegisterChallenge(t *testing.T) {
 	t.Parallel()
 	ctx := context.Background()

lib/srv/authhandlers.go

@@ -603,7 +603,7 @@ func (a *ahLoginChecker) canLoginWithRBAC(cert *ssh.Certificate, ca types.CertAu
 		auth.RoleSupportsModeratedSessions(accessChecker.Roles()) {
 
 		// allow joining if cluster wide MFA is not required
-		if state.MFARequired != services.MFARequiredAlways {
+		if state.MFARequired == services.MFARequiredNever {
 			return nil
 		}