From 3c214149341fcf5c7e7f794b1a5336ad74dcc9d1 Mon Sep 17 00:00:00 2001 From: joerger Date: Fri, 20 Sep 2024 12:22:16 -0700 Subject: [PATCH 01/12] Add SSOMFACeremony to MFA prompt; Add SSOMFACeremonyConstructor to MFA ceremony. --- api/mfa/ceremony.go | 20 ++++++++++++++++++++ api/mfa/prompt.go | 9 +++++++++ lib/client/api.go | 3 +++ lib/client/mfa.go | 1 + lib/client/mfa/cli.go | 29 +++++++++++++++++++++++++++++ lib/client/sso/ceremony.go | 14 ++++++++++++++ 6 files changed, 76 insertions(+) diff --git a/api/mfa/ceremony.go b/api/mfa/ceremony.go index c542ba36c42b8..7d527c4ca329f 100644 --- a/api/mfa/ceremony.go +++ b/api/mfa/ceremony.go @@ -32,6 +32,17 @@ type Ceremony struct { CreateAuthenticateChallenge CreateAuthenticateChallengeFunc // PromptConstructor creates a prompt to prompt the user to solve an authentication challenge. PromptConstructor PromptConstructor + // SSOMFACeremonyConstructor is an optional SSO MFA ceremony constructor. If provided, + // the MFA ceremony will also attempt to retrieve an SSO MFA challenge. The provided + // context will be closed once the ceremony is complete. + SSOMFACeremonyConstructor func(ctx context.Context) (SSOMFACeremony, error) +} + +// SSOMFACeremony is an SSO MFA ceremony. +type SSOMFACeremony interface { + GetClientCallbackURL() string + HandleRedirect(ctx context.Context, redirectURL string) error + GetCallbackMFAToken(ctx context.Context) (string, error) } // CreateAuthenticateChallengeFunc is a function that creates an authentication challenge. @@ -54,6 +65,15 @@ func (c *Ceremony) Run(ctx context.Context, req *proto.CreateAuthenticateChallen return nil, trace.BadParameter("mfa challenge scope must be specified") } + if c.SSOMFACeremonyConstructor != nil { + ssoMFACeremony, err := c.SSOMFACeremonyConstructor(ctx) + if err != nil { + return nil, trace.Wrap(err, "failed to handle SSO MFA ceremony") + } + req.SSOClientRedirectURL = ssoMFACeremony.GetClientCallbackURL() + promptOpts = append(promptOpts, withSSOMFACeremony(ssoMFACeremony)) + } + chal, err := c.CreateAuthenticateChallenge(ctx, req) if err != nil { // CreateAuthenticateChallenge returns a bad parameter error when the client diff --git a/api/mfa/prompt.go b/api/mfa/prompt.go index 1d5c0673d8c1d..db245621e13c7 100644 --- a/api/mfa/prompt.go +++ b/api/mfa/prompt.go @@ -54,6 +54,8 @@ type PromptConfig struct { // Extensions are the challenge extensions used to create the prompt's challenge. // Used to enrich certain prompts. Extensions *mfav1.ChallengeExtensions + // SSOMFACeremony is an SSO MFA ceremony. + SSOMFACeremony } // DeviceDescriptor is a descriptor for a device, such as "registered". @@ -117,3 +119,10 @@ func WithPromptChallengeExtensions(exts *mfav1.ChallengeExtensions) PromptOpt { cfg.Extensions = exts } } + +// withSSOMFACeremony sets the SSO MFA ceremony for the MFA prompt. +func withSSOMFACeremony(ssoMFACeremony SSOMFACeremony) PromptOpt { + return func(cfg *PromptConfig) { + cfg.SSOMFACeremony = ssoMFACeremony + } +} diff --git a/lib/client/api.go b/lib/client/api.go index 0a7b35dbd5e51..d52aabf5e0f98 100644 --- a/lib/client/api.go +++ b/lib/client/api.go @@ -495,6 +495,9 @@ type Config struct { // If empty, a default CLI prompt is used. CustomHardwareKeyPrompt keys.HardwareKeyPrompt + // SSOCeremonyConstructor is a custom SSO ceremony constructor to use. + SSOCeremonyConstructor func() *sso.Ceremony + // DisableSSHResumption disables transparent SSH connection resumption. DisableSSHResumption bool diff --git a/lib/client/mfa.go b/lib/client/mfa.go index 90c6f975d3a1c..6e77aeb2b9abf 100644 --- a/lib/client/mfa.go +++ b/lib/client/mfa.go @@ -79,5 +79,6 @@ func (tc *TeleportClient) newPromptConfig(opts ...mfa.PromptOpt) *libmfa.PromptC cfg.WebauthnLoginFunc = tc.WebauthnLogin cfg.WebauthnSupported = true } + return cfg } diff --git a/lib/client/mfa/cli.go b/lib/client/mfa/cli.go index 309a8d7000636..6fbdb521fbfb2 100644 --- a/lib/client/mfa/cli.go +++ b/lib/client/mfa/cli.go @@ -110,6 +110,7 @@ func (c *CLIPrompt) Run(ctx context.Context, chal *proto.MFAAuthenticateChalleng promptOTP := chal.TOTP != nil promptWebauthn := chal.WebauthnChallenge != nil + promptSSO := chal.SSOChallenge != nil // No prompt to run, no-op. if !promptOTP && !promptWebauthn { @@ -125,6 +126,11 @@ func (c *CLIPrompt) Run(ctx context.Context, chal *proto.MFAAuthenticateChalleng } } + if promptSSO && c.cfg.SSOMFACeremony == nil { + promptSSO = false + slog.DebugContext(ctx, "SSO MFA not supported by this client, this is likely a bug") + } + // Prefer whatever method is requested by the client. if c.cfg.PreferOTP && promptOTP { promptWebauthn = false @@ -157,6 +163,9 @@ func (c *CLIPrompt) Run(ctx context.Context, chal *proto.MFAAuthenticateChalleng case promptWebauthn: resp, err := c.promptWebauthn(ctx, chal, c.getWebauthnPrompt(ctx)) return resp, trace.Wrap(err) + case promptSSO: + resp, err := c.promptSSO(ctx, chal) + return resp, trace.Wrap(err) case promptOTP: resp, err := c.promptOTP(ctx, c.cfg.Quiet) return resp, trace.Wrap(err) @@ -315,3 +324,23 @@ func (w *webauthnPromptWithOTP) PromptPIN() (string, error) { return w.LoginPrompt.PromptPIN() } + +func (c *CLIPrompt) promptSSO(ctx context.Context, chal *proto.MFAAuthenticateChallenge) (*proto.MFAAuthenticateResponse, error) { + if err := c.cfg.SSOMFACeremony.HandleRedirect(ctx, chal.SSOChallenge.RedirectUrl); err != nil { + return nil, trace.Wrap(err) + } + + mfaToken, err := c.cfg.SSOMFACeremony.GetCallbackMFAToken(ctx) + if err != nil { + return nil, trace.Wrap(err) + } + + return &proto.MFAAuthenticateResponse{ + Response: &proto.MFAAuthenticateResponse_SSO{ + SSO: &proto.SSOResponse{ + RequestId: chal.SSOChallenge.RequestId, + Token: mfaToken, + }, + }, + }, nil +} diff --git a/lib/client/sso/ceremony.go b/lib/client/sso/ceremony.go index cb5b57c5a3183..2fbec3816939f 100644 --- a/lib/client/sso/ceremony.go +++ b/lib/client/sso/ceremony.go @@ -52,6 +52,20 @@ func (c *Ceremony) Run(ctx context.Context) (*authclient.SSHLoginResponse, error return resp, trace.Wrap(err) } +// GetCallbackMFAResponse returns an SSO MFA auth response once the callback is complete. +func (c *Ceremony) GetCallbackMFAToken(ctx context.Context) (string, error) { + loginResp, err := c.GetCallbackResponse(ctx) + if err != nil { + return "", trace.Wrap(err) + } + + if loginResp.MFAToken == "" { + return "", trace.BadParameter("login response for SSO MFA flow missing MFA token") + } + + return loginResp.MFAToken, nil +} + // NewCLICeremony creates a new CLI SSO ceremony from the given redirector. func NewCLICeremony(rd *Redirector, init CeremonyInit) *Ceremony { return &Ceremony{ From f3c64109760270cebb327cc912ed30c756eea150 Mon Sep 17 00:00:00 2001 From: joerger Date: Fri, 20 Sep 2024 19:05:35 -0700 Subject: [PATCH 02/12] Set SSO MFA ceremony from client configuration. --- lib/client/mfa.go | 1 + lib/client/sso.go | 16 ++++++++++++++++ lib/client/sso/ceremony.go | 36 +++++++++++++++++++++++++----------- 3 files changed, 42 insertions(+), 11 deletions(-) diff --git a/lib/client/mfa.go b/lib/client/mfa.go index 6e77aeb2b9abf..bc20c58a84502 100644 --- a/lib/client/mfa.go +++ b/lib/client/mfa.go @@ -33,6 +33,7 @@ func (tc *TeleportClient) NewMFACeremony() *mfa.Ceremony { return &mfa.Ceremony{ CreateAuthenticateChallenge: tc.createAuthenticateChallenge, PromptConstructor: tc.NewMFAPrompt, + SSOMFACeremonyConstructor: tc.newSSOMFACeremony, } } diff --git a/lib/client/sso.go b/lib/client/sso.go index 27b30080e212c..5b69af16e8a32 100644 --- a/lib/client/sso.go +++ b/lib/client/sso.go @@ -26,11 +26,27 @@ import ( "github.com/gravitational/trace" + "github.com/gravitational/teleport/api/mfa" "github.com/gravitational/teleport/api/utils/prompt" "github.com/gravitational/teleport/lib/client/sso" "github.com/gravitational/teleport/lib/utils" ) +func (tc *TeleportClient) newSSOMFACeremony(ctx context.Context) (mfa.SSOMFACeremony, error) { + rdConfig, err := tc.ssoRedirectorConfig(ctx, "" /*connectorDisplayName*/) + if err != nil { + return nil, trace.Wrap(err) + } + + rd, err := sso.NewRedirector(rdConfig) + if err != nil { + return nil, trace.Wrap(err) + } + defer rd.Close() + + return &sso.MFACeremony{Ceremony: sso.NewCLICeremony(rd, nil /*init*/)}, nil +} + // ssoRedirectorConfig returns a standard configured sso redirector for login. // A display name for the SSO connector can optionally be provided for minor UI improvements. func (tc *TeleportClient) ssoRedirectorConfig(ctx context.Context, connectorDisplayName string) (sso.RedirectorConfig, error) { diff --git a/lib/client/sso/ceremony.go b/lib/client/sso/ceremony.go index 2fbec3816939f..985010eb9a561 100644 --- a/lib/client/sso/ceremony.go +++ b/lib/client/sso/ceremony.go @@ -52,8 +52,32 @@ func (c *Ceremony) Run(ctx context.Context) (*authclient.SSHLoginResponse, error return resp, trace.Wrap(err) } +// NewCLICeremony creates a new CLI SSO ceremony from the given redirector. +func NewCLICeremony(rd *Redirector, init CeremonyInit) *Ceremony { + return &Ceremony{ + clientCallbackURL: rd.ClientCallbackURL, + Init: init, + HandleRedirect: rd.OpenRedirect, + GetCallbackResponse: rd.WaitForResponse, + } +} + +type MFACeremony struct { + *Ceremony +} + +// GetClientCallbackURL returns the SSO client callback URL. +func (c *MFACeremony) GetClientCallbackURL() string { + return c.clientCallbackURL +} + +// HandleRedirect handles redirect. +func (c *MFACeremony) HandleRedirect(ctx context.Context, redirectURL string) error { + return c.Ceremony.HandleRedirect(ctx, redirectURL) +} + // GetCallbackMFAResponse returns an SSO MFA auth response once the callback is complete. -func (c *Ceremony) GetCallbackMFAToken(ctx context.Context) (string, error) { +func (c *MFACeremony) GetCallbackMFAToken(ctx context.Context) (string, error) { loginResp, err := c.GetCallbackResponse(ctx) if err != nil { return "", trace.Wrap(err) @@ -65,13 +89,3 @@ func (c *Ceremony) GetCallbackMFAToken(ctx context.Context) (string, error) { return loginResp.MFAToken, nil } - -// NewCLICeremony creates a new CLI SSO ceremony from the given redirector. -func NewCLICeremony(rd *Redirector, init CeremonyInit) *Ceremony { - return &Ceremony{ - clientCallbackURL: rd.ClientCallbackURL, - Init: init, - HandleRedirect: rd.OpenRedirect, - GetCallbackResponse: rd.WaitForResponse, - } -} From 6ee59e13953205466b9a1efbcc180cc35187c3f6 Mon Sep 17 00:00:00 2001 From: joerger Date: Fri, 4 Oct 2024 19:19:51 -0700 Subject: [PATCH 03/12] Close sso mfa redirector once mfa ceremony is complete. --- api/mfa/ceremony.go | 3 +++ lib/client/mfa.go | 16 +++++++++++++++- lib/client/sso.go | 16 ---------------- 3 files changed, 18 insertions(+), 17 deletions(-) diff --git a/api/mfa/ceremony.go b/api/mfa/ceremony.go index 7d527c4ca329f..a72d30c2c1a8a 100644 --- a/api/mfa/ceremony.go +++ b/api/mfa/ceremony.go @@ -53,6 +53,9 @@ type CreateAuthenticateChallengeFunc func(ctx context.Context, req *proto.Create // req may be nil if ceremony.CreateAuthenticateChallenge does not require it, e.g. in // the moderated session mfa ceremony which uses a custom stream rpc to create challenges. func (c *Ceremony) Run(ctx context.Context, req *proto.CreateAuthenticateChallengeRequest, promptOpts ...PromptOpt) (*proto.MFAAuthenticateResponse, error) { + ctx, cancel := context.WithCancel(ctx) + defer cancel() + switch { case c.CreateAuthenticateChallenge == nil: return nil, trace.BadParameter("mfa ceremony must have CreateAuthenticateChallenge set in order to begin") diff --git a/lib/client/mfa.go b/lib/client/mfa.go index bc20c58a84502..34b8db55d3aa7 100644 --- a/lib/client/mfa.go +++ b/lib/client/mfa.go @@ -26,6 +26,7 @@ import ( "github.com/gravitational/teleport/api/client/proto" "github.com/gravitational/teleport/api/mfa" libmfa "github.com/gravitational/teleport/lib/client/mfa" + "github.com/gravitational/teleport/lib/client/sso" ) // NewMFACeremony returns a new MFA ceremony configured for this client. @@ -33,7 +34,20 @@ func (tc *TeleportClient) NewMFACeremony() *mfa.Ceremony { return &mfa.Ceremony{ CreateAuthenticateChallenge: tc.createAuthenticateChallenge, PromptConstructor: tc.NewMFAPrompt, - SSOMFACeremonyConstructor: tc.newSSOMFACeremony, + SSOMFACeremonyConstructor: func(ctx context.Context) (mfa.SSOMFACeremony, error) { + rdConfig, err := tc.ssoRedirectorConfig(ctx, "" /*connectorDisplayName*/) + if err != nil { + return nil, trace.Wrap(err) + } + + rd, err := sso.NewRedirector(rdConfig) + if err != nil { + return nil, trace.Wrap(err) + } + + context.AfterFunc(ctx, rd.Close) + return &sso.MFACeremony{Ceremony: sso.NewCLICeremony(rd, nil /*init*/)}, nil + }, } } diff --git a/lib/client/sso.go b/lib/client/sso.go index 5b69af16e8a32..27b30080e212c 100644 --- a/lib/client/sso.go +++ b/lib/client/sso.go @@ -26,27 +26,11 @@ import ( "github.com/gravitational/trace" - "github.com/gravitational/teleport/api/mfa" "github.com/gravitational/teleport/api/utils/prompt" "github.com/gravitational/teleport/lib/client/sso" "github.com/gravitational/teleport/lib/utils" ) -func (tc *TeleportClient) newSSOMFACeremony(ctx context.Context) (mfa.SSOMFACeremony, error) { - rdConfig, err := tc.ssoRedirectorConfig(ctx, "" /*connectorDisplayName*/) - if err != nil { - return nil, trace.Wrap(err) - } - - rd, err := sso.NewRedirector(rdConfig) - if err != nil { - return nil, trace.Wrap(err) - } - defer rd.Close() - - return &sso.MFACeremony{Ceremony: sso.NewCLICeremony(rd, nil /*init*/)}, nil -} - // ssoRedirectorConfig returns a standard configured sso redirector for login. // A display name for the SSO connector can optionally be provided for minor UI improvements. func (tc *TeleportClient) ssoRedirectorConfig(ctx context.Context, connectorDisplayName string) (sso.RedirectorConfig, error) { From 900f442ed056fb04d7f2ebeb85a2f6ae46de6204 Mon Sep 17 00:00:00 2001 From: joerger Date: Mon, 7 Oct 2024 16:34:17 -0700 Subject: [PATCH 04/12] Refactor sso mfa ceremony. --- api/mfa/ceremony.go | 5 ++-- lib/client/mfa.go | 2 +- lib/client/mfa/cli.go | 19 ++----------- lib/client/sso/ceremony.go | 58 +++++++++++++++++++++++++++----------- 4 files changed, 48 insertions(+), 36 deletions(-) diff --git a/api/mfa/ceremony.go b/api/mfa/ceremony.go index a72d30c2c1a8a..acf5b0fa77565 100644 --- a/api/mfa/ceremony.go +++ b/api/mfa/ceremony.go @@ -41,8 +41,7 @@ type Ceremony struct { // SSOMFACeremony is an SSO MFA ceremony. type SSOMFACeremony interface { GetClientCallbackURL() string - HandleRedirect(ctx context.Context, redirectURL string) error - GetCallbackMFAToken(ctx context.Context) (string, error) + Run(ctx context.Context, chal *proto.MFAAuthenticateChallenge) (*proto.MFAAuthenticateResponse, error) } // CreateAuthenticateChallengeFunc is a function that creates an authentication challenge. @@ -68,6 +67,8 @@ func (c *Ceremony) Run(ctx context.Context, req *proto.CreateAuthenticateChallen return nil, trace.BadParameter("mfa challenge scope must be specified") } + // If available, prepare an SSO MFA ceremony and set the client redirect URL in the challenge + // request to request an SSO challenge in addition to other challenges. if c.SSOMFACeremonyConstructor != nil { ssoMFACeremony, err := c.SSOMFACeremonyConstructor(ctx) if err != nil { diff --git a/lib/client/mfa.go b/lib/client/mfa.go index 34b8db55d3aa7..e7152472f822d 100644 --- a/lib/client/mfa.go +++ b/lib/client/mfa.go @@ -46,7 +46,7 @@ func (tc *TeleportClient) NewMFACeremony() *mfa.Ceremony { } context.AfterFunc(ctx, rd.Close) - return &sso.MFACeremony{Ceremony: sso.NewCLICeremony(rd, nil /*init*/)}, nil + return sso.NewCLIMFACeremony(rd), nil }, } } diff --git a/lib/client/mfa/cli.go b/lib/client/mfa/cli.go index 6fbdb521fbfb2..256c110f3f497 100644 --- a/lib/client/mfa/cli.go +++ b/lib/client/mfa/cli.go @@ -326,21 +326,6 @@ func (w *webauthnPromptWithOTP) PromptPIN() (string, error) { } func (c *CLIPrompt) promptSSO(ctx context.Context, chal *proto.MFAAuthenticateChallenge) (*proto.MFAAuthenticateResponse, error) { - if err := c.cfg.SSOMFACeremony.HandleRedirect(ctx, chal.SSOChallenge.RedirectUrl); err != nil { - return nil, trace.Wrap(err) - } - - mfaToken, err := c.cfg.SSOMFACeremony.GetCallbackMFAToken(ctx) - if err != nil { - return nil, trace.Wrap(err) - } - - return &proto.MFAAuthenticateResponse{ - Response: &proto.MFAAuthenticateResponse_SSO{ - SSO: &proto.SSOResponse{ - RequestId: chal.SSOChallenge.RequestId, - Token: mfaToken, - }, - }, - }, nil + resp, err := c.cfg.SSOMFACeremony.Run(ctx, chal) + return resp, trace.Wrap(err) } diff --git a/lib/client/sso/ceremony.go b/lib/client/sso/ceremony.go index 985010eb9a561..84dae6c14d6a8 100644 --- a/lib/client/sso/ceremony.go +++ b/lib/client/sso/ceremony.go @@ -23,6 +23,7 @@ import ( "github.com/gravitational/trace" + "github.com/gravitational/teleport/api/client/proto" "github.com/gravitational/teleport/lib/auth/authclient" ) @@ -62,30 +63,55 @@ func NewCLICeremony(rd *Redirector, init CeremonyInit) *Ceremony { } } +// Ceremony is a customizable SSO MFA ceremony. type MFACeremony struct { - *Ceremony + clientCallbackURL string + HandleRedirect func(ctx context.Context, redirectURL string) error + GetCallbackMFAToken func(ctx context.Context) (string, error) } -// GetClientCallbackURL returns the SSO client callback URL. -func (c *MFACeremony) GetClientCallbackURL() string { - return c.clientCallbackURL +// GetClientCallbackURL returns the client callback URL. +func (m *MFACeremony) GetClientCallbackURL() string { + return m.clientCallbackURL } -// HandleRedirect handles redirect. -func (c *MFACeremony) HandleRedirect(ctx context.Context, redirectURL string) error { - return c.Ceremony.HandleRedirect(ctx, redirectURL) -} +// Run the SSO MFA ceremony. +func (m *MFACeremony) Run(ctx context.Context, chal *proto.MFAAuthenticateChallenge) (*proto.MFAAuthenticateResponse, error) { + if err := m.HandleRedirect(ctx, chal.SSOChallenge.RedirectUrl); err != nil { + return nil, trace.Wrap(err) + } -// GetCallbackMFAResponse returns an SSO MFA auth response once the callback is complete. -func (c *MFACeremony) GetCallbackMFAToken(ctx context.Context) (string, error) { - loginResp, err := c.GetCallbackResponse(ctx) + mfaToken, err := m.GetCallbackMFAToken(ctx) if err != nil { - return "", trace.Wrap(err) + return nil, trace.Wrap(err) } - if loginResp.MFAToken == "" { - return "", trace.BadParameter("login response for SSO MFA flow missing MFA token") - } + return &proto.MFAAuthenticateResponse{ + Response: &proto.MFAAuthenticateResponse_SSO{ + SSO: &proto.SSOResponse{ + RequestId: chal.SSOChallenge.RequestId, + Token: mfaToken, + }, + }, + }, nil +} - return loginResp.MFAToken, nil +// NewCLIMFACeremony creates a new CLI SSO ceremony from the given redirector. +func NewCLIMFACeremony(rd *Redirector) *MFACeremony { + return &MFACeremony{ + clientCallbackURL: rd.ClientCallbackURL, + HandleRedirect: rd.OpenRedirect, + GetCallbackMFAToken: func(ctx context.Context) (string, error) { + loginResp, err := rd.WaitForResponse(ctx) + if err != nil { + return "", trace.Wrap(err) + } + + if loginResp.MFAToken == "" { + return "", trace.BadParameter("login response for SSO MFA flow missing MFA token") + } + + return loginResp.MFAToken, nil + }, + } } From 4d4ee896671e26396c961aebbecfa66d3969f8b8 Mon Sep 17 00:00:00 2001 From: joerger Date: Mon, 21 Oct 2024 16:26:25 -0700 Subject: [PATCH 05/12] Add SSOMFACeremonyConstructor. --- api/client/client.go | 8 ++++++++ api/client/mfa.go | 1 + api/mfa/ceremony.go | 5 ++++- api/mfa/prompt.go | 2 +- lib/client/api.go | 9 ++++++--- lib/client/mfa.go | 31 +++++++++++++++++-------------- tool/tctl/common/tctl.go | 14 ++++++++++++++ 7 files changed, 51 insertions(+), 19 deletions(-) diff --git a/api/client/client.go b/api/client/client.go index ee1ea80087791..5f24aa66b9fc3 100644 --- a/api/client/client.go +++ b/api/client/client.go @@ -664,6 +664,9 @@ type Config struct { // MFAPromptConstructor is used to create MFA prompts when needed. // If nil, the client will not prompt for MFA. MFAPromptConstructor mfa.PromptConstructor + // SSOMFACeremonyConstructor is used to handle SSO MFA when needed. + // If nil, the client will not prompt for MFA. + SSOMFACeremonyConstructor mfa.SSOMFACeremonyConstructor } // CheckAndSetDefaults checks and sets default config values. @@ -730,6 +733,11 @@ func (c *Client) SetMFAPromptConstructor(pc mfa.PromptConstructor) { c.c.MFAPromptConstructor = pc } +// SetSSOMFACeremonyConstructor sets the SSO MFA ceremony constructor for this client. +func (c *Client) SetSSOMFACeremonyConstructor(scc mfa.SSOMFACeremonyConstructor) { + c.c.SSOMFACeremonyConstructor = scc +} + // Close closes the Client connection to the auth server. func (c *Client) Close() error { if c.setClosed() && c.conn != nil { diff --git a/api/client/mfa.go b/api/client/mfa.go index beba5b20c79dd..8db9af2b318f0 100644 --- a/api/client/mfa.go +++ b/api/client/mfa.go @@ -30,6 +30,7 @@ func (c *Client) PerformMFACeremony(ctx context.Context, challengeRequest *proto mfaCeremony := &mfa.Ceremony{ CreateAuthenticateChallenge: c.CreateAuthenticateChallenge, PromptConstructor: c.c.MFAPromptConstructor, + SSOMFACeremonyConstructor: c.c.SSOMFACeremonyConstructor, } return mfaCeremony.Run(ctx, challengeRequest, promptOpts...) } diff --git a/api/mfa/ceremony.go b/api/mfa/ceremony.go index acf5b0fa77565..1a8940ddc8089 100644 --- a/api/mfa/ceremony.go +++ b/api/mfa/ceremony.go @@ -35,7 +35,7 @@ type Ceremony struct { // SSOMFACeremonyConstructor is an optional SSO MFA ceremony constructor. If provided, // the MFA ceremony will also attempt to retrieve an SSO MFA challenge. The provided // context will be closed once the ceremony is complete. - SSOMFACeremonyConstructor func(ctx context.Context) (SSOMFACeremony, error) + SSOMFACeremonyConstructor SSOMFACeremonyConstructor } // SSOMFACeremony is an SSO MFA ceremony. @@ -44,6 +44,9 @@ type SSOMFACeremony interface { Run(ctx context.Context, chal *proto.MFAAuthenticateChallenge) (*proto.MFAAuthenticateResponse, error) } +// SSOMFACeremonyConstructor constructs a new SSO MFA ceremony. +type SSOMFACeremonyConstructor func(ctx context.Context) (SSOMFACeremony, error) + // CreateAuthenticateChallengeFunc is a function that creates an authentication challenge. type CreateAuthenticateChallengeFunc func(ctx context.Context, req *proto.CreateAuthenticateChallengeRequest) (*proto.MFAAuthenticateChallenge, error) diff --git a/api/mfa/prompt.go b/api/mfa/prompt.go index db245621e13c7..9efd9fcb1fe44 100644 --- a/api/mfa/prompt.go +++ b/api/mfa/prompt.go @@ -55,7 +55,7 @@ type PromptConfig struct { // Used to enrich certain prompts. Extensions *mfav1.ChallengeExtensions // SSOMFACeremony is an SSO MFA ceremony. - SSOMFACeremony + SSOMFACeremony SSOMFACeremony } // DeviceDescriptor is a descriptor for a device, such as "registered". diff --git a/lib/client/api.go b/lib/client/api.go index d52aabf5e0f98..a03cdfd443a94 100644 --- a/lib/client/api.go +++ b/lib/client/api.go @@ -3046,6 +3046,8 @@ func (tc *TeleportClient) ConnectToCluster(ctx context.Context) (_ *ClusterClien return nil, trace.NewAggregate(err, pclt.Close()) } authClientCfg.MFAPromptConstructor = tc.NewMFAPrompt + authClientCfg.SSOMFACeremonyConstructor = tc.NewSSOMFACeremony + authClient, err := authclient.NewClient(authClientCfg) if err != nil { return nil, trace.NewAggregate(err, pclt.Close()) @@ -5065,9 +5067,10 @@ func (tc *TeleportClient) NewKubernetesServiceClient(ctx context.Context, cluste Credentials: []client.Credentials{ client.LoadTLS(tlsConfig), }, - ALPNConnUpgradeRequired: tc.TLSRoutingConnUpgradeRequired, - InsecureAddressDiscovery: tc.InsecureSkipVerify, - MFAPromptConstructor: tc.NewMFAPrompt, + ALPNConnUpgradeRequired: tc.TLSRoutingConnUpgradeRequired, + InsecureAddressDiscovery: tc.InsecureSkipVerify, + MFAPromptConstructor: tc.NewMFAPrompt, + SSOMFACeremonyConstructor: tc.NewSSOMFACeremony, }) if err != nil { return nil, trace.Wrap(err) diff --git a/lib/client/mfa.go b/lib/client/mfa.go index e7152472f822d..b3431cebd5937 100644 --- a/lib/client/mfa.go +++ b/lib/client/mfa.go @@ -34,20 +34,7 @@ func (tc *TeleportClient) NewMFACeremony() *mfa.Ceremony { return &mfa.Ceremony{ CreateAuthenticateChallenge: tc.createAuthenticateChallenge, PromptConstructor: tc.NewMFAPrompt, - SSOMFACeremonyConstructor: func(ctx context.Context) (mfa.SSOMFACeremony, error) { - rdConfig, err := tc.ssoRedirectorConfig(ctx, "" /*connectorDisplayName*/) - if err != nil { - return nil, trace.Wrap(err) - } - - rd, err := sso.NewRedirector(rdConfig) - if err != nil { - return nil, trace.Wrap(err) - } - - context.AfterFunc(ctx, rd.Close) - return sso.NewCLIMFACeremony(rd), nil - }, + SSOMFACeremonyConstructor: tc.NewSSOMFACeremony, } } @@ -97,3 +84,19 @@ func (tc *TeleportClient) newPromptConfig(opts ...mfa.PromptOpt) *libmfa.PromptC return cfg } + +// NewSSOMFACeremony creates a new SSO MFA ceremony. +func (tc *TeleportClient) NewSSOMFACeremony(ctx context.Context) (mfa.SSOMFACeremony, error) { + rdConfig, err := tc.ssoRedirectorConfig(ctx, "" /*connectorDisplayName*/) + if err != nil { + return nil, trace.Wrap(err) + } + + rd, err := sso.NewRedirector(rdConfig) + if err != nil { + return nil, trace.Wrap(err) + } + + context.AfterFunc(ctx, rd.Close) + return sso.NewCLIMFACeremony(rd), nil +} diff --git a/tool/tctl/common/tctl.go b/tool/tctl/common/tctl.go index 48ad1f0b75b6d..3cf5b16345274 100644 --- a/tool/tctl/common/tctl.go +++ b/tool/tctl/common/tctl.go @@ -47,6 +47,7 @@ import ( "github.com/gravitational/teleport/lib/client" "github.com/gravitational/teleport/lib/client/identityfile" libmfa "github.com/gravitational/teleport/lib/client/mfa" + "github.com/gravitational/teleport/lib/client/sso" "github.com/gravitational/teleport/lib/config" "github.com/gravitational/teleport/lib/defaults" "github.com/gravitational/teleport/lib/modules" @@ -257,6 +258,19 @@ func TryRun(commands []CLICommand, args []string) error { PromptConfig: *promptCfg, }) }) + client.SetSSOMFACeremonyConstructor(func(ctx context.Context) (mfa.SSOMFACeremony, error) { + rdConfig := sso.RedirectorConfig{ + ProxyAddr: proxyAddr, + } + + rd, err := sso.NewRedirector(rdConfig) + if err != nil { + return nil, trace.Wrap(err) + } + + context.AfterFunc(ctx, rd.Close) + return sso.NewCLIMFACeremony(rd), nil + }) // execute whatever is selected: var match bool From 3e0e4ff95b05ccd890adbdd85059069f8fd50994 Mon Sep 17 00:00:00 2001 From: joerger Date: Tue, 22 Oct 2024 20:40:39 -0700 Subject: [PATCH 06/12] Add tests. --- api/mfa/ceremony_test.go | 75 ++++++++++++++++++++++++++++++- lib/client/sso/ceremony_test.go | 78 +++++++++++++++++++++++++++++++-- 2 files changed, 148 insertions(+), 5 deletions(-) diff --git a/api/mfa/ceremony_test.go b/api/mfa/ceremony_test.go index 7d94fd4de5327..a988778264d3e 100644 --- a/api/mfa/ceremony_test.go +++ b/api/mfa/ceremony_test.go @@ -23,13 +23,14 @@ import ( "github.com/gravitational/trace" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" "github.com/gravitational/teleport/api/client/proto" mfav1 "github.com/gravitational/teleport/api/gen/proto/go/teleport/mfa/v1" "github.com/gravitational/teleport/api/mfa" ) -func TestPerformMFACeremony(t *testing.T) { +func TestMFACeremony(t *testing.T) { t.Parallel() ctx := context.Background() @@ -128,3 +129,75 @@ func TestPerformMFACeremony(t *testing.T) { }) } } + +func TestMFACeremony_SSO(t *testing.T) { + t.Parallel() + ctx := context.Background() + + testMFAChallenge := &proto.MFAAuthenticateChallenge{ + SSOChallenge: &proto.SSOChallenge{ + RedirectUrl: "redirect", + RequestId: "request-id", + }, + } + testMFAResponse := &proto.MFAAuthenticateResponse{ + Response: &proto.MFAAuthenticateResponse_SSO{ + SSO: &proto.SSOResponse{ + Token: "token", + RequestId: "request-id", + }, + }, + } + + ssoMFACeremony := &mfa.Ceremony{ + CreateAuthenticateChallenge: func(ctx context.Context, req *proto.CreateAuthenticateChallengeRequest) (*proto.MFAAuthenticateChallenge, error) { + return testMFAChallenge, nil + }, + PromptConstructor: func(opts ...mfa.PromptOpt) mfa.Prompt { + cfg := new(mfa.PromptConfig) + for _, opt := range opts { + opt(cfg) + } + + return mfa.PromptFunc(func(ctx context.Context, chal *proto.MFAAuthenticateChallenge) (*proto.MFAAuthenticateResponse, error) { + if cfg.SSOMFACeremony == nil { + return nil, trace.BadParameter("expected sso mfa ceremony") + } + + return cfg.SSOMFACeremony.Run(ctx, chal) + }) + }, + SSOMFACeremonyConstructor: func(ctx context.Context) (mfa.SSOMFACeremony, error) { + return &mockSSOMFACeremony{ + clientCallbackURL: "client-redirect", + prompt: func(ctx context.Context, chal *proto.MFAAuthenticateChallenge) (*proto.MFAAuthenticateResponse, error) { + return testMFAResponse, nil + }, + }, nil + }, + } + + resp, err := ssoMFACeremony.Run(ctx, &proto.CreateAuthenticateChallengeRequest{ + ChallengeExtensions: &mfav1.ChallengeExtensions{ + Scope: mfav1.ChallengeScope_CHALLENGE_SCOPE_ADMIN_ACTION, + }, + MFARequiredCheck: &proto.IsMFARequiredRequest{}, + }) + require.NoError(t, err) + require.Equal(t, testMFAResponse, resp) +} + +type mockSSOMFACeremony struct { + clientCallbackURL string + prompt mfa.PromptFunc +} + +// GetClientCallbackURL returns the client callback URL. +func (m *mockSSOMFACeremony) GetClientCallbackURL() string { + return m.clientCallbackURL +} + +// Run the SSO MFA ceremony. +func (m *mockSSOMFACeremony) Run(ctx context.Context, chal *proto.MFAAuthenticateChallenge) (*proto.MFAAuthenticateResponse, error) { + return m.prompt(ctx, chal) +} diff --git a/lib/client/sso/ceremony_test.go b/lib/client/sso/ceremony_test.go index 0851ac1b4daf2..a2b15b532e54b 100644 --- a/lib/client/sso/ceremony_test.go +++ b/lib/client/sso/ceremony_test.go @@ -26,17 +26,20 @@ import ( "net/http/httptest" "regexp" "testing" - "text/template" "github.com/gravitational/trace" "github.com/stretchr/testify/require" + "gotest.tools/assert" "github.com/gravitational/teleport" + "github.com/gravitational/teleport/api/client/proto" "github.com/gravitational/teleport/lib/client/sso" "github.com/gravitational/teleport/lib/web" ) func TestCLICeremony(t *testing.T) { + ctx := context.Background() + mockProxy := newMockProxy(t) username := "alice" @@ -69,8 +72,6 @@ func TestCLICeremony(t *testing.T) { return mockIdPServer.URL, nil }) - template.New("Failed to open a browser window for login: %v\n") - // Modify handle redirect to also browse to the clickable URL printed to stderr. baseHandleRedirect := ceremony.HandleRedirect ceremony.HandleRedirect = func(ctx context.Context, redirectURL string) error { @@ -94,7 +95,76 @@ func TestCLICeremony(t *testing.T) { return nil } - loginResp, err := ceremony.Run(context.Background()) + loginResp, err := ceremony.Run(ctx) require.NoError(t, err) require.Equal(t, username, loginResp.Username) } + +func TestCLICeremony_MFA(t *testing.T) { + const token = "sso-mfa-token" + const requestID = "soo-mfa-request-id" + + ctx := context.Background() + mockProxy := newMockProxy(t) + + // Capture stderr. + stderr := bytes.NewBuffer([]byte{}) + + // Create a basic redirector. + rd, err := sso.NewRedirector(sso.RedirectorConfig{ + ProxyAddr: mockProxy.URL, + Browser: teleport.BrowserNone, + Stderr: stderr, + }) + require.NoError(t, err) + t.Cleanup(rd.Close) + + // Construct a fake mfa response with the redirector's client callback URL. + successResponseURL, err := web.ConstructSSHResponse(web.AuthParams{ + ClientRedirectURL: rd.ClientCallbackURL, + MFAToken: token, + }) + require.NoError(t, err) + + // Open a mock IdP server which will handle a redirect and result in the expected IdP session payload. + mockIdPServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + http.Redirect(w, r, successResponseURL.String(), http.StatusPermanentRedirect) + })) + t.Cleanup(mockIdPServer.Close) + + ceremony := sso.NewCLIMFACeremony(rd) + + // Modify handle redirect to also browse to the clickable URL printed to stderr. + baseHandleRedirect := ceremony.HandleRedirect + ceremony.HandleRedirect = func(ctx context.Context, redirectURL string) error { + if err := baseHandleRedirect(ctx, redirectURL); err != nil { + return trace.Wrap(err) + } + + // Read the clickable url from stderr and navigate to it + // using a simplified regexp for http://127.0.0.1:/ + clickableURLPattern := "http://127.0.0.1:.*/.*[0-9a-f]" + clickableURL := regexp.MustCompile(clickableURLPattern).Find(stderr.Bytes()) + + resp, err := http.Get(string(clickableURL)) + require.NoError(t, err) + defer resp.Body.Close() + + // User should be redirected to success screen. + body, err := io.ReadAll(resp.Body) + require.NoError(t, err) + require.Equal(t, sso.LoginSuccessRedirectURL, string(body)) + return nil + } + + mfaResponse, err := ceremony.Run(ctx, &proto.MFAAuthenticateChallenge{ + SSOChallenge: &proto.SSOChallenge{ + RedirectUrl: mockIdPServer.URL, + RequestId: requestID, + }, + }) + require.NoError(t, err) + require.NotNil(t, mfaResponse.GetSSO()) + assert.Equal(t, token, mfaResponse.GetSSO().Token) + assert.Equal(t, requestID, mfaResponse.GetSSO().RequestId) +} From 7c5025f49c437b44907d4c36ff29693dd89a2edc Mon Sep 17 00:00:00 2001 From: joerger Date: Wed, 23 Oct 2024 13:42:10 -0700 Subject: [PATCH 07/12] Add --mfa-mode=sso support; Add cli prompt UX changes. --- lib/client/api.go | 3 + lib/client/mfa.go | 1 + lib/client/mfa/cli.go | 73 ++++++++-- lib/client/mfa/cli_test.go | 242 +++++++++++++++++++++++++++++--- lib/client/sso/ceremony_test.go | 2 +- tool/tsh/common/tsh.go | 8 +- 6 files changed, 298 insertions(+), 31 deletions(-) diff --git a/lib/client/api.go b/lib/client/api.go index a03cdfd443a94..73c9883a115cb 100644 --- a/lib/client/api.go +++ b/lib/client/api.go @@ -360,6 +360,9 @@ type Config struct { // authenticators, such as remote hosts or virtual machines. PreferOTP bool + // PreferSSO prefers SSO in favor of other MFA methods. + PreferSSO bool + // CheckVersions will check that client version is compatible // with auth server version when connecting. CheckVersions bool diff --git a/lib/client/mfa.go b/lib/client/mfa.go index b3431cebd5937..b5a7c8729141e 100644 --- a/lib/client/mfa.go +++ b/lib/client/mfa.go @@ -63,6 +63,7 @@ func (tc *TeleportClient) NewMFAPrompt(opts ...mfa.PromptOpt) mfa.Prompt { PromptConfig: *cfg, Writer: tc.Stderr, PreferOTP: tc.PreferOTP, + PreferSSO: tc.PreferSSO, AllowStdinHijack: tc.AllowStdinHijack, StdinFunc: tc.StdinFunc, }) diff --git a/lib/client/mfa/cli.go b/lib/client/mfa/cli.go index 256c110f3f497..872c61fb5773d 100644 --- a/lib/client/mfa/cli.go +++ b/lib/client/mfa/cli.go @@ -25,6 +25,7 @@ import ( "log/slog" "os" "runtime" + "strings" "sync" "github.com/gravitational/trace" @@ -38,6 +39,15 @@ import ( "github.com/gravitational/teleport/lib/auth/webauthnwin" ) +const ( + // cliMFATypeOTP is the CLI display name for OTP. + cliMFATypeOTP = "OTP" + // cliMFATypeWebauthn is the CLI display name for Webauthn. + cliMFATypeWebauthn = "WEBAUTHN" + // cliMFATypeSSO is the CLI display name for SSO. + cliMFATypeSSO = "SSO" +) + // CLIPromptConfig contains CLI prompt config options. type CLIPromptConfig struct { PromptConfig @@ -52,6 +62,9 @@ type CLIPromptConfig struct { // PreferOTP favors OTP challenges, if applicable. // Takes precedence over AuthenticatorAttachment settings. PreferOTP bool + // PreferSSO favors SSO challenges, if applicable. + // Takes precedence over AuthenticatorAttachment settings. + PreferSSO bool // StdinFunc allows tests to override prompt.Stdin(). // If nil prompt.Stdin() is used. StdinFunc func() prompt.StdinReader @@ -113,17 +126,25 @@ func (c *CLIPrompt) Run(ctx context.Context, chal *proto.MFAAuthenticateChalleng promptSSO := chal.SSOChallenge != nil // No prompt to run, no-op. - if !promptOTP && !promptWebauthn { + if !promptOTP && !promptWebauthn && !promptSSO { return &proto.MFAAuthenticateResponse{}, nil } + var availableMethods []string + if promptWebauthn { + availableMethods = append(availableMethods, cliMFATypeWebauthn) + } + if promptSSO { + availableMethods = append(availableMethods, cliMFATypeSSO) + } + if promptOTP { + availableMethods = append(availableMethods, cliMFATypeOTP) + } + // Check off unsupported methods. if promptWebauthn && !c.cfg.WebauthnSupported { promptWebauthn = false slog.DebugContext(ctx, "hardware device MFA not supported by your platform") - if !promptOTP { - return nil, trace.BadParameter("hardware device MFA not supported by your platform, please register an OTP device") - } } if promptSSO && c.cfg.SSOMFACeremony == nil { @@ -132,8 +153,17 @@ func (c *CLIPrompt) Run(ctx context.Context, chal *proto.MFAAuthenticateChalleng } // Prefer whatever method is requested by the client. - if c.cfg.PreferOTP && promptOTP { - promptWebauthn = false + var chosenMethods []string + var userSpecifiedMethod bool + switch { + case c.cfg.PreferSSO && promptSSO: + chosenMethods = []string{cliMFATypeSSO} + promptWebauthn, promptOTP = false, false + userSpecifiedMethod = true + case c.cfg.PreferOTP && promptOTP: + chosenMethods = []string{cliMFATypeOTP} + promptWebauthn, promptSSO = false, false + userSpecifiedMethod = true } // Use stronger auth methods if hijack is not allowed. @@ -141,10 +171,32 @@ func (c *CLIPrompt) Run(ctx context.Context, chal *proto.MFAAuthenticateChalleng promptOTP = false } - // If a specific webauthn attachment was requested, skip OTP. - // Otherwise, allow dual prompt with OTP. - if promptWebauthn && c.cfg.AuthenticatorAttachment != wancli.AttachmentAuto { + // If we have multiple viable options, prefer Webauthn > SSO > OTP. + switch { + case promptWebauthn: + chosenMethods = []string{cliMFATypeWebauthn} + promptSSO = false + + // If a specific webauthn attachment was requested, skip OTP. + // Otherwise, allow dual prompt with OTP. + promptOTP = promptOTP && c.cfg.AuthenticatorAttachment == wancli.AttachmentAuto + if promptOTP { + chosenMethods = append(chosenMethods, cliMFATypeOTP) + } + case promptSSO: + chosenMethods = []string{cliMFATypeSSO} promptOTP = false + case promptOTP: + chosenMethods = []string{cliMFATypeOTP} + } + + // If there are multiple options and we chose one without it being specifically + // requested by the user, notify the user about it and how to request a specific method. + if len(availableMethods) > len(chosenMethods) && len(chosenMethods) > 0 && !userSpecifiedMethod { + const msg = "" + + "Available MFA methods [%v]. Continuing with %v.\n" + + "If you wish to perform MFA with another method, specify with flag --mfa-mode=.\n\n" + fmt.Fprintf(c.writer(), msg, strings.Join(availableMethods, ", "), strings.Join(chosenMethods, " and ")) } // DELETE IN v18.0 after TOTP session MFA support is removed (codingllama) @@ -170,8 +222,7 @@ func (c *CLIPrompt) Run(ctx context.Context, chal *proto.MFAAuthenticateChalleng resp, err := c.promptOTP(ctx, c.cfg.Quiet) return resp, trace.Wrap(err) default: - // We shouldn't reach this case as we would have hit the no-op case above. - return nil, trace.BadParameter("no MFA methods to prompt") + return nil, trace.BadParameter("client does not support any available MFA methods [%v], see debug logs for details", strings.Join(availableMethods, ", ")) } } diff --git a/lib/client/mfa/cli_test.go b/lib/client/mfa/cli_test.go index 54e0fcfd92fd9..683932fb7c47b 100644 --- a/lib/client/mfa/cli_test.go +++ b/lib/client/mfa/cli_test.go @@ -43,6 +43,7 @@ func TestCLIPrompt(t *testing.T) { name string stdin string challenge *proto.MFAAuthenticateChallenge + modifyPromptConfig func(cfg *mfa.CLIPromptConfig) expectErr error expectStdOut string expectResp *proto.MFAAuthenticateResponse @@ -65,7 +66,7 @@ func TestCLIPrompt(t *testing.T) { }, }, }, { - name: "OK totp", + name: "OK otp", expectStdOut: "Enter an OTP code from a device:\n", stdin: "123456", challenge: &proto.MFAAuthenticateChallenge{ @@ -79,11 +80,86 @@ func TestCLIPrompt(t *testing.T) { }, }, }, { - name: "OK webauthn or totp choose webauthn", - expectStdOut: "Tap any security key or enter a code from a OTP device\n", + name: "OK sso", + expectStdOut: "", // sso stdout is handled internally in the SSO ceremony, which is mocked in this test. + challenge: &proto.MFAAuthenticateChallenge{ + SSOChallenge: &proto.SSOChallenge{}, + }, + expectResp: &proto.MFAAuthenticateResponse{ + Response: &proto.MFAAuthenticateResponse_SSO{ + SSO: &proto.SSOResponse{ + RequestId: "request-id", + Token: "mfa-token", + }, + }, + }, + }, { + name: "OK prefer otp when specified", + expectStdOut: "Enter an OTP code from a device:\n", + stdin: "123456", + challenge: &proto.MFAAuthenticateChallenge{ + WebauthnChallenge: &webauthnpb.CredentialAssertion{}, + TOTP: &proto.TOTPChallenge{}, + SSOChallenge: &proto.SSOChallenge{}, + }, + modifyPromptConfig: func(cfg *mfa.CLIPromptConfig) { + cfg.PreferOTP = true + }, + expectResp: &proto.MFAAuthenticateResponse{ + Response: &proto.MFAAuthenticateResponse_TOTP{ + TOTP: &proto.TOTPResponse{ + Code: "123456", + }, + }, + }, + }, { + name: "OK prefer sso when specified", + expectStdOut: "", + challenge: &proto.MFAAuthenticateChallenge{ + WebauthnChallenge: &webauthnpb.CredentialAssertion{}, + TOTP: &proto.TOTPChallenge{}, + SSOChallenge: &proto.SSOChallenge{}, + }, + modifyPromptConfig: func(cfg *mfa.CLIPromptConfig) { + cfg.PreferSSO = true + }, + expectResp: &proto.MFAAuthenticateResponse{ + Response: &proto.MFAAuthenticateResponse_SSO{ + SSO: &proto.SSOResponse{ + RequestId: "request-id", + Token: "mfa-token", + }, + }, + }, + }, { + name: "OK prefer webauthn with authenticator attachment requested", + expectStdOut: "" + + "Available MFA methods [WEBAUTHN, SSO, OTP]. Continuing with WEBAUTHN.\n" + + "If you wish to perform MFA with another method, specify with flag --mfa-mode=.\n\n" + + "Tap any security key\n", challenge: &proto.MFAAuthenticateChallenge{ WebauthnChallenge: &webauthnpb.CredentialAssertion{}, TOTP: &proto.TOTPChallenge{}, + SSOChallenge: &proto.SSOChallenge{}, + }, + modifyPromptConfig: func(cfg *mfa.CLIPromptConfig) { + cfg.AuthenticatorAttachment = wancli.AttachmentPlatform + }, + expectResp: &proto.MFAAuthenticateResponse{ + Response: &proto.MFAAuthenticateResponse_Webauthn{ + Webauthn: &webauthnpb.CredentialAssertionResponse{}, + }, + }, + }, + { + name: "OK prefer webauthn over sso", + expectStdOut: "" + + "Available MFA methods [WEBAUTHN, SSO]. Continuing with WEBAUTHN.\n" + + "If you wish to perform MFA with another method, specify with flag --mfa-mode=.\n\n" + + "Tap any security key\n", + challenge: &proto.MFAAuthenticateChallenge{ + WebauthnChallenge: &webauthnpb.CredentialAssertion{}, + SSOChallenge: &proto.SSOChallenge{}, }, expectResp: &proto.MFAAuthenticateResponse{ Response: &proto.MFAAuthenticateResponse_Webauthn{ @@ -91,13 +167,90 @@ func TestCLIPrompt(t *testing.T) { }, }, }, { - name: "OK webauthn or totp choose totp", - expectStdOut: "Tap any security key or enter a code from a OTP device\n", - stdin: "123456", + name: "OK prefer webauthn+otp over sso", + expectStdOut: "" + + "Available MFA methods [WEBAUTHN, SSO, OTP]. Continuing with WEBAUTHN and OTP.\n" + + "If you wish to perform MFA with another method, specify with flag --mfa-mode=.\n\n" + + "Tap any security key or enter a code from a OTP device\n", + challenge: &proto.MFAAuthenticateChallenge{ + WebauthnChallenge: &webauthnpb.CredentialAssertion{}, + TOTP: &proto.TOTPChallenge{}, + SSOChallenge: &proto.SSOChallenge{}, + }, + modifyPromptConfig: func(cfg *mfa.CLIPromptConfig) { + cfg.AllowStdinHijack = true + }, + expectResp: &proto.MFAAuthenticateResponse{ + Response: &proto.MFAAuthenticateResponse_Webauthn{ + Webauthn: &webauthnpb.CredentialAssertionResponse{}, + }, + }, + }, { + name: "OK prefer sso over otp", + expectStdOut: "" + + "Available MFA methods [SSO, OTP]. Continuing with SSO.\n" + + "If you wish to perform MFA with another method, specify with flag --mfa-mode=.\n\n", + challenge: &proto.MFAAuthenticateChallenge{ + TOTP: &proto.TOTPChallenge{}, + SSOChallenge: &proto.SSOChallenge{}, + }, + expectResp: &proto.MFAAuthenticateResponse{ + Response: &proto.MFAAuthenticateResponse_SSO{ + SSO: &proto.SSOResponse{ + RequestId: "request-id", + Token: "mfa-token", + }, + }, + }, + }, { + name: "OK prefer webauthn over otp when stdin hijack disallowed", + expectStdOut: "" + + "Available MFA methods [WEBAUTHN, OTP]. Continuing with WEBAUTHN.\n" + + "If you wish to perform MFA with another method, specify with flag --mfa-mode=.\n\n" + + "Tap any security key\n", challenge: &proto.MFAAuthenticateChallenge{ WebauthnChallenge: &webauthnpb.CredentialAssertion{}, TOTP: &proto.TOTPChallenge{}, }, + expectResp: &proto.MFAAuthenticateResponse{ + Response: &proto.MFAAuthenticateResponse_Webauthn{ + Webauthn: &webauthnpb.CredentialAssertionResponse{}, + }, + }, + }, { + name: "OK webauthn or otp with stdin hijack allowed, choose webauthn", + expectStdOut: "" + + "Available MFA methods [WEBAUTHN, SSO, OTP]. Continuing with WEBAUTHN and OTP.\n" + + "If you wish to perform MFA with another method, specify with flag --mfa-mode=.\n\n" + + "Tap any security key or enter a code from a OTP device\n", + challenge: &proto.MFAAuthenticateChallenge{ + WebauthnChallenge: &webauthnpb.CredentialAssertion{}, + TOTP: &proto.TOTPChallenge{}, + SSOChallenge: &proto.SSOChallenge{}, + }, + modifyPromptConfig: func(cfg *mfa.CLIPromptConfig) { + cfg.AllowStdinHijack = true + }, + expectResp: &proto.MFAAuthenticateResponse{ + Response: &proto.MFAAuthenticateResponse_Webauthn{ + Webauthn: &webauthnpb.CredentialAssertionResponse{}, + }, + }, + }, { + name: "OK webauthn or otp with stdin hijack allowed, choose otp", + expectStdOut: "" + + "Available MFA methods [WEBAUTHN, SSO, OTP]. Continuing with WEBAUTHN and OTP.\n" + + "If you wish to perform MFA with another method, specify with flag --mfa-mode=.\n\n" + + "Tap any security key or enter a code from a OTP device\n", + stdin: "123456", + challenge: &proto.MFAAuthenticateChallenge{ + WebauthnChallenge: &webauthnpb.CredentialAssertion{}, + TOTP: &proto.TOTPChallenge{}, + SSOChallenge: &proto.SSOChallenge{}, + }, + modifyPromptConfig: func(cfg *mfa.CLIPromptConfig) { + cfg.AllowStdinHijack = true + }, expectResp: &proto.MFAAuthenticateResponse{ Response: &proto.MFAAuthenticateResponse_TOTP{ TOTP: &proto.TOTPResponse{ @@ -113,19 +266,29 @@ func TestCLIPrompt(t *testing.T) { }, expectErr: context.DeadlineExceeded, }, { - name: "NOK no totp response", + name: "NOK no sso response", + expectStdOut: "", + challenge: &proto.MFAAuthenticateChallenge{ + SSOChallenge: &proto.SSOChallenge{}, + }, + expectErr: context.DeadlineExceeded, + }, { + name: "NOK no otp response", expectStdOut: "Enter an OTP code from a device:\n", challenge: &proto.MFAAuthenticateChallenge{ TOTP: &proto.TOTPChallenge{}, }, expectErr: context.DeadlineExceeded, }, { - name: "NOK no webauthn or totp response", + name: "NOK no webauthn or otp response", expectStdOut: "Tap any security key or enter a code from a OTP device\n", challenge: &proto.MFAAuthenticateChallenge{ WebauthnChallenge: &webauthnpb.CredentialAssertion{}, TOTP: &proto.TOTPChallenge{}, }, + modifyPromptConfig: func(cfg *mfa.CLIPromptConfig) { + cfg.AllowStdinHijack = true + }, expectErr: context.DeadlineExceeded, }, { @@ -134,6 +297,9 @@ func TestCLIPrompt(t *testing.T) { TOTP: &proto.TOTPChallenge{}, WebauthnChallenge: &webauthnpb.CredentialAssertion{}, }, + modifyPromptConfig: func(cfg *mfa.CLIPromptConfig) { + cfg.AllowStdinHijack = true + }, expectStdOut: `Tap any security key or enter a code from a OTP device Detected security key tap Enter your security key PIN: @@ -185,6 +351,9 @@ Enter your security key PIN: TOTP: nil, // no TOTP challenge WebauthnChallenge: &webauthnpb.CredentialAssertion{}, }, + modifyPromptConfig: func(cfg *mfa.CLIPromptConfig) { + cfg.AllowStdinHijack = true + }, stdin: "1234", expectStdOut: `Tap any security key Detected security key tap @@ -224,19 +393,27 @@ Enter your security key PIN: } }, }, + { + name: "NOK webauthn and SSO not supported", + challenge: &proto.MFAAuthenticateChallenge{ + SSOChallenge: &proto.SSOChallenge{}, + WebauthnChallenge: &webauthnpb.CredentialAssertion{}, + }, + modifyPromptConfig: func(cfg *mfa.CLIPromptConfig) { + cfg.WebauthnSupported = false + cfg.SSOMFACeremony = nil + }, + expectErr: trace.BadParameter("client does not support any available MFA methods [WEBAUTHN, SSO], see debug logs for details"), + }, } { t.Run(tc.name, func(t *testing.T) { ctx, cancel := context.WithTimeout(ctx, 100*time.Millisecond) defer cancel() - oldStdin := prompt.Stdin() - t.Cleanup(func() { prompt.SetStdin(oldStdin) }) - stdin := prompt.NewFakeReader() if tc.stdin != "" { stdin.AddString(tc.stdin) } - prompt.SetStdin(stdin) cfg := mfa.NewPromptConfig("proxy.example.com") cfg.WebauthnSupported = true @@ -257,16 +434,26 @@ Enter your security key PIN: } } + cfg.SSOMFACeremony = &mockSSOMFACeremony{ + mfaResp: tc.expectResp, + } + buffer := make([]byte, 0, 100) out := bytes.NewBuffer(buffer) - prompt := mfa.NewCLIPromptV2(&mfa.CLIPromptConfig{ - PromptConfig: *cfg, - Writer: out, - AllowStdinHijack: true, - }) - resp, err := prompt.Run(ctx, tc.challenge) + cliPromptConfig := &mfa.CLIPromptConfig{ + PromptConfig: *cfg, + Writer: out, + StdinFunc: func() prompt.StdinReader { + return stdin + }, + } + if tc.modifyPromptConfig != nil { + tc.modifyPromptConfig(cliPromptConfig) + } + + resp, err := mfa.NewCLIPromptV2(cliPromptConfig).Run(ctx, tc.challenge) if tc.expectErr != nil { require.ErrorIs(t, err, tc.expectErr) } else { @@ -278,3 +465,22 @@ Enter your security key PIN: }) } } + +type mockSSOMFACeremony struct { + mfaResp *proto.MFAAuthenticateResponse +} + +func (m *mockSSOMFACeremony) GetClientCallbackURL() string { + return "" +} + +// Run the SSO MFA ceremony. +func (m *mockSSOMFACeremony) Run(ctx context.Context, chal *proto.MFAAuthenticateChallenge) (*proto.MFAAuthenticateResponse, error) { + if m.mfaResp == nil { + return nil, context.DeadlineExceeded + } + if m.mfaResp.GetSSO() == nil { + return nil, trace.BadParameter("expected an SSO response but got %T", m.mfaResp.Response) + } + return m.mfaResp, nil +} diff --git a/lib/client/sso/ceremony_test.go b/lib/client/sso/ceremony_test.go index a2b15b532e54b..a914a8f76fe5a 100644 --- a/lib/client/sso/ceremony_test.go +++ b/lib/client/sso/ceremony_test.go @@ -28,8 +28,8 @@ import ( "testing" "github.com/gravitational/trace" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" - "gotest.tools/assert" "github.com/gravitational/teleport" "github.com/gravitational/teleport/api/client/proto" diff --git a/tool/tsh/common/tsh.go b/tool/tsh/common/tsh.go index 2164dea15d3ea..1adabe7b337c1 100644 --- a/tool/tsh/common/tsh.go +++ b/tool/tsh/common/tsh.go @@ -118,6 +118,8 @@ const ( mfaModePlatform = "platform" // mfaModeOTP utilizes only OTP devices. mfaModeOTP = "otp" + // mfaModeSSO utilizes only SSO devices. + mfaModeSSO = "sso" ) const ( @@ -766,7 +768,7 @@ func Run(ctx context.Context, args []string, opts ...CliOption) error { app.Flag("bind-addr", "Override host:port used when opening a browser for cluster logins").Envar(bindAddrEnvVar).StringVar(&cf.BindAddr) app.Flag("callback", "Override the base URL (host:port) of the link shown when opening a browser for cluster logins. Must be used with --bind-addr.").StringVar(&cf.CallbackAddr) app.Flag("browser-login", browserHelp).Hidden().Envar(browserEnvVar).StringVar(&cf.Browser) - modes := []string{mfaModeAuto, mfaModeCrossPlatform, mfaModePlatform, mfaModeOTP} + modes := []string{mfaModeAuto, mfaModeCrossPlatform, mfaModePlatform, mfaModeOTP, mfaModeSSO} app.Flag("mfa-mode", fmt.Sprintf("Preferred mode for MFA and Passwordless assertions (%v)", strings.Join(modes, ", "))). Default(mfaModeAuto). Envar(mfaModeEnvVar). @@ -4253,6 +4255,7 @@ func loadClientConfigFromCLIConf(cf *CLIConf, proxy string) (*client.Config, err } c.AuthenticatorAttachment = mfaOpts.AuthenticatorAttachment c.PreferOTP = mfaOpts.PreferOTP + c.PreferSSO = mfaOpts.PreferSSO // If agent forwarding was specified on the command line enable it. c.ForwardAgent = options.ForwardAgent @@ -4434,6 +4437,7 @@ func (c *CLIConf) GetProfile() (*profile.Profile, error) { type mfaModeOpts struct { AuthenticatorAttachment wancli.AuthenticatorAttachment PreferOTP bool + PreferSSO bool } func parseMFAMode(mode string) (*mfaModeOpts, error) { @@ -4446,6 +4450,8 @@ func parseMFAMode(mode string) (*mfaModeOpts, error) { opts.AuthenticatorAttachment = wancli.AttachmentPlatform case mfaModeOTP: opts.PreferOTP = true + case mfaModeSSO: + opts.PreferSSO = true default: return nil, fmt.Errorf("invalid MFA mode: %q", mode) } From bc9c9eb83141917c655a85cea542b4e72c5c98de Mon Sep 17 00:00:00 2001 From: joerger Date: Thu, 24 Oct 2024 17:25:17 -0700 Subject: [PATCH 08/12] Remove unused field, fix test. --- lib/client/api.go | 3 --- lib/client/cluster_client_test.go | 5 +++-- 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/lib/client/api.go b/lib/client/api.go index 73c9883a115cb..d087fb02d1e34 100644 --- a/lib/client/api.go +++ b/lib/client/api.go @@ -498,9 +498,6 @@ type Config struct { // If empty, a default CLI prompt is used. CustomHardwareKeyPrompt keys.HardwareKeyPrompt - // SSOCeremonyConstructor is a custom SSO ceremony constructor to use. - SSOCeremonyConstructor func() *sso.Ceremony - // DisableSSHResumption disables transparent SSH connection resumption. DisableSSHResumption bool diff --git a/lib/client/cluster_client_test.go b/lib/client/cluster_client_test.go index 70a9985853ceb..7a90be3f30d80 100644 --- a/lib/client/cluster_client_test.go +++ b/lib/client/cluster_client_test.go @@ -361,8 +361,9 @@ func TestIssueUserCertsWithMFA(t *testing.T) { tc: &TeleportClient{ localAgent: agent, Config: Config{ - SiteName: "test", - Tracer: tracing.NoopTracer("test"), + WebProxyAddr: "proxy.example.com", + SiteName: "test", + Tracer: tracing.NoopTracer("test"), MFAPromptConstructor: func(cfg *libmfa.PromptConfig) mfa.Prompt { return test.prompt }, From c2d89f0d0739fe8086863595768e19b65e890f87 Mon Sep 17 00:00:00 2001 From: joerger Date: Mon, 28 Oct 2024 10:41:29 -0700 Subject: [PATCH 09/12] Resolve comments. --- lib/client/mfa/cli.go | 9 +++++---- lib/client/mfa/cli_test.go | 10 ++-------- lib/client/sso/ceremony_test.go | 9 ++++----- 3 files changed, 11 insertions(+), 17 deletions(-) diff --git a/lib/client/mfa/cli.go b/lib/client/mfa/cli.go index 872c61fb5773d..400ad7d88e6c6 100644 --- a/lib/client/mfa/cli.go +++ b/lib/client/mfa/cli.go @@ -164,6 +164,10 @@ func (c *CLIPrompt) Run(ctx context.Context, chal *proto.MFAAuthenticateChalleng chosenMethods = []string{cliMFATypeOTP} promptWebauthn, promptSSO = false, false userSpecifiedMethod = true + case c.cfg.AuthenticatorAttachment != wancli.AttachmentAuto: + chosenMethods = []string{cliMFATypeWebauthn} + promptSSO, promptOTP = false, false + userSpecifiedMethod = true } // Use stronger auth methods if hijack is not allowed. @@ -176,10 +180,7 @@ func (c *CLIPrompt) Run(ctx context.Context, chal *proto.MFAAuthenticateChalleng case promptWebauthn: chosenMethods = []string{cliMFATypeWebauthn} promptSSO = false - - // If a specific webauthn attachment was requested, skip OTP. - // Otherwise, allow dual prompt with OTP. - promptOTP = promptOTP && c.cfg.AuthenticatorAttachment == wancli.AttachmentAuto + // Allow dual prompt with OTP. if promptOTP { chosenMethods = append(chosenMethods, cliMFATypeOTP) } diff --git a/lib/client/mfa/cli_test.go b/lib/client/mfa/cli_test.go index 683932fb7c47b..c0a81dfac97e6 100644 --- a/lib/client/mfa/cli_test.go +++ b/lib/client/mfa/cli_test.go @@ -132,11 +132,8 @@ func TestCLIPrompt(t *testing.T) { }, }, }, { - name: "OK prefer webauthn with authenticator attachment requested", - expectStdOut: "" + - "Available MFA methods [WEBAUTHN, SSO, OTP]. Continuing with WEBAUTHN.\n" + - "If you wish to perform MFA with another method, specify with flag --mfa-mode=.\n\n" + - "Tap any security key\n", + name: "OK prefer webauthn with authenticator attachment requested", + expectStdOut: "Tap any security key\n", challenge: &proto.MFAAuthenticateChallenge{ WebauthnChallenge: &webauthnpb.CredentialAssertion{}, TOTP: &proto.TOTPChallenge{}, @@ -351,9 +348,6 @@ Enter your security key PIN: TOTP: nil, // no TOTP challenge WebauthnChallenge: &webauthnpb.CredentialAssertion{}, }, - modifyPromptConfig: func(cfg *mfa.CLIPromptConfig) { - cfg.AllowStdinHijack = true - }, stdin: "1234", expectStdOut: `Tap any security key Detected security key tap diff --git a/lib/client/sso/ceremony_test.go b/lib/client/sso/ceremony_test.go index a914a8f76fe5a..87d01ee2fdba9 100644 --- a/lib/client/sso/ceremony_test.go +++ b/lib/client/sso/ceremony_test.go @@ -44,7 +44,7 @@ func TestCLICeremony(t *testing.T) { username := "alice" // Capture stderr. - stderr := bytes.NewBuffer([]byte{}) + stderr := &bytes.Buffer{} // Create a basic redirector. rd, err := sso.NewRedirector(sso.RedirectorConfig{ @@ -81,10 +81,9 @@ func TestCLICeremony(t *testing.T) { // Read the clickable url from stderr and navigate to it // using a simplified regexp for http://127.0.0.1:/ - clickableURLPattern := "http://127.0.0.1:.*/.*[0-9a-f]" - clickableURL := regexp.MustCompile(clickableURLPattern).Find(stderr.Bytes()) - - resp, err := http.Get(string(clickableURL)) + const clickableURLPattern = `^http://127.0.0.1:\d+/[0-9A-Fa-f-]+$` + clickableURL := regexp.MustCompile(clickableURLPattern).FindString(stderr.String()) + resp, err := http.Get(clickableURL) require.NoError(t, err) defer resp.Body.Close() From f121ef1dd0676a6e46efde7178dce5563612ad9c Mon Sep 17 00:00:00 2001 From: joerger Date: Mon, 28 Oct 2024 10:47:19 -0700 Subject: [PATCH 10/12] Remove convoluted context closing logic for sso redirector. --- api/mfa/ceremony.go | 9 ++++----- api/mfa/ceremony_test.go | 2 ++ lib/client/mfa.go | 1 - lib/client/mfa/cli_test.go | 2 ++ lib/client/sso/ceremony.go | 8 ++++++++ lib/client/sso/ceremony_test.go | 1 + tool/tctl/common/tctl.go | 1 - 7 files changed, 17 insertions(+), 7 deletions(-) diff --git a/api/mfa/ceremony.go b/api/mfa/ceremony.go index 1a8940ddc8089..3b28162e62164 100644 --- a/api/mfa/ceremony.go +++ b/api/mfa/ceremony.go @@ -33,8 +33,7 @@ type Ceremony struct { // PromptConstructor creates a prompt to prompt the user to solve an authentication challenge. PromptConstructor PromptConstructor // SSOMFACeremonyConstructor is an optional SSO MFA ceremony constructor. If provided, - // the MFA ceremony will also attempt to retrieve an SSO MFA challenge. The provided - // context will be closed once the ceremony is complete. + // the MFA ceremony will also attempt to retrieve an SSO MFA challenge. SSOMFACeremonyConstructor SSOMFACeremonyConstructor } @@ -42,6 +41,7 @@ type Ceremony struct { type SSOMFACeremony interface { GetClientCallbackURL() string Run(ctx context.Context, chal *proto.MFAAuthenticateChallenge) (*proto.MFAAuthenticateResponse, error) + Close() } // SSOMFACeremonyConstructor constructs a new SSO MFA ceremony. @@ -55,9 +55,6 @@ type CreateAuthenticateChallengeFunc func(ctx context.Context, req *proto.Create // req may be nil if ceremony.CreateAuthenticateChallenge does not require it, e.g. in // the moderated session mfa ceremony which uses a custom stream rpc to create challenges. func (c *Ceremony) Run(ctx context.Context, req *proto.CreateAuthenticateChallengeRequest, promptOpts ...PromptOpt) (*proto.MFAAuthenticateResponse, error) { - ctx, cancel := context.WithCancel(ctx) - defer cancel() - switch { case c.CreateAuthenticateChallenge == nil: return nil, trace.BadParameter("mfa ceremony must have CreateAuthenticateChallenge set in order to begin") @@ -77,6 +74,8 @@ func (c *Ceremony) Run(ctx context.Context, req *proto.CreateAuthenticateChallen if err != nil { return nil, trace.Wrap(err, "failed to handle SSO MFA ceremony") } + defer ssoMFACeremony.Close() + req.SSOClientRedirectURL = ssoMFACeremony.GetClientCallbackURL() promptOpts = append(promptOpts, withSSOMFACeremony(ssoMFACeremony)) } diff --git a/api/mfa/ceremony_test.go b/api/mfa/ceremony_test.go index a988778264d3e..d29b03a22487e 100644 --- a/api/mfa/ceremony_test.go +++ b/api/mfa/ceremony_test.go @@ -201,3 +201,5 @@ func (m *mockSSOMFACeremony) GetClientCallbackURL() string { func (m *mockSSOMFACeremony) Run(ctx context.Context, chal *proto.MFAAuthenticateChallenge) (*proto.MFAAuthenticateResponse, error) { return m.prompt(ctx, chal) } + +func (m *mockSSOMFACeremony) Close() {} diff --git a/lib/client/mfa.go b/lib/client/mfa.go index b5a7c8729141e..51f2073b71e45 100644 --- a/lib/client/mfa.go +++ b/lib/client/mfa.go @@ -98,6 +98,5 @@ func (tc *TeleportClient) NewSSOMFACeremony(ctx context.Context) (mfa.SSOMFACere return nil, trace.Wrap(err) } - context.AfterFunc(ctx, rd.Close) return sso.NewCLIMFACeremony(rd), nil } diff --git a/lib/client/mfa/cli_test.go b/lib/client/mfa/cli_test.go index c0a81dfac97e6..b9b69b7c16f2d 100644 --- a/lib/client/mfa/cli_test.go +++ b/lib/client/mfa/cli_test.go @@ -478,3 +478,5 @@ func (m *mockSSOMFACeremony) Run(ctx context.Context, chal *proto.MFAAuthenticat } return m.mfaResp, nil } + +func (m *mockSSOMFACeremony) Close() {} diff --git a/lib/client/sso/ceremony.go b/lib/client/sso/ceremony.go index 84dae6c14d6a8..b9643a52578bb 100644 --- a/lib/client/sso/ceremony.go +++ b/lib/client/sso/ceremony.go @@ -66,6 +66,7 @@ func NewCLICeremony(rd *Redirector, init CeremonyInit) *Ceremony { // Ceremony is a customizable SSO MFA ceremony. type MFACeremony struct { clientCallbackURL string + close func() HandleRedirect func(ctx context.Context, redirectURL string) error GetCallbackMFAToken func(ctx context.Context) (string, error) } @@ -96,10 +97,17 @@ func (m *MFACeremony) Run(ctx context.Context, chal *proto.MFAAuthenticateChalle }, nil } +func (m *MFACeremony) Close() { + if m.close != nil { + m.close() + } +} + // NewCLIMFACeremony creates a new CLI SSO ceremony from the given redirector. func NewCLIMFACeremony(rd *Redirector) *MFACeremony { return &MFACeremony{ clientCallbackURL: rd.ClientCallbackURL, + close: rd.Close, HandleRedirect: rd.OpenRedirect, GetCallbackMFAToken: func(ctx context.Context) (string, error) { loginResp, err := rd.WaitForResponse(ctx) diff --git a/lib/client/sso/ceremony_test.go b/lib/client/sso/ceremony_test.go index 87d01ee2fdba9..82969672eab21 100644 --- a/lib/client/sso/ceremony_test.go +++ b/lib/client/sso/ceremony_test.go @@ -132,6 +132,7 @@ func TestCLICeremony_MFA(t *testing.T) { t.Cleanup(mockIdPServer.Close) ceremony := sso.NewCLIMFACeremony(rd) + defer rd.Close() // Modify handle redirect to also browse to the clickable URL printed to stderr. baseHandleRedirect := ceremony.HandleRedirect diff --git a/tool/tctl/common/tctl.go b/tool/tctl/common/tctl.go index 3cf5b16345274..448a459df1653 100644 --- a/tool/tctl/common/tctl.go +++ b/tool/tctl/common/tctl.go @@ -268,7 +268,6 @@ func TryRun(commands []CLICommand, args []string) error { return nil, trace.Wrap(err) } - context.AfterFunc(ctx, rd.Close) return sso.NewCLIMFACeremony(rd), nil }) From 80edc5c69c27ff822f3a1d356ca266d123d56bce Mon Sep 17 00:00:00 2001 From: joerger Date: Mon, 28 Oct 2024 12:50:58 -0700 Subject: [PATCH 11/12] Fix test. --- lib/client/sso/ceremony_test.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/lib/client/sso/ceremony_test.go b/lib/client/sso/ceremony_test.go index 82969672eab21..c86c2f63cb598 100644 --- a/lib/client/sso/ceremony_test.go +++ b/lib/client/sso/ceremony_test.go @@ -81,7 +81,7 @@ func TestCLICeremony(t *testing.T) { // Read the clickable url from stderr and navigate to it // using a simplified regexp for http://127.0.0.1:/ - const clickableURLPattern = `^http://127.0.0.1:\d+/[0-9A-Fa-f-]+$` + const clickableURLPattern = `http://127.0.0.1:\d+/[0-9A-Fa-f-]+` clickableURL := regexp.MustCompile(clickableURLPattern).FindString(stderr.String()) resp, err := http.Get(clickableURL) require.NoError(t, err) @@ -132,7 +132,6 @@ func TestCLICeremony_MFA(t *testing.T) { t.Cleanup(mockIdPServer.Close) ceremony := sso.NewCLIMFACeremony(rd) - defer rd.Close() // Modify handle redirect to also browse to the clickable URL printed to stderr. baseHandleRedirect := ceremony.HandleRedirect From 90fc543aa890f829084d05d9af4bfef201263943 Mon Sep 17 00:00:00 2001 From: Brian Joerger Date: Tue, 29 Oct 2024 10:52:31 -0700 Subject: [PATCH 12/12] Update lib/client/sso/ceremony.go Co-authored-by: Alan Parra --- lib/client/sso/ceremony.go | 2 ++ lib/client/sso/ceremony_test.go | 2 +- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/lib/client/sso/ceremony.go b/lib/client/sso/ceremony.go index b9643a52578bb..8a2a64debfe49 100644 --- a/lib/client/sso/ceremony.go +++ b/lib/client/sso/ceremony.go @@ -97,6 +97,7 @@ func (m *MFACeremony) Run(ctx context.Context, chal *proto.MFAAuthenticateChalle }, nil } +// Close closes resources associated with the SSO MFA ceremony. func (m *MFACeremony) Close() { if m.close != nil { m.close() @@ -104,6 +105,7 @@ func (m *MFACeremony) Close() { } // NewCLIMFACeremony creates a new CLI SSO ceremony from the given redirector. +// The returned MFACeremony takes ownership of the Redirector. func NewCLIMFACeremony(rd *Redirector) *MFACeremony { return &MFACeremony{ clientCallbackURL: rd.ClientCallbackURL, diff --git a/lib/client/sso/ceremony_test.go b/lib/client/sso/ceremony_test.go index c86c2f63cb598..4ea904697c8aa 100644 --- a/lib/client/sso/ceremony_test.go +++ b/lib/client/sso/ceremony_test.go @@ -116,7 +116,6 @@ func TestCLICeremony_MFA(t *testing.T) { Stderr: stderr, }) require.NoError(t, err) - t.Cleanup(rd.Close) // Construct a fake mfa response with the redirector's client callback URL. successResponseURL, err := web.ConstructSSHResponse(web.AuthParams{ @@ -132,6 +131,7 @@ func TestCLICeremony_MFA(t *testing.T) { t.Cleanup(mockIdPServer.Close) ceremony := sso.NewCLIMFACeremony(rd) + t.Cleanup(ceremony.Close) // Modify handle redirect to also browse to the clickable URL printed to stderr. baseHandleRedirect := ceremony.HandleRedirect