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

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)

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)

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 {

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++ {

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)