Make relogin attempts use the strongest auth method (#11781)

Fixes a potential stdin hijacking bug by making relogin attempts default to a
single MFA method (the strongest available).

The problematic scenario is as follows:

1. User has both OTP and security keys registered
2. "Relogin" is triggered via a tsh command (say,
   `tsh logout; tsh ssh --proxy=example.com llama@myserver`)
3. User is prompted to pick either OTP or security key ("Tap any security key or
   enter a code from a OTP device")
4. An stdin read is fired in the background to read the OTP code (via
   prompt.Stdin)
5. User picks the security method, thus the stdin read is "abandoned"

In most cases this is fine, as the program ends right after. The issue is when a
relogin is triggered by a long living tsh invocation (again, `tsh ssh ...`): in
this case the stdin hijack causes input to be swallowed.

Forcing a single MFA option avoids the potential stdin hijack, fixing the
problem for all relogin invocations. `tsh login` behavior remains the same.

Note that we have to default to cluster's most secure method _without_ checking
the user devices. The user is not logged in yet, thus the backend cannot reveal
any information about that user.

Issue #11709.

lib/client/api.go

@@ -349,6 +349,12 @@ 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
 }
 
 // CachePolicy defines cache policy for local clients
@@ -516,8 +522,10 @@ func (p *ProfileStatus) AppNames() (result []string) {
 	return result
 }
 
-// RetryWithRelogin is a helper error handling method,
-// attempts to relogin and retry the function once
+// 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 {
@@ -534,10 +542,21 @@ 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) {
-			return trace.Wrap(err, "refusing to connect to untrusted proxy %v without --insecure flag\n", tc.Config.SSHProxyAddr)
+			return trace.Wrap(err, "refusing to connect to untrusted proxy %v without --insecure flag\n", tc.SSHProxyAddr)
 		}
 		return trace.Wrap(err)
 	}
