Expand --mfa-mode and disable stdin hijack by default (#13134)

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.

Fixes #12675 and #13021.

* Expand --mfa-mode and disable stdin hijack by default
* Use TELEPORT_ instead of TSH_ for FIDO2 env var
* Use t.Setenv in tests

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) {
@@ -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)

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
 	}
 

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)

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