Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 1 addition & 1 deletion lib/client/mfa/prompt.go
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is outside of scope of this PR, but while testing I've noticed that neither tsh nor Connect react to hardware key insertion. Connect even says "Insert and press your key". But if there was no key at the time when the prompt was shown, inserting a key is going to have no effect. I'll create an issue for this on Monday.

Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@ type MFAGoroutineResponse struct {
// HandleMFAPromptGoroutines spawns MFA prompt goroutines and returns the first successful response,
// terminating error, or an aggregated error if they all fail.
func HandleMFAPromptGoroutines(ctx context.Context, startGoroutines func(context.Context, *sync.WaitGroup, chan<- MFAGoroutineResponse)) (*proto.MFAAuthenticateResponse, error) {
respC := make(chan MFAGoroutineResponse, 2)
respC := make(chan MFAGoroutineResponse, 3)
var wg sync.WaitGroup

ctx, cancel := context.WithCancel(ctx)
Expand Down
45 changes: 39 additions & 6 deletions lib/client/sso/ceremony.go
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,9 @@ type MFACeremony struct {
HandleRedirect func(ctx context.Context, redirectURL string) error
GetCallbackMFAToken func(ctx context.Context) (string, error)
GetCallbackWebauthn func(ctx context.Context) (*wantypes.CredentialAssertionResponse, error)
// GetCallbackResponse returns the response from the server without assuming
// if it is SSO or Browser MFA, so it can be inspected and actioned.
GetCallbackResponse func(ctx context.Context) (*authclient.CLILoginResponse, error)
}

// GetClientCallbackURL returns the client callback URL.
Expand All @@ -124,9 +127,38 @@ func (m *MFACeremony) GetProxyAddress() string {

// Run the SSO/Browser MFA ceremony.
func (m *MFACeremony) Run(ctx context.Context, chal *proto.MFAAuthenticateChallenge) (*proto.MFAAuthenticateResponse, error) {
// The proxy will only ever return one of SSO or Browser challenge. However,
// check for SSO challenge first as it takes priority over browser MFA.
switch {
// If both SSO and Browser MFA challenges are set then Connect has initiated
// this MFA ceremony. In which case, don't print the redirect URL and listen
// for either response to be returned.
case chal.SSOChallenge != nil && chal.BrowserMFAChallenge != nil:
loginResp, err := m.GetCallbackResponse(ctx)
if err != nil {
return nil, trace.Wrap(err)
}

switch {
case loginResp.MFAToken != "":
return &proto.MFAAuthenticateResponse{
Response: &proto.MFAAuthenticateResponse_SSO{
SSO: &proto.SSOResponse{
RequestId: chal.SSOChallenge.RequestId,
Token: loginResp.MFAToken,
},
},
}, nil
case loginResp.BrowserMFAWebauthnResponse != nil:
return &proto.MFAAuthenticateResponse{
Response: &proto.MFAAuthenticateResponse_Browser{
Browser: &proto.BrowserMFAResponse{
RequestId: chal.BrowserMFAChallenge.RequestId,
WebauthnResponse: wantypes.CredentialAssertionResponseToProto(loginResp.BrowserMFAWebauthnResponse),
},
},
}, nil
default:
return nil, trace.BadParameter("login response missing both SSO MFA token and Browser WebAuthn response")
}
case chal.SSOChallenge != nil:
if err := m.HandleRedirect(ctx, chal.SSOChallenge.RedirectUrl); err != nil {
return nil, trace.Wrap(err)
Expand Down Expand Up @@ -220,12 +252,13 @@ func NewCLIMFACeremony(rd *Redirector) *MFACeremony {
}
}

// NewConnectMFACeremony creates a new Teleport Connect SSO ceremony from the given redirector.
// NewConnectMFACeremony creates a new Teleport Connect SSO/Browser ceremony from the given redirector.
func NewConnectMFACeremony(rd *Redirector) mfa.CallbackCeremony {
return &MFACeremony{
close: rd.Close,
ClientCallbackURL: rd.ClientCallbackURL,
ProxyAddress: rd.ProxyAddr,
close: rd.Close,
ClientCallbackURL: rd.ClientCallbackURL,
ProxyAddress: rd.ProxyAddr,
GetCallbackResponse: rd.WaitForResponse,
HandleRedirect: func(ctx context.Context, redirectURL string) error {
// Connect handles redirect on the Electron side.
return nil
Expand Down
8 changes: 6 additions & 2 deletions lib/teleterm/clusters/cluster.go
Original file line number Diff line number Diff line change
Expand Up @@ -328,9 +328,13 @@ func (c *Cluster) GetProfileStatusError() error {
}

// GetProxyHost returns proxy address (hostname:port) of the root cluster, even when called on a
// Cluster that represents a leaf cluster.
// Cluster that represents a leaf cluster. Falls back to WebProxyAddr when the profile status has
// not been loaded yet (e.g., before the first login).
func (c *Cluster) GetProxyHost() string {
return c.status.ProxyURL.Host
if c.status.ProxyURL.Host != "" {
return c.status.ProxyURL.Host
}
return c.WebProxyAddr
}

// HasDeviceTrustExtensions indicates if the cert contains all required
Expand Down
9 changes: 5 additions & 4 deletions lib/teleterm/clusters/cluster_auth.go
Original file line number Diff line number Diff line change
Expand Up @@ -194,10 +194,11 @@ func (c *Cluster) localMFALogin(user, password string) client.SSHLoginFunc {
}

response, err := client.SSHAgentMFALogin(ctx, client.SSHLoginMFA{
SSHLogin: sshLogin,
User: user,
Password: password,
MFAPromptConstructor: c.clusterClient.NewMFAPrompt,
SSHLogin: sshLogin,
User: user,
Password: password,
MFAPromptConstructor: c.clusterClient.NewMFAPrompt,
MFACeremonyConstructor: c.clusterClient.NewRedirectorMFACeremony,
})
if err != nil {
return nil, trace.Wrap(err)
Expand Down
30 changes: 15 additions & 15 deletions lib/teleterm/daemon/mfaprompt.go
Comment thread
ravicious marked this conversation as resolved.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is there any way we can test it? Grzegorz added e2e tests for headless auth, perhaps something similar could be utilized here? Seeing how browser MFA utilizes both Connect and the Web UI too.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

An e2e test could be a follow-up to this PR (so that it's not blocked on this), but it'd definitely be great to have it.

Original file line number Diff line number Diff line change
Expand Up @@ -75,18 +75,10 @@ func (p *mfaPrompt) Run(ctx context.Context, chal *proto.MFAAuthenticateChalleng
promptOTP := chal.TOTP != nil
promptWebauthn := chal.WebauthnChallenge != nil && p.cfg.WebauthnSupported
promptSSO := chal.SSOChallenge != nil && p.cfg.MFACeremony != nil
promptBrowser := chal.BrowserMFAChallenge != nil

// TODO(danielashare): Implement Browser MFA for connect
if promptBrowser && !promptOTP && !promptWebauthn && !promptSSO {
return nil, trace.AccessDenied(
"Browser MFA was the only challenge returned and is not supported in Connect yet",
)
}

promptBrowserMfa := chal.BrowserMFAChallenge != nil && p.cfg.MFACeremony != nil
scope := p.cfg.Extensions.GetScope()
// No prompt to run, no-op.
if !promptOTP && !promptWebauthn && !promptSSO {
if !promptOTP && !promptWebauthn && !promptSSO && !promptBrowserMfa {
return &proto.MFAAuthenticateResponse{}, nil
}

Expand All @@ -100,6 +92,13 @@ func (p *mfaPrompt) Run(ctx context.Context, chal *proto.MFAAuthenticateChalleng
}
}

var browserMfaChallenge *mfav1.BrowserMFAChallenge
if promptBrowserMfa {
browserMfaChallenge = &mfav1.BrowserMFAChallenge{
RequestId: chal.BrowserMFAChallenge.RequestId,
}
}

spawnGoroutines := func(ctx context.Context, wg *sync.WaitGroup, respC chan<- libmfa.MFAGoroutineResponse) {
ctx, cancel := context.WithCancelCause(ctx)

Expand All @@ -114,6 +113,7 @@ func (p *mfaPrompt) Run(ctx context.Context, chal *proto.MFAAuthenticateChalleng
Totp: promptOTP,
Webauthn: promptWebauthn,
Sso: ssoChallenge,
Browser: browserMfaChallenge,
PerSessionMfa: scope == mfav1.ChallengeScope_CHALLENGE_SCOPE_USER_SESSION,
})
respC <- libmfa.MFAGoroutineResponse{Resp: resp, Err: err}
Expand All @@ -136,14 +136,14 @@ func (p *mfaPrompt) Run(ctx context.Context, chal *proto.MFAAuthenticateChalleng
}()
}

// Fire SSO goroutine.
if promptSSO {
// Fire SSO/Browser MFA goroutine. They both share the same callback handler.
if promptSSO || promptBrowserMfa {
wg.Add(1)
go func() {
defer wg.Done()

resp, err := p.promptSSO(ctx, chal)
respC <- libmfa.MFAGoroutineResponse{Resp: resp, Err: trace.Wrap(err, "SSO authentication failed")}
resp, err := p.promptBrowserOrSSO(ctx, chal)
respC <- libmfa.MFAGoroutineResponse{Resp: resp, Err: trace.Wrap(err, "SSO/Browser MFA authentication failed")}
}()
}
}
Expand Down Expand Up @@ -174,7 +174,7 @@ func (p *mfaPrompt) promptWebauthn(ctx context.Context, chal *proto.MFAAuthentic
return resp, nil
}

func (c *mfaPrompt) promptSSO(ctx context.Context, chal *proto.MFAAuthenticateChallenge) (*proto.MFAAuthenticateResponse, error) {
func (c *mfaPrompt) promptBrowserOrSSO(ctx context.Context, chal *proto.MFAAuthenticateChallenge) (*proto.MFAAuthenticateResponse, error) {
resp, err := c.cfg.MFACeremony.Run(ctx, chal)
return resp, trace.Wrap(err)
}
1 change: 1 addition & 0 deletions web/packages/teleterm/src/ui/ModalsHost/ModalsHost.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -206,6 +206,7 @@ function renderDialog({
}}
// This function needs to be stable between renders.
onSsoContinue={dialog.onSsoContinue}
onBrowserMfaContinue={dialog.onBrowserMfaContinue}
onCancel={() => {
handleClose();
dialog.onCancel();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@ export const WithWebauthn = () => (
'You somehow submitted a form while only Webauthn was available.'
);
}}
onBrowserMfaContinue={() => {}}
/>
</MockAppContextProvider>
);
Expand All @@ -69,6 +70,7 @@ export const WithTotp = () => (
onSsoContinue={() => {}}
onCancel={() => {}}
onOtpSubmit={showToken}
onBrowserMfaContinue={() => {}}
/>
</MockAppContextProvider>
);
Expand All @@ -83,6 +85,7 @@ export const WithTotpPerSessionMfa = () => (
onSsoContinue={() => {}}
onCancel={() => {}}
onOtpSubmit={showToken}
onBrowserMfaContinue={() => {}}
/>
</MockAppContextProvider>
);
Expand All @@ -106,6 +109,28 @@ export const WithSso = () => (
'You somehow submitted a form while only SSO was available.'
);
}}
onBrowserMfaContinue={() => {}}
/>
</MockAppContextProvider>
);

export const WithBrowserMfa = () => (
<MockAppContextProvider>
<ReAuthenticate
promptMfaRequest={{
...loginMfaRequest,
browser: {
requestId: 'request-id',
},
}}
onSsoContinue={() => {}}
onCancel={() => {}}
onOtpSubmit={() => {
window.alert(
'You somehow submitted a form while only Browser MFA was available.'
);
}}
onBrowserMfaContinue={() => {}}
/>
</MockAppContextProvider>
);
Expand All @@ -127,6 +152,7 @@ export const WithWebauthnAndTotpAndSSO = () => (
onSsoContinue={() => {}}
onCancel={() => {}}
onOtpSubmit={showToken}
onBrowserMfaContinue={() => {}}
/>
</MockAppContextProvider>
);
Expand All @@ -148,6 +174,26 @@ export const WithWebauthnAndTotpAndSSOPerSessionMfa = () => (
onSsoContinue={() => {}}
onCancel={() => {}}
onOtpSubmit={showToken}
onBrowserMfaContinue={() => {}}
/>
</MockAppContextProvider>
);

export const WithWebauthnAndTotpAndBrowserMfa = () => (
<MockAppContextProvider>
<ReAuthenticate
promptMfaRequest={{
...loginMfaRequest,
webauthn: true,
totp: true,
browser: {
requestId: 'request-id',
},
}}
onSsoContinue={() => {}}
onCancel={() => {}}
onOtpSubmit={showToken}
onBrowserMfaContinue={() => {}}
/>
</MockAppContextProvider>
);
Expand All @@ -165,6 +211,7 @@ export const MultilineTitle = () => (
onSsoContinue={() => {}}
onCancel={() => {}}
onOtpSubmit={showToken}
onBrowserMfaContinue={() => {}}
/>
</MockAppContextProvider>
);
Expand All @@ -181,6 +228,7 @@ export const ForLeafCluster = () => (
onSsoContinue={() => {}}
onCancel={() => {}}
onOtpSubmit={showToken}
onBrowserMfaContinue={() => {}}
/>
</MockAppContextProvider>
);
Loading
Loading