Skip to content

Commit

Permalink
[v9] Expand --mfa-mode and disable stdin hijack by default (#13134) (#…
Browse files Browse the repository at this point in the history 
…13212)

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.

* Expand --mfa-mode and disable stdin hijack by default (#13134)
* Use TeleportClient.PromptMFAChallenge where applicable
* Adapt/backport tests and fix prefer OTP
* Appease linter
  • Loading branch information
@codingllama
codingllama authored Jun 8, 2022
1 parent 75e8247 commit abd8c62
Show file tree
Hide file tree
Showing 6 changed files with 155 additions and 79 deletions.
40 changes: 14 additions & 26 deletions lib/client/api.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -321,6 +321,11 @@ type Config struct {
// AuthConnector is the name of the authentication connector to use.
AuthConnector string

// 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 @@ -371,11 +376,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 @@ -682,8 +687,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 @@ -700,17 +703,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 @@ -1584,8 +1576,8 @@ func (tc *TeleportClient) IssueUserCertsWithMFA(ctx context.Context, params Reis
defer proxyClient.Close()

key, err := proxyClient.IssueUserCertsWithMFA(ctx, params,
func(ctx context.Context, proxyAddr string, c *proto.MFAAuthenticateChallenge) (*proto.MFAAuthenticateResponse, error) {
return PromptMFAChallenge(ctx, c, proxyAddr, nil /* opts */)
func(ctx context.Context, _ string, c *proto.MFAAuthenticateChallenge) (*proto.MFAAuthenticateResponse, error) {
return tc.PromptMFAChallenge(ctx, c, nil /* optsOverride */)
})

return key, err
Expand Down Expand Up @@ -2762,11 +2754,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 @@ -3323,7 +3310,8 @@ func (tc *TeleportClient) mfaLocalLogin(ctx context.Context, pub []byte) (*auth.
},
User: tc.Username,
Password: password,
UseStrongestAuth: tc.UseStrongestAuth,
AllowStdinHijack: tc.AllowStdinHijack,
PreferOTP: tc.PreferOTP,
})

return response, trace.Wrap(err)
Expand Down
48 changes: 32 additions & 16 deletions lib/client/api_login_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -159,46 +159,61 @@ func TestTeleportClient_Login_localMFALogin(t *testing.T) {
solveOTP func(context.Context) (string, error)
solveU2F func(ctx context.Context, facet string, challenges ...u2flib.AuthenticateChallenge) (*u2flib.AuthenticateChallengeResponse, error)
solveWebauthn func(ctx context.Context, origin string, assertion *wanlib.CredentialAssertion) (*proto.MFAAuthenticateResponse, error)
useStrongestAuth bool
allowStdinHijack bool
preferOTP bool
}{
{
name: "OK OTP device login",
name: "OK OTP device login with hijack",
secondFactor: constants.SecondFactorOptional,
solveOTP: solveOTP,
solveU2F: func(context.Context, string, ...u2flib.AuthenticateChallenge) (*u2flib.AuthenticateChallengeResponse, error) {
panic("unused")
},
solveWebauthn: promptWebauthnNoop,
solveWebauthn: promptWebauthnNoop,
allowStdinHijack: true,
},
{
name: "OK Webauthn device login",
name: "OK Webauthn device login with hijack",
secondFactor: constants.SecondFactorOptional,
solveOTP: promptOTPNoop,
solveU2F: func(context.Context, string, ...u2flib.AuthenticateChallenge) (*u2flib.AuthenticateChallengeResponse, error) {
panic("unused")
},
solveWebauthn: solveWebauthn,
solveWebauthn: solveWebauthn,
allowStdinHijack: true,
},
{
name: "Webauthn and UseStrongestAuth",
secondFactor: constants.SecondFactorOptional,
solveOTP: func(ctx context.Context) (string, error) {
name: "OK U2F device login with hijack",
secondFactor: constants.SecondFactorU2F,
solveOTP: promptOTPNoop,
solveU2F: solveU2F,
solveWebauthn: func(context.Context, string, *wanlib.CredentialAssertion) (*proto.MFAAuthenticateResponse, error) {
panic("unused")
},
allowStdinHijack: true,
},
{
name: "OTP preferred",
secondFactor: constants.SecondFactorOptional,
solveOTP: solveOTP,
solveU2F: func(context.Context, string, ...u2flib.AuthenticateChallenge) (*u2flib.AuthenticateChallengeResponse, error) {
panic("unused")
},
solveWebauthn: solveWebauthn,
useStrongestAuth: true,
solveWebauthn: func(ctx context.Context, origin string, assertion *wanlib.CredentialAssertion) (*proto.MFAAuthenticateResponse, error) {
panic("unused")
},
preferOTP: true,
},
{
name: "OK U2F device login",
secondFactor: constants.SecondFactorU2F,
solveOTP: promptOTPNoop,
solveU2F: solveU2F,
solveWebauthn: func(context.Context, string, *wanlib.CredentialAssertion) (*proto.MFAAuthenticateResponse, error) {
name: "Webauthn device login",
secondFactor: constants.SecondFactorOptional,
solveOTP: func(ctx context.Context) (string, error) {
panic("unused")
},
solveU2F: func(context.Context, string, ...u2flib.AuthenticateChallenge) (*u2flib.AuthenticateChallengeResponse, error) {
panic("unused")
},
solveWebauthn: solveWebauthn,
},
}
for _, test := range tests {
Expand All @@ -222,7 +237,8 @@ func TestTeleportClient_Login_localMFALogin(t *testing.T) {

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

clock.Advance(30 * time.Second)
_, err = tc.Login(ctx)
Expand Down
4 changes: 2 additions & 2 deletions lib/client/client.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1637,8 +1637,8 @@ func (proxy *ProxyClient) sessionSSHCertificate(ctx context.Context, nodeAddr No
NodeName: nodeName(nodeAddr.Addr),
RouteToCluster: nodeAddr.Cluster,
},
func(ctx context.Context, proxyAddr string, c *proto.MFAAuthenticateChallenge) (*proto.MFAAuthenticateResponse, error) {
return PromptMFAChallenge(ctx, c, proxyAddr, nil /* opts */)
func(ctx context.Context, _ string, c *proto.MFAAuthenticateChallenge) (*proto.MFAAuthenticateResponse, error) {
return proxy.teleportClient.PromptMFAChallenge(ctx, c, nil /* optsOverride */)
},
)
if err != nil {
Expand Down
80 changes: 52 additions & 28 deletions lib/client/mfa.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -49,19 +49,37 @@ 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
// PreferOTP favors OTP challenges, if applicable.
// Takes precedence over AuthenticatorAttachment settings.
PreferOTP bool
}

// PromptMFAChallenge prompts the user to complete MFA authentication
// challenges.
// See client.PromptMFAChallenge.
func (tc *TeleportClient) PromptMFAChallenge(
ctx context.Context, c *proto.MFAAuthenticateChallenge, optsOverride *PromptMFAChallengeOpts) (*proto.MFAAuthenticateResponse, error) {
opts := &PromptMFAChallengeOpts{
AllowStdinHijack: tc.AllowStdinHijack,
PreferOTP: tc.PreferOTP,
}
if optsOverride != nil {
opts.PromptDevicePrefix = optsOverride.PromptDevicePrefix
opts.Quiet = optsOverride.Quiet
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 && len(c.U2F) == 0 && c.WebauthnChallenge == nil {
Expand All @@ -87,8 +105,12 @@ func PromptMFAChallenge(ctx context.Context, c *proto.MFAAuthenticateChallenge,
hasNonTOTP = false
}

// Prompt only for the strongest auth method available?
if opts.UseStrongestAuth && hasNonTOTP {
// Tweak enabled/disabled methods according to opts.
switch {
case hasTOTP && opts.PreferOTP:
hasNonTOTP = false
case hasNonTOTP && !opts.AllowStdinHijack:
// Use strongest auth if hijack is not allowed.
hasTOTP = false
}

Expand Down Expand Up @@ -137,23 +159,25 @@ func PromptMFAChallenge(ctx context.Context, c *proto.MFAAuthenticateChallenge,
}

// Fire Webauthn or U2F goroutine.
origin := proxyAddr
if !strings.HasPrefix(origin, "https://") {
origin = "https://" + origin
}
switch {
case c.WebauthnChallenge != nil:
go func() {
log.Debugf("WebAuthn: prompting U2F devices with origin %q", origin)
resp, err := promptWebauthn(ctx, origin, wanlib.CredentialAssertionFromProto(c.WebauthnChallenge))
respC <- response{kind: "WEBAUTHN", resp: resp, err: err}
}()
case len(c.U2F) > 0:
go func() {
log.Debugf("prompting U2F devices with facet %q", origin)
resp, err := promptU2FChallenges(ctx, proxyAddr, c.U2F)
respC <- response{kind: "U2F", resp: resp, err: err}
}()
if hasNonTOTP {
origin := proxyAddr
if !strings.HasPrefix(origin, "https://") {
origin = "https://" + origin
}
switch {
case c.WebauthnChallenge != nil:
go func() {
log.Debugf("WebAuthn: prompting U2F devices with origin %q", origin)
resp, err := promptWebauthn(ctx, origin, wanlib.CredentialAssertionFromProto(c.WebauthnChallenge))
respC <- response{kind: "WEBAUTHN", resp: resp, err: err}
}()
case len(c.U2F) > 0:
go func() {
log.Debugf("prompting U2F devices with facet %q", origin)
resp, err := promptU2FChallenges(ctx, proxyAddr, c.U2F)
respC <- response{kind: "U2F", resp: resp, err: err}
}()
}
}

for i := 0; i < numGoroutines; i++ {
Expand Down
16 changes: 9 additions & 7 deletions lib/client/weblogin.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -215,13 +215,14 @@ 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

// AllowStdinHijack allows stdin hijack during MFA prompts.
// Do not set this options unless you deeply understand what you are doing.
AllowStdinHijack bool
// 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 @@ -416,7 +417,8 @@ func SSHAgentMFALogin(ctx context.Context, login SSHLoginMFA) (*auth.SSHLoginRes
}

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

0 comments on commit abd8c62

Please # to comment.