Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove CLI-based config options from Teleport Connect MFA prompts #47875

Merged
merged 2 commits into from
Oct 28, 2024
Merged

Remove CLI-based config options from Teleport Connect MFA prompts #47875

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
33bf6b7
Allow Teleport Connect users to choose either WebAuthn or OTP in MFA …
Joerger Oct 23, 2024
f4cdf26
Merge branch 'master' into joerger/fix-otp-teleport-connect
Joerger Oct 28, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 0 additions & 44 deletions lib/client/mfa/prompt.go
View file
Open in desktop
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was about to forward #46820 to you and that's how I found this PR.

I don't think this is going to affect #46820 that much nor bring it closer to completion. I assume #46820 is purely about SSH as we show the regular MFA prompt for other resources.

When you SSH to a node, the Electron app executes tsh ssh underneath, without going through the tsh daemon. That tsh ssh process has no knowledge about the Electron app. Making it Electron-aware would likely require too much effort.

I think this could be solved in two ways. tsh could show some kind of prompt letting you choose which auth method you want to use. Historically, it seems that we eschewed that in favor of supplying a specific flag if someone wants to use OTP.

The other way to solve this would be to add a new config option to Connect (ssh.mfaMode?) that adds an appropriate flag to all tsh ssh invocations coming from Connect. The user wouldn't be able to set this flag per-resource or even per profile, but at least it'd unblock users who are not able to use OTP at the moment.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do we think about replacing the terminal-based mfa prompt for SSH with the custom connect MFA prompt? It should be possible to inject the custom prompt in there, but I need to check.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried to explain why that's not hard to do in the third paragraph. ;) The MFA prompt for every other resource kind goes through tsh daemon, e.g., the Electron app tells tshd to create a local proxy, tshd attempts to generate a cert, lib/client code prompts for an MFA check and tshd forwards this to the Electron app.

SSH connections do not go through the tsh daemon, the Electron app just executes tsh ssh.

If there's some other way of doing it without forcing tsh ssh to be Electron-aware then that's something we can explore.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tsh could show some kind of prompt letting you choose which auth method you want to use.

IIUC this is not something we want to do, as it requires us to set AllowStdinHijack=true so that tsh can accept input. We'd need to ask Alan for more details on what issues that can cause.

If there's some other way of doing it without forcing tsh ssh to be Electron-aware then that's something we can explore.

Regardless I think we want this to start with an Electron dialog window if possible, it's cleaner than trying to add a config option or prompt for stdin.

The first idea to come to mind is to add a new tshd request to check if MFA is required before running the tsh ssh command. If MFA is required, we could see what options are available to them (Webauthn, totp, sso) and present them with a diagog to choose one, e.g. by clicking one of 3 buttons. Then depending on what they click, we could run tsh ssh --mfa-mode=sso/otp/webauthn and let the cli prompt continue as normal. WDYT?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would be fine UX-wise, but has the unfortunate effect of degrading performance for people not using per-session MFA, which is something we've strived to avoid. I remember this being the case when Michael was working on MFA in file transfers in the Web UI, for example.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair point. In that case we should let tsh ssh do it's thing, racing an MFA and non-MFA session and only prompting when needed.

Outside of making a whole new ssh for Connect implementation that utilizes tshd <-> Electron communication, we could experiment with other ways of having tshd/Electron communicate with tsh ssh.

For example, similar to our Teleport re-exec logic, we could pass a file descriptor to tsh ssh to handle back and forth communication with the Electron App. Over this file descriptor, tsh ssh could send a generic MFA prompt request when needed, just like to the request from tshd.

Or if a file descriptor isn't a viable solution (I don't know javascript limitations well), we could use a unix/tcp socket instead. Or maybe we could plug into the tshd socket from tsh ssh. e.g. tsh ssh --tshd=/path/to/socket.

Original file line number Diff line number Diff line change
Expand Up @@ -51,17 +51,8 @@ type PromptConfig struct {
ProxyAddress string
// WebauthnLoginFunc performs client-side Webauthn login.
WebauthnLoginFunc WebauthnLoginFunc
// 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
// AuthenticatorAttachment specifies the desired authenticator attachment.
AuthenticatorAttachment wancli.AuthenticatorAttachment
// PreferOTP favors OTP challenges, if applicable.
// Takes precedence over AuthenticatorAttachment settings.
PreferOTP bool
// WebauthnSupported indicates whether Webauthn is supported.
WebauthnSupported bool
}
Expand All @@ -81,41 +72,6 @@ func NewPromptConfig(proxyAddr string, opts ...mfa.PromptOpt) *PromptConfig {
return cfg
}

// RunOpts are mfa prompt run options.
type RunOpts struct {
PromptTOTP bool
PromptWebauthn bool
}

// GetRunOptions gets mfa prompt run options by cross referencing the mfa challenge with prompt configuration.
func (c PromptConfig) GetRunOptions(ctx context.Context, chal *proto.MFAAuthenticateChallenge) (RunOpts, error) {
promptTOTP := chal.TOTP != nil
promptWebauthn := chal.WebauthnChallenge != nil

// Does the current platform support hardware MFA? Adjust accordingly.
switch {
case !promptTOTP && promptWebauthn && !c.WebauthnSupported:
return RunOpts{}, trace.BadParameter("hardware device MFA not supported by your platform, please register an OTP device")
case !c.WebauthnSupported:
// Do not prompt for hardware devices, it won't work.
promptWebauthn = false
}

// Tweak enabled/disabled methods according to opts.
switch {
case promptTOTP && c.PreferOTP:
promptWebauthn = false
case promptWebauthn && c.AuthenticatorAttachment != wancli.AttachmentAuto:
// Prefer Webauthn if an specific attachment was requested.
promptTOTP = false
case promptWebauthn && !c.AllowStdinHijack:
// Use strongest auth if hijack is not allowed.
promptTOTP = false
}

return RunOpts{promptTOTP, promptWebauthn}, nil
}

func (c PromptConfig) GetWebauthnOrigin() string {
if !strings.HasPrefix(c.ProxyAddress, "https://") {
return "https://" + c.ProxyAddress
Expand Down
11 changes: 0 additions & 11 deletions lib/teleterm/clusters/storage.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -285,17 +285,6 @@ func (s *Storage) makeDefaultClientConfig(rootClusterURI uri.ResourceURI) *clien
cfg.InsecureSkipVerify = s.InsecureSkipVerify
cfg.AddKeysToAgent = s.AddKeysToAgent
cfg.WebauthnLogin = s.WebauthnLogin
// Set AllowStdinHijack to true to enable daemon.mfaPrompt to ask for both TOTP and Webauthn at
// the same time if available.
//
// tsh sets AllowStdinHijack to true only during tsh login to avoid input swallowing bugs where
// calling a command would prompt for MFA and then expect some further data through stdin. tsh
// login does not ask for any further input after the MFA prompt.
//
// Since tsh daemon ran by Connect never expects data over stdin, it can always set this flag to
// true.
cfg.AllowStdinHijack = true

cfg.CustomHardwareKeyPrompt = s.HardwareKeyPromptConstructor(rootClusterURI)
cfg.DTAuthnRunCeremony = dtauthn.NewCeremony().Run
cfg.DTAutoEnroll = dtenroll.AutoEnroll
Expand Down
18 changes: 8 additions & 10 deletions lib/teleterm/daemon/mfaprompt.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,13 +73,11 @@ func (s *Service) promptAppMFA(ctx context.Context, in *api.PromptMFARequest) (*

// Run prompts the user to complete an MFA authentication challenge.
func (p *mfaPrompt) Run(ctx context.Context, chal *proto.MFAAuthenticateChallenge) (*proto.MFAAuthenticateResponse, error) {
runOpts, err := p.cfg.GetRunOptions(ctx, chal)
if err != nil {
return nil, trace.Wrap(err)
}
promptOTP := chal.TOTP != nil
promptWebauthn := chal.WebauthnChallenge != nil && p.cfg.WebauthnSupported

// No prompt to run, no-op.
if !runOpts.PromptTOTP && !runOpts.PromptWebauthn {
if !promptOTP && !promptWebauthn {
return &proto.MFAAuthenticateResponse{}, nil
}

Expand All @@ -92,7 +90,7 @@ func (p *mfaPrompt) Run(ctx context.Context, chal *proto.MFAAuthenticateChalleng
go func() {
defer wg.Done()

resp, err := p.promptMFA(ctx, runOpts)
resp, err := p.promptMFA(ctx, promptOTP, promptWebauthn)
respC <- libmfa.MFAGoroutineResponse{Resp: resp, Err: err}

// If the user closes the modal in the Electron app, we need to be able to cancel the other
Expand All @@ -103,7 +101,7 @@ func (p *mfaPrompt) Run(ctx context.Context, chal *proto.MFAAuthenticateChalleng
}()

// Fire Webauthn goroutine.
if runOpts.PromptWebauthn {
if promptWebauthn {
wg.Add(1)
go func() {
defer wg.Done()
Expand All @@ -128,12 +126,12 @@ func (p *mfaPrompt) promptWebauthn(ctx context.Context, chal *proto.MFAAuthentic
return resp, nil
}

func (p *mfaPrompt) promptMFA(ctx context.Context, runOpts libmfa.RunOpts) (*proto.MFAAuthenticateResponse, error) {
func (p *mfaPrompt) promptMFA(ctx context.Context, promptOTP, promptWebauthn bool) (*proto.MFAAuthenticateResponse, error) {
resp, err := p.promptAppMFA(ctx, &api.PromptMFARequest{
ClusterUri: p.resourceURI.GetClusterURI().String(),
Reason: p.cfg.PromptReason,
Totp: runOpts.PromptTOTP,
Webauthn: runOpts.PromptWebauthn,
Totp: promptOTP,
Webauthn: promptWebauthn,
})
if err != nil {
return nil, trail.FromGRPC(err)
Expand Down
8 changes: 0 additions & 8 deletions web/packages/teleterm/src/ui/ModalsHost/modals/ReAuthenticate/ReAuthenticate.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,14 +61,6 @@ export const ReAuthenticate: FC<{
const logger = useLogger('ReAuthenticate');
const { promptMfaRequest: req } = props;

// TODO(ravicious): At the moment it doesn't seem like it's possible for both Webauthn and TOTP to
// be available at the same time (see lib/client/mfa.PromptConfig/GetRunOptions). Whenever both
// Webauthn and TOTP are supported, Webauthn is preferred. Well, unless AllowStdinHijack is
// specified, but lib/teleterm doesn't do this and AllowStdinHijack has a scary comment next to it
// telling you not to use it.
//
// Alas, the data structure certainly allows for this so the modal was designed with supporting
// such scenario in mind.
const availableMfaTypes: MfaType[] = [];
// Add Webauthn first to prioritize it if both Webauthn and TOTP are available.
if (req.webauthn) {
Expand Down
Loading