diff --git a/lib/services/access_checker_test.go b/lib/services/access_checker_test.go index 2a8f5812ec2..0db019f6700 100644 --- a/lib/services/access_checker_test.go +++ b/lib/services/access_checker_test.go @@ -924,6 +924,49 @@ func TestAccessChecker_CheckConditionalAccess_RoleRequiresMFA_ForceInBandMFAEnv_ ) } +// TODO(cthach): Remove in v20.0 when the legacy out-of-band MFA flow is removed. +func TestAccessChecker_CheckAccess_ReadOnlyBypassWhenMFAForced(t *testing.T) { + t.Setenv("TELEPORT_UNSTABLE_FORCE_IN_BAND_MFA", "yes") + + const roleName = "mfa-required" + + roleSet := NewRoleSet(newRole(func(r *types.RoleV6) { + r.SetName(roleName) + + r.SetOptions(types.RoleOptions{ + RequireMFAType: types.RequireMFAType_SESSION, + }) + })) + + accessInfo := &AccessInfo{ + Roles: []string{roleName}, + } + + accessChecker := NewAccessCheckerWithRoleSet(accessInfo, "cluster", roleSet) + + srv, err := types.NewServer( + "test-server", + types.KindNode, + types.ServerSpecV2{}, + ) + require.NoError(t, err) + + node := &serverStub{Server: srv} + + err = accessChecker.CheckAccess( + node, + AccessState{ + // Simulate a read-only access check that is being allowed to bypass MFA requirements even when the role + // requires MFA and the force in-band MFA env var is set. This is to allow users to perform read-only + // operations like listing nodes without being forced to complete MFA verification. + MFARequired: MFARequiredPerRole, + MFAVerified: true, + ReturnPreconditions: false, + }, + ) + require.NoError(t, err) +} + func TestSSHPortForwarding(t *testing.T) { anyLabels := types.Labels{"*": {"*"}} localCluster := "cluster" diff --git a/lib/services/role.go b/lib/services/role.go index 2e0eef7a6e2..03f65fb2c96 100644 --- a/lib/services/role.go +++ b/lib/services/role.go @@ -2776,13 +2776,18 @@ func (set RoleSet) checkAccess( // MFA checks can be bypassed if either: // 1. The cluster doesn't require per-session MFA (MFARequiredNever), OR - // 2. The legacy out-of-band MFA flow is allowed (see below) AND MFA has already been verified for the session. + // 2. Legacy out-of-band MFA has already been verified for the session AND + // a. The legacy out-of-band MFA flow is allowed (TELEPORT_UNSTABLE_FORCE_IN_BAND_MFA is not set to "yes") OR + // b. The caller doesn't want preconditions returned (state.ReturnPreconditions is false) // - // The legacy out-of-band MFA flow is allowed as long as TELEPORT_UNSTABLE_FORCE_IN_BAND_MFA is not set to "yes" - // and MFA has already been verified for this session. + // Listing resources sets state.MFAVerified to true and state.ReturnPreconditions to false to allow bypassing MFA + // checks for resources that require per-session MFA. This is because listing resources is a read-only operation and + // MFA is not required to list resources, even if MFA is required to access the resource. The actual enforcement + // will happen at connection time, so this is not a concern from a security perspective. // // TODO(cthach): Remove in v20.0 when the legacy out-of-band MFA flow is removed. - bypassMFAChecks := state.MFARequired == MFARequiredNever || (os.Getenv("TELEPORT_UNSTABLE_FORCE_IN_BAND_MFA") != "yes" && state.MFAVerified) + bypassMFAChecks := state.MFARequired == MFARequiredNever || + (state.MFAVerified && (os.Getenv("TELEPORT_UNSTABLE_FORCE_IN_BAND_MFA") != "yes" || !state.ReturnPreconditions)) // TODO(codingllama): Consider making EnableDeviceVerification opt-out instead // of opt-in.