@@ -1288,7 +1307,7 @@ func (tc *TeleportClient) IssueUserCertsWithMFA(ctx context.Context, params Reis
 
 	key, err := proxyClient.IssueUserCertsWithMFA(ctx, params,
 		func(ctx context.Context, proxyAddr string, c *proto.MFAAuthenticateChallenge) (*proto.MFAAuthenticateResponse, error) {
-			return PromptMFAChallenge(ctx, proxyAddr, c, "")
+			return PromptMFAChallenge(ctx, c, proxyAddr, nil /* opts */)
 		})
 
 	return key, err
@@ -2395,9 +2414,13 @@ func (tc *TeleportClient) PingAndShowMOTD(ctx context.Context) (*webclient.PingR
 
 // 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) {
 	// Ping the endpoint to see if it's up and find the type of authentication
 	// supported, also show the message of the day if available.

lib/client/api_login_test.go

@@ -154,11 +154,12 @@ func TestTeleportClient_Login_localMFALogin(t *testing.T) {
 
 	ctx := context.Background()
 	tests := []struct {
-		name          string
-		secondFactor  constants.SecondFactorType
-		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)
+		name             string
+		secondFactor     constants.SecondFactorType
+		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
 	}{
 		{
 			name:         "OK OTP device login",
@@ -178,6 +179,17 @@ func TestTeleportClient_Login_localMFALogin(t *testing.T) {
 			},
 			solveWebauthn: solveWebauthn,
 		},
+		{
+			name:         "Webauthn and UseStrongestAuth",
+			secondFactor: constants.SecondFactorOptional,
+			inputReader: prompt.NewFakeReader().
+				AddString(password).
+				AddReply(func(ctx context.Context) (string, error) {
+					panic("this should not be called")
+				}),
+			solveWebauthn:    solveWebauthn,
+			useStrongestAuth: true,
+		},
 		{
 			name:         "OK U2F device login",
 			secondFactor: constants.SecondFactorU2F,
@@ -209,6 +221,7 @@ func TestTeleportClient_Login_localMFALogin(t *testing.T) {
 
 			tc, err := client.NewClient(cfg)
 			require.NoError(t, err)
+			tc.UseStrongestAuth = test.useStrongestAuth
 
 			clock.Advance(30 * time.Second)
 			_, err = tc.Login(ctx)

lib/client/client.go

@@ -1513,7 +1513,7 @@ func (proxy *ProxyClient) sessionSSHCertificate(ctx context.Context, nodeAddr No
 			RouteToCluster: nodeAddr.Cluster,
 		},
 		func(ctx context.Context, proxyAddr string, c *proto.MFAAuthenticateChallenge) (*proto.MFAAuthenticateResponse, error) {
-			return PromptMFAChallenge(ctx, proxyAddr, c, "")
+			return PromptMFAChallenge(ctx, c, proxyAddr, nil /* opts */)
 		},
 	)
 	if err != nil {

lib/client/mfa.go

@@ -41,17 +41,36 @@ var promptU2F = u2f.AuthenticateSignChallenge
 // promptWebauthn allows tests to override the Webauthn prompt function.
 var promptWebauthn = wancli.Login
 
+// PromptMFAChallengeOpts groups optional settings for PromptMFAChallenge.
+type PromptMFAChallengeOpts struct {
+	// PromptDevicePrefix is an optional prefix printed before "security key" or
+	// "device". It is used to emphasize between different kinds of devices, like
+	// registered vs new.
+	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
+}
+
 // PromptMFAChallenge prompts the user to complete MFA authentication
 // challenges.
-//
-// If promptDevicePrefix is set, it will be printed in prompts before "security
-// key" or "device". This is used to emphasize between different kinds of
-// devices, like registered vs new.
-func PromptMFAChallenge(ctx context.Context, proxyAddr string, c *proto.MFAAuthenticateChallenge, promptDevicePrefix string) (*proto.MFAAuthenticateResponse, error) {
+// 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 {
 		return &proto.MFAAuthenticateResponse{}, nil
 	}
+	if opts == nil {
+		opts = &PromptMFAChallengeOpts{}
+	}
+	promptDevicePrefix := opts.PromptDevicePrefix
+	quiet := opts.Quiet
 
 	// We have three maximum challenges, from which we only pick two: TOTP and
 	// either Webauthn (preferred) or U2F.
@@ -67,6 +86,11 @@ func PromptMFAChallenge(ctx context.Context, proxyAddr string, c *proto.MFAAuthe
 		hasNonTOTP = false
 	}
 
+	// Prompt only for the strongest auth method available?
+	if opts.UseStrongestAuth && hasWebauthn {
+		hasTOTP = false
+	}
+
 	var numGoroutines int
 	if hasTOTP && hasNonTOTP {
 		numGoroutines = 2

lib/client/weblogin.go

@@ -212,6 +212,11 @@ type SSHLoginMFA struct {
 	User string
 	// User 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
 }
 
 // initClient creates a new client to the HTTPS web proxy.
@@ -405,7 +410,9 @@ func SSHAgentMFALogin(ctx context.Context, login SSHLoginMFA) (*auth.SSHLoginRes
 		challengePB.WebauthnChallenge = wanlib.CredentialAssertionToProto(challenge.WebauthnChallenge)
 	}
 
-	respPB, err := PromptMFAChallenge(ctx, login.ProxyAddr, challengePB, "")
+	respPB, err := PromptMFAChallenge(ctx, challengePB, login.ProxyAddr, &PromptMFAChallengeOpts{
+		UseStrongestAuth: login.UseStrongestAuth,
+	})
 	if err != nil {
 		return nil, trace.Wrap(err)
 	}

tool/tsh/mfa.go

@@ -278,7 +278,9 @@ func (c *mfaAddCommand) addDeviceRPC(ctx context.Context, tc *client.TeleportCli
 		if authChallenge == nil {
 			return trace.BadParameter("server bug: server sent %T when client expected AddMFADeviceResponse_ExistingMFAChallenge", resp.Response)
 		}
-		authResp, err := client.PromptMFAChallenge(ctx, tc.Config.WebProxyAddr, authChallenge, "*registered* ")
+		authResp, err := client.PromptMFAChallenge(ctx, authChallenge, tc.Config.WebProxyAddr, &client.PromptMFAChallengeOpts{
+			PromptDevicePrefix: "*registered*",
+		})
 		if err != nil {
 			return trace.Wrap(err)
 		}
@@ -498,7 +500,7 @@ func (c *mfaRemoveCommand) run(cf *CLIConf) error {
 		if authChallenge == nil {
 			return trace.BadParameter("server bug: server sent %T when client expected DeleteMFADeviceResponse_MFAChallenge", resp.Response)
 		}
-		authResp, err := client.PromptMFAChallenge(cf.Context, tc.Config.WebProxyAddr, authChallenge, "")
+		authResp, err := client.PromptMFAChallenge(cf.Context, authChallenge, tc.Config.WebProxyAddr, nil /* opts */)
 		if err != nil {
 			return trace.Wrap(err)
 		}