diff --git a/api/proto/teleport/legacy/types/types.proto b/api/proto/teleport/legacy/types/types.proto index 13904a858a916..4771ea5488ec9 100644 --- a/api/proto/teleport/legacy/types/types.proto +++ b/api/proto/teleport/legacy/types/types.proto @@ -5286,9 +5286,8 @@ enum RequireMFAType { // and login sessions must use a private key backed by a hardware key. SESSION_AND_HARDWARE_KEY = 2; // HARDWARE_KEY_TOUCH means login sessions must use a hardware private key that - // requires touch to be used. This touch requirement applies to all API requests - // rather than only session requests. This touch is different from MFA, so to prevent - // requiring double touch on session requests, normal Session MFA is disabled. + // requires touch to be used. This touch is required for all private key operations, + // so the key is always treated as MFA verified for sessions. HARDWARE_KEY_TOUCH = 3; } diff --git a/api/types/authentication.go b/api/types/authentication.go index 24f125e5db398..5459c50b43373 100644 --- a/api/types/authentication.go +++ b/api/types/authentication.go @@ -853,7 +853,7 @@ func (d *MFADevice) UnmarshalJSON(buf []byte) error { // IsSessionMFARequired returns whether this RequireMFAType requires per-session MFA. func (r RequireMFAType) IsSessionMFARequired() bool { - return r == RequireMFAType_SESSION || r == RequireMFAType_SESSION_AND_HARDWARE_KEY + return r != RequireMFAType_OFF } // MarshalJSON marshals RequireMFAType to boolean or string. diff --git a/api/types/types.pb.go b/api/types/types.pb.go index e7f5943e4a357..0afaf1c05ec53 100644 --- a/api/types/types.pb.go +++ b/api/types/types.pb.go @@ -449,9 +449,8 @@ const ( // and login sessions must use a private key backed by a hardware key. RequireMFAType_SESSION_AND_HARDWARE_KEY RequireMFAType = 2 // HARDWARE_KEY_TOUCH means login sessions must use a hardware private key that - // requires touch to be used. This touch requirement applies to all API requests - // rather than only session requests. This touch is different from MFA, so to prevent - // requiring double touch on session requests, normal Session MFA is disabled. + // requires touch to be used. This touch is required for all private key operations, + // so the key is always treated as MFA verified for sessions. RequireMFAType_HARDWARE_KEY_TOUCH RequireMFAType = 3 ) diff --git a/api/utils/keys/policy.go b/api/utils/keys/policy.go index fa09e8180d893..3623179c22bd1 100644 --- a/api/utils/keys/policy.go +++ b/api/utils/keys/policy.go @@ -55,6 +55,11 @@ func (p PrivateKeyPolicy) VerifyPolicy(policy PrivateKeyPolicy) error { return NewPrivateKeyPolicyError(p) } +// MFAVerified checks that meet this private key policy counts towards MFA verification. +func (p PrivateKeyPolicy) MFAVerified() bool { + return p == PrivateKeyPolicyHardwareKeyTouch +} + func (p PrivateKeyPolicy) validate() error { switch p { case PrivateKeyPolicyNone, PrivateKeyPolicyHardwareKey, PrivateKeyPolicyHardwareKeyTouch: diff --git a/lib/auth/auth_with_roles.go b/lib/auth/auth_with_roles.go index 0d9a9d838727e..799038998981c 100644 --- a/lib/auth/auth_with_roles.go +++ b/lib/auth/auth_with_roles.go @@ -5442,6 +5442,12 @@ func (a *ServerWithRoles) IsMFARequired(ctx context.Context, req *proto.IsMFAReq if !hasLocalUserRole(a.context) && !hasRemoteUserRole(a.context) { return nil, trace.AccessDenied("only a user role can call IsMFARequired, got %T", a.context.Checker) } + // Certain hardware-key based private key policies are treated as MFA verification. + if a.context.Identity.GetIdentity().PrivateKeyPolicy.MFAVerified() { + return &proto.IsMFARequiredResponse{ + Required: false, + }, nil + } return a.authServer.isMFARequired(ctx, a.context.Checker, req) } diff --git a/lib/auth/grpcserver_test.go b/lib/auth/grpcserver_test.go index 96fc293353097..cc6145197c60d 100644 --- a/lib/auth/grpcserver_test.go +++ b/lib/auth/grpcserver_test.go @@ -1945,49 +1945,61 @@ func TestIsMFARequired(t *testing.T) { _, err := srv.Auth().UpsertNode(ctx, node) require.NoError(t, err) - // Create a fake user. - user, role, err := CreateUserAndRole(srv.Auth(), "no-mfa-user", []string{"no-mfa-user"}, nil) - require.NoError(t, err) - for _, authPrefRequireMFAType := range requireMFATypes { - authPref, err := types.NewAuthPreference(types.AuthPreferenceSpecV2{ - Type: constants.Local, - SecondFactor: constants.SecondFactorOptional, - RequireMFAType: authPrefRequireMFAType, - Webauthn: &types.Webauthn{ - RPID: "teleport", - }, - }) - require.NoError(t, err) - err = srv.Auth().SetAuthPreference(ctx, authPref) - require.NoError(t, err) + t.Run(fmt.Sprintf("authPref=%v", authPrefRequireMFAType.String()), func(t *testing.T) { + + authPref, err := types.NewAuthPreference(types.AuthPreferenceSpecV2{ + Type: constants.Local, + SecondFactor: constants.SecondFactorOptional, + RequireMFAType: authPrefRequireMFAType, + Webauthn: &types.Webauthn{ + RPID: "teleport", + }, + }) + require.NoError(t, err) + err = srv.Auth().SetAuthPreference(ctx, authPref) + require.NoError(t, err) - for _, roleRequireMFAType := range requireMFATypes { - // If role or auth pref have "hardware_key_touch", expect not required. - expectRequired := !(roleRequireMFAType == types.RequireMFAType_HARDWARE_KEY_TOUCH || authPrefRequireMFAType == types.RequireMFAType_HARDWARE_KEY_TOUCH) - // Otherwise, if auth pref or role require session MFA, expect required. - expectRequired = expectRequired && (roleRequireMFAType.IsSessionMFARequired() || authPrefRequireMFAType.IsSessionMFARequired()) - - t.Run(fmt.Sprintf("authPref=%v/role=%v/expect=%v", authPrefRequireMFAType, roleRequireMFAType, expectRequired), func(t *testing.T) { - roleOpt := role.GetOptions() - roleOpt.RequireMFAType = roleRequireMFAType - role.SetOptions(roleOpt) - err = srv.Auth().UpsertRole(ctx, role) - require.NoError(t, err) + for _, roleRequireMFAType := range requireMFATypes { + roleRequireMFAType := roleRequireMFAType + t.Run(fmt.Sprintf("role=%v", roleRequireMFAType.String()), func(t *testing.T) { + t.Parallel() - cl, err := srv.NewClient(TestUser(user.GetName())) - require.NoError(t, err) + user, err := types.NewUser(roleRequireMFAType.String()) + require.NoError(t, err) + + role := services.RoleForUser(user) + roleOpt := role.GetOptions() + roleOpt.RequireMFAType = roleRequireMFAType + role.SetOptions(roleOpt) + role.SetLogins(types.Allow, []string{user.GetName()}) - resp, err := cl.IsMFARequired(ctx, &proto.IsMFARequiredRequest{ - Target: &proto.IsMFARequiredRequest_Node{Node: &proto.NodeLogin{ - Login: user.GetName(), - Node: "node-a", - }}, + err = srv.Auth().UpsertRole(ctx, role) + require.NoError(t, err) + + user.AddRole(role.GetName()) + err = srv.Auth().UpsertUser(user) + require.NoError(t, err) + + cl, err := srv.NewClient(TestUser(user.GetName())) + require.NoError(t, err) + + resp, err := cl.IsMFARequired(ctx, &proto.IsMFARequiredRequest{ + Target: &proto.IsMFARequiredRequest_Node{Node: &proto.NodeLogin{ + Login: user.GetName(), + Node: "node-a", + }}, + }) + require.NoError(t, err) + + // If auth pref or role require session MFA, and MFA is not already + // verified according to private key policy, expect MFA required. + expectRequired := (role.GetOptions().RequireMFAType.IsSessionMFARequired() || authPref.GetRequireMFAType().IsSessionMFARequired()) && + !role.GetPrivateKeyPolicy().MFAVerified() && !authPref.GetPrivateKeyPolicy().MFAVerified() + require.Equal(t, expectRequired, resp.Required, "Expected IsMFARequired to return %v but got %v", expectRequired, resp.Required) }) - require.NoError(t, err) - require.Equal(t, expectRequired, resp.Required) - }) - } + } + }) } } diff --git a/lib/authz/permissions.go b/lib/authz/permissions.go index 8df6cf3c3e31b..53bb0a188db6c 100644 --- a/lib/authz/permissions.go +++ b/lib/authz/permissions.go @@ -245,7 +245,7 @@ func (c *Context) GetAccessState(authPref types.AuthPreference) services.AccessS // Builtin services (like proxy_service and kube_service) are not gated // on MFA and only need to pass normal RBAC action checks. _, isService := c.Identity.(BuiltinRole) - state.MFAVerified = isService || identity.MFAVerified != "" + state.MFAVerified = isService || identity.IsMFAVerified() state.EnableDeviceVerification = !c.disableDeviceAuthorization state.DeviceVerified = isService || dtauthz.IsTLSDeviceVerified(&identity.DeviceExtensions) diff --git a/lib/services/role.go b/lib/services/role.go index 7c005866f2a7a..0cf266441ffb0 100644 --- a/lib/services/role.go +++ b/lib/services/role.go @@ -1193,17 +1193,6 @@ func (set RoleSet) GetAccessState(authPref types.AuthPreference) AccessState { } func (set RoleSet) getMFARequired(clusterRequireMFAType types.RequireMFAType) MFARequired { - // per-session MFA is overridden by hardware key PIV touch requirement. - // check if the auth pref or any roles have this option. - if clusterRequireMFAType == types.RequireMFAType_HARDWARE_KEY_TOUCH { - return MFARequiredNever - } - for _, role := range set { - if role.GetOptions().RequireMFAType == types.RequireMFAType_HARDWARE_KEY_TOUCH { - return MFARequiredNever - } - } - // MFA is always required according to the cluster auth pref. if clusterRequireMFAType.IsSessionMFARequired() { return MFARequiredAlways diff --git a/lib/services/role_test.go b/lib/services/role_test.go index fb7570177c4af..c1d65974ffd7b 100644 --- a/lib/services/role_test.go +++ b/lib/services/role_test.go @@ -1426,6 +1426,7 @@ func TestCheckAccessToServer(t *testing.T) { {server: serverDB, login: "root", hasAccess: true}, }, }, + // MFA with private key policy. { name: "cluster requires session+hardware key, MFA not verified", roles: []*types.RoleV6{ @@ -1469,9 +1470,27 @@ func TestCheckAccessToServer(t *testing.T) { }), }, authSpec: types.AuthPreferenceSpecV2{ - // Functionally equivalent to "off". + // Functionally equivalent to "session". RequireMFAType: types.RequireMFAType_HARDWARE_KEY_TOUCH, }, + checks: []check{ + {server: serverNoLabels, login: "root", hasAccess: false}, + {server: serverWorker, login: "root", hasAccess: false}, + {server: serverDB, login: "root", hasAccess: false}, + }, + }, + { + name: "cluster requires hardware key touch, MFA verified", + roles: []*types.RoleV6{ + newRole(func(r *types.RoleV6) { + r.Spec.Allow.Logins = []string{"root"} + }), + }, + authSpec: types.AuthPreferenceSpecV2{ + // Functionally equivalent to "session". + RequireMFAType: types.RequireMFAType_HARDWARE_KEY_TOUCH, + }, + mfaVerified: true, checks: []check{ {server: serverNoLabels, login: "root", hasAccess: true}, {server: serverWorker, login: "root", hasAccess: true}, @@ -7548,26 +7567,44 @@ func TestRoleSet_GetAccessState(t *testing.T) { MFARequired: MFARequiredAlways, }, }, + { + name: "auth pref requires hardware key", + roleMFARequireTypes: []types.RequireMFAType{ + types.RequireMFAType_OFF, + }, + authPrefMFARequireType: types.RequireMFAType_SESSION_AND_HARDWARE_KEY, + expectState: AccessState{ + MFARequired: MFARequiredAlways, + }, + }, { name: "auth pref requires hardware key touch", roleMFARequireTypes: []types.RequireMFAType{ - types.RequireMFAType_SESSION, - types.RequireMFAType_SESSION, + types.RequireMFAType_OFF, }, authPrefMFARequireType: types.RequireMFAType_HARDWARE_KEY_TOUCH, expectState: AccessState{ - MFARequired: MFARequiredNever, + MFARequired: MFARequiredAlways, + }, + }, + { + name: "role requires hardware key", + roleMFARequireTypes: []types.RequireMFAType{ + types.RequireMFAType_SESSION_AND_HARDWARE_KEY, + }, + authPrefMFARequireType: types.RequireMFAType_OFF, + expectState: AccessState{ + MFARequired: MFARequiredAlways, }, }, { name: "role requires hardware key touch", roleMFARequireTypes: []types.RequireMFAType{ - types.RequireMFAType_SESSION, types.RequireMFAType_HARDWARE_KEY_TOUCH, }, - authPrefMFARequireType: types.RequireMFAType_SESSION, + authPrefMFARequireType: types.RequireMFAType_OFF, expectState: AccessState{ - MFARequired: MFARequiredNever, + MFARequired: MFARequiredAlways, }, }, } diff --git a/lib/srv/authhandlers.go b/lib/srv/authhandlers.go index 9c80078ac09b2..4efccb5b1f2ea 100644 --- a/lib/srv/authhandlers.go +++ b/lib/srv/authhandlers.go @@ -34,6 +34,7 @@ import ( "github.com/gravitational/teleport" "github.com/gravitational/teleport/api/types" apievents "github.com/gravitational/teleport/api/types/events" + "github.com/gravitational/teleport/api/utils/keys" apisshutils "github.com/gravitational/teleport/api/utils/sshutils" "github.com/gravitational/teleport/lib/auditd" "github.com/gravitational/teleport/lib/auth" @@ -595,6 +596,13 @@ func (a *ahLoginChecker) canLoginWithRBAC(cert *ssh.Certificate, ca types.CertAu state := accessChecker.GetAccessState(authPref) _, state.MFAVerified = cert.Extensions[teleport.CertExtensionMFAVerified] + // Certain hardware-key based private key policies are treated as MFA verification. + if policyString, ok := cert.Extensions[teleport.CertExtensionPrivateKeyPolicy]; ok { + if keys.PrivateKeyPolicy(policyString).MFAVerified() { + state.MFAVerified = true + } + } + // we don't need to check the RBAC for the node if they are only allowed to join sessions if osUser == teleport.SSHSessionJoinPrincipal && auth.RoleSupportsModeratedSessions(accessChecker.Roles()) { diff --git a/lib/srv/db/common/session.go b/lib/srv/db/common/session.go index e991a50d85c94..95f9e8afd951b 100644 --- a/lib/srv/db/common/session.go +++ b/lib/srv/db/common/session.go @@ -72,7 +72,7 @@ func (c *Session) String() string { // [services.AccessChecker] and [tlsca.Identity]. func (c *Session) GetAccessState(authPref types.AuthPreference) services.AccessState { state := c.Checker.GetAccessState(authPref) - state.MFAVerified = c.Identity.MFAVerified != "" + state.MFAVerified = c.Identity.IsMFAVerified() state.EnableDeviceVerification = true state.DeviceVerified = dtauthz.IsTLSDeviceVerified(&c.Identity.DeviceExtensions) return state diff --git a/lib/tlsca/ca.go b/lib/tlsca/ca.go index 63f57f4db583b..6f45fffad2c98 100644 --- a/lib/tlsca/ca.go +++ b/lib/tlsca/ca.go @@ -1051,6 +1051,11 @@ func (id Identity) GetUserMetadata() events.UserMetadata { } } +// IsMFAVerified returns whether this identity is MFA verified. +func (id *Identity) IsMFAVerified() bool { + return id.MFAVerified != "" || id.PrivateKeyPolicy.MFAVerified() +} + // CertificateRequest is a X.509 signing certificate request type CertificateRequest struct { // Clock is a clock used to get current or test time