From fe0d42ff6ac934149e8a775908c2d3ab681070dd Mon Sep 17 00:00:00 2001 From: Chris Thach Date: Thu, 14 Aug 2025 12:12:12 -0400 Subject: [PATCH 1/5] fix: return friendly error message when MFA is required but the user has no devices enrolled Signed-off-by: Chris Thach --- lib/client/cluster_client.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/lib/client/cluster_client.go b/lib/client/cluster_client.go index 40962d53ccc74..ecf82be99005e 100644 --- a/lib/client/cluster_client.go +++ b/lib/client/cluster_client.go @@ -801,11 +801,11 @@ func PerformSessionMFACeremony(ctx context.Context, params PerformSessionMFACere return nil, trace.Wrap(err) } - // If mfaResp is nil, the ceremony was a no-op (no devices registered). - // TODO(Joerger): CreateAuthenticateChallenge, should return - // this error directly instead of an empty challenge, without - // regressing https://github.com/gravitational/teleport/issues/36482. - if mfaResp == nil { + // If mfaResp.GetResponse() is nil, the ceremony was a no-op (no devices + // registered). TODO(Joerger): CreateAuthenticateChallenge, should return + // this error directly instead of an empty challenge, without regressing + // https://github.com/gravitational/teleport/issues/36482. + if mfaResp.GetResponse() == nil { return nil, trace.Wrap(authclient.ErrNoMFADevices) } From 99802bba89dce076e0ea5a2c93c8100c7ed9aab9 Mon Sep 17 00:00:00 2001 From: Chris Thach Date: Fri, 15 Aug 2025 15:31:18 -0400 Subject: [PATCH 2/5] fix: improve error handling to provide clearer feedback when MFA requirements are not met upon connection to an App Signed-off-by: Chris Thach --- lib/srv/app/connections_handler.go | 21 ++++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-) diff --git a/lib/srv/app/connections_handler.go b/lib/srv/app/connections_handler.go index 33948663e847b..9b88733937bfa 100644 --- a/lib/srv/app/connections_handler.go +++ b/lib/srv/app/connections_handler.go @@ -561,10 +561,12 @@ func (c *ConnectionsHandler) authorizeContext(ctx context.Context) (*authz.Conte app, state, matchers...); { - case errors.Is(err, services.ErrTrustedDeviceRequired): - // Let the trusted device error through for clarity. - return nil, nil, trace.Wrap(services.ErrTrustedDeviceRequired) + case errors.Is(err, services.ErrTrustedDeviceRequired) || errors.Is(err, services.ErrSessionMFARequired): + // When access is denied due to trusted device or session MFA requirements, these specific errors + // are returned directly to provide clarity to the client about the additional authentication steps needed. + return nil, nil, trace.Wrap(err) case err != nil: + // Other access denial errors are wrapped and obfuscated to prevent leaking sensitive details. c.log.WarnContext(c.closeContext, "Access denied to application.", "app", app.GetName(), "error", err, @@ -719,18 +721,23 @@ func (c *ConnectionsHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) { if err != nil { c.log.WarnContext(c.closeContext, "Failed to serve request", "error", err) - // Covert trace error type to HTTP and write response, make sure we close the + // Convert trace error type to HTTP and write response, make sure we close the // connection afterwards so that the monitor is recreated if needed. code := trace.ErrorToCode(err) var text string - if errors.Is(err, services.ErrTrustedDeviceRequired) { - // Return a nicer error message for device trust errors. + switch { + case errors.Is(err, services.ErrTrustedDeviceRequired): text = `Access to this app requires a trusted device. See https://goteleport.com/docs/admin-guides/access-controls/device-trust/device-management/#troubleshooting for help. ` - } else { + case errors.Is(err, services.ErrSessionMFARequired): + text = `Access to this app requires multi-factor authentication (MFA), but user has no MFA devices; see Account Settings in the Web UI or use 'tsh mfa add' to register MFA devices. + +See https://goteleport.com/docs/zero-trust-access/access-controls/guides/per-session-mfa/ for help. +` + default: text = http.StatusText(code) } From 2d9c589c7beffc98d3c1613c37298c659d7b8bc3 Mon Sep 17 00:00:00 2001 From: Chris Thach Date: Mon, 18 Aug 2025 15:55:40 -0400 Subject: [PATCH 3/5] refactor: make error messages consistent Signed-off-by: Chris Thach --- lib/auth/authclient/clt.go | 2 +- lib/srv/app/connections_handler.go | 4 ++-- lib/web/desktop.go | 3 ++- lib/web/terminal.go | 2 +- 4 files changed, 6 insertions(+), 5 deletions(-) diff --git a/lib/auth/authclient/clt.go b/lib/auth/authclient/clt.go index e938e91cd97c7..b020624a65709 100644 --- a/lib/auth/authclient/clt.go +++ b/lib/auth/authclient/clt.go @@ -83,7 +83,7 @@ var ( // ErrNoMFADevices is returned when an MFA ceremony is performed without possible devices to // complete the challenge with. ErrNoMFADevices = &trace.AccessDeniedError{ - Message: "MFA is required to access this resource but user has no MFA devices; see Account Settings in the Web UI or use 'tsh mfa add' to register MFA devices", + Message: "Multi-factor authentication (MFA) is required to access this resource but the current user has no supported MFA devices enrolled; see Account Settings in the Web UI or use 'tsh mfa add' to register an MFA device", } // InvalidUserPassError is the error for when either the provided username or // password is incorrect. diff --git a/lib/srv/app/connections_handler.go b/lib/srv/app/connections_handler.go index 9b88733937bfa..733bce0a18b1e 100644 --- a/lib/srv/app/connections_handler.go +++ b/lib/srv/app/connections_handler.go @@ -728,12 +728,12 @@ func (c *ConnectionsHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) { var text string switch { case errors.Is(err, services.ErrTrustedDeviceRequired): - text = `Access to this app requires a trusted device. + text = `A trusted device is required to access this resource but this device has not been registered as a trusted device; use 'tsh device enroll' to register as a trusted device. See https://goteleport.com/docs/admin-guides/access-controls/device-trust/device-management/#troubleshooting for help. ` case errors.Is(err, services.ErrSessionMFARequired): - text = `Access to this app requires multi-factor authentication (MFA), but user has no MFA devices; see Account Settings in the Web UI or use 'tsh mfa add' to register MFA devices. + text = `Multi-factor authentication (MFA) is required to access this resource but the current user has no supported MFA devices enrolled; see Account Settings in the Web UI or use 'tsh mfa add' to register an MFA device. See https://goteleport.com/docs/zero-trust-access/access-controls/guides/per-session-mfa/ for help. ` diff --git a/lib/web/desktop.go b/lib/web/desktop.go index 45301cf1cb61a..25b6d0fd221b9 100644 --- a/lib/web/desktop.go +++ b/lib/web/desktop.go @@ -38,6 +38,7 @@ import ( "github.com/gravitational/teleport/api/mfa" "github.com/gravitational/teleport/api/types" "github.com/gravitational/teleport/api/utils/keys" + "github.com/gravitational/teleport/lib/auth/authclient" wantypes "github.com/gravitational/teleport/lib/auth/webauthntypes" "github.com/gravitational/teleport/lib/authz" "github.com/gravitational/teleport/lib/client" @@ -416,7 +417,7 @@ func (h *Handler) performSessionMFACeremony( } if chal.WebauthnChallenge == nil && chal.SSOChallenge == nil { - return nil, trace.AccessDenied("Only WebAuthn and SSO MFA methods are supported on the web, please register a supported MFA method to connect to this desktop") + return nil, trace.WrapWithMessage(authclient.ErrNoMFADevices, "Only WebAuthn and SSO MFA methods are supported for desktop access") } // Send the challenge over the socket. diff --git a/lib/web/terminal.go b/lib/web/terminal.go index a0ab57995145d..148f8748d363f 100644 --- a/lib/web/terminal.go +++ b/lib/web/terminal.go @@ -654,7 +654,7 @@ func newMFACeremony(stream *terminal.WSStream, createAuthenticateChallenge mfa.C } if chal.WebauthnChallenge == nil && chal.SSOChallenge == nil { - return nil, trace.AccessDenied("only WebAuthn and SSO MFA methods are supported on the web terminal, please register a supported mfa method to connect to this server") + return nil, trace.WrapWithMessage(authclient.ErrNoMFADevices, "Only WebAuthn and SSO MFA methods are supported on the web terminal") } var codec protobufMFACodec From 06b49274054a372a684d2618b62edde0432f4b17 Mon Sep 17 00:00:00 2001 From: Chris Thach Date: Mon, 18 Aug 2025 17:03:56 -0400 Subject: [PATCH 4/5] refactor: remove wrapped message Signed-off-by: Chris Thach --- lib/srv/app/connections_handler.go | 4 +--- lib/web/desktop.go | 2 +- lib/web/terminal.go | 2 +- 3 files changed, 3 insertions(+), 5 deletions(-) diff --git a/lib/srv/app/connections_handler.go b/lib/srv/app/connections_handler.go index 733bce0a18b1e..152f057341a33 100644 --- a/lib/srv/app/connections_handler.go +++ b/lib/srv/app/connections_handler.go @@ -733,10 +733,8 @@ func (c *ConnectionsHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) { See https://goteleport.com/docs/admin-guides/access-controls/device-trust/device-management/#troubleshooting for help. ` case errors.Is(err, services.ErrSessionMFARequired): - text = `Multi-factor authentication (MFA) is required to access this resource but the current user has no supported MFA devices enrolled; see Account Settings in the Web UI or use 'tsh mfa add' to register an MFA device. + text = authclient.ErrNoMFADevices.Error() -See https://goteleport.com/docs/zero-trust-access/access-controls/guides/per-session-mfa/ for help. -` default: text = http.StatusText(code) } diff --git a/lib/web/desktop.go b/lib/web/desktop.go index 25b6d0fd221b9..7e7f8b377f2fa 100644 --- a/lib/web/desktop.go +++ b/lib/web/desktop.go @@ -417,7 +417,7 @@ func (h *Handler) performSessionMFACeremony( } if chal.WebauthnChallenge == nil && chal.SSOChallenge == nil { - return nil, trace.WrapWithMessage(authclient.ErrNoMFADevices, "Only WebAuthn and SSO MFA methods are supported for desktop access") + return nil, trace.Wrap(authclient.ErrNoMFADevices) } // Send the challenge over the socket. diff --git a/lib/web/terminal.go b/lib/web/terminal.go index 148f8748d363f..c5e8d5d4582ba 100644 --- a/lib/web/terminal.go +++ b/lib/web/terminal.go @@ -654,7 +654,7 @@ func newMFACeremony(stream *terminal.WSStream, createAuthenticateChallenge mfa.C } if chal.WebauthnChallenge == nil && chal.SSOChallenge == nil { - return nil, trace.WrapWithMessage(authclient.ErrNoMFADevices, "Only WebAuthn and SSO MFA methods are supported on the web terminal") + return nil, trace.Wrap(authclient.ErrNoMFADevices) } var codec protobufMFACodec From cb8c83977c3927d3ef18db427c92c8adfe15c3d0 Mon Sep 17 00:00:00 2001 From: Chris Thach Date: Mon, 18 Aug 2025 17:42:20 -0400 Subject: [PATCH 5/5] test: add test for enrolled MFA device and update trusted device resp body assertion Signed-off-by: Chris Thach --- lib/srv/app/server_test.go | 52 +++++++++++++++++++++++++++----------- 1 file changed, 37 insertions(+), 15 deletions(-) diff --git a/lib/srv/app/server_test.go b/lib/srv/app/server_test.go index 489cb6b3d668a..57fbb2572d1dd 100644 --- a/lib/srv/app/server_test.go +++ b/lib/srv/app/server_test.go @@ -923,6 +923,7 @@ func TestAuthorize(t *testing.T) { roleAppLabels types.Labels appLabels map[string]string requireTrustedDevice bool // assigns user to a role that requires trusted devices + requireMFADevice bool // assigns user to a role that requires an enrolled MFA device wantStatus int assertBody func(t *testing.T, gotBody string) bool // optional, matched against s.message if nil }{ @@ -966,7 +967,16 @@ func TestAuthorize(t *testing.T) { requireTrustedDevice: true, wantStatus: http.StatusForbidden, assertBody: func(t *testing.T, gotBody string) bool { - const want = "app requires a trusted device" + const want = "trusted device is required to access this resource" + return assert.Contains(t, gotBody, want, "response body mismatch") + }, + }, + { + name: "enrolled MFA device required", + requireMFADevice: true, + wantStatus: http.StatusForbidden, + assertBody: func(t *testing.T, gotBody string) bool { + const want = "Multi-factor authentication (MFA) is required to access this resource" return assert.Contains(t, gotBody, want, "response body mismatch") }, }, @@ -981,11 +991,24 @@ func TestAuthorize(t *testing.T) { RoleAppLabels: test.roleAppLabels, }) - if test.requireTrustedDevice { + // Helper to create and assign a role to the user, then refresh certificate. + assignRoleAndRefreshCert := func(roleName string, roleSpec types.RoleSpecV6) { authServer := s.authServer.AuthServer + role, err := types.NewRole(roleName, roleSpec) + require.NoError(t, err, "NewRole") + role, err = authServer.CreateRole(ctx, role) + require.NoError(t, err, "CreateRole") + + user := s.user + user.AddRole(role.GetName()) + user, err = authServer.Services.UpdateUser(ctx, user) + require.NoError(t, err, "UpdateUser") + + s.clientCertificate = s.generateCertificate(t, user, s.appFoo.GetPublicAddr(), "" /* awsRoleARN */) + } - // Create a role that requires a trusted device. - requiredDevRole, err := types.NewRole("require-trusted-devices-app", types.RoleSpecV6{ + if test.requireTrustedDevice { + assignRoleAndRefreshCert("require-trusted-devices-app", types.RoleSpecV6{ Options: types.RoleOptions{ DeviceTrustMode: constants.DeviceTrustModeRequired, }, @@ -993,18 +1016,17 @@ func TestAuthorize(t *testing.T) { AppLabels: types.Labels{"*": []string{"*"}}, }, }) - require.NoError(t, err, "NewRole") - requiredDevRole, err = authServer.CreateRole(ctx, requiredDevRole) - require.NoError(t, err, "CreateRole") - - // Add role to test user. - user := s.user - user.AddRole(requiredDevRole.GetName()) - user, err = authServer.Services.UpdateUser(ctx, user) - require.NoError(t, err, "UpdateUser") + } - // Refresh user certificate. - s.clientCertificate = s.generateCertificate(t, user, s.appFoo.GetPublicAddr(), "" /* awsRoleARN */) + if test.requireMFADevice { + assignRoleAndRefreshCert("require-mfa-app", types.RoleSpecV6{ + Options: types.RoleOptions{ + RequireMFAType: types.RequireMFAType_SESSION, + }, + Allow: types.RoleConditions{ + AppLabels: types.Labels{"*": []string{"*"}}, + }, + }) } if test.cloudLabels != nil {