Refactor CLI prompt and config logic

api/mfa/prompt.go

@@ -40,7 +40,7 @@ func (f PromptFunc) Run(ctx context.Context, chal *proto.MFAAuthenticateChalleng
 // PromptConstructor is a function that creates a new MFA prompt.
 type PromptConstructor func(...PromptOpt) Prompt
 
-// PromptConfig contains common mfa prompt config options.
+// PromptConfig contains universal mfa prompt config options.
 type PromptConfig struct {
 	// PromptReason is an optional message to share with the user before an MFA Prompt.
 	// It is intended to provide context about why the user is being prompted where it may

lib/client/mfa.go

@@ -57,7 +57,14 @@ type WebauthnLoginFunc = libmfa.WebauthnLoginFunc
 func (tc *TeleportClient) NewMFAPrompt(opts ...mfa.PromptOpt) mfa.Prompt {
 	cfg := tc.newPromptConfig(opts...)
 
-	var prompt mfa.Prompt = libmfa.NewCLIPrompt(cfg, tc.Stderr)
+	var prompt mfa.Prompt = libmfa.NewCLIPromptV2(&libmfa.CLIPromptConfig{
+		PromptConfig:     *cfg,
+		Writer:           tc.Stderr,
+		PreferOTP:        tc.PreferOTP,
+		AllowStdinHijack: tc.AllowStdinHijack,
+		StdinFunc:        tc.StdinFunc,
+	})
+
 	if tc.MFAPromptConstructor != nil {
 		prompt = tc.MFAPromptConstructor(cfg)
 	}
@@ -68,10 +75,6 @@ func (tc *TeleportClient) NewMFAPrompt(opts ...mfa.PromptOpt) mfa.Prompt {
 func (tc *TeleportClient) newPromptConfig(opts ...mfa.PromptOpt) *libmfa.PromptConfig {
 	cfg := libmfa.NewPromptConfig(tc.WebProxyAddr, opts...)
 	cfg.AuthenticatorAttachment = tc.AuthenticatorAttachment
-	cfg.PreferOTP = tc.PreferOTP
-	cfg.AllowStdinHijack = tc.AllowStdinHijack
-	cfg.StdinFunc = tc.StdinFunc
-
 	if tc.WebauthnLogin != nil {
 		cfg.WebauthnLoginFunc = tc.WebauthnLogin
 		cfg.WebauthnSupported = true

lib/client/mfa/cli.go

@@ -22,6 +22,8 @@ import (
 	"context"
 	"fmt"
 	"io"
+	"log/slog"
+	"os"
 	"runtime"
 	"sync"
 
@@ -35,17 +37,53 @@ import (
 	"github.com/gravitational/teleport/lib/auth/webauthnwin"
 )
 
+// CLIPromptConfig contains CLI prompt config options.
+type CLIPromptConfig struct {
+	PromptConfig
+	// Writer is where the prompt outputs the prompt. Defaults to os.Stderr.
+	Writer io.Writer
+	// 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
+	// StdinFunc allows tests to override prompt.Stdin().
+	// If nil prompt.Stdin() is used.
+	StdinFunc func() prompt.StdinReader
+}
+
 // CLIPrompt is the default CLI mfa prompt implementation.
 type CLIPrompt struct {
-	cfg    PromptConfig
-	writer io.Writer
+	cfg CLIPromptConfig
 }
 
 // NewCLIPrompt returns a new CLI mfa prompt with the config and writer.
+// TODO(Joerger): Delete once /e is no longer dependent on it.
 func NewCLIPrompt(cfg *PromptConfig, writer io.Writer) *CLIPrompt {
+	// If no config is provided, use defaults (zero value).
+	if cfg == nil {
+		cfg = new(PromptConfig)
+	}
+	return NewCLIPromptV2(&CLIPromptConfig{
+		PromptConfig: *cfg,
+		Writer:       writer,
+	})
+}
+
+// NewCLIPromptV2 returns a new CLI mfa prompt with the given config.
+// TODO(Joerger): this is V2 because /e depends on a different function
+// signature for NewCLIPrompt, so this requires a couple follow up PRs to fix.
+func NewCLIPromptV2(cfg *CLIPromptConfig) *CLIPrompt {
+	// If no config is provided, use defaults (zero value).
+	if cfg == nil {
+		cfg = new(CLIPromptConfig)
+	}
 	return &CLIPrompt{
-		cfg:    *cfg,
-		writer: writer,
+		cfg: *cfg,
 	}
 }
 
@@ -56,96 +94,75 @@ func (c *CLIPrompt) stdin() prompt.StdinReader {
 	return c.cfg.StdinFunc()
 }
 
+func (c *CLIPrompt) writer() io.Writer {
+	if c.cfg.Writer == nil {
+		return os.Stderr
+	}
+	return c.cfg.Writer
+}
+
 // Run prompts the user to complete an MFA authentication challenge.
 func (c *CLIPrompt) Run(ctx context.Context, chal *proto.MFAAuthenticateChallenge) (*proto.MFAAuthenticateResponse, error) {
 	if c.cfg.PromptReason != "" {
-		fmt.Fprintln(c.writer, c.cfg.PromptReason)
+		fmt.Fprintln(c.writer(), c.cfg.PromptReason)
 	}
 
-	runOpts, err := c.cfg.GetRunOptions(ctx, chal)
-	if err != nil {
-		return nil, trace.Wrap(err)
-	}
+	promptOTP := chal.TOTP != nil
+	promptWebauthn := chal.WebauthnChallenge != nil
 
 	// No prompt to run, no-op.
-	if !runOpts.PromptTOTP && !runOpts.PromptWebauthn {
+	if !promptOTP && !promptWebauthn {
 		return &proto.MFAAuthenticateResponse{}, nil
 	}
 
-	// Depending on the run opts, we may spawn a TOTP goroutine, webauth goroutine, or both.
-	spawnGoroutines := func(ctx context.Context, wg *sync.WaitGroup, respC chan<- MFAGoroutineResponse) {
-		dualPrompt := runOpts.PromptTOTP && runOpts.PromptWebauthn
-
-		// Print the prompt message directly here in case of dualPrompt.
-		// This avoids problems with a goroutine failing before any message is
-		// printed.
-		if dualPrompt {
-			var message string
-			if runtime.GOOS == constants.WindowsOS {
-				message = "Follow the OS dialogs for platform authentication, or enter an OTP code here:"
-				webauthnwin.SetPromptPlatformMessage("")
-			} else {
-				message = fmt.Sprintf("Tap any %ssecurity key or enter a code from a %sOTP device", c.promptDevicePrefix(), c.promptDevicePrefix())
-			}
-			fmt.Fprintln(c.writer, message)
+	// Check off unsupported methods.
+	if promptWebauthn && !c.cfg.WebauthnSupported {
+		promptWebauthn = false
+		slog.DebugContext(ctx, "hardware device MFA not supported by your platform")
+		if !promptOTP {
+			return nil, trace.BadParameter("hardware device MFA not supported by your platform, please register an OTP device")
 		}
+	}
 
-		// Fire TOTP goroutine.
-		var otpCancelAndWait func()
-		if runOpts.PromptTOTP {
-			otpCtx, otpCancel := context.WithCancel(ctx)
-			otpDone := make(chan struct{})
-			otpCancelAndWait = func() {
-				otpCancel()
-				<-otpDone
-			}
+	// Prefer whatever method is requested by the client.
+	if c.cfg.PreferOTP && promptOTP {
+		promptWebauthn = false
+	}
 
-			wg.Add(1)
-			go func() {
-				defer func() {
-					wg.Done()
-					otpCancel()
-					close(otpDone)
-				}()
-
-				quiet := c.cfg.Quiet || dualPrompt
-				resp, err := c.promptTOTP(otpCtx, quiet)
-				respC <- MFAGoroutineResponse{Resp: resp, Err: trace.Wrap(err, "TOTP authentication failed")}
-			}()
-		}
+	// Use stronger auth methods if hijack is not allowed.
+	if !c.cfg.AllowStdinHijack && promptWebauthn {
+		promptOTP = false
+	}
 
-		// Fire Webauthn goroutine.
-		if runOpts.PromptWebauthn {
-			wg.Add(1)
-			go func() {
-				defer func() {
-					wg.Done()
-					// Important for dual-prompt, harmless otherwise.
-					webauthnwin.ResetPromptPlatformMessage()
-				}()
-
-				// Get webauthn prompt and wrap with otp context handler.
-				prompt := &webauthnPromptWithOTP{
-					LoginPrompt:      c.getWebauthnPrompt(ctx, dualPrompt),
-					otpCancelAndWait: otpCancelAndWait,
-				}
-
-				resp, err := c.promptWebauthn(ctx, chal, prompt)
-				respC <- MFAGoroutineResponse{Resp: resp, Err: trace.Wrap(err, "Webauthn authentication failed")}
-			}()
-		}
+	// If a specific webauthn attachment was requested, skip OTP.
+	// Otherwise, allow dual prompt with OTP.
+	if promptWebauthn && c.cfg.AuthenticatorAttachment != wancli.AttachmentAuto {
+		promptOTP = false
 	}
 
-	return HandleMFAPromptGoroutines(ctx, spawnGoroutines)
+	switch {
+	case promptOTP && promptWebauthn:
+		resp, err := c.promptWebauthnAndOTP(ctx, chal)
+		return resp, trace.Wrap(err)
+	case promptWebauthn:
+		resp, err := c.promptWebauthn(ctx, chal, c.getWebauthnPrompt(ctx))
+		return resp, trace.Wrap(err)
+	case promptOTP:
+		resp, err := c.promptOTP(ctx, c.cfg.Quiet)
+		return resp, trace.Wrap(err)
+	default:
+		// We shouldn't reach this case as we would have hit the no-op case above.
+		return nil, trace.BadParameter("no MFA methods to prompt")
+	}
 }
 
-func (c *CLIPrompt) promptTOTP(ctx context.Context, quiet bool) (*proto.MFAAuthenticateResponse, error) {
+func (c *CLIPrompt) promptOTP(ctx context.Context, quiet bool) (*proto.MFAAuthenticateResponse, error) {
 	var msg string
 	if !quiet {
 		msg = fmt.Sprintf("Enter an OTP code from a %sdevice", c.promptDevicePrefix())
 	}
 
-	otp, err := prompt.Password(ctx, c.writer, c.stdin(), msg)
+	otp, err := prompt.Password(ctx, c.writer(), c.stdin(), msg)
 	if err != nil {
 		return nil, trace.Wrap(err)
 	}
@@ -157,8 +174,8 @@ func (c *CLIPrompt) promptTOTP(ctx context.Context, quiet bool) (*proto.MFAAuthe
 	}, nil
 }
 
-func (c *CLIPrompt) getWebauthnPrompt(ctx context.Context, dualPrompt bool) wancli.LoginPrompt {
-	writer := c.writer
+func (c *CLIPrompt) getWebauthnPrompt(ctx context.Context) *wancli.DefaultPrompt {
+	writer := c.writer()
 	if c.cfg.Quiet {
 		writer = io.Discard
 	}
@@ -167,13 +184,6 @@ func (c *CLIPrompt) getWebauthnPrompt(ctx context.Context, dualPrompt bool) wanc
 	prompt.StdinFunc = c.cfg.StdinFunc
 	prompt.SecondTouchMessage = fmt.Sprintf("Tap your %ssecurity key to complete login", c.promptDevicePrefix())
 	prompt.FirstTouchMessage = fmt.Sprintf("Tap any %ssecurity key", c.promptDevicePrefix())
-
-	// Skip when both OTP and WebAuthn are possible, as the prompt happens
-	// externally.
-	if dualPrompt {
-		prompt.FirstTouchMessage = ""
-	}
-
 	return prompt
 }
 
@@ -194,6 +204,66 @@ func (c *CLIPrompt) promptDevicePrefix() string {
 	return ""
 }
 
+func (c *CLIPrompt) promptWebauthnAndOTP(ctx context.Context, chal *proto.MFAAuthenticateChallenge) (*proto.MFAAuthenticateResponse, error) {
+	spawnGoroutines := func(ctx context.Context, wg *sync.WaitGroup, respC chan<- MFAGoroutineResponse) {
+		var message string
+		if runtime.GOOS == constants.WindowsOS {
+			message = "Follow the OS dialogs for platform authentication, or enter an OTP code here:"
+			webauthnwin.SetPromptPlatformMessage("")
+		} else {
+			message = fmt.Sprintf("Tap any %ssecurity key or enter a code from a %sOTP device", c.promptDevicePrefix(), c.promptDevicePrefix())
+		}
+		fmt.Fprintln(c.writer(), message)
+
+		// Fire OTP goroutine.
+		var otpCancelAndWait func()
+		otpCtx, otpCancel := context.WithCancel(ctx)
+		otpDone := make(chan struct{})
+		otpCancelAndWait = func() {
+			otpCancel()
+			<-otpDone
+		}
+
+		wg.Add(1)
+		go func() {
+			defer func() {
+				wg.Done()
+				otpCancel()
+				close(otpDone)
+			}()
+
+			resp, err := c.promptOTP(otpCtx, true /*quiet*/)
+			respC <- MFAGoroutineResponse{Resp: resp, Err: trace.Wrap(err, "TOTP authentication failed")}
+		}()
+
+		// Fire Webauthn goroutine.
+		wg.Add(1)
+		go func() {
+			defer func() {
+				wg.Done()
+				// Important for dual-prompt.
+				webauthnwin.ResetPromptPlatformMessage()
+			}()
+
+			// Skip FirstTouchMessage when both OTP and WebAuthn are possible,
+			// as the prompt happens externally.
+			defaultPrompt := c.getWebauthnPrompt(ctx)
+			defaultPrompt.FirstTouchMessage = ""
+
+			// Wrap the prompt with otp context handler.
+			prompt := &webauthnPromptWithOTP{
+				LoginPrompt:      defaultPrompt,
+				otpCancelAndWait: otpCancelAndWait,
+			}
+
+			resp, err := c.promptWebauthn(ctx, chal, prompt)
+			respC <- MFAGoroutineResponse{Resp: resp, Err: trace.Wrap(err, "Webauthn authentication failed")}
+		}()
+	}
+
+	return HandleMFAPromptGoroutines(ctx, spawnGoroutines)
+}
+
 // webauthnPromptWithOTP implements wancli.LoginPrompt for MFA logins.
 // In most cases authenticators shouldn't require PINs or additional touches for
 // MFA, but the implementation exists in case we find some unusual

lib/client/mfa/cli_test.go

@@ -239,7 +239,6 @@ Enter your security key PIN:
 			prompt.SetStdin(stdin)
 
 			cfg := mfa.NewPromptConfig("proxy.example.com")
-			cfg.AllowStdinHijack = true
 			cfg.WebauthnSupported = true
 			if tc.makeWebauthnLoginFunc != nil {
 				cfg.WebauthnLoginFunc = tc.makeWebauthnLoginFunc(stdin)
@@ -261,7 +260,11 @@ Enter your security key PIN:
 			buffer := make([]byte, 0, 100)
 			out := bytes.NewBuffer(buffer)
 
-			prompt := mfa.NewCLIPrompt(cfg, out)
+			prompt := mfa.NewCLIPromptV2(&mfa.CLIPromptConfig{
+				PromptConfig:     *cfg,
+				Writer:           out,
+				AllowStdinHijack: true,
+			})
 			resp, err := prompt.Run(ctx, tc.challenge)
 
 			if tc.expectErr != nil {

lib/client/mfa/prompt.go

@@ -29,7 +29,6 @@ import (
 
 	"github.com/gravitational/teleport/api/client/proto"
 	"github.com/gravitational/teleport/api/mfa"
-	"github.com/gravitational/teleport/api/utils/prompt"
 	wancli "github.com/gravitational/teleport/lib/auth/webauthncli"
 	wantypes "github.com/gravitational/teleport/lib/auth/webauthntypes"
 )
@@ -44,7 +43,8 @@ type WebauthnLoginFunc func(
 	opts *wancli.LoginOpts,
 ) (*proto.MFAAuthenticateResponse, string, error)
 
-// PromptConfig contains common mfa prompt config options.
+// PromptConfig contains common mfa prompt config options shared by
+// different implementations of [mfa.Prompt].
 type PromptConfig struct {
 	mfa.PromptConfig
 	// ProxyAddress is the address of the authenticating proxy. required.
@@ -64,9 +64,6 @@ type PromptConfig struct {
 	PreferOTP bool
 	// WebauthnSupported indicates whether Webauthn is supported.
 	WebauthnSupported bool
-	// StdinFunc allows tests to override prompt.Stdin().
-	// If nil prompt.Stdin() is used.
-	StdinFunc func() prompt.StdinReader
 }
 
 // NewPromptConfig returns a prompt config that will induce default behavior.