self-hosted/teleport
self-hosted/teleport
1
0
Fork 0
mirror of https://github.com/gravitational/teleport synced 2024-10-21 17:53:28 +00:00
Code Issues Packages Projects Releases Wiki Activity

Make relogin attempts use the strongest auth method (#11781)

Browse source 
Fixes a potential stdin hijacking bug by making relogin attempts default to a
single MFA method (the strongest available).

The problematic scenario is as follows:

1. User has both OTP and security keys registered
2. "Relogin" is triggered via a tsh command (say,
   `tsh logout; tsh ssh --proxy=example.com llama@myserver`)
3. User is prompted to pick either OTP or security key ("Tap any security key or
   enter a code from a OTP device")
4. An stdin read is fired in the background to read the OTP code (via
   prompt.Stdin)
5. User picks the security method, thus the stdin read is "abandoned"

In most cases this is fine, as the program ends right after. The issue is when a
relogin is triggered by a long living tsh invocation (again, `tsh ssh ...`): in
this case the stdin hijack causes input to be swallowed.

Forcing a single MFA option avoids the potential stdin hijack, fixing the
problem for all relogin invocations. `tsh login` behavior remains the same.

Note that we have to default to cluster's most secure method _without_ checking
the user devices. The user is not logged in yet, thus the backend cannot reveal
any information about that user.

Fixes #11709.

* Add UseStrongestAuth flag to PromptMFAChallenge
* Add TeleportClient.UseStrongestAuth and set it true for relogin
* Proper testing
* Address review comments
This commit is contained in:
Alan Parra 2022-04-08 17:54:45 -03:00 committed by GitHub
parent 960d1b4202
commit 9abdb2a118
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
7 changed files with 91 additions and 23 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

39
lib/client/api.go
View file

@ -373,6 +373,12 @@ type Config struct {
// Affects the TeleportClient.Login and, indirectly, RetryWithRelogin
// functions.
Passwordless bool
// 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
}
// CachePolicy defines cache policy for local clients
@ -541,6 +547,8 @@ 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 {
@ -557,10 +565,21 @@ 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) {
return trace.Wrap(err, "refusing to connect to untrusted proxy %v without --insecure flag\n", tc.Config.SSHProxyAddr)
return trace.Wrap(err, "refusing to connect to untrusted proxy %v without --insecure flag\n", tc.SSHProxyAddr)
}
return trace.Wrap(err)
}
@ -1330,7 +1349,7 @@ func (tc *TeleportClient) IssueUserCertsWithMFA(ctx context.Context, params Reis
key, err := proxyClient.IssueUserCertsWithMFA(ctx, params,
func(ctx context.Context, proxyAddr string, c *proto.MFAAuthenticateChallenge) (*proto.MFAAuthenticateResponse, error) {
return PromptMFAChallenge(ctx, proxyAddr, c, "", false)
return PromptMFAChallenge(ctx, c, proxyAddr, nil /* opts */)
})
return key, err
@ -2507,6 +2526,11 @@ 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.
//
// If tc.Passwordless is set, then the passwordless authentication flow is used.
//
// The returned Key should typically be passed to ActivateKey in order to
@ -2698,7 +2722,7 @@ func (tc *TeleportClient) directLogin(ctx context.Context, secondFactorType cons
return nil, trace.Wrap(err)
}
// only ask for a second factor if it's enabled
// Only ask for a second factor if it's enabled.
var otpToken string
if secondFactorType == constants.SecondFactorOTP {
otpToken, err = tc.AskOTP()
@ -2707,7 +2731,7 @@ func (tc *TeleportClient) directLogin(ctx context.Context, secondFactorType cons
}
}
// ask the CA (via proxy) to sign our public key:
// Ask the CA (via proxy) to sign our public key:
response, err := SSHAgentLogin(ctx, SSHLoginDirect{
SSHLogin: SSHLogin{
ProxyAddr: tc.WebProxyAddr,
@ -2719,7 +2743,7 @@ func (tc *TeleportClient) directLogin(ctx context.Context, secondFactorType cons
RouteToCluster: tc.SiteName,
KubernetesCluster: tc.KubernetesCluster,
},
User: tc.Config.Username,
User: tc.Username,
Password: password,
OTPToken: otpToken,
})
@ -2745,8 +2769,9 @@ func (tc *TeleportClient) mfaLocalLogin(ctx context.Context, pub []byte) (*auth.
RouteToCluster: tc.SiteName,
KubernetesCluster: tc.KubernetesCluster,
},
User: tc.Config.Username,
Password: password,
User: tc.Username,
Password: password,
UseStrongestAuth: tc.UseStrongestAuth,
})
return response, trace.Wrap(err)

22
lib/client/api_login_test.go
View file

@ -138,11 +138,11 @@ func TestTeleportClient_Login_local(t *testing.T) {
ctx := context.Background()
tests := []struct {
name string
secondFactor constants.SecondFactorType
inputReader *prompt.FakeReader
solveWebauthn func(ctx context.Context, origin string, assertion *wanlib.CredentialAssertion, prompt wancli.LoginPrompt) (*proto.MFAAuthenticateResponse, error)
pwdless bool
name string
secondFactor constants.SecondFactorType
inputReader *prompt.FakeReader
solveWebauthn func(ctx context.Context, origin string, assertion *wanlib.CredentialAssertion, prompt wancli.LoginPrompt) (*proto.MFAAuthenticateResponse, error)
pwdless, useStrongestAuth bool
}{
{
name: "OTP device login",
@ -156,6 +156,17 @@ func TestTeleportClient_Login_local(t *testing.T) {
inputReader: prompt.NewFakeReader().AddString(password).AddReply(waitForCancelFn),
solveWebauthn: solveWebauthn,
},
{
name: "Webauthn and UseStrongestAuth",
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,
@ -195,6 +206,7 @@ func TestTeleportClient_Login_local(t *testing.T) {
tc, err := client.NewClient(cfg)
require.NoError(t, err)
tc.Passwordless = test.pwdless
tc.UseStrongestAuth = test.useStrongestAuth
clock.Advance(30 * time.Second)
_, err = tc.Login(ctx)

2
lib/client/client.go
View file

@ -1639,7 +1639,7 @@ func (proxy *ProxyClient) sessionSSHCertificate(ctx context.Context, nodeAddr No
RouteToCluster: nodeAddr.Cluster,
},
func(ctx context.Context, proxyAddr string, c *proto.MFAAuthenticateChallenge) (*proto.MFAAuthenticateResponse, error) {
return PromptMFAChallenge(ctx, proxyAddr, c, "", false)
return PromptMFAChallenge(ctx, c, proxyAddr, nil /* opts */)
},
)
if err != nil {

32
lib/client/mfa.go
View file

@ -48,21 +48,36 @@ func (p *mfaPrompt) PromptPIN() (string, error) {
return p.LoginPrompt.PromptPIN()
}
// PromptMFAChallengeOpts groups optional settings for PromptMFAChallenge.
type PromptMFAChallengeOpts struct {
// PromptDevicePrefix is an optional prefix printed before "security key" or
// "device". It is used to emphasize between different kinds of devices, like
// registered vs new.
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
}
// PromptMFAChallenge prompts the user to complete MFA authentication
// challenges.
// If promptDevicePrefix is set, it will be printed in prompts before "security
// key" or "device". This is used to emphasize between different kinds of
// devices, like registered vs new.
// 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.
func PromptMFAChallenge(
ctx context.Context,
proxyAddr string, c *proto.MFAAuthenticateChallenge, promptDevicePrefix string, quiet bool) (*proto.MFAAuthenticateResponse, error) {
// 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 {
return &proto.MFAAuthenticateResponse{}, nil
}
if opts == nil {
opts = &PromptMFAChallengeOpts{}
}
promptDevicePrefix := opts.PromptDevicePrefix
quiet := opts.Quiet
hasTOTP := c.TOTP != nil
hasWebauthn := c.WebauthnChallenge != nil
@ -76,6 +91,11 @@ func PromptMFAChallenge(
hasWebauthn = false
}
// Prompt only for the strongest auth method available?
if opts.UseStrongestAuth && hasWebauthn {
hasTOTP = false
}
var numGoroutines int
if hasTOTP && hasWebauthn {
numGoroutines = 2

4
lib/client/presence.go
View file

@ -86,7 +86,9 @@ func solveMFA(ctx context.Context, term io.Writer, tc *TeleportClient, challenge
// We don't support TOTP for live presence.
challenge.TOTP = nil
response, err := PromptMFAChallenge(ctx, tc.Config.WebProxyAddr, challenge, "", true)
response, err := PromptMFAChallenge(ctx, challenge, tc.Config.WebProxyAddr, &PromptMFAChallengeOpts{
Quiet: true,
})
if err != nil {
fmt.Fprintf(term, "\r\nTeleport > Failed to confirm presence: %v\r\n", err)
return nil, trace.Wrap(err)

9
lib/client/weblogin.go
View file

@ -214,6 +214,11 @@ type SSHLoginMFA struct {
User string
// User 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
}
// initClient creates a new client to the HTTPS web proxy.
@ -388,7 +393,9 @@ func SSHAgentMFALogin(ctx context.Context, login SSHLoginMFA) (*auth.SSHLoginRes
challengePB.WebauthnChallenge = wanlib.CredentialAssertionToProto(challenge.WebauthnChallenge)
}
respPB, err := PromptMFAChallenge(ctx, login.ProxyAddr, challengePB, "", false)
respPB, err := PromptMFAChallenge(ctx, challengePB, login.ProxyAddr, &PromptMFAChallengeOpts{
UseStrongestAuth: login.UseStrongestAuth,
})
if err != nil {
return nil, trace.Wrap(err)
}

6
tool/tsh/mfa.go
View file

@ -312,7 +312,9 @@ func (c *mfaAddCommand) addDeviceRPC(
if authChallenge == nil {
return trace.BadParameter("server bug: server sent %T when client expected AddMFADeviceResponse_ExistingMFAChallenge", resp.Response)
}
authResp, err := client.PromptMFAChallenge(ctx, tc.Config.WebProxyAddr, authChallenge, "*registered* ", false)
authResp, err := client.PromptMFAChallenge(ctx, authChallenge, tc.Config.WebProxyAddr, &client.PromptMFAChallengeOpts{
PromptDevicePrefix: "*registered*",
})
if err != nil {
return trace.Wrap(err)
}
@ -516,7 +518,7 @@ func (c *mfaRemoveCommand) run(cf *CLIConf) error {
if authChallenge == nil {
return trace.BadParameter("server bug: server sent %T when client expected DeleteMFADeviceResponse_MFAChallenge", resp.Response)
}
authResp, err := client.PromptMFAChallenge(cf.Context, tc.Config.WebProxyAddr, authChallenge, "", false)
authResp, err := client.PromptMFAChallenge(cf.Context, authChallenge, tc.Config.WebProxyAddr, nil /* opts */)
if err != nil {
return trace.Wrap(err)
}