From abd8c6260c779aa871eaaec7edb8ac78194fc92e Mon Sep 17 00:00:00 2001 From: Alan Parra Date: Wed, 8 Jun 2022 11:22:24 -0300 Subject: [PATCH] [v9] Expand --mfa-mode and disable stdin hijack by default (#13134) (#13212) Avoid "input swallowing" bugs by disabling stdin hijacking by default. Only `tsh login` is allowed to hijack stdin, as it is expected to exit right after authentication. Any MFA authentication attempts resulting from non-`tsh login` invocations default to the user's strongest auth method. Defaulting to the strongest auth method can cause problems in constrained environments for users that have both Webauthn and OTP registered. For example, someone using `tsh` under WSL (Windows Subsystem for Linux) or a remote machine could be locked into Webauthn MFA, which they can't use because their environment lacks USB access or they don't have physical access to it. In order to solve this problem I've slightly modified the meaning of the `--mfa-mode` flag so `otp` can be specified. The `TELEPORT_MFA_MODE` environment variable may be set to avoid constant flag passing. * Expand --mfa-mode and disable stdin hijack by default (#13134) * Use TeleportClient.PromptMFAChallenge where applicable * Adapt/backport tests and fix prefer OTP * Appease linter --- lib/client/api.go | 40 +++++++----------- lib/client/api_login_test.go | 48 ++++++++++++++-------- lib/client/client.go | 4 +- lib/client/mfa.go | 80 +++++++++++++++++++++++------------- lib/client/weblogin.go | 16 ++++---- tool/tsh/tsh.go | 46 +++++++++++++++++++++ 6 files changed, 155 insertions(+), 79 deletions(-) diff --git a/lib/client/api.go b/lib/client/api.go index fa47d1c48d8bf..b2cdaa8c16f3c 100644 --- a/lib/client/api.go +++ b/lib/client/api.go @@ -321,6 +321,11 @@ type Config struct { // AuthConnector is the name of the authentication connector to use. AuthConnector string + // PreferOTP prefers OTP in favor of other MFA methods. + // Useful in constrained environments without access to USB or platform + // authenticators, such as remote hosts or virtual machines. + PreferOTP bool + // CheckVersions will check that client version is compatible // with auth server version when connecting. CheckVersions bool @@ -371,11 +376,11 @@ type Config struct { // ExtraProxyHeaders is a collection of http headers to be included in requests to the WebProxy. ExtraProxyHeaders map[string]string - // UseStrongestAuth instructs TeleportClient to use the strongest - // authentication method supported by the cluster in Login attempts. - // Apart from the obvious benefits, UseStrongestAuth also avoids stdin - // hijacking issues from Login, as a single auth method is used. - UseStrongestAuth bool + // AllowStdinHijack allows stdin hijack during MFA prompts. + // Stdin hijack provides a better login UX, but it can be difficult to reason + // about and is often a source of bugs. + // Do not set this options unless you deeply understand what you are doing. + AllowStdinHijack bool } // CachePolicy defines cache policy for local clients @@ -682,8 +687,6 @@ func (p *ProfileStatus) AppNames() (result []string) { // RetryWithRelogin is a helper error handling method, attempts to relogin and // retry the function once. -// RetryWithRelogin automatically enables tc.UseStrongestAuth for Login attempts -// in order to avoid stdin hijack bugs. func RetryWithRelogin(ctx context.Context, tc *TeleportClient, fn func() error) error { err := fn() if err == nil { @@ -700,17 +703,6 @@ func RetryWithRelogin(ctx context.Context, tc *TeleportClient, fn func() error) } log.Debugf("Activating relogin on %v.", err) - if !tc.UseStrongestAuth { - defer func() { - tc.UseStrongestAuth = false - }() - // Avoid stdin hijack on relogin attempts. - // Users can pick an alternative MFA method by explicitly calling Login (or - // running `tsh login`). - tc.UseStrongestAuth = true - log.Debug("Enabling strongest auth for login. Use `tsh login` for alternative authentication methods.") - } - key, err := tc.Login(ctx) if err != nil { if trace.IsTrustError(err) { @@ -1584,8 +1576,8 @@ func (tc *TeleportClient) IssueUserCertsWithMFA(ctx context.Context, params Reis defer proxyClient.Close() key, err := proxyClient.IssueUserCertsWithMFA(ctx, params, - func(ctx context.Context, proxyAddr string, c *proto.MFAAuthenticateChallenge) (*proto.MFAAuthenticateResponse, error) { - return PromptMFAChallenge(ctx, c, proxyAddr, nil /* opts */) + func(ctx context.Context, _ string, c *proto.MFAAuthenticateChallenge) (*proto.MFAAuthenticateResponse, error) { + return tc.PromptMFAChallenge(ctx, c, nil /* optsOverride */) }) return key, err @@ -2762,11 +2754,6 @@ func (tc *TeleportClient) GetWebConfig(ctx context.Context) (*webclient.WebConfi // Login logs the user into a Teleport cluster by talking to a Teleport proxy. // -// Login may hijack stdin in some scenarios; it's strongly recommended for -// callers to rely exclusively on prompt.Stdin after calling this method. -// Alternatively, if tc.UseStrongestAuth is set, then no stdin hijacking -// happens. -// // The returned Key should typically be passed to ActivateKey in order to // update local agent state. func (tc *TeleportClient) Login(ctx context.Context) (*Key, error) { @@ -3323,7 +3310,8 @@ func (tc *TeleportClient) mfaLocalLogin(ctx context.Context, pub []byte) (*auth. }, User: tc.Username, Password: password, - UseStrongestAuth: tc.UseStrongestAuth, + AllowStdinHijack: tc.AllowStdinHijack, + PreferOTP: tc.PreferOTP, }) return response, trace.Wrap(err) diff --git a/lib/client/api_login_test.go b/lib/client/api_login_test.go index 982fdae4c2fd5..238e182f9789d 100644 --- a/lib/client/api_login_test.go +++ b/lib/client/api_login_test.go @@ -159,46 +159,61 @@ func TestTeleportClient_Login_localMFALogin(t *testing.T) { solveOTP func(context.Context) (string, error) solveU2F func(ctx context.Context, facet string, challenges ...u2flib.AuthenticateChallenge) (*u2flib.AuthenticateChallengeResponse, error) solveWebauthn func(ctx context.Context, origin string, assertion *wanlib.CredentialAssertion) (*proto.MFAAuthenticateResponse, error) - useStrongestAuth bool + allowStdinHijack bool + preferOTP bool }{ { - name: "OK OTP device login", + name: "OK OTP device login with hijack", secondFactor: constants.SecondFactorOptional, solveOTP: solveOTP, solveU2F: func(context.Context, string, ...u2flib.AuthenticateChallenge) (*u2flib.AuthenticateChallengeResponse, error) { panic("unused") }, - solveWebauthn: promptWebauthnNoop, + solveWebauthn: promptWebauthnNoop, + allowStdinHijack: true, }, { - name: "OK Webauthn device login", + name: "OK Webauthn device login with hijack", secondFactor: constants.SecondFactorOptional, solveOTP: promptOTPNoop, solveU2F: func(context.Context, string, ...u2flib.AuthenticateChallenge) (*u2flib.AuthenticateChallengeResponse, error) { panic("unused") }, - solveWebauthn: solveWebauthn, + solveWebauthn: solveWebauthn, + allowStdinHijack: true, }, { - name: "Webauthn and UseStrongestAuth", - secondFactor: constants.SecondFactorOptional, - solveOTP: func(ctx context.Context) (string, error) { + name: "OK U2F device login with hijack", + secondFactor: constants.SecondFactorU2F, + solveOTP: promptOTPNoop, + solveU2F: solveU2F, + solveWebauthn: func(context.Context, string, *wanlib.CredentialAssertion) (*proto.MFAAuthenticateResponse, error) { panic("unused") }, + allowStdinHijack: true, + }, + { + name: "OTP preferred", + secondFactor: constants.SecondFactorOptional, + solveOTP: solveOTP, solveU2F: func(context.Context, string, ...u2flib.AuthenticateChallenge) (*u2flib.AuthenticateChallengeResponse, error) { panic("unused") }, - solveWebauthn: solveWebauthn, - useStrongestAuth: true, + solveWebauthn: func(ctx context.Context, origin string, assertion *wanlib.CredentialAssertion) (*proto.MFAAuthenticateResponse, error) { + panic("unused") + }, + preferOTP: true, }, { - name: "OK U2F device login", - secondFactor: constants.SecondFactorU2F, - solveOTP: promptOTPNoop, - solveU2F: solveU2F, - solveWebauthn: func(context.Context, string, *wanlib.CredentialAssertion) (*proto.MFAAuthenticateResponse, error) { + name: "Webauthn device login", + secondFactor: constants.SecondFactorOptional, + solveOTP: func(ctx context.Context) (string, error) { + panic("unused") + }, + solveU2F: func(context.Context, string, ...u2flib.AuthenticateChallenge) (*u2flib.AuthenticateChallengeResponse, error) { panic("unused") }, + solveWebauthn: solveWebauthn, }, } for _, test := range tests { @@ -222,7 +237,8 @@ func TestTeleportClient_Login_localMFALogin(t *testing.T) { tc, err := client.NewClient(cfg) require.NoError(t, err) - tc.UseStrongestAuth = test.useStrongestAuth + tc.AllowStdinHijack = test.allowStdinHijack + tc.PreferOTP = test.preferOTP clock.Advance(30 * time.Second) _, err = tc.Login(ctx) diff --git a/lib/client/client.go b/lib/client/client.go index 6b2d62ab11483..59c7bb8ed1793 100644 --- a/lib/client/client.go +++ b/lib/client/client.go @@ -1637,8 +1637,8 @@ func (proxy *ProxyClient) sessionSSHCertificate(ctx context.Context, nodeAddr No NodeName: nodeName(nodeAddr.Addr), RouteToCluster: nodeAddr.Cluster, }, - func(ctx context.Context, proxyAddr string, c *proto.MFAAuthenticateChallenge) (*proto.MFAAuthenticateResponse, error) { - return PromptMFAChallenge(ctx, c, proxyAddr, nil /* opts */) + func(ctx context.Context, _ string, c *proto.MFAAuthenticateChallenge) (*proto.MFAAuthenticateResponse, error) { + return proxy.teleportClient.PromptMFAChallenge(ctx, c, nil /* optsOverride */) }, ) if err != nil { diff --git a/lib/client/mfa.go b/lib/client/mfa.go index 8133ddd21e433..02293108d330f 100644 --- a/lib/client/mfa.go +++ b/lib/client/mfa.go @@ -49,19 +49,37 @@ type PromptMFAChallengeOpts struct { PromptDevicePrefix string // Quiet suppresses users prompts. Quiet bool - // UseStrongestAuth prompts the user to solve only the strongest challenge - // available. - // If set it also avoids stdin hijacking, as only one prompt is necessary. - UseStrongestAuth bool + // AllowStdinHijack allows stdin hijack during MFA prompts. + // Stdin hijack provides a better login UX, but it can be difficult to reason + // about and is often a source of bugs. + // Do not set this options unless you deeply understand what you are doing. + // If false then only the strongest auth method is prompted. + AllowStdinHijack bool + // PreferOTP favors OTP challenges, if applicable. + // Takes precedence over AuthenticatorAttachment settings. + PreferOTP bool +} + +// PromptMFAChallenge prompts the user to complete MFA authentication +// challenges. +// See client.PromptMFAChallenge. +func (tc *TeleportClient) PromptMFAChallenge( + ctx context.Context, c *proto.MFAAuthenticateChallenge, optsOverride *PromptMFAChallengeOpts) (*proto.MFAAuthenticateResponse, error) { + opts := &PromptMFAChallengeOpts{ + AllowStdinHijack: tc.AllowStdinHijack, + PreferOTP: tc.PreferOTP, + } + if optsOverride != nil { + opts.PromptDevicePrefix = optsOverride.PromptDevicePrefix + opts.Quiet = optsOverride.Quiet + opts.PreferOTP = optsOverride.PreferOTP + opts.AllowStdinHijack = optsOverride.AllowStdinHijack + } + return PromptMFAChallenge(ctx, c, tc.WebProxyAddr, opts) } // PromptMFAChallenge prompts the user to complete MFA authentication // challenges. -// -// PromptMFAChallenge makes an attempt to read OTPs from prompt.Stdin and -// abandons the read if the user chooses WebAuthn instead. For this reason -// callers must use prompt.Stdin exclusively after calling this function. -// Set opts.UseStrongestAuth to avoid stdin hijacking. func PromptMFAChallenge(ctx context.Context, c *proto.MFAAuthenticateChallenge, proxyAddr string, opts *PromptMFAChallengeOpts) (*proto.MFAAuthenticateResponse, error) { // Is there a challenge present? if c.TOTP == nil && len(c.U2F) == 0 && c.WebauthnChallenge == nil { @@ -87,8 +105,12 @@ func PromptMFAChallenge(ctx context.Context, c *proto.MFAAuthenticateChallenge, hasNonTOTP = false } - // Prompt only for the strongest auth method available? - if opts.UseStrongestAuth && hasNonTOTP { + // Tweak enabled/disabled methods according to opts. + switch { + case hasTOTP && opts.PreferOTP: + hasNonTOTP = false + case hasNonTOTP && !opts.AllowStdinHijack: + // Use strongest auth if hijack is not allowed. hasTOTP = false } @@ -137,23 +159,25 @@ func PromptMFAChallenge(ctx context.Context, c *proto.MFAAuthenticateChallenge, } // Fire Webauthn or U2F goroutine. - origin := proxyAddr - if !strings.HasPrefix(origin, "https://") { - origin = "https://" + origin - } - switch { - case c.WebauthnChallenge != nil: - go func() { - log.Debugf("WebAuthn: prompting U2F devices with origin %q", origin) - resp, err := promptWebauthn(ctx, origin, wanlib.CredentialAssertionFromProto(c.WebauthnChallenge)) - respC <- response{kind: "WEBAUTHN", resp: resp, err: err} - }() - case len(c.U2F) > 0: - go func() { - log.Debugf("prompting U2F devices with facet %q", origin) - resp, err := promptU2FChallenges(ctx, proxyAddr, c.U2F) - respC <- response{kind: "U2F", resp: resp, err: err} - }() + if hasNonTOTP { + origin := proxyAddr + if !strings.HasPrefix(origin, "https://") { + origin = "https://" + origin + } + switch { + case c.WebauthnChallenge != nil: + go func() { + log.Debugf("WebAuthn: prompting U2F devices with origin %q", origin) + resp, err := promptWebauthn(ctx, origin, wanlib.CredentialAssertionFromProto(c.WebauthnChallenge)) + respC <- response{kind: "WEBAUTHN", resp: resp, err: err} + }() + case len(c.U2F) > 0: + go func() { + log.Debugf("prompting U2F devices with facet %q", origin) + resp, err := promptU2FChallenges(ctx, proxyAddr, c.U2F) + respC <- response{kind: "U2F", resp: resp, err: err} + }() + } } for i := 0; i < numGoroutines; i++ { diff --git a/lib/client/weblogin.go b/lib/client/weblogin.go index 9ef571440b397..92fb9523d4487 100644 --- a/lib/client/weblogin.go +++ b/lib/client/weblogin.go @@ -215,13 +215,14 @@ type SSHLoginMFA struct { SSHLogin // User is the login username. User string - // User is the login password. + // Password is the login password. Password string - // UseStrongestAuth instructs the MFA prompt to use the strongest - // authentication method supported by the cluster. - // Apart from the obvious benefits, UseStrongestAuth also avoids stdin - // hijacking issues from MFA prompts, as a single auth method is used. - UseStrongestAuth bool + + // AllowStdinHijack allows stdin hijack during MFA prompts. + // Do not set this options unless you deeply understand what you are doing. + AllowStdinHijack bool + // PreferOTP prefers OTP in favor of other MFA methods. + PreferOTP bool } // initClient creates a new client to the HTTPS web proxy. @@ -416,7 +417,8 @@ func SSHAgentMFALogin(ctx context.Context, login SSHLoginMFA) (*auth.SSHLoginRes } respPB, err := PromptMFAChallenge(ctx, challengePB, login.ProxyAddr, &PromptMFAChallengeOpts{ - UseStrongestAuth: login.UseStrongestAuth, + AllowStdinHijack: login.AllowStdinHijack, + PreferOTP: login.PreferOTP, }) if err != nil { return nil, trace.Wrap(err) diff --git a/tool/tsh/tsh.go b/tool/tsh/tsh.go index 6eeb992725b9b..6e64216bb51af 100644 --- a/tool/tsh/tsh.go +++ b/tool/tsh/tsh.go @@ -74,6 +74,15 @@ var log = logrus.WithFields(logrus.Fields{ trace.Component: teleport.ComponentTSH, }) +const ( + // mfaModeAuto automatically chooses the best MFA device(s), without any + // restrictions. + // Allows both Webauthn and OTP. + mfaModeAuto = "auto" + // mfaModeOTP utilizes only OTP devices. + mfaModeOTP = "otp" +) + // CLIConf stores command line arguments and flags: type CLIConf struct { // UserHost contains "[login]@hostname" argument to SSH command @@ -199,6 +208,9 @@ type CLIConf struct { // AuthConnector is the name of the connector to use. AuthConnector string + // MFAMode is the preferred mode for MFA assertions. + MFAMode string + // SkipVersionCheck skips version checking for client and server SkipVersionCheck bool @@ -369,6 +381,7 @@ const ( addKeysToAgentEnvVar = "TELEPORT_ADD_KEYS_TO_AGENT" useLocalSSHAgentEnvVar = "TELEPORT_USE_LOCAL_SSH_AGENT" globalTshConfigEnvVar = "TELEPORT_GLOBAL_TSH_CONFIG" + mfaModeEnvVar = "TELEPORT_MFA_MODE" clusterHelp = "Specify the Teleport cluster to connect" browserHelp = "Set to 'none' to suppress browser opening on login" @@ -437,6 +450,11 @@ func Run(args []string, opts ...cliOption) error { Default("true"). BoolVar(&cf.EnableEscapeSequences) app.Flag("bind-addr", "Override host:port used when opening a browser for cluster logins").Envar(bindAddrEnvVar).StringVar(&cf.BindAddr) + modes := []string{mfaModeAuto, mfaModeOTP} + app.Flag("mfa-mode", fmt.Sprintf("Preferred mode for MFA and Passwordless assertions (%v)", strings.Join(modes, ", "))). + Default(mfaModeAuto). + Envar(mfaModeEnvVar). + EnumVar(&cf.MFAMode, modes...) app.HelpFlag.Short('h') ver := app.Command("version", "Print the version") ver.Flag("format", formatFlagDescription(defaultFormats...)).Short('f').Default(teleport.Text).EnumVar(&cf.Format, defaultFormats...) @@ -977,6 +995,7 @@ func onLogin(cf *CLIConf) error { return trace.Wrap(err) } tc.HomePath = cf.HomePath + // client is already logged in and profile is not expired if profile != nil && !profile.IsExpired(clockwork.NewRealClock()) { switch { @@ -1057,10 +1076,16 @@ func onLogin(cf *CLIConf) error { // -i flag specified? save the retrieved cert into an identity file makeIdentityFile := (cf.IdentityFileOut != "") + // stdin hijack is OK for login, since it tsh doesn't read input after the + // login ceremony is complete. + // Only allow the option during the login ceremony. + tc.AllowStdinHijack = true + key, err := tc.Login(cf.Context) if err != nil { return trace.Wrap(err) } + tc.AllowStdinHijack = false // the login operation may update the username and should be considered the more // "authoritative" source. @@ -2296,6 +2321,11 @@ func makeClient(cf *CLIConf, useProfileLogin bool) (*client.TeleportClient, erro if cf.AuthConnector != "" { c.AuthConnector = cf.AuthConnector } + mfaOpts, err := parseMFAMode(cf.MFAMode) + if err != nil { + return nil, trace.Wrap(err) + } + c.PreferOTP = mfaOpts.PreferOTP // If agent forwarding was specified on the command line enable it. c.ForwardAgent = options.ForwardAgent @@ -2374,6 +2404,22 @@ func makeClient(cf *CLIConf, useProfileLogin bool) (*client.TeleportClient, erro return tc, nil } +type mfaModeOpts struct { + PreferOTP bool +} + +func parseMFAMode(mode string) (*mfaModeOpts, error) { + opts := &mfaModeOpts{} + switch mode { + case "", mfaModeAuto: + case mfaModeOTP: + opts.PreferOTP = true + default: + return nil, fmt.Errorf("invalid MFA mode: %q", mode) + } + return opts, nil +} + // setX11Config sets X11 config using CLI and SSH option flags. func setX11Config(c *client.Config, cf *CLIConf, o Options, fn envGetter) error { // X11 forwarding can be enabled with -X, -Y, or -oForwardX11=yes