diff --git a/lib/client/api.go b/lib/client/api.go index 12d0855982e..e5e9d391f42 100644 --- a/lib/client/api.go +++ b/lib/client/api.go @@ -373,6 +373,12 @@ type Config struct { // Affects the TeleportClient.Login and, indirectly, RetryWithRelogin // functions. Passwordless bool + + // 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 @@ -541,6 +547,8 @@ 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 { @@ -557,10 +565,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) } @@ -1330,7 +1349,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, "", false) + return PromptMFAChallenge(ctx, c, proxyAddr, nil /* opts */) }) return key, err @@ -2507,6 +2526,11 @@ 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. +// // If tc.Passwordless is set, then the passwordless authentication flow is used. // // The returned Key should typically be passed to ActivateKey in order to @@ -2698,7 +2722,7 @@ func (tc *TeleportClient) directLogin(ctx context.Context, secondFactorType cons return nil, trace.Wrap(err) } - // only ask for a second factor if it's enabled + // Only ask for a second factor if it's enabled. var otpToken string if secondFactorType == constants.SecondFactorOTP { otpToken, err = tc.AskOTP() @@ -2707,7 +2731,7 @@ func (tc *TeleportClient) directLogin(ctx context.Context, secondFactorType cons } } - // ask the CA (via proxy) to sign our public key: + // Ask the CA (via proxy) to sign our public key: response, err := SSHAgentLogin(ctx, SSHLoginDirect{ SSHLogin: SSHLogin{ ProxyAddr: tc.WebProxyAddr, @@ -2719,7 +2743,7 @@ func (tc *TeleportClient) directLogin(ctx context.Context, secondFactorType cons RouteToCluster: tc.SiteName, KubernetesCluster: tc.KubernetesCluster, }, - User: tc.Config.Username, + User: tc.Username, Password: password, OTPToken: otpToken, }) @@ -2745,8 +2769,9 @@ func (tc *TeleportClient) mfaLocalLogin(ctx context.Context, pub []byte) (*auth. RouteToCluster: tc.SiteName, KubernetesCluster: tc.KubernetesCluster, }, - User: tc.Config.Username, - Password: password, + User: tc.Username, + Password: password, + UseStrongestAuth: tc.UseStrongestAuth, }) return response, trace.Wrap(err) diff --git a/lib/client/api_login_test.go b/lib/client/api_login_test.go index 42e39ff5e37..5fe4121465e 100644 --- a/lib/client/api_login_test.go +++ b/lib/client/api_login_test.go @@ -138,11 +138,11 @@ func TestTeleportClient_Login_local(t *testing.T) { ctx := context.Background() tests := []struct { - name string - secondFactor constants.SecondFactorType - inputReader *prompt.FakeReader - solveWebauthn func(ctx context.Context, origin string, assertion *wanlib.CredentialAssertion, prompt wancli.LoginPrompt) (*proto.MFAAuthenticateResponse, error) - pwdless bool + name string + secondFactor constants.SecondFactorType + inputReader *prompt.FakeReader + solveWebauthn func(ctx context.Context, origin string, assertion *wanlib.CredentialAssertion, prompt wancli.LoginPrompt) (*proto.MFAAuthenticateResponse, error) + pwdless, useStrongestAuth bool }{ { name: "OTP device login", @@ -156,6 +156,17 @@ func TestTeleportClient_Login_local(t *testing.T) { inputReader: prompt.NewFakeReader().AddString(password).AddReply(waitForCancelFn), 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: "Webauthn device with PIN", // a bit hypothetical, but _could_ happen. secondFactor: constants.SecondFactorOptional, @@ -195,6 +206,7 @@ func TestTeleportClient_Login_local(t *testing.T) { tc, err := client.NewClient(cfg) require.NoError(t, err) tc.Passwordless = test.pwdless + tc.UseStrongestAuth = test.useStrongestAuth clock.Advance(30 * time.Second) _, err = tc.Login(ctx) diff --git a/lib/client/client.go b/lib/client/client.go index 56089e0475d..2deefdf50bc 100644 --- a/lib/client/client.go +++ b/lib/client/client.go @@ -1639,7 +1639,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, "", false) + return PromptMFAChallenge(ctx, c, proxyAddr, nil /* opts */) }, ) if err != nil { diff --git a/lib/client/mfa.go b/lib/client/mfa.go index 4b8a8cfdbff..060c2d8c742 100644 --- a/lib/client/mfa.go +++ b/lib/client/mfa.go @@ -48,21 +48,36 @@ func (p *mfaPrompt) PromptPIN() (string, error) { return p.LoginPrompt.PromptPIN() } +// 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. // 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. -func PromptMFAChallenge( - ctx context.Context, - proxyAddr string, c *proto.MFAAuthenticateChallenge, promptDevicePrefix string, quiet bool) (*proto.MFAAuthenticateResponse, error) { +// 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 && c.WebauthnChallenge == nil { return &proto.MFAAuthenticateResponse{}, nil } + if opts == nil { + opts = &PromptMFAChallengeOpts{} + } + promptDevicePrefix := opts.PromptDevicePrefix + quiet := opts.Quiet hasTOTP := c.TOTP != nil hasWebauthn := c.WebauthnChallenge != nil @@ -76,6 +91,11 @@ func PromptMFAChallenge( hasWebauthn = false } + // Prompt only for the strongest auth method available? + if opts.UseStrongestAuth && hasWebauthn { + hasTOTP = false + } + var numGoroutines int if hasTOTP && hasWebauthn { numGoroutines = 2 diff --git a/lib/client/presence.go b/lib/client/presence.go index 21f42b005a3..e128713b6b9 100644 --- a/lib/client/presence.go +++ b/lib/client/presence.go @@ -86,7 +86,9 @@ func solveMFA(ctx context.Context, term io.Writer, tc *TeleportClient, challenge // We don't support TOTP for live presence. challenge.TOTP = nil - response, err := PromptMFAChallenge(ctx, tc.Config.WebProxyAddr, challenge, "", true) + response, err := PromptMFAChallenge(ctx, challenge, tc.Config.WebProxyAddr, &PromptMFAChallengeOpts{ + Quiet: true, + }) if err != nil { fmt.Fprintf(term, "\r\nTeleport > Failed to confirm presence: %v\r\n", err) return nil, trace.Wrap(err) diff --git a/lib/client/weblogin.go b/lib/client/weblogin.go index 151da093726..0ac9963d975 100644 --- a/lib/client/weblogin.go +++ b/lib/client/weblogin.go @@ -214,6 +214,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. @@ -388,7 +393,9 @@ func SSHAgentMFALogin(ctx context.Context, login SSHLoginMFA) (*auth.SSHLoginRes challengePB.WebauthnChallenge = wanlib.CredentialAssertionToProto(challenge.WebauthnChallenge) } - respPB, err := PromptMFAChallenge(ctx, login.ProxyAddr, challengePB, "", false) + respPB, err := PromptMFAChallenge(ctx, challengePB, login.ProxyAddr, &PromptMFAChallengeOpts{ + UseStrongestAuth: login.UseStrongestAuth, + }) if err != nil { return nil, trace.Wrap(err) } diff --git a/tool/tsh/mfa.go b/tool/tsh/mfa.go index 228fbbdab90..4a8e81208f1 100644 --- a/tool/tsh/mfa.go +++ b/tool/tsh/mfa.go @@ -312,7 +312,9 @@ func (c *mfaAddCommand) addDeviceRPC( 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* ", false) + authResp, err := client.PromptMFAChallenge(ctx, authChallenge, tc.Config.WebProxyAddr, &client.PromptMFAChallengeOpts{ + PromptDevicePrefix: "*registered*", + }) if err != nil { return trace.Wrap(err) } @@ -516,7 +518,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, "", false) + authResp, err := client.PromptMFAChallenge(cf.Context, authChallenge, tc.Config.WebProxyAddr, nil /* opts */) if err != nil { return trace.Wrap(err) }