From 4756f37fdf1823801d39f238682f9229b328ff1c Mon Sep 17 00:00:00 2001 From: Brian Joerger Date: Fri, 25 Oct 2024 10:22:37 -0700 Subject: [PATCH] Refactor CLI prompt and config logic (#47874) (#47925) * Refactor CLI prompt and config logic. * Move CLI prompt configuration to CLIPrompt * Factor out dual prompt in CLI prompt logic. * Make cli config a field rather than embedded; Add NewCLIPromptV2. --- api/mfa/prompt.go | 2 +- lib/client/mfa.go | 13 +- lib/client/mfa/cli.go | 230 +++++++++++++++++--------- lib/client/mfa/cli_test.go | 7 +- lib/client/mfa/prompt.go | 7 +- lib/client/mfa_test.go | 13 +- tool/tctl/common/admin_action_test.go | 5 +- tool/tctl/common/tctl.go | 4 +- 8 files changed, 181 insertions(+), 100 deletions(-) diff --git a/api/mfa/prompt.go b/api/mfa/prompt.go index 3d430e20f0ecb..e139ff561fa64 100644 --- a/api/mfa/prompt.go +++ b/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 diff --git a/lib/client/mfa.go b/lib/client/mfa.go index 7ade0186f6922..90c6f975d3a1c 100644 --- a/lib/client/mfa.go +++ b/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 diff --git a/lib/client/mfa/cli.go b/lib/client/mfa/cli.go index 6340080ca2b23..a5e4cc8f26178 100644 --- a/lib/client/mfa/cli.go +++ b/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 diff --git a/lib/client/mfa/cli_test.go b/lib/client/mfa/cli_test.go index 0f0d6cc8323b2..54e0fcfd92fd9 100644 --- a/lib/client/mfa/cli_test.go +++ b/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 { diff --git a/lib/client/mfa/prompt.go b/lib/client/mfa/prompt.go index 85733fcf9f847..4f99487810833 100644 --- a/lib/client/mfa/prompt.go +++ b/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. diff --git a/lib/client/mfa_test.go b/lib/client/mfa_test.go index 00f4d7e75b026..845e9b1f975e5 100644 --- a/lib/client/mfa_test.go +++ b/lib/client/mfa_test.go @@ -21,7 +21,6 @@ package client_test import ( "context" "errors" - "os" "testing" "time" @@ -73,7 +72,7 @@ func TestPromptMFAChallenge_usingNonRegisteredDevice(t *testing.T) { tests := []struct { name string challenge *proto.MFAAuthenticateChallenge - customizePrompt func(p *mfa.PromptConfig) + customizePrompt func(p *mfa.CLIPromptConfig) }{ { name: "webauthn only", @@ -82,7 +81,7 @@ func TestPromptMFAChallenge_usingNonRegisteredDevice(t *testing.T) { { name: "webauthn and OTP", challenge: challengeWebauthnOTP, - customizePrompt: func(p *mfa.PromptConfig) { + customizePrompt: func(p *mfa.CLIPromptConfig) { p.AllowStdinHijack = true // required for OTP+WebAuthn prompt. }, }, @@ -109,11 +108,15 @@ func TestPromptMFAChallenge_usingNonRegisteredDevice(t *testing.T) { return nil, "", wancli.ErrUsingNonRegisteredDevice } + cliConfig := &mfa.CLIPromptConfig{ + PromptConfig: *promptConfig, + } + if test.customizePrompt != nil { - test.customizePrompt(promptConfig) + test.customizePrompt(cliConfig) } - _, err := mfa.NewCLIPrompt(promptConfig, os.Stderr).Run(ctx, test.challenge) + _, err := mfa.NewCLIPromptV2(cliConfig).Run(ctx, test.challenge) if !errors.Is(err, wancli.ErrUsingNonRegisteredDevice) { t.Errorf("PromptMFAChallenge returned err=%q, want %q", err, wancli.ErrUsingNonRegisteredDevice) } diff --git a/tool/tctl/common/admin_action_test.go b/tool/tctl/common/admin_action_test.go index e999a312b36d2..c05132b16cd13 100644 --- a/tool/tctl/common/admin_action_test.go +++ b/tool/tctl/common/admin_action_test.go @@ -1028,7 +1028,10 @@ func newAdminActionTestSuite(t *testing.T) *adminActionTestSuite { mockMFAPromptConstructor := func(opts ...mfa.PromptOpt) mfa.Prompt { promptCfg := libmfa.NewPromptConfig(proxyPublicAddr.String(), opts...) promptCfg.WebauthnLoginFunc = mockWebauthnLogin - return libmfa.NewCLIPrompt(promptCfg, os.Stderr) + promptCfg.WebauthnSupported = true + return libmfa.NewCLIPromptV2(&libmfa.CLIPromptConfig{ + PromptConfig: *promptCfg, + }) } // Login as the admin user. diff --git a/tool/tctl/common/tctl.go b/tool/tctl/common/tctl.go index eacd36f91c12b..07a97f31363a9 100644 --- a/tool/tctl/common/tctl.go +++ b/tool/tctl/common/tctl.go @@ -252,7 +252,9 @@ func TryRun(commands []CLICommand, args []string) error { proxyAddr := resp.ProxyPublicAddr client.SetMFAPromptConstructor(func(opts ...mfa.PromptOpt) mfa.Prompt { promptCfg := libmfa.NewPromptConfig(proxyAddr, opts...) - return libmfa.NewCLIPrompt(promptCfg, os.Stderr) + return libmfa.NewCLIPromptV2(&libmfa.CLIPromptConfig{ + PromptConfig: *promptCfg, + }) }) // execute whatever is selected: