diff --git a/lib/client/api.go b/lib/client/api.go index 074d638bc1a1b..f48198f1ce606 100644 --- a/lib/client/api.go +++ b/lib/client/api.go @@ -2113,53 +2113,44 @@ func (tc *TeleportClient) ConnectToNode(ctx context.Context, clt *ClusterClient, case !trace.IsAccessDenied(directErr) && errors.Is(mfaErr, authclient.ErrNoMFADevices): return nil, trace.Wrap(directErr) case !errors.Is(mfaErr, io.EOF) && // Ignore any errors from MFA due to locks being enforced, the direct error will be friendlier - !errors.Is(mfaErr, MFARequiredUnknownErr{}) && // Ignore any failures that occurred before determining if MFA was required + !errors.As(mfaErr, new(*MFARequiredUnknownError)) && // Ignore any failures that occurred before determining if MFA was required !errors.Is(mfaErr, services.ErrSessionMFANotRequired): // Ignore any errors caused by attempting the MFA ceremony when MFA will not grant access + log.DebugContext(ctx, "Failed to connect to node, returning MFA ceremony error and ignoring direct connection error", "direct_error", directErr) return nil, trace.Wrap(mfaErr) default: + log.DebugContext(ctx, "Failed to connect to node, returning direct connection error and ignoring MFA ceremony error", "mfa_error", mfaErr) return nil, trace.Wrap(directErr) } } -// MFARequiredUnknownErr indicates that connections to an instance failed -// due to being unable to determine if mfa is required -type MFARequiredUnknownErr struct { +// MFARequiredUnknownError indicates that connections to an instance failed due +// to being unable to determine if MFA is required. +type MFARequiredUnknownError struct { err error } -// MFARequiredUnknown creates a new MFARequiredUnknownErr that wraps the -// error encountered attempting to determine if the mfa ceremony should proceed. +// MFARequiredUnknown creates a new MFARequiredUnknownError that wraps the error +// encountered attempting to determine if the MFA ceremony should proceed. func MFARequiredUnknown(err error) error { - return MFARequiredUnknownErr{err: err} + return &MFARequiredUnknownError{err: err} } // Error returns the error string of the wrapped error if one exists. -func (m MFARequiredUnknownErr) Error() string { +func (m *MFARequiredUnknownError) Error() string { + const msg = "unable to determine if a MFA ceremony is required" if m.err == nil { - return "" + return msg } - return m.err.Error() + return msg + ": " + m.err.Error() } -// Unwrap returns the underlying error from checking if an mfa -// ceremony should have been performed. -func (m MFARequiredUnknownErr) Unwrap() error { +// Unwrap returns the underlying error from checking if an MFA ceremony should +// have been performed. +func (m *MFARequiredUnknownError) Unwrap() error { return m.err } -// Is determines if the provided error is an MFARequiredUnknownErr. -func (m MFARequiredUnknownErr) Is(err error) bool { - switch err.(type) { - case MFARequiredUnknownErr: - return true - case *MFARequiredUnknownErr: - return true - default: - return false - } -} - // connectToNodeWithMFA checks if per session mfa is required to connect to the target host, and // if it is required, then the mfa ceremony is attempted. The target host is dialed once the ceremony // completes and new certificates are retrieved. diff --git a/lib/client/cluster_client.go b/lib/client/cluster_client.go index ecf82be99005e..6cec5b5144d3a 100644 --- a/lib/client/cluster_client.go +++ b/lib/client/cluster_client.go @@ -795,10 +795,22 @@ func PerformSessionMFACeremony(ctx context.Context, params PerformSessionMFACere AllowReuse: allowReuse, }, }, promptOpts...) - if errors.Is(err, &mfa.ErrMFANotRequired) { - return nil, trace.Wrap(services.ErrSessionMFANotRequired) - } else if err != nil { - return nil, trace.Wrap(err) + if err != nil { + switch { + case errors.Is(err, &mfa.ErrMFANotRequired): + return nil, trace.Wrap(services.ErrSessionMFANotRequired) + // this is not a completely exhaustive list for errors that are + // definitely not errors coming from the MFA ceremony itself, but we + // (and gRPC in general) don't seem to have a great way of + // distinguishing errors that are definitely coming from the upper + // layers of the connection from errors deep in the server handler; + // connection problem and limit exceeded are errors that are + // overwhelmingly more likely to happen in the former, tho + case trace.IsConnectionProblem(err), trace.IsLimitExceeded(err): + return nil, trace.Wrap(MFARequiredUnknown(trace.Unwrap(err))) + default: + return nil, trace.Wrap(err) + } } // If mfaResp.GetResponse() is nil, the ceremony was a no-op (no devices diff --git a/lib/web/terminal.go b/lib/web/terminal.go index 8754ffc1ff640..20df479fca33f 100644 --- a/lib/web/terminal.go +++ b/lib/web/terminal.go @@ -767,7 +767,7 @@ func (t *sshBaseHandler) connectToHost(ctx context.Context, ws terminal.WSConn, case !trace.IsAccessDenied(directErr) && errors.Is(mfaErr, authclient.ErrNoMFADevices): return nil, trace.Wrap(directErr) case !errors.Is(mfaErr, io.EOF) && // Ignore any errors from MFA due to locks being enforced, the direct error will be friendlier - !errors.Is(mfaErr, client.MFARequiredUnknownErr{}) && // Ignore any failures that occurred before determining if MFA was required + !errors.As(mfaErr, new(*client.MFARequiredUnknownError)) && // Ignore any failures that occurred before determining if MFA was required !errors.Is(mfaErr, services.ErrSessionMFANotRequired): // Ignore any errors caused by attempting the MFA ceremony when MFA will not grant access return nil, trace.Wrap(mfaErr) default: