Skip to content

Commit

Permalink
Expand --mfa-mode and disable stdin hijack by default (#13134)
Browse files Browse the repository at this point in the history 
Avoid "input swallowing" bugs by disabling stdin hijacking by default.

Only `tsh login` is allowed to hijack stdin, as it is expected to exit right
after authentication. Any MFA authentication attempts resulting from
non-`tsh login` invocations default to the user's strongest auth method.

Defaulting to the strongest auth method can cause problems in constrained
environments for users that have both Webauthn and OTP registered. For example,
someone using `tsh` under WSL (Windows Subsystem for Linux) or a remote machine
could be locked into Webauthn MFA, which they can't use because their
environment lacks USB access or they don't have physical access to it. In order
to solve this problem I've slightly modified the meaning of the `--mfa-mode`
flag so `otp` can be specified.

The `TELEPORT_MFA_MODE` environment variable may be set to avoid constant flag
passing.

Fixes #12675 and #13021.

* Expand --mfa-mode and disable stdin hijack by default
* Use TELEPORT_ instead of TSH_ for FIDO2 env var
* Use t.Setenv in tests
  • Loading branch information
@codingllama
codingllama authored Jun 3, 2022
1 parent c865e7e commit 9ca045b
Show file tree
Hide file tree
Showing 7 changed files with 121 additions and 85 deletions.
2 changes: 1 addition & 1 deletion lib/auth/webauthncli/fido2.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ var fidoNewDevice = func(path string) (FIDODevice, error) {

// IsFIDO2Available returns true if libfido2 is available in the current build.
func IsFIDO2Available() bool {
val, ok := os.LookupEnv("TSH_FIDO2")
val, ok := os.LookupEnv("TELEPORT_FIDO2")
// Default to enabled, otherwise obey the env variable.
return !ok || val == "1"
}
Expand Down
16 changes: 6 additions & 10 deletions lib/auth/webauthncli/fido2_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -130,34 +130,30 @@ func (p pinCancelPrompt) PromptTouch() {
}

func TestIsFIDO2Available(t *testing.T) {
const fido2Key = "TSH_FIDO2"
defer func() {
os.Unsetenv(fido2Key)
}()

const fido2Key = "TELEPORT_FIDO2"
tests := []struct {
name string
setenv func()
want bool
}{
{
name: "TSH_FIDO2 unset",
name: "env var unset",
setenv: func() {
os.Unsetenv(fido2Key)
},
want: true,
},
{
name: "TSH_FIDO2=1",
name: "env var set to 1",
setenv: func() {
os.Setenv(fido2Key, "1")
t.Setenv(fido2Key, "1")
},
want: true,
},
{
name: "TSH_FIDO2=0",
name: "env var set to 0",
setenv: func() {
os.Setenv(fido2Key, "0")
t.Setenv(fido2Key, "0")
},
want: false,
},
Expand Down
36 changes: 12 additions & 24 deletions lib/client/api.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -326,6 +326,11 @@ type Config struct {
// AuthenticatorAttachment is the desired authenticator attachment.
AuthenticatorAttachment wancli.AuthenticatorAttachment

// PreferOTP prefers OTP in favor of other MFA methods.
// Useful in constrained environments without access to USB or platform
// authenticators, such as remote hosts or virtual machines.
PreferOTP bool

// CheckVersions will check that client version is compatible
// with auth server version when connecting.
CheckVersions bool
Expand Down Expand Up @@ -376,11 +381,11 @@ 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
// 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.
AllowStdinHijack bool
}

// CachePolicy defines cache policy for local clients
Expand Down Expand Up @@ -699,8 +704,6 @@ 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 {
Expand All @@ -717,17 +720,6 @@ 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) {
Expand Down Expand Up @@ -2775,11 +2767,6 @@ 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.
//
// 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) {
Expand Down Expand Up @@ -3015,8 +3002,9 @@ func (tc *TeleportClient) mfaLocalLogin(ctx context.Context, pub []byte) (*auth.
},
User: tc.Username,
Password: password,
UseStrongestAuth: tc.UseStrongestAuth,
AuthenticatorAttachment: tc.AuthenticatorAttachment,
PreferOTP: tc.PreferOTP,
AllowStdinHijack: tc.AllowStdinHijack,
})

return response, trace.Wrap(err)
Expand Down
51 changes: 32 additions & 19 deletions lib/client/api_login_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -144,36 +144,48 @@ func TestTeleportClient_Login_local(t *testing.T) {
inputReader *prompt.FakeReader
solveWebauthn func(ctx context.Context, origin string, assertion *wanlib.CredentialAssertion, prompt wancli.LoginPrompt) (*proto.MFAAuthenticateResponse, error)
authConnector string
useStrongestAuth bool
allowStdinHijack bool
preferOTP bool
}{
{
name: "OTP device login",
secondFactor: constants.SecondFactorOptional,
inputReader: prompt.NewFakeReader().AddString(password).AddReply(solveOTP),
solveWebauthn: noopWebauthnFn,
name: "OTP device login with hijack",
secondFactor: constants.SecondFactorOptional,
inputReader: prompt.NewFakeReader().AddString(password).AddReply(solveOTP),
solveWebauthn: noopWebauthnFn,
allowStdinHijack: true,
},
{
name: "Webauthn device login",
secondFactor: constants.SecondFactorOptional,
inputReader: prompt.NewFakeReader().AddString(password).AddReply(waitForCancelFn),
solveWebauthn: solveWebauthn,
name: "Webauthn device login with hijack",
secondFactor: constants.SecondFactorOptional,
inputReader: prompt.NewFakeReader().AddString(password).AddReply(waitForCancelFn),
solveWebauthn: solveWebauthn,
allowStdinHijack: true,
},
{
name: "Webauthn and UseStrongestAuth",
name: "Webauthn device with PIN and hijack", // a bit hypothetical, but _could_ happen.
secondFactor: constants.SecondFactorOptional,
inputReader: prompt.NewFakeReader().AddString(password).AddReply(waitForCancelFn).AddReply(userPINFn),
solveWebauthn: solvePIN,
allowStdinHijack: true,
},
{
name: "OTP preferred",
secondFactor: constants.SecondFactorOptional,
inputReader: prompt.NewFakeReader().AddString(password).AddReply(solveOTP),
solveWebauthn: func(ctx context.Context, origin string, assertion *wanlib.CredentialAssertion, prompt wancli.LoginPrompt) (*proto.MFAAuthenticateResponse, error) {
panic("this should not be called")
},
preferOTP: true,
},
{
name: "Webauthn device login",
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,
inputReader: prompt.NewFakeReader().AddString(password).AddReply(waitForCancelFn).AddReply(userPINFn),
solveWebauthn: solvePIN,
solveWebauthn: solveWebauthn,
},
{
name: "passwordless login",
Expand Down Expand Up @@ -207,8 +219,9 @@ func TestTeleportClient_Login_local(t *testing.T) {

tc, err := client.NewClient(cfg)
require.NoError(t, err)
tc.AllowStdinHijack = test.allowStdinHijack
tc.AuthConnector = test.authConnector
tc.UseStrongestAuth = test.useStrongestAuth
tc.PreferOTP = test.preferOTP

clock.Advance(30 * time.Second)
_, err = tc.Login(ctx)
Expand Down
32 changes: 21 additions & 11 deletions lib/client/mfa.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -56,12 +56,17 @@ type PromptMFAChallengeOpts struct {
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
// 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
}

// PromptMFAChallenge prompts the user to complete MFA authentication
Expand All @@ -71,24 +76,22 @@ func (tc *TeleportClient) PromptMFAChallenge(
ctx context.Context, c *proto.MFAAuthenticateChallenge, optsOverride *PromptMFAChallengeOpts) (*proto.MFAAuthenticateResponse, error) {
opts := &PromptMFAChallengeOpts{
AuthenticatorAttachment: tc.AuthenticatorAttachment,
PreferOTP: tc.PreferOTP,
}
if optsOverride != nil {
opts.PromptDevicePrefix = optsOverride.PromptDevicePrefix
opts.Quiet = optsOverride.Quiet
opts.UseStrongestAuth = optsOverride.UseStrongestAuth
if optsOverride.AuthenticatorAttachment != wancli.AttachmentAuto {
opts.AuthenticatorAttachment = optsOverride.AuthenticatorAttachment
}
opts.PreferOTP = optsOverride.PreferOTP
opts.AllowStdinHijack = optsOverride.AllowStdinHijack
}
return PromptMFAChallenge(ctx, c, tc.WebProxyAddr, opts)
}

// PromptMFAChallenge prompts the user to complete MFA authentication
// challenges.
// 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 && c.WebauthnChallenge == nil {
Expand All @@ -112,8 +115,15 @@ func PromptMFAChallenge(ctx context.Context, c *proto.MFAAuthenticateChallenge,
hasWebauthn = false
}

// Prompt only for the strongest auth method available?
if opts.UseStrongestAuth && hasWebauthn {
// Tweak enabled/disabled methods according to opts.
switch {
case hasTOTP && opts.PreferOTP:
hasWebauthn = false
case hasWebauthn && opts.AuthenticatorAttachment != wancli.AttachmentAuto:
// Prefer Webauthn if an specific attachment was requested.
hasTOTP = false
case hasWebauthn && !opts.AllowStdinHijack:
// Use strongest auth if hijack is not allowed.
hasTOTP = false
}

Expand Down
18 changes: 10 additions & 8 deletions lib/client/weblogin.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -213,15 +213,16 @@ type SSHLoginMFA struct {
SSHLogin
// User is the login username.
User string
// User is the login password.
// Password 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
// AuthenticatorAttachment is the desired authenticator attachment.

// AllowStdinHijack allows stdin hijack during MFA prompts.
// Do not set this options unless you deeply understand what you are doing.
AllowStdinHijack bool
// AuthenticatorAttachment is the authenticator attachment for MFA prompts.
AuthenticatorAttachment wancli.AuthenticatorAttachment
// PreferOTP prefers OTP in favor of other MFA methods.
PreferOTP bool
}

// initClient creates a new client to the HTTPS web proxy.
Expand Down Expand Up @@ -397,8 +398,9 @@ func SSHAgentMFALogin(ctx context.Context, login SSHLoginMFA) (*auth.SSHLoginRes
}

respPB, err := PromptMFAChallenge(ctx, challengePB, login.ProxyAddr, &PromptMFAChallengeOpts{
UseStrongestAuth: login.UseStrongestAuth,
AllowStdinHijack: login.AllowStdinHijack,
AuthenticatorAttachment: login.AuthenticatorAttachment,
PreferOTP: login.PreferOTP,
})
if err != nil {
return nil, trace.Wrap(err)
Expand Down
Loading

0 comments on commit 9ca045b

Please sign in to comment.