From 45b5b04e83be7936e7a8eda9e46fed9a780d2588 Mon Sep 17 00:00:00 2001 From: joerger Date: Fri, 20 Sep 2024 13:39:34 -0700 Subject: [PATCH 01/11] Refactor MFA ceremony helpers. --- api/client/mfa.go | 20 ++------- api/mfa/ceremony.go | 53 ++++++++++++---------- api/mfa/ceremony_test.go | 10 ++++- api/utils/grpc/interceptors/mfa.go | 2 +- lib/auth/helpers_mfa.go | 22 ++++----- lib/client/api.go | 17 ++----- lib/client/cluster_client.go | 55 +++++++++++------------ lib/client/mfa.go | 22 +++++++++ lib/teleterm/clusters/cluster_headless.go | 15 +++---- lib/web/desktop.go | 2 +- lib/web/kube.go | 2 +- lib/web/terminal.go | 2 +- tool/tsh/common/mfa.go | 40 ++++++----------- 13 files changed, 126 insertions(+), 136 deletions(-) diff --git a/api/client/mfa.go b/api/client/mfa.go index 03cfc13b88a52..51340d036506f 100644 --- a/api/client/mfa.go +++ b/api/client/mfa.go @@ -19,8 +19,6 @@ package client import ( "context" - "github.com/gravitational/trace" - "github.com/gravitational/teleport/api/client/proto" "github.com/gravitational/teleport/api/mfa" ) @@ -29,19 +27,9 @@ import ( // and prompts the user to answer the challenge with the given promptOpts, and ultimately returning // an MFA challenge response for the user. func (c *Client) PerformMFACeremony(ctx context.Context, challengeRequest *proto.CreateAuthenticateChallengeRequest, promptOpts ...mfa.PromptOpt) (*proto.MFAAuthenticateResponse, error) { - // Don't attempt the MFA ceremony if we can't prompt for a response. - if c.c.MFAPromptConstructor == nil { - return nil, trace.Wrap(&mfa.ErrMFANotSupported, "missing MFAPromptConstructor field, client cannot perform MFA ceremony") - } - - return mfa.PerformMFACeremony(ctx, c, challengeRequest, promptOpts...) -} - -// PromptMFA prompts the user for MFA. Implements [mfa.MFACeremonyClient]. -func (c *Client) PromptMFA(ctx context.Context, chal *proto.MFAAuthenticateChallenge, promptOpts ...mfa.PromptOpt) (*proto.MFAAuthenticateResponse, error) { - if c.c.MFAPromptConstructor == nil { - return nil, trace.Wrap(&mfa.ErrMFANotSupported, "missing MFAPromptConstructor field, client cannot prompt for MFA") + mfaCeremony := &mfa.Ceremony{ + CreateAuthenticateChallenge: c.CreateAuthenticateChallenge, + NewPrompt: c.c.MFAPromptConstructor, } - - return c.c.MFAPromptConstructor(promptOpts...).Run(ctx, chal) + return mfaCeremony.Run(ctx, challengeRequest, promptOpts...) } diff --git a/api/mfa/ceremony.go b/api/mfa/ceremony.go index 09fd11c910271..95be7bdc67f67 100644 --- a/api/mfa/ceremony.go +++ b/api/mfa/ceremony.go @@ -25,32 +25,34 @@ import ( mfav1 "github.com/gravitational/teleport/api/gen/proto/go/teleport/mfa/v1" ) -// MFACeremonyClient is a client that can perform an MFA ceremony, from retrieving -// the MFA challenge to prompting for an MFA response from the user. -type MFACeremonyClient interface { - // CreateAuthenticateChallenge creates and returns MFA challenges for a users registered MFA devices. - CreateAuthenticateChallenge(ctx context.Context, in *proto.CreateAuthenticateChallengeRequest) (*proto.MFAAuthenticateChallenge, error) - // PromptMFA prompts the user for MFA. - PromptMFA(ctx context.Context, chal *proto.MFAAuthenticateChallenge, promptOpts ...PromptOpt) (*proto.MFAAuthenticateResponse, error) +// Ceremony is an MFA ceremony. +type Ceremony struct { + CreateAuthenticateChallenge func(ctx context.Context, in *proto.CreateAuthenticateChallengeRequest) (*proto.MFAAuthenticateChallenge, error) + SolveAuthenticateChallenge func(ctx context.Context, in *proto.MFAAuthenticateChallenge) (*proto.MFAAuthenticateResponse, error) + NewPrompt PromptConstructor } -// PerformMFACeremony retrieves an MFA challenge from the server with the given challenge extensions -// and prompts the user to answer the challenge with the given promptOpts, and ultimately returning -// an MFA challenge response for the user. -func PerformMFACeremony(ctx context.Context, clt MFACeremonyClient, challengeRequest *proto.CreateAuthenticateChallengeRequest, promptOpts ...PromptOpt) (*proto.MFAAuthenticateResponse, error) { - if challengeRequest == nil { - return nil, trace.BadParameter("missing challenge request") +// Run runs the MFA ceremony. +func (c *Ceremony) Run(ctx context.Context, req *proto.CreateAuthenticateChallengeRequest, promptOpts ...PromptOpt) (*proto.MFAAuthenticateResponse, error) { + if c.CreateAuthenticateChallenge == nil { + return nil, trace.BadParameter("mfa ceremony must have CreateAuthenticateChallenge set in order to begin") } - if challengeRequest.ChallengeExtensions == nil { - return nil, trace.BadParameter("missing challenge extensions") + if c.SolveAuthenticateChallenge == nil && c.NewPrompt == nil { + return nil, trace.BadParameter("mfa ceremony must have SolveAuthenticateChallenge or NewPrompt set in order to succeed") } - if challengeRequest.ChallengeExtensions.Scope == mfav1.ChallengeScope_CHALLENGE_SCOPE_UNSPECIFIED { - return nil, trace.BadParameter("mfa challenge scope must be specified") + if req != nil { + if req.ChallengeExtensions == nil { + return nil, trace.BadParameter("missing challenge extensions") + } + + if req.ChallengeExtensions.Scope == mfav1.ChallengeScope_CHALLENGE_SCOPE_UNSPECIFIED { + return nil, trace.BadParameter("mfa challenge scope must be specified") + } } - chal, err := clt.CreateAuthenticateChallenge(ctx, challengeRequest) + chal, err := c.CreateAuthenticateChallenge(ctx, req) if err != nil { // CreateAuthenticateChallenge returns a bad parameter error when the client // user is not a Teleport user - for example, the AdminRole. Treat this as an MFA @@ -67,21 +69,25 @@ func PerformMFACeremony(ctx context.Context, clt MFACeremonyClient, challengeReq return nil, &ErrMFANotRequired } - return clt.PromptMFA(ctx, chal, promptOpts...) + if c.SolveAuthenticateChallenge != nil { + return c.SolveAuthenticateChallenge(ctx, chal) + } + + return c.NewPrompt(promptOpts...).Run(ctx, chal) } -type MFACeremony func(ctx context.Context, challengeRequest *proto.CreateAuthenticateChallengeRequest, promptOpts ...PromptOpt) (*proto.MFAAuthenticateResponse, error) +// CeremonyFn is a function that will carry out an MFA ceremony. +type CeremonyFn func(ctx context.Context, in *proto.CreateAuthenticateChallengeRequest, promptOpts ...PromptOpt) (*proto.MFAAuthenticateResponse, error) // PerformAdminActionMFACeremony retrieves an MFA challenge from the server for an admin // action, prompts the user to answer the challenge, and returns the resulting MFA response. -func PerformAdminActionMFACeremony(ctx context.Context, mfaCeremony MFACeremony, allowReuse bool) (*proto.MFAAuthenticateResponse, error) { +func PerformAdminActionMFACeremony(ctx context.Context, mfaCeremony CeremonyFn, allowReuse bool) (*proto.MFAAuthenticateResponse, error) { allowReuseExt := mfav1.ChallengeAllowReuse_CHALLENGE_ALLOW_REUSE_NO if allowReuse { allowReuseExt = mfav1.ChallengeAllowReuse_CHALLENGE_ALLOW_REUSE_YES } challengeRequest := &proto.CreateAuthenticateChallengeRequest{ - Request: &proto.CreateAuthenticateChallengeRequest_ContextUser{}, MFARequiredCheck: &proto.IsMFARequiredRequest{ Target: &proto.IsMFARequiredRequest_AdminAction{ AdminAction: &proto.AdminAction{}, @@ -93,5 +99,6 @@ func PerformAdminActionMFACeremony(ctx context.Context, mfaCeremony MFACeremony, }, } - return mfaCeremony(ctx, challengeRequest, WithPromptReasonAdminAction()) + resp, err := mfaCeremony(ctx, challengeRequest, WithPromptReasonAdminAction()) + return resp, trace.Wrap(err) } diff --git a/api/mfa/ceremony_test.go b/api/mfa/ceremony_test.go index 5e9df622534b7..32f6e7c5685dd 100644 --- a/api/mfa/ceremony_test.go +++ b/api/mfa/ceremony_test.go @@ -87,7 +87,13 @@ func TestPerformMFACeremony(t *testing.T) { }, } { t.Run(tt.name, func(t *testing.T) { - resp, err := mfa.PerformMFACeremony(ctx, tt.ceremonyClient, &proto.CreateAuthenticateChallengeRequest{ + ceremony := mfa.Ceremony{ + CreateAuthenticateChallenge: tt.ceremonyClient.CreateAuthenticateChallenge, + NewPrompt: func(po ...mfa.PromptOpt) mfa.Prompt { + return mfa.PromptFunc(tt.ceremonyClient.PromptMFA) + }, + } + resp, err := ceremony.Run(ctx, &proto.CreateAuthenticateChallengeRequest{ ChallengeExtensions: &mfav1.ChallengeExtensions{ Scope: mfav1.ChallengeScope_CHALLENGE_SCOPE_ADMIN_ACTION, }, @@ -121,7 +127,7 @@ func (c *fakeMFACeremonyClient) CreateAuthenticateChallenge(ctx context.Context, return chal, nil } -func (c *fakeMFACeremonyClient) PromptMFA(ctx context.Context, chal *proto.MFAAuthenticateChallenge, promptOpts ...mfa.PromptOpt) (*proto.MFAAuthenticateResponse, error) { +func (c *fakeMFACeremonyClient) PromptMFA(ctx context.Context, chal *proto.MFAAuthenticateChallenge) (*proto.MFAAuthenticateResponse, error) { if c.promptMFAErr != nil { return nil, c.promptMFAErr } diff --git a/api/utils/grpc/interceptors/mfa.go b/api/utils/grpc/interceptors/mfa.go index 367851fef1cc7..e8dae8e45e1f7 100644 --- a/api/utils/grpc/interceptors/mfa.go +++ b/api/utils/grpc/interceptors/mfa.go @@ -31,7 +31,7 @@ import ( // to the rpc call when an MFA response is provided through the context. Additionally, // when the call returns an error that indicates that MFA is required, this interceptor // will prompt for MFA using the given mfaCeremony and retry. -func WithMFAUnaryInterceptor(mfaCeremony mfa.MFACeremony) grpc.UnaryClientInterceptor { +func WithMFAUnaryInterceptor(mfaCeremony mfa.CeremonyFn) grpc.UnaryClientInterceptor { return func(ctx context.Context, method string, req, reply interface{}, cc *grpc.ClientConn, invoker grpc.UnaryInvoker, opts ...grpc.CallOption) error { // Check for MFA response passed through the context. if mfaResp, err := mfa.MFAResponseFromContext(ctx); err == nil { diff --git a/lib/auth/helpers_mfa.go b/lib/auth/helpers_mfa.go index 853c3ae8e5e15..1c61788603061 100644 --- a/lib/auth/helpers_mfa.go +++ b/lib/auth/helpers_mfa.go @@ -29,6 +29,7 @@ import ( "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" "github.com/gravitational/teleport/api/types" "github.com/gravitational/teleport/lib/auth/mocku2f" wantypes "github.com/gravitational/teleport/lib/auth/webauthntypes" @@ -101,24 +102,19 @@ type authClientI interface { AddMFADeviceSync(context.Context, *proto.AddMFADeviceSyncRequest) (*proto.AddMFADeviceSyncResponse, error) } -func (d *TestDevice) registerDevice( - ctx context.Context, authClient authClientI, devName string, devType proto.DeviceType, authenticator *TestDevice) error { - // Re-authenticate using MFA. - authnChal, err := authClient.CreateAuthenticateChallenge(ctx, &proto.CreateAuthenticateChallengeRequest{ - Request: &proto.CreateAuthenticateChallengeRequest_ContextUser{ - ContextUser: &proto.ContextUser{}, +func (d *TestDevice) registerDevice(ctx context.Context, authClient authClientI, devName string, devType proto.DeviceType, authenticator *TestDevice) error { + mfaCeremony := &mfa.Ceremony{ + CreateAuthenticateChallenge: authClient.CreateAuthenticateChallenge, + SolveAuthenticateChallenge: func(_ context.Context, chal *proto.MFAAuthenticateChallenge) (*proto.MFAAuthenticateResponse, error) { + return authenticator.SolveAuthn(chal) }, + } + + authnSolved, err := mfaCeremony.Run(ctx, &proto.CreateAuthenticateChallengeRequest{ ChallengeExtensions: &mfav1.ChallengeExtensions{ Scope: mfav1.ChallengeScope_CHALLENGE_SCOPE_MANAGE_DEVICES, }, }) - if err != nil { - return trace.Wrap(err) - } - authnSolved, err := authenticator.SolveAuthn(authnChal) - if err != nil { - return trace.Wrap(err) - } // Acquire and solve registration challenge. usage := proto.DeviceUsage_DEVICE_USAGE_MFA diff --git a/lib/client/api.go b/lib/client/api.go index 2847f53938741..b304fe2a44660 100644 --- a/lib/client/api.go +++ b/lib/client/api.go @@ -5184,23 +5184,14 @@ func (tc *TeleportClient) HeadlessApprove(ctx context.Context, headlessAuthentic } } - chal, err := rootClient.CreateAuthenticateChallenge(ctx, &proto.CreateAuthenticateChallengeRequest{ - Request: &proto.CreateAuthenticateChallengeRequest_ContextUser{ - ContextUser: &proto.ContextUser{}, - }, + mfaCeremony := tc.NewMFACeremony() + mfaCeremony.CreateAuthenticateChallenge = rootClient.CreateAuthenticateChallenge + mfaResp, err := mfaCeremony.Run(ctx, &proto.CreateAuthenticateChallengeRequest{ ChallengeExtensions: &mfav1.ChallengeExtensions{ Scope: mfav1.ChallengeScope_CHALLENGE_SCOPE_HEADLESS_LOGIN, }, }) - if err != nil { - return trace.Wrap(err) - } - - resp, err := tc.PromptMFA(ctx, chal) - if err != nil { - return trace.Wrap(err) - } - err = rootClient.UpdateHeadlessAuthenticationState(ctx, headlessAuthenticationID, types.HeadlessAuthenticationState_HEADLESS_AUTHENTICATION_STATE_APPROVED, resp) + err = rootClient.UpdateHeadlessAuthenticationState(ctx, headlessAuthenticationID, types.HeadlessAuthenticationState_HEADLESS_AUTHENTICATION_STATE_APPROVED, mfaResp) return trace.Wrap(err) } diff --git a/lib/client/cluster_client.go b/lib/client/cluster_client.go index 8783bb3b5168c..b1aa1d7ef52b1 100644 --- a/lib/client/cluster_client.go +++ b/lib/client/cluster_client.go @@ -20,6 +20,7 @@ package client import ( "context" + "errors" "net" "time" @@ -311,7 +312,7 @@ func (c *ClusterClient) SessionSSHConfig(ctx context.Context, user string, targe } log.Debug("Attempting to issue a single-use user certificate with an MFA check.") - keyRing, err = c.performMFACeremony(ctx, + keyRing, err = c.performSessionMFACeremony(ctx, mfaClt, ReissueParams{ NodeName: nodeName(TargetNode{Addr: target.Addr}), @@ -427,9 +428,9 @@ func (c *ClusterClient) prepareUserCertsRequest(ctx context.Context, params Reis }, nil } -// performMFACeremony runs the mfa ceremony to completion. +// performSessionMFACeremony runs the mfa ceremony to completion. // If successful the returned [KeyRing] will be authorized to connect to the target. -func (c *ClusterClient) performMFACeremony(ctx context.Context, rootClient *ClusterClient, params ReissueParams, keyRing *KeyRing, mfaPrompt mfa.Prompt) (*KeyRing, error) { +func (c *ClusterClient) performSessionMFACeremony(ctx context.Context, rootClient *ClusterClient, params ReissueParams, keyRing *KeyRing, mfaPrompt mfa.Prompt) (*KeyRing, error) { newUserKeys, certsReq, err := rootClient.prepareUserCertsRequest(ctx, params, keyRing) if err != nil { return nil, trace.Wrap(err) @@ -439,7 +440,7 @@ func (c *ClusterClient) performMFACeremony(ctx context.Context, rootClient *Clus if err != nil { return nil, trace.Wrap(err) } - keyRing, _, err = PerformMFACeremony(ctx, PerformMFACeremonyParams{ + keyRing, _, err = PerformSessionMFACeremony(ctx, PerformMFACeremonyParams{ CurrentAuthClient: c.AuthClient, RootAuthClient: rootClient.AuthClient, MFAPrompt: mfaPrompt, @@ -557,7 +558,7 @@ func (c *ClusterClient) IssueUserCertsWithMFA(ctx context.Context, params Reissu } // Perform the MFA ceremony and add the new credential to the KeyRing. - keyRing, err := c.performMFACeremony(ctx, certClient, params, keyRing, mfaPrompt) + keyRing, err := c.performSessionMFACeremony(ctx, certClient, params, keyRing, mfaPrompt) if err != nil { return nil, proto.MFARequired_MFA_REQUIRED_YES, trace.Wrap(err) } @@ -567,19 +568,19 @@ func (c *ClusterClient) IssueUserCertsWithMFA(ctx context.Context, params Reissu } // PerformMFARootClient is a subset of Auth methods required for MFA. -// Used by [PerformMFACeremony]. +// Used by [PerformSessionMFACeremony]. type PerformMFARootClient interface { CreateAuthenticateChallenge(ctx context.Context, req *proto.CreateAuthenticateChallengeRequest) (*proto.MFAAuthenticateChallenge, error) GenerateUserCerts(ctx context.Context, req proto.UserCertsRequest) (*proto.Certs, error) } // PerformMFACurrentClient is a subset of Auth methods required for MFA. -// Used by [PerformMFACeremony]. +// Used by [PerformSessionMFACeremony]. type PerformMFACurrentClient interface { IsMFARequired(ctx context.Context, req *proto.IsMFARequiredRequest) (*proto.IsMFARequiredResponse, error) } -// PerformMFACeremonyParams are the input parameters for [PerformMFACeremony]. +// PerformMFACeremonyParams are the input parameters for [PerformSessionMFACeremony]. type PerformMFACeremonyParams struct { // CurrentAuthClient is the Auth client for the target cluster. // Unused if MFAAgainstRoot is true. @@ -615,7 +616,7 @@ type newUserKeys struct { ssh, tls, app, db, kube *keys.PrivateKey } -// PerformMFACeremony issues single-use certificates via GenerateUserCerts, +// PerformSessionMFACeremony issues single-use certificates via GenerateUserCerts, // following its recommended RPC flow. // // It is a lower-level, less opinionated variant of @@ -631,7 +632,7 @@ type newUserKeys struct { // 4. Call RootAuthClient.GenerateUserCerts // // Returns the modified params.Key and the GenerateUserCertsResponse, or an error. -func PerformMFACeremony(ctx context.Context, params PerformMFACeremonyParams) (*KeyRing, *proto.Certs, error) { +func PerformSessionMFACeremony(ctx context.Context, params PerformMFACeremonyParams) (*KeyRing, *proto.Certs, error) { rootClient := params.RootAuthClient currentClient := params.CurrentAuthClient @@ -652,8 +653,12 @@ func PerformMFACeremony(ctx context.Context, params PerformMFACeremonyParams) (* mfaRequiredReq = nil // Already checked, don't check again at root. } - // Acquire MFA challenge. - authnChal, err := rootClient.CreateAuthenticateChallenge(ctx, &proto.CreateAuthenticateChallengeRequest{ + mfaCeremony := mfa.Ceremony{ + CreateAuthenticateChallenge: rootClient.CreateAuthenticateChallenge, + SolveAuthenticateChallenge: params.MFAPrompt.Run, + } + + mfaResp, err := mfaCeremony.Run(ctx, &proto.CreateAuthenticateChallengeRequest{ Request: &proto.CreateAuthenticateChallengeRequest_ContextUser{ ContextUser: &proto.ContextUser{}, }, @@ -662,31 +667,23 @@ func PerformMFACeremony(ctx context.Context, params PerformMFACeremonyParams) (* Scope: mfav1.ChallengeScope_CHALLENGE_SCOPE_USER_SESSION, }, }) - if err != nil { - return nil, nil, trace.Wrap(err) - } - - log.Debugf("MFA requirement from CreateAuthenticateChallenge, MFARequired=%s", authnChal.GetMFARequired()) - if authnChal.MFARequired == proto.MFARequired_MFA_REQUIRED_NO { + if errors.Is(err, &mfa.ErrMFANotRequired) { return nil, nil, trace.Wrap(services.ErrSessionMFANotRequired) + } else if err != nil { + return nil, nil, trace.Wrap(err) } - if authnChal.TOTP == nil && authnChal.WebauthnChallenge == nil { - // TODO(Joerger): CreateAuthenticateChallenge should return - // this error directly instead of an empty challenge, without - // regressing https://github.com/gravitational/teleport/issues/36482. + // If mfaResp is nil, the ceremony was a no-op (no devices registered). + // TODO(Joerger): CreateAuthenticateChallenge, should return + // this error directly instead of an empty challenge, without + // regressing https://github.com/gravitational/teleport/issues/36482. + if mfaResp == nil { return nil, nil, trace.Wrap(authclient.ErrNoMFADevices) } - // Prompt user for solution (eg, security key touch). - authnSolved, err := params.MFAPrompt.Run(ctx, authnChal) - if err != nil { - return nil, nil, trace.Wrap(ceremonyFailedErr{err}) - } - // Issue certificate. certsReq := params.CertsReq - certsReq.MFAResponse = authnSolved + certsReq.MFAResponse = mfaResp certsReq.Purpose = proto.UserCertsRequest_CERT_PURPOSE_SINGLE_USE_CERTS log.Debug("Issuing single-use certificate from unary GenerateUserCerts") newCerts, err := rootClient.GenerateUserCerts(ctx, *certsReq) diff --git a/lib/client/mfa.go b/lib/client/mfa.go index dbb1520a0fd64..7fd11b578d250 100644 --- a/lib/client/mfa.go +++ b/lib/client/mfa.go @@ -26,8 +26,30 @@ import ( wancli "github.com/gravitational/teleport/lib/auth/webauthncli" wantypes "github.com/gravitational/teleport/lib/auth/webauthntypes" libmfa "github.com/gravitational/teleport/lib/client/mfa" + "github.com/gravitational/trace" ) +// NewMFACeremony returns a new MFA ceremony configured for this client. +func (tc *TeleportClient) NewMFACeremony() *mfa.Ceremony { + return &mfa.Ceremony{ + CreateAuthenticateChallenge: tc.CreateAuthenticateChallenge, + NewPrompt: tc.NewMFAPrompt, + } +} + +// CreateAuthenticateChallenge creates and returns MFA challenges for a users registered MFA devices. +func (tc *TeleportClient) CreateAuthenticateChallenge(ctx context.Context, req *proto.CreateAuthenticateChallengeRequest) (*proto.MFAAuthenticateChallenge, error) { + clusterClient, err := tc.ConnectToCluster(ctx) + if err != nil { + return nil, trace.Wrap(err) + } + rootClient, err := clusterClient.ConnectToRootCluster(ctx) + if err != nil { + return nil, trace.Wrap(err) + } + return rootClient.CreateAuthenticateChallenge(ctx, req) +} + // WebauthnLoginFunc matches the signature of [wancli.Login]. type WebauthnLoginFunc func(ctx context.Context, origin string, assertion *wantypes.CredentialAssertion, prompt wancli.LoginPrompt, opts *wancli.LoginOpts) (*proto.MFAAuthenticateResponse, string, error) diff --git a/lib/teleterm/clusters/cluster_headless.go b/lib/teleterm/clusters/cluster_headless.go index 3313616551f49..29f835c3bd029 100644 --- a/lib/teleterm/clusters/cluster_headless.go +++ b/lib/teleterm/clusters/cluster_headless.go @@ -72,11 +72,11 @@ func (c *Cluster) UpdateHeadlessAuthenticationState(ctx context.Context, rootAut err := AddMetadataToRetryableError(ctx, func() error { // If changing state to approved, create an MFA challenge and prompt for MFA. var mfaResponse *proto.MFAAuthenticateResponse + var err error if state == types.HeadlessAuthenticationState_HEADLESS_AUTHENTICATION_STATE_APPROVED { - chall, err := rootAuthClient.CreateAuthenticateChallenge(ctx, &proto.CreateAuthenticateChallengeRequest{ - Request: &proto.CreateAuthenticateChallengeRequest_ContextUser{ - ContextUser: &proto.ContextUser{}, - }, + mfaCeremony := c.clusterClient.NewMFACeremony() + mfaCeremony.CreateAuthenticateChallenge = rootAuthClient.CreateAuthenticateChallenge + mfaResponse, err = mfaCeremony.Run(ctx, &proto.CreateAuthenticateChallengeRequest{ ChallengeExtensions: &mfav1.ChallengeExtensions{ Scope: mfav1.ChallengeScope_CHALLENGE_SCOPE_HEADLESS_LOGIN, }, @@ -84,14 +84,9 @@ func (c *Cluster) UpdateHeadlessAuthenticationState(ctx context.Context, rootAut if err != nil { return trace.Wrap(err) } - - mfaResponse, err = c.clusterClient.PromptMFA(ctx, chall) - if err != nil { - return trace.Wrap(err) - } } - err := rootAuthClient.UpdateHeadlessAuthenticationState(ctx, headlessID, state, mfaResponse) + err = rootAuthClient.UpdateHeadlessAuthenticationState(ctx, headlessID, state, mfaResponse) return trace.Wrap(err) }) return trace.Wrap(err) diff --git a/lib/web/desktop.go b/lib/web/desktop.go index 8b138aa063245..076e680fc7f4c 100644 --- a/lib/web/desktop.go +++ b/lib/web/desktop.go @@ -429,7 +429,7 @@ func (h *Handler) performMFACeremony( return assertion, nil }) - _, newCerts, err := client.PerformMFACeremony(ctx, client.PerformMFACeremonyParams{ + _, newCerts, err := client.PerformSessionMFACeremony(ctx, client.PerformMFACeremonyParams{ CurrentAuthClient: nil, // Only RootAuthClient is used. RootAuthClient: sctx.cfg.RootClient, MFAPrompt: promptMFA, diff --git a/lib/web/kube.go b/lib/web/kube.go index 16deeac8c4109..1811f644ef32e 100644 --- a/lib/web/kube.go +++ b/lib/web/kube.go @@ -241,7 +241,7 @@ func (p *podHandler) handler(r *http.Request) error { Usage: clientproto.UserCertsRequest_Kubernetes, } - _, certs, err := client.PerformMFACeremony(ctx, client.PerformMFACeremonyParams{ + _, certs, err := client.PerformSessionMFACeremony(ctx, client.PerformMFACeremonyParams{ CurrentAuthClient: p.userClient, RootAuthClient: p.sctx.cfg.RootClient, MFAPrompt: mfa.PromptFunc(func(ctx context.Context, chal *clientproto.MFAAuthenticateChallenge) (*clientproto.MFAAuthenticateResponse, error) { diff --git a/lib/web/terminal.go b/lib/web/terminal.go index 6c99ce864b52e..1ddca58c68576 100644 --- a/lib/web/terminal.go +++ b/lib/web/terminal.go @@ -586,7 +586,7 @@ func (t *sshBaseHandler) issueSessionMFACerts(ctx context.Context, tc *client.Te SSHLogin: tc.HostLogin, } - _, certs, err := client.PerformMFACeremony(ctx, client.PerformMFACeremonyParams{ + _, certs, err := client.PerformSessionMFACeremony(ctx, client.PerformMFACeremonyParams{ CurrentAuthClient: t.userAuthClient, RootAuthClient: t.ctx.cfg.RootClient, MFAPrompt: mfa.PromptFunc(func(ctx context.Context, chal *authproto.MFAAuthenticateChallenge) (*authproto.MFAAuthenticateResponse, error) { diff --git a/tool/tsh/common/mfa.go b/tool/tsh/common/mfa.go index 73ae0b8024a6e..68fdfb8b7dde6 100644 --- a/tool/tsh/common/mfa.go +++ b/tool/tsh/common/mfa.go @@ -36,7 +36,6 @@ import ( "github.com/gravitational/teleport/api/client/proto" "github.com/gravitational/teleport/api/constants" mfav1 "github.com/gravitational/teleport/api/gen/proto/go/teleport/mfa/v1" - "github.com/gravitational/teleport/api/mfa" "github.com/gravitational/teleport/api/types" "github.com/gravitational/teleport/api/utils/prompt" "github.com/gravitational/teleport/lib/asciitable" @@ -333,36 +332,31 @@ func (c *mfaAddCommand) addDeviceRPC(ctx context.Context, tc *client.TeleportCli usage = proto.DeviceUsage_DEVICE_USAGE_PASSWORDLESS } - // Issue the authn challenge. - // Required for the registration challenge. - authChallenge, err := rootAuthClient.CreateAuthenticateChallenge(ctx, &proto.CreateAuthenticateChallengeRequest{ - ChallengeExtensions: &mfav1.ChallengeExtensions{ - Scope: mfav1.ChallengeScope_CHALLENGE_SCOPE_MANAGE_DEVICES, - }, - }) - if err != nil { - return trace.Wrap(err) - } - // Tweak Windows platform messages so it's clear we whether we are prompting // for the *registered* or *new* device. // We do it here, preemptively, because it's the simpler solution (instead // of finding out whether it is a Windows prompt or not). + // + // TODO(Joerger): this should live in lib/client/mfa/cli.go using the prompt device prefix. const registeredMsg = "Using platform authentication for *registered* device, follow the OS dialogs" const newMsg = "Using platform authentication for *new* device, follow the OS dialogs" defer wanwin.ResetPromptPlatformMessage() wanwin.PromptPlatformMessage = registeredMsg - // Prompt for authentication. - // Does nothing if no challenges were issued (aka user has no devices). - authnResp, err := tc.NewMFAPrompt(mfa.WithPromptDeviceType(mfa.DeviceDescriptorRegistered)).Run(ctx, authChallenge) + mfaCeremony := tc.NewMFACeremony() + mfaCeremony.CreateAuthenticateChallenge = rootAuthClient.CreateAuthenticateChallenge + mfaResp, err := mfaCeremony.Run(ctx, &proto.CreateAuthenticateChallengeRequest{ + ChallengeExtensions: &mfav1.ChallengeExtensions{ + Scope: mfav1.ChallengeScope_CHALLENGE_SCOPE_MANAGE_DEVICES, + }, + }) if err != nil { return trace.Wrap(err) } // Issue the registration challenge. registerChallenge, err := rootAuthClient.CreateRegisterChallenge(ctx, &proto.CreateRegisterChallengeRequest{ - ExistingMFAResponse: authnResp, + ExistingMFAResponse: mfaResp, DeviceType: devTypePB, DeviceUsage: usage, }) @@ -596,11 +590,9 @@ func (c *mfaRemoveCommand) run(cf *CLIConf) error { return trace.NotFound("device %q not found", c.name) } - // Issue and solve authn challenge. - authnChal, err := rootAuthClient.CreateAuthenticateChallenge(ctx, &proto.CreateAuthenticateChallengeRequest{ - Request: &proto.CreateAuthenticateChallengeRequest_ContextUser{ - ContextUser: &proto.ContextUser{}, - }, + mfaCeremony := tc.NewMFACeremony() + mfaCeremony.CreateAuthenticateChallenge = rootAuthClient.CreateAuthenticateChallenge + mfaResponse, err := mfaCeremony.Run(ctx, &proto.CreateAuthenticateChallengeRequest{ ChallengeExtensions: &mfav1.ChallengeExtensions{ Scope: mfav1.ChallengeScope_CHALLENGE_SCOPE_MANAGE_DEVICES, }, @@ -608,15 +600,11 @@ func (c *mfaRemoveCommand) run(cf *CLIConf) error { if err != nil { return trace.Wrap(err) } - authnSolved, err := tc.PromptMFA(ctx, authnChal) - if err != nil { - return trace.Wrap(err) - } // Delete device. if err := rootAuthClient.DeleteMFADeviceSync(ctx, &proto.DeleteMFADeviceSyncRequest{ DeviceName: c.name, - ExistingMFAResponse: authnSolved, + ExistingMFAResponse: mfaResponse, }); err != nil { return trace.Wrap(err) } From fa0c4ab73c1b64a653e8e7d21aa4f1d36f1d9694 Mon Sep 17 00:00:00 2001 From: joerger Date: Fri, 20 Sep 2024 14:39:39 -0700 Subject: [PATCH 02/11] Refactor session MFA ceremony to use new MFA ceremony helpers. --- lib/client/api.go | 4 +- lib/client/cluster_client.go | 60 +++++++----- lib/client/cluster_client_test.go | 6 +- lib/client/local_proxy_middleware.go | 5 +- lib/client/presence.go | 41 +++++--- lib/teleterm/clusters/cluster_apps.go | 3 +- lib/teleterm/clusters/cluster_databases.go | 3 +- lib/teleterm/clusters/cluster_kubes.go | 2 - lib/web/desktop.go | 109 +++++++++++---------- lib/web/kube.go | 17 ++-- lib/web/terminal.go | 26 +++-- tool/tsh/common/app.go | 4 +- tool/tsh/common/kube_proxy.go | 2 - 13 files changed, 157 insertions(+), 125 deletions(-) diff --git a/lib/client/api.go b/lib/client/api.go index b304fe2a44660..e51a97c593d3a 100644 --- a/lib/client/api.go +++ b/lib/client/api.go @@ -1499,7 +1499,7 @@ func (tc *TeleportClient) ReissueUserCerts(ctx context.Context, cachePolicy Cert // (according to RBAC), IssueCertsWithMFA will: // - for SSH certs, return the existing Key from the keystore. // - for TLS certs, fall back to ReissueUserCerts. -func (tc *TeleportClient) IssueUserCertsWithMFA(ctx context.Context, params ReissueParams, mfaPromptOpts ...mfa.PromptOpt) (*KeyRing, error) { +func (tc *TeleportClient) IssueUserCertsWithMFA(ctx context.Context, params ReissueParams) (*KeyRing, error) { ctx, span := tc.Tracer.Start( ctx, "teleportClient/IssueUserCertsWithMFA", @@ -1513,7 +1513,7 @@ func (tc *TeleportClient) IssueUserCertsWithMFA(ctx context.Context, params Reis } defer clusterClient.Close() - keyRing, _, err := clusterClient.IssueUserCertsWithMFA(ctx, params, tc.NewMFAPrompt(mfaPromptOpts...)) + keyRing, _, err := clusterClient.IssueUserCertsWithMFA(ctx, params) return keyRing, trace.Wrap(err) } diff --git a/lib/client/cluster_client.go b/lib/client/cluster_client.go index b1aa1d7ef52b1..7fa1694cac0ba 100644 --- a/lib/client/cluster_client.go +++ b/lib/client/cluster_client.go @@ -320,7 +320,6 @@ func (c *ClusterClient) SessionSSHConfig(ctx context.Context, user string, targe MFACheck: target.MFACheck, }, keyRing, - c.tc.NewMFAPrompt(), ) if err != nil { return nil, trace.Wrap(err) @@ -430,7 +429,7 @@ func (c *ClusterClient) prepareUserCertsRequest(ctx context.Context, params Reis // performSessionMFACeremony runs the mfa ceremony to completion. // If successful the returned [KeyRing] will be authorized to connect to the target. -func (c *ClusterClient) performSessionMFACeremony(ctx context.Context, rootClient *ClusterClient, params ReissueParams, keyRing *KeyRing, mfaPrompt mfa.Prompt) (*KeyRing, error) { +func (c *ClusterClient) performSessionMFACeremony(ctx context.Context, rootClient *ClusterClient, params ReissueParams, keyRing *KeyRing) (*KeyRing, error) { newUserKeys, certsReq, err := rootClient.prepareUserCertsRequest(ctx, params, keyRing) if err != nil { return nil, trace.Wrap(err) @@ -440,10 +439,26 @@ func (c *ClusterClient) performSessionMFACeremony(ctx context.Context, rootClien if err != nil { return nil, trace.Wrap(err) } - keyRing, _, err = PerformSessionMFACeremony(ctx, PerformMFACeremonyParams{ + + var promptOpts []mfa.PromptOpt + switch { + case params.NodeName != "": + promptOpts = append(promptOpts, mfa.WithPromptReasonSessionMFA("Node", params.NodeName)) + case params.KubernetesCluster != "": + promptOpts = append(promptOpts, mfa.WithPromptReasonSessionMFA("Kubernetes cluster", params.KubernetesCluster)) + case params.RouteToDatabase.ServiceName != "": + promptOpts = append(promptOpts, mfa.WithPromptReasonSessionMFA("Database", params.RouteToDatabase.ServiceName)) + case params.RouteToApp.Name != "": + promptOpts = append(promptOpts, mfa.WithPromptReasonSessionMFA("Application", params.RouteToApp.Name)) + } + + mfaCeremony := c.tc.NewMFACeremony() + mfaCeremony.CreateAuthenticateChallenge = c.AuthClient.CreateAuthenticateChallenge + + keyRing, _, err = PerformSessionMFACeremony(ctx, PerformSessionMFACeremonyParams{ CurrentAuthClient: c.AuthClient, RootAuthClient: rootClient.AuthClient, - MFAPrompt: mfaPrompt, + MFACeremony: mfaCeremony, MFAAgainstRoot: c.cluster == rootClient.cluster, MFARequiredReq: mfaRequiredReq, ChallengeExtensions: mfav1.ChallengeExtensions{ @@ -452,13 +467,13 @@ func (c *ClusterClient) performSessionMFACeremony(ctx context.Context, rootClien CertsReq: certsReq, KeyRing: keyRing, newUserKeys: newUserKeys, - }) + }, promptOpts...) return keyRing, trace.Wrap(err) } // IssueUserCertsWithMFA generates a single-use certificate for the user. If MFA is required // to access the resource the provided [mfa.Prompt] will be used to perform the MFA ceremony. -func (c *ClusterClient) IssueUserCertsWithMFA(ctx context.Context, params ReissueParams, mfaPrompt mfa.Prompt) (*KeyRing, proto.MFARequired, error) { +func (c *ClusterClient) IssueUserCertsWithMFA(ctx context.Context, params ReissueParams) (*KeyRing, proto.MFARequired, error) { ctx, span := c.Tracer.Start( ctx, "ClusterClient/IssueUserCertsWithMFA", @@ -558,7 +573,7 @@ func (c *ClusterClient) IssueUserCertsWithMFA(ctx context.Context, params Reissu } // Perform the MFA ceremony and add the new credential to the KeyRing. - keyRing, err := c.performSessionMFACeremony(ctx, certClient, params, keyRing, mfaPrompt) + keyRing, err := c.performSessionMFACeremony(ctx, certClient, params, keyRing) if err != nil { return nil, proto.MFARequired_MFA_REQUIRED_YES, trace.Wrap(err) } @@ -567,30 +582,30 @@ func (c *ClusterClient) IssueUserCertsWithMFA(ctx context.Context, params Reissu return keyRing, proto.MFARequired_MFA_REQUIRED_YES, nil } -// PerformMFARootClient is a subset of Auth methods required for MFA. +// PerformSessionMFARootClient is a subset of Auth methods required for MFA. // Used by [PerformSessionMFACeremony]. -type PerformMFARootClient interface { +type PerformSessionMFARootClient interface { CreateAuthenticateChallenge(ctx context.Context, req *proto.CreateAuthenticateChallengeRequest) (*proto.MFAAuthenticateChallenge, error) GenerateUserCerts(ctx context.Context, req proto.UserCertsRequest) (*proto.Certs, error) } -// PerformMFACurrentClient is a subset of Auth methods required for MFA. +// PerformSessionMFACurrentClient is a subset of Auth methods required for MFA. // Used by [PerformSessionMFACeremony]. -type PerformMFACurrentClient interface { +type PerformSessionMFACurrentClient interface { IsMFARequired(ctx context.Context, req *proto.IsMFARequiredRequest) (*proto.IsMFARequiredResponse, error) } -// PerformMFACeremonyParams are the input parameters for [PerformSessionMFACeremony]. -type PerformMFACeremonyParams struct { +// PerformSessionMFACeremonyParams are the input parameters for [PerformSessionMFACeremony]. +type PerformSessionMFACeremonyParams struct { // CurrentAuthClient is the Auth client for the target cluster. // Unused if MFAAgainstRoot is true. - CurrentAuthClient PerformMFACurrentClient + CurrentAuthClient PerformSessionMFACurrentClient // RootAuthClient is the Auth client for the root cluster. // This is the client used to acquire the authn challenge and issue the user // certificates. - RootAuthClient PerformMFARootClient - // MFAPrompt is used to prompt the user for an MFA solution. - MFAPrompt mfa.Prompt + RootAuthClient PerformSessionMFARootClient + // MFACeremony handles the MFA ceremony. + MFACeremony *mfa.Ceremony // MFAAgainstRoot tells whether to run the MFA required check against root or // current cluster. @@ -632,7 +647,7 @@ type newUserKeys struct { // 4. Call RootAuthClient.GenerateUserCerts // // Returns the modified params.Key and the GenerateUserCertsResponse, or an error. -func PerformSessionMFACeremony(ctx context.Context, params PerformMFACeremonyParams) (*KeyRing, *proto.Certs, error) { +func PerformSessionMFACeremony(ctx context.Context, params PerformSessionMFACeremonyParams, promptOpts ...mfa.PromptOpt) (*KeyRing, *proto.Certs, error) { rootClient := params.RootAuthClient currentClient := params.CurrentAuthClient @@ -653,12 +668,7 @@ func PerformSessionMFACeremony(ctx context.Context, params PerformMFACeremonyPar mfaRequiredReq = nil // Already checked, don't check again at root. } - mfaCeremony := mfa.Ceremony{ - CreateAuthenticateChallenge: rootClient.CreateAuthenticateChallenge, - SolveAuthenticateChallenge: params.MFAPrompt.Run, - } - - mfaResp, err := mfaCeremony.Run(ctx, &proto.CreateAuthenticateChallengeRequest{ + mfaResp, err := params.MFACeremony.Run(ctx, &proto.CreateAuthenticateChallengeRequest{ Request: &proto.CreateAuthenticateChallengeRequest_ContextUser{ ContextUser: &proto.ContextUser{}, }, @@ -666,7 +676,7 @@ func PerformSessionMFACeremony(ctx context.Context, params PerformMFACeremonyPar ChallengeExtensions: &mfav1.ChallengeExtensions{ Scope: mfav1.ChallengeScope_CHALLENGE_SCOPE_USER_SESSION, }, - }) + }, promptOpts...) if errors.Is(err, &mfa.ErrMFANotRequired) { return nil, nil, trace.Wrap(services.ErrSessionMFANotRequired) } else if err != nil { diff --git a/lib/client/cluster_client_test.go b/lib/client/cluster_client_test.go index 893c8412ff02e..bfcd088d34d52 100644 --- a/lib/client/cluster_client_test.go +++ b/lib/client/cluster_client_test.go @@ -36,6 +36,7 @@ import ( webauthnpb "github.com/gravitational/teleport/api/types/webauthn" "github.com/gravitational/teleport/api/utils/keys" "github.com/gravitational/teleport/lib/auth/authclient" + libmfa "github.com/gravitational/teleport/lib/client/mfa" "github.com/gravitational/teleport/lib/fixtures" "github.com/gravitational/teleport/lib/observability/tracing" "github.com/gravitational/teleport/lib/services" @@ -362,6 +363,9 @@ func TestIssueUserCertsWithMFA(t *testing.T) { Config: Config{ SiteName: "test", Tracer: tracing.NoopTracer("test"), + MFAPromptConstructor: func(cfg *libmfa.PromptConfig) mfa.Prompt { + return test.prompt.Prompt + }, }, lastPing: &webclient.PingResponse{ Auth: webclient.AuthenticationSettings{ @@ -424,7 +428,7 @@ func TestIssueUserCertsWithMFA(t *testing.T) { ctx := context.Background() - keyRing, mfaRequired, err := clt.IssueUserCertsWithMFA(ctx, test.params, test.prompt) + keyRing, mfaRequired, err := clt.IssueUserCertsWithMFA(ctx, test.params) test.assertion(t, keyRing, mfaRequired, err) }) } diff --git a/lib/client/local_proxy_middleware.go b/lib/client/local_proxy_middleware.go index 3e3f422f73855..a514cb6516277 100644 --- a/lib/client/local_proxy_middleware.go +++ b/lib/client/local_proxy_middleware.go @@ -36,7 +36,6 @@ import ( "github.com/gravitational/teleport/api/client/proto" "github.com/gravitational/teleport/api/constants" - "github.com/gravitational/teleport/api/mfa" "github.com/gravitational/teleport/api/utils/keys" "github.com/gravitational/teleport/lib/auth/authclient" "github.com/gravitational/teleport/lib/defaults" @@ -238,7 +237,7 @@ func (c *DBCertIssuer) IssueCert(ctx context.Context) (tls.Certificate, error) { return trace.Wrap(err) } - newKey, mfaRequired, err := clusterClient.IssueUserCertsWithMFA(ctx, dbCertParams, c.Client.NewMFAPrompt(mfa.WithPromptReasonSessionMFA("database", c.RouteToApp.ServiceName))) + newKey, mfaRequired, err := clusterClient.IssueUserCertsWithMFA(ctx, dbCertParams) if err != nil { return trace.Wrap(err) } @@ -315,7 +314,7 @@ func (c *AppCertIssuer) IssueCert(ctx context.Context) (tls.Certificate, error) return trace.Wrap(err) } - newKey, mfaRequired, err := clusterClient.IssueUserCertsWithMFA(ctx, appCertParams, c.Client.NewMFAPrompt(mfa.WithPromptReasonSessionMFA("application", c.RouteToApp.Name))) + newKey, mfaRequired, err := clusterClient.IssueUserCertsWithMFA(ctx, appCertParams) if err != nil { return trace.Wrap(err) } diff --git a/lib/client/presence.go b/lib/client/presence.go index edf101e74994c..46f2a31e0637e 100644 --- a/lib/client/presence.go +++ b/lib/client/presence.go @@ -57,6 +57,7 @@ func WithPresenceClock(clock clockwork.Clock) PresenceOption { // RunPresenceTask periodically performs and MFA ceremony to detect that a user is // still present and attentive. func RunPresenceTask(ctx context.Context, term io.Writer, maintainer PresenceMaintainer, sessionID string, mfaPrompt mfa.Prompt, opts ...PresenceOption) error { + fmt.Fprintf(term, "\r\nTeleport > MFA presence enabled\r\n") o := &presenceOptions{ @@ -76,46 +77,58 @@ func RunPresenceTask(ctx context.Context, term io.Writer, maintainer PresenceMai return trace.Wrap(err) } - for { - select { - case <-ticker.Chan(): + mfaCeremony := &mfa.Ceremony{ + CreateAuthenticateChallenge: func(ctx context.Context, _ *proto.CreateAuthenticateChallengeRequest) (*proto.MFAAuthenticateChallenge, error) { req := &proto.PresenceMFAChallengeSend{ Request: &proto.PresenceMFAChallengeSend_ChallengeRequest{ ChallengeRequest: &proto.PresenceMFAChallengeRequest{SessionID: sessionID}, }, } - err = stream.Send(req) - if err != nil { - return trace.Wrap(err) + if err := stream.Send(req); err != nil { + return nil, trace.Wrap(err) } challenge, err := stream.Recv() if err != nil { - return trace.Wrap(err) + return nil, trace.Wrap(err) } - fmt.Fprint(term, "\r\nTeleport > Please tap your MFA key\r\n") - // This is here to enforce the usage of a MFA device. // We don't support TOTP for live presence. challenge.TOTP = nil - solution, err := mfaPrompt.Run(ctx, challenge) + return challenge, nil + }, + SolveAuthenticateChallenge: func(ctx context.Context, chal *proto.MFAAuthenticateChallenge) (*proto.MFAAuthenticateResponse, error) { + fmt.Fprint(term, "\r\nTeleport > Please tap your MFA key\r\n") + + mfaResp, err := mfaPrompt.Run(ctx, chal) if err != nil { fmt.Fprintf(term, "\r\nTeleport > Failed to confirm presence: %v\r\n", err) - return trace.Wrap(err) + return nil, trace.Wrap(err) } fmt.Fprint(term, "\r\nTeleport > Received MFA presence confirmation\r\n") + return mfaResp, nil + }, + } + + for { + select { + case <-ticker.Chan(): + mfaResp, err := mfaCeremony.Run(ctx, nil) + if err != nil { + return trace.Wrap(err) + } - req = &proto.PresenceMFAChallengeSend{ + resp := &proto.PresenceMFAChallengeSend{ Request: &proto.PresenceMFAChallengeSend_ChallengeResponse{ - ChallengeResponse: solution, + ChallengeResponse: mfaResp, }, } - err = stream.Send(req) + err = stream.Send(resp) if err != nil { return trace.Wrap(err) } diff --git a/lib/teleterm/clusters/cluster_apps.go b/lib/teleterm/clusters/cluster_apps.go index 12a82c6052521..bcbd3317365d4 100644 --- a/lib/teleterm/clusters/cluster_apps.go +++ b/lib/teleterm/clusters/cluster_apps.go @@ -26,7 +26,6 @@ import ( apiclient "github.com/gravitational/teleport/api/client" "github.com/gravitational/teleport/api/client/proto" "github.com/gravitational/teleport/api/defaults" - "github.com/gravitational/teleport/api/mfa" "github.com/gravitational/teleport/api/types" api "github.com/gravitational/teleport/gen/proto/go/teleport/lib/teleterm/v1" "github.com/gravitational/teleport/lib/auth/authclient" @@ -189,7 +188,7 @@ func (c *Cluster) ReissueAppCert(ctx context.Context, clusterClient *client.Clus AccessRequests: c.status.ActiveRequests.AccessRequests, RequesterName: proto.UserCertsRequest_TSH_APP_LOCAL_PROXY, TTL: c.clusterClient.KeyTTL, - }, c.clusterClient.NewMFAPrompt(mfa.WithPromptReasonSessionMFA("application", routeToApp.Name))) + }) if err != nil { return tls.Certificate{}, trace.Wrap(err) } diff --git a/lib/teleterm/clusters/cluster_databases.go b/lib/teleterm/clusters/cluster_databases.go index ed65f859ca6ed..d4f792bb08f13 100644 --- a/lib/teleterm/clusters/cluster_databases.go +++ b/lib/teleterm/clusters/cluster_databases.go @@ -28,7 +28,6 @@ import ( apiclient "github.com/gravitational/teleport/api/client" "github.com/gravitational/teleport/api/client/proto" "github.com/gravitational/teleport/api/defaults" - "github.com/gravitational/teleport/api/mfa" "github.com/gravitational/teleport/api/types" api "github.com/gravitational/teleport/gen/proto/go/teleport/lib/teleterm/v1" "github.com/gravitational/teleport/lib/auth/authclient" @@ -142,7 +141,7 @@ func (c *Cluster) reissueDBCerts(ctx context.Context, clusterClient *client.Clus AccessRequests: c.status.ActiveRequests.AccessRequests, RequesterName: proto.UserCertsRequest_TSH_DB_LOCAL_PROXY_TUNNEL, TTL: c.clusterClient.KeyTTL, - }, c.clusterClient.NewMFAPrompt(mfa.WithPromptReasonSessionMFA("database", routeToDatabase.ServiceName))) + }) if err != nil { return tls.Certificate{}, trace.Wrap(err) } diff --git a/lib/teleterm/clusters/cluster_kubes.go b/lib/teleterm/clusters/cluster_kubes.go index d18a3bd7db65d..ab654398dad62 100644 --- a/lib/teleterm/clusters/cluster_kubes.go +++ b/lib/teleterm/clusters/cluster_kubes.go @@ -28,7 +28,6 @@ import ( apiclient "github.com/gravitational/teleport/api/client" "github.com/gravitational/teleport/api/client/proto" "github.com/gravitational/teleport/api/defaults" - "github.com/gravitational/teleport/api/mfa" "github.com/gravitational/teleport/api/types" api "github.com/gravitational/teleport/gen/proto/go/teleport/lib/teleterm/v1" "github.com/gravitational/teleport/lib/auth/authclient" @@ -118,7 +117,6 @@ func (c *Cluster) reissueKubeCert(ctx context.Context, clusterClient *client.Clu RequesterName: proto.UserCertsRequest_TSH_KUBE_LOCAL_PROXY, TTL: c.clusterClient.KeyTTL, }, - c.clusterClient.NewMFAPrompt(mfa.WithPromptReasonSessionMFA("Kubernetes cluster", kubeCluster)), ) if err != nil { return tls.Certificate{}, trace.Wrap(err) diff --git a/lib/web/desktop.go b/lib/web/desktop.go index 076e680fc7f4c..6a00b05788bfe 100644 --- a/lib/web/desktop.go +++ b/lib/web/desktop.go @@ -315,7 +315,7 @@ func (h *Handler) issueCerts( withheld *[]tdp.Message, ) (certs *proto.Certs, err error) { if mfaRequired { - certs, err = h.performMFACeremony(ctx, ws, sctx, certsReq, withheld) + certs, err = h.performSessionMFACeremony(ctx, ws, sctx, certsReq, withheld) if err != nil { return nil, trace.Wrap(err) } @@ -354,85 +354,88 @@ func (h *Handler) createDesktopTLSConfig( return tlsConfig, nil } -// performMFACeremony completes the mfa ceremony and returns the raw TLS certificate +// performSessionMFACeremony completes the mfa ceremony and returns the raw TLS certificate // on success. The user will be prompted to tap their security key by the UI // in order to perform the assertion. -func (h *Handler) performMFACeremony( +func (h *Handler) performSessionMFACeremony( ctx context.Context, ws *websocket.Conn, sctx *SessionContext, certsReq *proto.UserCertsRequest, withheld *[]tdp.Message, ) (_ *proto.Certs, err error) { - ctx, span := h.tracer.Start(ctx, "desktop/performMFACeremony") + ctx, span := h.tracer.Start(ctx, "desktop/performSessionMFACeremony") defer func() { span.RecordError(err) span.End() }() - promptMFA := mfa.PromptFunc(func(ctx context.Context, chal *proto.MFAAuthenticateChallenge) (*proto.MFAAuthenticateResponse, error) { - codec := tdpMFACodec{} - - // Send the challenge over the socket. - msg, err := codec.Encode( - &client.MFAAuthenticateChallenge{ - WebauthnChallenge: wantypes.CredentialAssertionFromProto(chal.WebauthnChallenge), - }, - defaults.WebsocketWebauthnChallenge, - ) - if err != nil { - return nil, trace.Wrap(err) - } - - if err := ws.WriteMessage(websocket.BinaryMessage, msg); err != nil { - return nil, trace.Wrap(err) - } - - span.AddEvent("waiting for user to complete mfa ceremony") - var buf []byte - // Loop through incoming messages until we receive an MFA message that lets us - // complete the ceremony. Non-MFA messages (e.g. ClientScreenSpecs representing - // screen resizes) are withheld for later. - for { - var ty int - ty, buf, err = ws.ReadMessage() + mfaCeremony := &mfa.Ceremony{ + CreateAuthenticateChallenge: sctx.cfg.RootClient.CreateAuthenticateChallenge, + SolveAuthenticateChallenge: func(ctx context.Context, chal *proto.MFAAuthenticateChallenge) (*proto.MFAAuthenticateResponse, error) { + codec := tdpMFACodec{} + + // Send the challenge over the socket. + msg, err := codec.Encode( + &client.MFAAuthenticateChallenge{ + WebauthnChallenge: wantypes.CredentialAssertionFromProto(chal.WebauthnChallenge), + }, + defaults.WebsocketWebauthnChallenge, + ) if err != nil { return nil, trace.Wrap(err) } - if ty != websocket.BinaryMessage { - return nil, trace.BadParameter("received unexpected web socket message type %d", ty) - } - if len(buf) == 0 { - return nil, trace.BadParameter("empty message received") + + if err := ws.WriteMessage(websocket.BinaryMessage, msg); err != nil { + return nil, trace.Wrap(err) } - if tdp.MessageType(buf[0]) != tdp.TypeMFA { - // This is not an MFA message, withhold it for later. - msg, err := tdp.Decode(buf) - h.log.Debugf("Received non-MFA message, withholding:", msg) + span.AddEvent("waiting for user to complete mfa ceremony") + var buf []byte + // Loop through incoming messages until we receive an MFA message that lets us + // complete the ceremony. Non-MFA messages (e.g. ClientScreenSpecs representing + // screen resizes) are withheld for later. + for { + var ty int + ty, buf, err = ws.ReadMessage() if err != nil { return nil, trace.Wrap(err) } - *withheld = append(*withheld, msg) - continue - } + if ty != websocket.BinaryMessage { + return nil, trace.BadParameter("received unexpected web socket message type %d", ty) + } + if len(buf) == 0 { + return nil, trace.BadParameter("empty message received") + } - break - } + if tdp.MessageType(buf[0]) != tdp.TypeMFA { + // This is not an MFA message, withhold it for later. + msg, err := tdp.Decode(buf) + h.log.Debugf("Received non-MFA message, withholding:", msg) + if err != nil { + return nil, trace.Wrap(err) + } + *withheld = append(*withheld, msg) + continue + } - assertion, err := codec.DecodeResponse(buf, defaults.WebsocketWebauthnChallenge) - if err != nil { - return nil, trace.Wrap(err) - } - span.AddEvent("mfa ceremony completed") + break + } - return assertion, nil - }) + assertion, err := codec.DecodeResponse(buf, defaults.WebsocketWebauthnChallenge) + if err != nil { + return nil, trace.Wrap(err) + } + span.AddEvent("mfa ceremony completed") + + return assertion, nil + }, + } - _, newCerts, err := client.PerformSessionMFACeremony(ctx, client.PerformMFACeremonyParams{ + _, newCerts, err := client.PerformSessionMFACeremony(ctx, client.PerformSessionMFACeremonyParams{ CurrentAuthClient: nil, // Only RootAuthClient is used. RootAuthClient: sctx.cfg.RootClient, - MFAPrompt: promptMFA, + MFACeremony: mfaCeremony, MFAAgainstRoot: true, MFARequiredReq: nil, // No need to verify. ChallengeExtensions: mfav1.ChallengeExtensions{ diff --git a/lib/web/kube.go b/lib/web/kube.go index 1811f644ef32e..b59f21d1fb0ea 100644 --- a/lib/web/kube.go +++ b/lib/web/kube.go @@ -241,14 +241,19 @@ func (p *podHandler) handler(r *http.Request) error { Usage: clientproto.UserCertsRequest_Kubernetes, } - _, certs, err := client.PerformSessionMFACeremony(ctx, client.PerformMFACeremonyParams{ + mfaCeremony := &mfa.Ceremony{ + CreateAuthenticateChallenge: p.sctx.cfg.RootClient.CreateAuthenticateChallenge, + SolveAuthenticateChallenge: func(ctx context.Context, chal *clientproto.MFAAuthenticateChallenge) (*clientproto.MFAAuthenticateResponse, error) { + assertion, err := mfaPrompt(stream.WSStream, protobufMFACodec{}).Run(ctx, chal) + return assertion, trace.Wrap(err) + }, + } + + _, certs, err := client.PerformSessionMFACeremony(ctx, client.PerformSessionMFACeremonyParams{ CurrentAuthClient: p.userClient, RootAuthClient: p.sctx.cfg.RootClient, - MFAPrompt: mfa.PromptFunc(func(ctx context.Context, chal *clientproto.MFAAuthenticateChallenge) (*clientproto.MFAAuthenticateResponse, error) { - assertion, err := promptMFAChallenge(stream.WSStream, protobufMFACodec{}).Run(ctx, chal) - return assertion, trace.Wrap(err) - }), - MFAAgainstRoot: p.sctx.cfg.RootClusterName == p.teleportCluster, + MFACeremony: mfaCeremony, + MFAAgainstRoot: p.sctx.cfg.RootClusterName == p.teleportCluster, MFARequiredReq: &clientproto.IsMFARequiredRequest{ Target: &clientproto.IsMFARequiredRequest_KubernetesCluster{KubernetesCluster: p.req.KubeCluster}, }, diff --git a/lib/web/terminal.go b/lib/web/terminal.go index 1ddca58c68576..b8e61bbe595e6 100644 --- a/lib/web/terminal.go +++ b/lib/web/terminal.go @@ -586,17 +586,22 @@ func (t *sshBaseHandler) issueSessionMFACerts(ctx context.Context, tc *client.Te SSHLogin: tc.HostLogin, } - _, certs, err := client.PerformSessionMFACeremony(ctx, client.PerformMFACeremonyParams{ - CurrentAuthClient: t.userAuthClient, - RootAuthClient: t.ctx.cfg.RootClient, - MFAPrompt: mfa.PromptFunc(func(ctx context.Context, chal *authproto.MFAAuthenticateChallenge) (*authproto.MFAAuthenticateResponse, error) { + mfaCeremony := &mfa.Ceremony{ + CreateAuthenticateChallenge: t.ctx.cfg.RootClient.CreateAuthenticateChallenge, + SolveAuthenticateChallenge: func(ctx context.Context, chal *authproto.MFAAuthenticateChallenge) (*authproto.MFAAuthenticateResponse, error) { span.AddEvent("prompting user with mfa challenge") - assertion, err := promptMFAChallenge(wsStream, protobufMFACodec{}).Run(ctx, chal) + assertion, err := mfaPrompt(wsStream, protobufMFACodec{}).Run(ctx, chal) span.AddEvent("user completed mfa challenge") return assertion, trace.Wrap(err) - }), - MFAAgainstRoot: t.ctx.cfg.RootClusterName == tc.SiteName, - MFARequiredReq: mfaRequiredReq, + }, + } + + _, certs, err := client.PerformSessionMFACeremony(ctx, client.PerformSessionMFACeremonyParams{ + CurrentAuthClient: t.userAuthClient, + RootAuthClient: t.ctx.cfg.RootClient, + MFACeremony: mfaCeremony, + MFAAgainstRoot: t.ctx.cfg.RootClusterName == tc.SiteName, + MFARequiredReq: mfaRequiredReq, ChallengeExtensions: mfav1.ChallengeExtensions{ Scope: mfav1.ChallengeScope_CHALLENGE_SCOPE_USER_SESSION, }, @@ -617,7 +622,7 @@ func (t *sshBaseHandler) issueSessionMFACerts(ctx context.Context, tc *client.Te return []ssh.AuthMethod{am}, nil } -func promptMFAChallenge(stream *terminal.WSStream, codec terminal.MFACodec) mfa.Prompt { +func mfaPrompt(stream *terminal.WSStream, codec terminal.MFACodec) mfa.Prompt { return mfa.PromptFunc(func(ctx context.Context, chal *authproto.MFAAuthenticateChallenge) (*authproto.MFAAuthenticateResponse, error) { var challenge *client.MFAAuthenticateChallenge @@ -638,6 +643,7 @@ func promptMFAChallenge(stream *terminal.WSStream, codec terminal.MFACodec) mfa. resp, err := stream.ReadChallengeResponse(codec) return resp, trace.Wrap(err) }) + } type connectWithMFAFn = func(ctx context.Context, ws terminal.WSConn, tc *client.TeleportClient, accessChecker services.AccessChecker, getAgent teleagent.Getter, signer agentless.SignerCreator) (*client.NodeClient, error) @@ -795,7 +801,7 @@ func (t *TerminalHandler) streamTerminal(ctx context.Context, tc *client.Telepor if t.participantMode == types.SessionModeratorMode { beforeStart = func(out io.Writer) { nc.OnMFA = func() { - if err := t.presenceChecker(ctx, out, t.userAuthClient, t.sessionData.ID.String(), promptMFAChallenge(t.stream.WSStream, protobufMFACodec{})); err != nil { + if err := t.presenceChecker(ctx, out, t.userAuthClient, t.sessionData.ID.String(), mfaPrompt(t.stream.WSStream, protobufMFACodec{})); err != nil { t.log.WithError(err).Warn("Unable to stream terminal - failure performing presence checks") return } diff --git a/tool/tsh/common/app.go b/tool/tsh/common/app.go index 92b438bd43b61..6196fc5aae3f5 100644 --- a/tool/tsh/common/app.go +++ b/tool/tsh/common/app.go @@ -34,7 +34,6 @@ import ( "github.com/gravitational/teleport" apiclient "github.com/gravitational/teleport/api/client" "github.com/gravitational/teleport/api/client/proto" - "github.com/gravitational/teleport/api/mfa" "github.com/gravitational/teleport/api/types" "github.com/gravitational/teleport/lib/asciitable" "github.com/gravitational/teleport/lib/auth/authclient" @@ -120,8 +119,7 @@ func appLogin( return nil, trace.Wrap(err) } - keyRing, _, err := clusterClient.IssueUserCertsWithMFA(ctx, appCertParams, - tc.NewMFAPrompt(mfa.WithPromptReasonSessionMFA("Application", appCertParams.RouteToApp.Name))) + keyRing, _, err := clusterClient.IssueUserCertsWithMFA(ctx, appCertParams) return keyRing, trace.Wrap(err) } diff --git a/tool/tsh/common/kube_proxy.go b/tool/tsh/common/kube_proxy.go index c6c7334ab54cc..df8aab146776e 100644 --- a/tool/tsh/common/kube_proxy.go +++ b/tool/tsh/common/kube_proxy.go @@ -38,7 +38,6 @@ import ( "github.com/gravitational/teleport/api/client/proto" apidefaults "github.com/gravitational/teleport/api/defaults" - "github.com/gravitational/teleport/api/mfa" "github.com/gravitational/teleport/api/types" "github.com/gravitational/teleport/api/utils/keys" "github.com/gravitational/teleport/lib/asciitable" @@ -584,7 +583,6 @@ func issueKubeCert(ctx context.Context, tc *client.TeleportClient, clusterClient RequesterName: requesterName, TTL: tc.KeyTTL, }, - tc.NewMFAPrompt(mfa.WithPromptReasonSessionMFA("Kubernetes cluster", kubeCluster)), ) if err != nil { return tls.Certificate{}, trace.Wrap(err) From 13e69437e1bd04c5aa4d1b8151147e19cd2c6f08 Mon Sep 17 00:00:00 2001 From: joerger Date: Fri, 20 Sep 2024 14:47:51 -0700 Subject: [PATCH 03/11] Simplify calls to NewMFACeremony. --- api/mfa/ceremony_test.go | 2 +- lib/client/api.go | 4 +--- lib/client/cluster_client.go | 5 +---- lib/teleterm/clusters/cluster_headless.go | 4 +--- tool/tsh/common/mfa.go | 8 ++------ 5 files changed, 6 insertions(+), 17 deletions(-) diff --git a/api/mfa/ceremony_test.go b/api/mfa/ceremony_test.go index 32f6e7c5685dd..d87f8348e4695 100644 --- a/api/mfa/ceremony_test.go +++ b/api/mfa/ceremony_test.go @@ -89,7 +89,7 @@ func TestPerformMFACeremony(t *testing.T) { t.Run(tt.name, func(t *testing.T) { ceremony := mfa.Ceremony{ CreateAuthenticateChallenge: tt.ceremonyClient.CreateAuthenticateChallenge, - NewPrompt: func(po ...mfa.PromptOpt) mfa.Prompt { + NewPrompt: func(_ ...mfa.PromptOpt) mfa.Prompt { return mfa.PromptFunc(tt.ceremonyClient.PromptMFA) }, } diff --git a/lib/client/api.go b/lib/client/api.go index e51a97c593d3a..b64416171ca6b 100644 --- a/lib/client/api.go +++ b/lib/client/api.go @@ -5184,9 +5184,7 @@ func (tc *TeleportClient) HeadlessApprove(ctx context.Context, headlessAuthentic } } - mfaCeremony := tc.NewMFACeremony() - mfaCeremony.CreateAuthenticateChallenge = rootClient.CreateAuthenticateChallenge - mfaResp, err := mfaCeremony.Run(ctx, &proto.CreateAuthenticateChallengeRequest{ + mfaResp, err := tc.NewMFACeremony().Run(ctx, &proto.CreateAuthenticateChallengeRequest{ ChallengeExtensions: &mfav1.ChallengeExtensions{ Scope: mfav1.ChallengeScope_CHALLENGE_SCOPE_HEADLESS_LOGIN, }, diff --git a/lib/client/cluster_client.go b/lib/client/cluster_client.go index 7fa1694cac0ba..c551fe0743736 100644 --- a/lib/client/cluster_client.go +++ b/lib/client/cluster_client.go @@ -452,13 +452,10 @@ func (c *ClusterClient) performSessionMFACeremony(ctx context.Context, rootClien promptOpts = append(promptOpts, mfa.WithPromptReasonSessionMFA("Application", params.RouteToApp.Name)) } - mfaCeremony := c.tc.NewMFACeremony() - mfaCeremony.CreateAuthenticateChallenge = c.AuthClient.CreateAuthenticateChallenge - keyRing, _, err = PerformSessionMFACeremony(ctx, PerformSessionMFACeremonyParams{ CurrentAuthClient: c.AuthClient, RootAuthClient: rootClient.AuthClient, - MFACeremony: mfaCeremony, + MFACeremony: c.tc.NewMFACeremony(), MFAAgainstRoot: c.cluster == rootClient.cluster, MFARequiredReq: mfaRequiredReq, ChallengeExtensions: mfav1.ChallengeExtensions{ diff --git a/lib/teleterm/clusters/cluster_headless.go b/lib/teleterm/clusters/cluster_headless.go index 29f835c3bd029..a714810480c60 100644 --- a/lib/teleterm/clusters/cluster_headless.go +++ b/lib/teleterm/clusters/cluster_headless.go @@ -74,9 +74,7 @@ func (c *Cluster) UpdateHeadlessAuthenticationState(ctx context.Context, rootAut var mfaResponse *proto.MFAAuthenticateResponse var err error if state == types.HeadlessAuthenticationState_HEADLESS_AUTHENTICATION_STATE_APPROVED { - mfaCeremony := c.clusterClient.NewMFACeremony() - mfaCeremony.CreateAuthenticateChallenge = rootAuthClient.CreateAuthenticateChallenge - mfaResponse, err = mfaCeremony.Run(ctx, &proto.CreateAuthenticateChallengeRequest{ + mfaResponse, err = c.clusterClient.NewMFACeremony().Run(ctx, &proto.CreateAuthenticateChallengeRequest{ ChallengeExtensions: &mfav1.ChallengeExtensions{ Scope: mfav1.ChallengeScope_CHALLENGE_SCOPE_HEADLESS_LOGIN, }, diff --git a/tool/tsh/common/mfa.go b/tool/tsh/common/mfa.go index 68fdfb8b7dde6..744fdb4ff1a1a 100644 --- a/tool/tsh/common/mfa.go +++ b/tool/tsh/common/mfa.go @@ -343,9 +343,7 @@ func (c *mfaAddCommand) addDeviceRPC(ctx context.Context, tc *client.TeleportCli defer wanwin.ResetPromptPlatformMessage() wanwin.PromptPlatformMessage = registeredMsg - mfaCeremony := tc.NewMFACeremony() - mfaCeremony.CreateAuthenticateChallenge = rootAuthClient.CreateAuthenticateChallenge - mfaResp, err := mfaCeremony.Run(ctx, &proto.CreateAuthenticateChallengeRequest{ + mfaResp, err := tc.NewMFACeremony().Run(ctx, &proto.CreateAuthenticateChallengeRequest{ ChallengeExtensions: &mfav1.ChallengeExtensions{ Scope: mfav1.ChallengeScope_CHALLENGE_SCOPE_MANAGE_DEVICES, }, @@ -590,9 +588,7 @@ func (c *mfaRemoveCommand) run(cf *CLIConf) error { return trace.NotFound("device %q not found", c.name) } - mfaCeremony := tc.NewMFACeremony() - mfaCeremony.CreateAuthenticateChallenge = rootAuthClient.CreateAuthenticateChallenge - mfaResponse, err := mfaCeremony.Run(ctx, &proto.CreateAuthenticateChallengeRequest{ + mfaResponse, err := tc.NewMFACeremony().Run(ctx, &proto.CreateAuthenticateChallengeRequest{ ChallengeExtensions: &mfav1.ChallengeExtensions{ Scope: mfav1.ChallengeScope_CHALLENGE_SCOPE_MANAGE_DEVICES, }, From 1a5a03553bc9eb5417591f6166b7582926b243b5 Mon Sep 17 00:00:00 2001 From: joerger Date: Fri, 20 Sep 2024 15:36:26 -0700 Subject: [PATCH 04/11] Remove remaining usage of tc.PromptMFA in favor of Ceremony. --- api/mfa/ceremony.go | 4 +- lib/client/api.go | 16 ++-- lib/client/mfa.go | 5 -- lib/client/weblogin.go | 101 ++++++++++---------------- lib/teleterm/clusters/cluster_auth.go | 8 +- 5 files changed, 51 insertions(+), 83 deletions(-) diff --git a/api/mfa/ceremony.go b/api/mfa/ceremony.go index 95be7bdc67f67..1e548fbe1c6a3 100644 --- a/api/mfa/ceremony.go +++ b/api/mfa/ceremony.go @@ -27,8 +27,8 @@ import ( // Ceremony is an MFA ceremony. type Ceremony struct { - CreateAuthenticateChallenge func(ctx context.Context, in *proto.CreateAuthenticateChallengeRequest) (*proto.MFAAuthenticateChallenge, error) - SolveAuthenticateChallenge func(ctx context.Context, in *proto.MFAAuthenticateChallenge) (*proto.MFAAuthenticateResponse, error) + CreateAuthenticateChallenge func(ctx context.Context, req *proto.CreateAuthenticateChallengeRequest) (*proto.MFAAuthenticateChallenge, error) + SolveAuthenticateChallenge func(ctx context.Context, chal *proto.MFAAuthenticateChallenge) (*proto.MFAAuthenticateResponse, error) NewPrompt PromptConstructor } diff --git a/lib/client/api.go b/lib/client/api.go index b64416171ca6b..7c485eda0cb6c 100644 --- a/lib/client/api.go +++ b/lib/client/api.go @@ -3674,10 +3674,10 @@ func (tc *TeleportClient) mfaLocalLoginWeb(ctx context.Context, keyRing *KeyRing } clt, session, err := SSHAgentMFAWebSessionLogin(ctx, SSHLoginMFA{ - SSHLogin: sshLogin, - User: tc.Username, - Password: password, - PromptMFA: tc.NewMFAPrompt(), + SSHLogin: sshLogin, + User: tc.Username, + Password: password, + MFAPromptConstructor: tc.NewMFAPrompt, }) return clt, session, trace.Wrap(err) } @@ -4005,10 +4005,10 @@ func (tc *TeleportClient) mfaLocalLogin(ctx context.Context, keyRing *KeyRing) ( } response, err := SSHAgentMFALogin(ctx, SSHLoginMFA{ - SSHLogin: sshLogin, - User: tc.Username, - Password: password, - PromptMFA: tc.NewMFAPrompt(), + SSHLogin: sshLogin, + User: tc.Username, + Password: password, + MFAPromptConstructor: tc.NewMFAPrompt, }) return response, trace.Wrap(err) diff --git a/lib/client/mfa.go b/lib/client/mfa.go index 7fd11b578d250..3c01baad11187 100644 --- a/lib/client/mfa.go +++ b/lib/client/mfa.go @@ -65,11 +65,6 @@ func (tc *TeleportClient) NewMFAPrompt(opts ...mfa.PromptOpt) mfa.Prompt { return prompt } -// PromptMFA runs a standard MFA prompt from client settings. -func (tc *TeleportClient) PromptMFA(ctx context.Context, chal *proto.MFAAuthenticateChallenge) (*proto.MFAAuthenticateResponse, error) { - return tc.NewMFAPrompt().Run(ctx, chal) -} - func (tc *TeleportClient) newPromptConfig(opts ...mfa.PromptOpt) *libmfa.PromptConfig { cfg := libmfa.NewPromptConfig(tc.WebProxyAddr, opts...) cfg.AuthenticatorAttachment = tc.AuthenticatorAttachment diff --git a/lib/client/weblogin.go b/lib/client/weblogin.go index 121dfae5baa39..1b41c5d740f52 100644 --- a/lib/client/weblogin.go +++ b/lib/client/weblogin.go @@ -52,7 +52,6 @@ import ( "github.com/gravitational/teleport/lib/auth/authclient" wancli "github.com/gravitational/teleport/lib/auth/webauthncli" wantypes "github.com/gravitational/teleport/lib/auth/webauthntypes" - libmfa "github.com/gravitational/teleport/lib/client/mfa" "github.com/gravitational/teleport/lib/defaults" "github.com/gravitational/teleport/lib/httplib" "github.com/gravitational/teleport/lib/httplib/csrf" @@ -401,9 +400,8 @@ type SSHLoginDirect struct { // SSHLoginMFA contains SSH login parameters for MFA login. type SSHLoginMFA struct { SSHLogin - // PromptMFA is a customizable MFA prompt function. - // Defaults to [mfa.NewPrompt().Run] - PromptMFA mfa.Prompt + // MFAPromptConstructor is a custom MFA prompt constructor to use when prompting for MFA. + MFAPromptConstructor mfa.PromptConstructor // User is the login username. User string // Password is the login password. @@ -764,35 +762,7 @@ func SSHAgentMFALogin(ctx context.Context, login SSHLoginMFA) (*authclient.SSHLo return nil, trace.Wrap(err) } - beginReq := MFAChallengeRequest{ - User: login.User, - Pass: login.Password, - } - challengeJSON, err := clt.PostJSON(ctx, clt.Endpoint("webapi", "mfa", "login", "begin"), beginReq) - if err != nil { - return nil, trace.Wrap(err) - } - - challenge := &MFAAuthenticateChallenge{} - if err := json.Unmarshal(challengeJSON.Bytes(), challenge); err != nil { - return nil, trace.Wrap(err) - } - - // Convert to auth gRPC proto challenge. - chal := &proto.MFAAuthenticateChallenge{} - if challenge.TOTPChallenge { - chal.TOTP = &proto.TOTPChallenge{} - } - if challenge.WebauthnChallenge != nil { - chal.WebauthnChallenge = wantypes.CredentialAssertionToProto(challenge.WebauthnChallenge) - } - - promptMFA := login.PromptMFA - if promptMFA == nil { - promptMFA = libmfa.NewCLIPrompt(libmfa.NewPromptConfig(login.ProxyAddr), os.Stderr) - } - - respPB, err := promptMFA.Run(ctx, chal) + mfaResp, err := newMFALoginCeremony(clt, login).Run(ctx, nil) if err != nil { return nil, trace.Wrap(err) } @@ -812,7 +782,7 @@ func SSHAgentMFALogin(ctx context.Context, login SSHLoginMFA) (*authclient.SSHLo KubernetesCluster: login.KubernetesCluster, } // Convert back from auth gRPC proto response. - switch r := respPB.Response.(type) { + switch r := mfaResp.Response.(type) { case *proto.MFAAuthenticateResponse_TOTP: challengeResp.TOTPCode = r.TOTP.Code case *proto.MFAAuthenticateResponse_Webauthn: @@ -830,6 +800,37 @@ func SSHAgentMFALogin(ctx context.Context, login SSHLoginMFA) (*authclient.SSHLo return loginResp, trace.Wrap(json.Unmarshal(loginRespJSON.Bytes(), loginResp)) } +func newMFALoginCeremony(clt *WebClient, login SSHLoginMFA) *mfa.Ceremony { + return &mfa.Ceremony{ + CreateAuthenticateChallenge: func(ctx context.Context, req *proto.CreateAuthenticateChallengeRequest) (*proto.MFAAuthenticateChallenge, error) { + beginReq := MFAChallengeRequest{ + User: login.User, + Pass: login.Password, + } + challengeJSON, err := clt.PostJSON(ctx, clt.Endpoint("webapi", "mfa", "login", "begin"), beginReq) + if err != nil { + return nil, trace.Wrap(err) + } + + challenge := &MFAAuthenticateChallenge{} + if err := json.Unmarshal(challengeJSON.Bytes(), challenge); err != nil { + return nil, trace.Wrap(err) + } + + // Convert to auth gRPC proto challenge. + chal := &proto.MFAAuthenticateChallenge{} + if challenge.TOTPChallenge { + chal.TOTP = &proto.TOTPChallenge{} + } + if challenge.WebauthnChallenge != nil { + chal.WebauthnChallenge = wantypes.CredentialAssertionToProto(challenge.WebauthnChallenge) + } + return chal, nil + }, + NewPrompt: login.MFAPromptConstructor, + } +} + // HostCredentials is used to fetch host credentials for a node. func HostCredentials(ctx context.Context, proxyAddr string, insecure bool, req types.RegisterUsingTokenRequest) (*proto.Certs, error) { clt, _, err := initClient(proxyAddr, insecure, nil, nil) @@ -966,35 +967,7 @@ func SSHAgentMFAWebSessionLogin(ctx context.Context, login SSHLoginMFA) (*WebCli return nil, nil, trace.Wrap(err) } - beginReq := MFAChallengeRequest{ - User: login.User, - Pass: login.Password, - } - challengeJSON, err := clt.PostJSON(ctx, clt.Endpoint("webapi", "mfa", "login", "begin"), beginReq) - if err != nil { - return nil, nil, trace.Wrap(err) - } - - challenge := &MFAAuthenticateChallenge{} - if err := json.Unmarshal(challengeJSON.Bytes(), challenge); err != nil { - return nil, nil, trace.Wrap(err) - } - - // Convert to auth gRPC proto challenge. - chal := &proto.MFAAuthenticateChallenge{} - if challenge.TOTPChallenge { - chal.TOTP = &proto.TOTPChallenge{} - } - if challenge.WebauthnChallenge != nil { - chal.WebauthnChallenge = wantypes.CredentialAssertionToProto(challenge.WebauthnChallenge) - } - - promptMFA := login.PromptMFA - if promptMFA == nil { - promptMFA = libmfa.NewCLIPrompt(libmfa.NewPromptConfig(login.ProxyAddr), os.Stderr) - } - - respPB, err := promptMFA.Run(ctx, chal) + mfaResp, err := newMFALoginCeremony(clt, login).Run(ctx, nil) if err != nil { return nil, nil, trace.Wrap(err) } @@ -1003,7 +976,7 @@ func SSHAgentMFAWebSessionLogin(ctx context.Context, login SSHLoginMFA) (*WebCli User: login.User, } // Convert back from auth gRPC proto response. - switch r := respPB.Response.(type) { + switch r := mfaResp.Response.(type) { case *proto.MFAAuthenticateResponse_Webauthn: challengeResp.WebauthnAssertionResponse = wantypes.CredentialAssertionResponseFromProto(r.Webauthn) default: diff --git a/lib/teleterm/clusters/cluster_auth.go b/lib/teleterm/clusters/cluster_auth.go index 7eec3e77aadce..ee0d7d1b58e82 100644 --- a/lib/teleterm/clusters/cluster_auth.go +++ b/lib/teleterm/clusters/cluster_auth.go @@ -229,10 +229,10 @@ func (c *Cluster) localMFALogin(user, password string) client.SSHLoginFunc { } response, err := client.SSHAgentMFALogin(ctx, client.SSHLoginMFA{ - SSHLogin: sshLogin, - User: user, - Password: password, - PromptMFA: c.clusterClient.NewMFAPrompt(), + SSHLogin: sshLogin, + User: user, + Password: password, + MFAPromptConstructor: c.clusterClient.NewMFAPrompt, }) if err != nil { return nil, trace.Wrap(err) From 95b030eeec83a006d5e132437a39d4a21a9b8be4 Mon Sep 17 00:00:00 2001 From: joerger Date: Fri, 20 Sep 2024 19:39:21 -0700 Subject: [PATCH 05/11] Rename prompt constructor. --- api/client/mfa.go | 2 +- api/mfa/ceremony.go | 8 ++++---- api/mfa/ceremony_test.go | 2 +- lib/client/mfa.go | 2 +- lib/client/weblogin.go | 2 +- 5 files changed, 8 insertions(+), 8 deletions(-) diff --git a/api/client/mfa.go b/api/client/mfa.go index 51340d036506f..beba5b20c79dd 100644 --- a/api/client/mfa.go +++ b/api/client/mfa.go @@ -29,7 +29,7 @@ import ( func (c *Client) PerformMFACeremony(ctx context.Context, challengeRequest *proto.CreateAuthenticateChallengeRequest, promptOpts ...mfa.PromptOpt) (*proto.MFAAuthenticateResponse, error) { mfaCeremony := &mfa.Ceremony{ CreateAuthenticateChallenge: c.CreateAuthenticateChallenge, - NewPrompt: c.c.MFAPromptConstructor, + PromptConstructor: c.c.MFAPromptConstructor, } return mfaCeremony.Run(ctx, challengeRequest, promptOpts...) } diff --git a/api/mfa/ceremony.go b/api/mfa/ceremony.go index 1e548fbe1c6a3..3f464ed26b9ca 100644 --- a/api/mfa/ceremony.go +++ b/api/mfa/ceremony.go @@ -29,7 +29,7 @@ import ( type Ceremony struct { CreateAuthenticateChallenge func(ctx context.Context, req *proto.CreateAuthenticateChallengeRequest) (*proto.MFAAuthenticateChallenge, error) SolveAuthenticateChallenge func(ctx context.Context, chal *proto.MFAAuthenticateChallenge) (*proto.MFAAuthenticateResponse, error) - NewPrompt PromptConstructor + PromptConstructor PromptConstructor } // Run runs the MFA ceremony. @@ -38,8 +38,8 @@ func (c *Ceremony) Run(ctx context.Context, req *proto.CreateAuthenticateChallen return nil, trace.BadParameter("mfa ceremony must have CreateAuthenticateChallenge set in order to begin") } - if c.SolveAuthenticateChallenge == nil && c.NewPrompt == nil { - return nil, trace.BadParameter("mfa ceremony must have SolveAuthenticateChallenge or NewPrompt set in order to succeed") + if c.SolveAuthenticateChallenge == nil && c.PromptConstructor == nil { + return nil, trace.BadParameter("mfa ceremony must have SolveAuthenticateChallenge or PromptConstructor set in order to succeed") } if req != nil { @@ -73,7 +73,7 @@ func (c *Ceremony) Run(ctx context.Context, req *proto.CreateAuthenticateChallen return c.SolveAuthenticateChallenge(ctx, chal) } - return c.NewPrompt(promptOpts...).Run(ctx, chal) + return c.PromptConstructor(promptOpts...).Run(ctx, chal) } // CeremonyFn is a function that will carry out an MFA ceremony. diff --git a/api/mfa/ceremony_test.go b/api/mfa/ceremony_test.go index d87f8348e4695..8bcfd4cafb3ce 100644 --- a/api/mfa/ceremony_test.go +++ b/api/mfa/ceremony_test.go @@ -89,7 +89,7 @@ func TestPerformMFACeremony(t *testing.T) { t.Run(tt.name, func(t *testing.T) { ceremony := mfa.Ceremony{ CreateAuthenticateChallenge: tt.ceremonyClient.CreateAuthenticateChallenge, - NewPrompt: func(_ ...mfa.PromptOpt) mfa.Prompt { + PromptConstructor: func(_ ...mfa.PromptOpt) mfa.Prompt { return mfa.PromptFunc(tt.ceremonyClient.PromptMFA) }, } diff --git a/lib/client/mfa.go b/lib/client/mfa.go index 3c01baad11187..ae5a4576286ce 100644 --- a/lib/client/mfa.go +++ b/lib/client/mfa.go @@ -33,7 +33,7 @@ import ( func (tc *TeleportClient) NewMFACeremony() *mfa.Ceremony { return &mfa.Ceremony{ CreateAuthenticateChallenge: tc.CreateAuthenticateChallenge, - NewPrompt: tc.NewMFAPrompt, + PromptConstructor: tc.NewMFAPrompt, } } diff --git a/lib/client/weblogin.go b/lib/client/weblogin.go index 1b41c5d740f52..5ad373a884099 100644 --- a/lib/client/weblogin.go +++ b/lib/client/weblogin.go @@ -827,7 +827,7 @@ func newMFALoginCeremony(clt *WebClient, login SSHLoginMFA) *mfa.Ceremony { } return chal, nil }, - NewPrompt: login.MFAPromptConstructor, + PromptConstructor: login.MFAPromptConstructor, } } From 1bc1f4294ad8e95e44c67fd484181b5690242059 Mon Sep 17 00:00:00 2001 From: joerger Date: Mon, 30 Sep 2024 11:19:54 -0700 Subject: [PATCH 06/11] Add godoc to ceremony; update tests. --- api/mfa/ceremony.go | 8 ++- api/mfa/ceremony_test.go | 121 ++++++++++++++++++++++----------------- 2 files changed, 76 insertions(+), 53 deletions(-) diff --git a/api/mfa/ceremony.go b/api/mfa/ceremony.go index 3f464ed26b9ca..b7d9f5106ce27 100644 --- a/api/mfa/ceremony.go +++ b/api/mfa/ceremony.go @@ -27,9 +27,13 @@ import ( // Ceremony is an MFA ceremony. type Ceremony struct { + // CreateAuthenticateChallenge creates an authentication challenge. CreateAuthenticateChallenge func(ctx context.Context, req *proto.CreateAuthenticateChallengeRequest) (*proto.MFAAuthenticateChallenge, error) - SolveAuthenticateChallenge func(ctx context.Context, chal *proto.MFAAuthenticateChallenge) (*proto.MFAAuthenticateResponse, error) - PromptConstructor PromptConstructor + // PromptConstructor creates a prompt to prompt the user to solve an authentication challenge. + PromptConstructor PromptConstructor + // SolveAuthenticateChallenge solves an authentication challenge. Used in non-interactive settings, + // such as the WebUI with layers abstracting user interaction, and tests. + SolveAuthenticateChallenge func(ctx context.Context, chal *proto.MFAAuthenticateChallenge) (*proto.MFAAuthenticateResponse, error) } // Run runs the MFA ceremony. diff --git a/api/mfa/ceremony_test.go b/api/mfa/ceremony_test.go index 8bcfd4cafb3ce..ef92a4380f1d8 100644 --- a/api/mfa/ceremony_test.go +++ b/api/mfa/ceremony_test.go @@ -23,6 +23,8 @@ import ( "github.com/stretchr/testify/assert" + "github.com/gravitational/trace" + "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" @@ -32,6 +34,9 @@ func TestPerformMFACeremony(t *testing.T) { t.Parallel() ctx := context.Background() + testMFAChallenge := &proto.MFAAuthenticateChallenge{ + TOTP: &proto.TOTPChallenge{}, + } testMFAResponse := &proto.MFAAuthenticateResponse{ Response: &proto.MFAAuthenticateResponse_TOTP{ TOTP: &proto.TOTPResponse{ @@ -42,13 +47,34 @@ func TestPerformMFACeremony(t *testing.T) { for _, tt := range []struct { name string - ceremonyClient *fakeMFACeremonyClient + ceremony *mfa.Ceremony assertCeremonyResponse func(*testing.T, *proto.MFAAuthenticateResponse, error, ...interface{}) }{ { - name: "OK ceremony success", - ceremonyClient: &fakeMFACeremonyClient{ - challengeResponse: testMFAResponse, + name: "OK ceremony success prompt", + ceremony: &mfa.Ceremony{ + CreateAuthenticateChallenge: func(ctx context.Context, req *proto.CreateAuthenticateChallengeRequest) (*proto.MFAAuthenticateChallenge, error) { + return testMFAChallenge, nil + }, + PromptConstructor: func(po ...mfa.PromptOpt) mfa.Prompt { + return mfa.PromptFunc(func(ctx context.Context, chal *proto.MFAAuthenticateChallenge) (*proto.MFAAuthenticateResponse, error) { + return testMFAResponse, nil + }) + }, + }, + assertCeremonyResponse: func(t *testing.T, mr *proto.MFAAuthenticateResponse, err error, i ...interface{}) { + assert.NoError(t, err) + assert.Equal(t, testMFAResponse, mr) + }, + }, { + name: "OK ceremony success solve", + ceremony: &mfa.Ceremony{ + CreateAuthenticateChallenge: func(ctx context.Context, req *proto.CreateAuthenticateChallengeRequest) (*proto.MFAAuthenticateChallenge, error) { + return testMFAChallenge, nil + }, + SolveAuthenticateChallenge: func(ctx context.Context, chal *proto.MFAAuthenticateChallenge) (*proto.MFAAuthenticateResponse, error) { + return testMFAResponse, nil + }, }, assertCeremonyResponse: func(t *testing.T, mr *proto.MFAAuthenticateResponse, err error, i ...interface{}) { assert.NoError(t, err) @@ -56,9 +82,15 @@ func TestPerformMFACeremony(t *testing.T) { }, }, { name: "OK ceremony not required", - ceremonyClient: &fakeMFACeremonyClient{ - challengeResponse: testMFAResponse, - mfaRequired: proto.MFARequired_MFA_REQUIRED_NO, + ceremony: &mfa.Ceremony{ + CreateAuthenticateChallenge: func(ctx context.Context, req *proto.CreateAuthenticateChallengeRequest) (*proto.MFAAuthenticateChallenge, error) { + return &proto.MFAAuthenticateChallenge{ + MFARequired: proto.MFARequired_MFA_REQUIRED_NO, + }, nil + }, + SolveAuthenticateChallenge: func(ctx context.Context, chal *proto.MFAAuthenticateChallenge) (*proto.MFAAuthenticateResponse, error) { + return nil, trace.BadParameter("expected mfa not required") + }, }, assertCeremonyResponse: func(t *testing.T, mr *proto.MFAAuthenticateResponse, err error, i ...interface{}) { assert.Error(t, err, mfa.ErrMFANotRequired) @@ -66,9 +98,13 @@ func TestPerformMFACeremony(t *testing.T) { }, }, { name: "NOK create challenge fail", - ceremonyClient: &fakeMFACeremonyClient{ - challengeResponse: testMFAResponse, - createAuthenticateChallengeErr: errors.New("create authenticate challenge failure"), + ceremony: &mfa.Ceremony{ + CreateAuthenticateChallenge: func(ctx context.Context, req *proto.CreateAuthenticateChallengeRequest) (*proto.MFAAuthenticateChallenge, error) { + return nil, errors.New("create authenticate challenge failure") + }, + SolveAuthenticateChallenge: func(ctx context.Context, chal *proto.MFAAuthenticateChallenge) (*proto.MFAAuthenticateResponse, error) { + return nil, trace.BadParameter("expected challenge failure") + }, }, assertCeremonyResponse: func(t *testing.T, mr *proto.MFAAuthenticateResponse, err error, i ...interface{}) { assert.ErrorContains(t, err, "create authenticate challenge failure") @@ -76,24 +112,38 @@ func TestPerformMFACeremony(t *testing.T) { }, }, { name: "NOK prompt mfa fail", - ceremonyClient: &fakeMFACeremonyClient{ - challengeResponse: testMFAResponse, - promptMFAErr: errors.New("prompt mfa failure"), + ceremony: &mfa.Ceremony{ + CreateAuthenticateChallenge: func(ctx context.Context, req *proto.CreateAuthenticateChallengeRequest) (*proto.MFAAuthenticateChallenge, error) { + return testMFAChallenge, nil + }, + PromptConstructor: func(po ...mfa.PromptOpt) mfa.Prompt { + return mfa.PromptFunc(func(ctx context.Context, chal *proto.MFAAuthenticateChallenge) (*proto.MFAAuthenticateResponse, error) { + return nil, errors.New("prompt mfa failure") + }) + }, }, assertCeremonyResponse: func(t *testing.T, mr *proto.MFAAuthenticateResponse, err error, i ...interface{}) { assert.ErrorContains(t, err, "prompt mfa failure") assert.Nil(t, mr) }, + }, { + name: "NOK solve mfa fail", + ceremony: &mfa.Ceremony{ + CreateAuthenticateChallenge: func(ctx context.Context, req *proto.CreateAuthenticateChallengeRequest) (*proto.MFAAuthenticateChallenge, error) { + return testMFAChallenge, nil + }, + SolveAuthenticateChallenge: func(ctx context.Context, chal *proto.MFAAuthenticateChallenge) (*proto.MFAAuthenticateResponse, error) { + return nil, errors.New("solve mfa failure") + }, + }, + assertCeremonyResponse: func(t *testing.T, mr *proto.MFAAuthenticateResponse, err error, i ...interface{}) { + assert.ErrorContains(t, err, "solve mfa failure") + assert.Nil(t, mr) + }, }, } { t.Run(tt.name, func(t *testing.T) { - ceremony := mfa.Ceremony{ - CreateAuthenticateChallenge: tt.ceremonyClient.CreateAuthenticateChallenge, - PromptConstructor: func(_ ...mfa.PromptOpt) mfa.Prompt { - return mfa.PromptFunc(tt.ceremonyClient.PromptMFA) - }, - } - resp, err := ceremony.Run(ctx, &proto.CreateAuthenticateChallengeRequest{ + resp, err := tt.ceremony.Run(ctx, &proto.CreateAuthenticateChallengeRequest{ ChallengeExtensions: &mfav1.ChallengeExtensions{ Scope: mfav1.ChallengeScope_CHALLENGE_SCOPE_ADMIN_ACTION, }, @@ -103,34 +153,3 @@ func TestPerformMFACeremony(t *testing.T) { }) } } - -type fakeMFACeremonyClient struct { - createAuthenticateChallengeErr error - promptMFAErr error - mfaRequired proto.MFARequired - challengeResponse *proto.MFAAuthenticateResponse -} - -func (c *fakeMFACeremonyClient) CreateAuthenticateChallenge(ctx context.Context, in *proto.CreateAuthenticateChallengeRequest) (*proto.MFAAuthenticateChallenge, error) { - if c.createAuthenticateChallengeErr != nil { - return nil, c.createAuthenticateChallengeErr - } - - chal := &proto.MFAAuthenticateChallenge{ - TOTP: &proto.TOTPChallenge{}, - } - - if in.MFARequiredCheck != nil { - chal.MFARequired = c.mfaRequired - } - - return chal, nil -} - -func (c *fakeMFACeremonyClient) PromptMFA(ctx context.Context, chal *proto.MFAAuthenticateChallenge) (*proto.MFAAuthenticateResponse, error) { - if c.promptMFAErr != nil { - return nil, c.promptMFAErr - } - - return c.challengeResponse, nil -} From add21a5b1d688f1700b708add79bd8d9e3c8bd23 Mon Sep 17 00:00:00 2001 From: joerger Date: Mon, 30 Sep 2024 11:55:45 -0700 Subject: [PATCH 07/11] Cleanup. --- lib/auth/helpers_mfa.go | 3 +++ lib/client/api.go | 3 +++ lib/client/cluster_client.go | 13 +++---------- lib/web/desktop.go | 8 ++------ lib/web/kube.go | 4 ---- lib/web/terminal.go | 6 +----- 6 files changed, 12 insertions(+), 25 deletions(-) diff --git a/lib/auth/helpers_mfa.go b/lib/auth/helpers_mfa.go index 1c61788603061..d41e5e6e95ac4 100644 --- a/lib/auth/helpers_mfa.go +++ b/lib/auth/helpers_mfa.go @@ -115,6 +115,9 @@ func (d *TestDevice) registerDevice(ctx context.Context, authClient authClientI, Scope: mfav1.ChallengeScope_CHALLENGE_SCOPE_MANAGE_DEVICES, }, }) + if err != nil { + return trace.Wrap(err) + } // Acquire and solve registration challenge. usage := proto.DeviceUsage_DEVICE_USAGE_MFA diff --git a/lib/client/api.go b/lib/client/api.go index 7c485eda0cb6c..8a4625a0a32de 100644 --- a/lib/client/api.go +++ b/lib/client/api.go @@ -5189,6 +5189,9 @@ func (tc *TeleportClient) HeadlessApprove(ctx context.Context, headlessAuthentic Scope: mfav1.ChallengeScope_CHALLENGE_SCOPE_HEADLESS_LOGIN, }, }) + if err != nil { + return trace.Wrap(err) + } err = rootClient.UpdateHeadlessAuthenticationState(ctx, headlessAuthenticationID, types.HeadlessAuthenticationState_HEADLESS_AUTHENTICATION_STATE_APPROVED, mfaResp) return trace.Wrap(err) diff --git a/lib/client/cluster_client.go b/lib/client/cluster_client.go index c551fe0743736..c68fa36179908 100644 --- a/lib/client/cluster_client.go +++ b/lib/client/cluster_client.go @@ -458,12 +458,9 @@ func (c *ClusterClient) performSessionMFACeremony(ctx context.Context, rootClien MFACeremony: c.tc.NewMFACeremony(), MFAAgainstRoot: c.cluster == rootClient.cluster, MFARequiredReq: mfaRequiredReq, - ChallengeExtensions: mfav1.ChallengeExtensions{ - Scope: mfav1.ChallengeScope_CHALLENGE_SCOPE_USER_SESSION, - }, - CertsReq: certsReq, - KeyRing: keyRing, - newUserKeys: newUserKeys, + CertsReq: certsReq, + KeyRing: keyRing, + newUserKeys: newUserKeys, }, promptOpts...) return keyRing, trace.Wrap(err) } @@ -609,9 +606,6 @@ type PerformSessionMFACeremonyParams struct { MFAAgainstRoot bool // MFARequiredReq is the request for the MFA verification check. MFARequiredReq *proto.IsMFARequiredRequest - // ChallengeExtensions is used to provide additional extensions to apply to the - // MFA challenge used in the ceremony. The scope extension must be supplied. - ChallengeExtensions mfav1.ChallengeExtensions // CertsReq is the request for new certificates. CertsReq *proto.UserCertsRequest @@ -647,7 +641,6 @@ type newUserKeys struct { func PerformSessionMFACeremony(ctx context.Context, params PerformSessionMFACeremonyParams, promptOpts ...mfa.PromptOpt) (*KeyRing, *proto.Certs, error) { rootClient := params.RootAuthClient currentClient := params.CurrentAuthClient - mfaRequiredReq := params.MFARequiredReq // If connecting to a host in a leaf cluster and MFA failed check to see diff --git a/lib/web/desktop.go b/lib/web/desktop.go index 6a00b05788bfe..5c27ce096481e 100644 --- a/lib/web/desktop.go +++ b/lib/web/desktop.go @@ -37,7 +37,6 @@ import ( "github.com/gravitational/teleport/api/client/proto" "github.com/gravitational/teleport/api/constants" - mfav1 "github.com/gravitational/teleport/api/gen/proto/go/teleport/mfa/v1" "github.com/gravitational/teleport/api/mfa" "github.com/gravitational/teleport/api/types" "github.com/gravitational/teleport/api/utils/keys" @@ -438,11 +437,8 @@ func (h *Handler) performSessionMFACeremony( MFACeremony: mfaCeremony, MFAAgainstRoot: true, MFARequiredReq: nil, // No need to verify. - ChallengeExtensions: mfav1.ChallengeExtensions{ - Scope: mfav1.ChallengeScope_CHALLENGE_SCOPE_USER_SESSION, - }, - CertsReq: certsReq, - KeyRing: nil, // We just want the certs. + CertsReq: certsReq, + KeyRing: nil, // We just want the certs. }) if err != nil { return nil, trace.Wrap(err) diff --git a/lib/web/kube.go b/lib/web/kube.go index b59f21d1fb0ea..3c8a7c32d7602 100644 --- a/lib/web/kube.go +++ b/lib/web/kube.go @@ -41,7 +41,6 @@ import ( clientproto "github.com/gravitational/teleport/api/client/proto" "github.com/gravitational/teleport/api/constants" - mfav1 "github.com/gravitational/teleport/api/gen/proto/go/teleport/mfa/v1" "github.com/gravitational/teleport/api/mfa" "github.com/gravitational/teleport/api/types" "github.com/gravitational/teleport/api/utils/keys" @@ -257,9 +256,6 @@ func (p *podHandler) handler(r *http.Request) error { MFARequiredReq: &clientproto.IsMFARequiredRequest{ Target: &clientproto.IsMFARequiredRequest_KubernetesCluster{KubernetesCluster: p.req.KubeCluster}, }, - ChallengeExtensions: mfav1.ChallengeExtensions{ - Scope: mfav1.ChallengeScope_CHALLENGE_SCOPE_USER_SESSION, - }, CertsReq: &certsReq, }) if err != nil && !errors.Is(err, services.ErrSessionMFANotRequired) { diff --git a/lib/web/terminal.go b/lib/web/terminal.go index b8e61bbe595e6..ed8ba51c56018 100644 --- a/lib/web/terminal.go +++ b/lib/web/terminal.go @@ -43,7 +43,6 @@ import ( "github.com/gravitational/teleport" authproto "github.com/gravitational/teleport/api/client/proto" apidefaults "github.com/gravitational/teleport/api/defaults" - mfav1 "github.com/gravitational/teleport/api/gen/proto/go/teleport/mfa/v1" "github.com/gravitational/teleport/api/mfa" "github.com/gravitational/teleport/api/observability/tracing" tracessh "github.com/gravitational/teleport/api/observability/tracing/ssh" @@ -602,10 +601,7 @@ func (t *sshBaseHandler) issueSessionMFACerts(ctx context.Context, tc *client.Te MFACeremony: mfaCeremony, MFAAgainstRoot: t.ctx.cfg.RootClusterName == tc.SiteName, MFARequiredReq: mfaRequiredReq, - ChallengeExtensions: mfav1.ChallengeExtensions{ - Scope: mfav1.ChallengeScope_CHALLENGE_SCOPE_USER_SESSION, - }, - CertsReq: certsReq, + CertsReq: certsReq, }) if err != nil { return nil, trace.Wrap(err) From 09dbce82cc41417e693e3c52f65957e14cfdbce1 Mon Sep 17 00:00:00 2001 From: joerger Date: Tue, 1 Oct 2024 13:58:02 -0700 Subject: [PATCH 08/11] Resolve comments; fix tests. --- api/mfa/ceremony.go | 36 ++++++++++++++++--------------- lib/client/cluster_client.go | 1 + lib/client/cluster_client_test.go | 2 +- lib/client/mfa.go | 9 ++++---- lib/client/presence.go | 1 - lib/web/terminal.go | 1 - 6 files changed, 26 insertions(+), 24 deletions(-) diff --git a/api/mfa/ceremony.go b/api/mfa/ceremony.go index b7d9f5106ce27..a0cf22cdfd994 100644 --- a/api/mfa/ceremony.go +++ b/api/mfa/ceremony.go @@ -38,22 +38,18 @@ type Ceremony struct { // Run runs the MFA ceremony. func (c *Ceremony) Run(ctx context.Context, req *proto.CreateAuthenticateChallengeRequest, promptOpts ...PromptOpt) (*proto.MFAAuthenticateResponse, error) { - if c.CreateAuthenticateChallenge == nil { + switch { + case c.CreateAuthenticateChallenge == nil: return nil, trace.BadParameter("mfa ceremony must have CreateAuthenticateChallenge set in order to begin") - } - - if c.SolveAuthenticateChallenge == nil && c.PromptConstructor == nil { - return nil, trace.BadParameter("mfa ceremony must have SolveAuthenticateChallenge or PromptConstructor set in order to succeed") - } - - if req != nil { - if req.ChallengeExtensions == nil { - return nil, trace.BadParameter("missing challenge extensions") - } - - if req.ChallengeExtensions.Scope == mfav1.ChallengeScope_CHALLENGE_SCOPE_UNSPECIFIED { - return nil, trace.BadParameter("mfa challenge scope must be specified") - } + case c.SolveAuthenticateChallenge != nil && c.PromptConstructor != nil: + return nil, trace.BadParameter("mfa ceremony should have SolveAuthenticateChallenge or PromptConstructor set, not both") + case req == nil: + // req may be nil in cases where the ceremony's CreateAuthenticateChallenge sources + // its own req or uses a different rpc, e.g. moderated sessions. + case req.ChallengeExtensions == nil: + return nil, trace.BadParameter("missing challenge extensions") + case req.ChallengeExtensions.Scope == mfav1.ChallengeScope_CHALLENGE_SCOPE_UNSPECIFIED: + return nil, trace.BadParameter("mfa challenge scope must be specified") } chal, err := c.CreateAuthenticateChallenge(ctx, req) @@ -73,11 +69,17 @@ func (c *Ceremony) Run(ctx context.Context, req *proto.CreateAuthenticateChallen return nil, &ErrMFANotRequired } + if c.SolveAuthenticateChallenge == nil && c.PromptConstructor == nil { + return nil, trace.Wrap(&ErrMFANotSupported, "mfa ceremony must have SolveAuthenticateChallenge or PromptConstructor set in order to succeed") + } + if c.SolveAuthenticateChallenge != nil { - return c.SolveAuthenticateChallenge(ctx, chal) + resp, err := c.SolveAuthenticateChallenge(ctx, chal) + return resp, trace.Wrap(err) } - return c.PromptConstructor(promptOpts...).Run(ctx, chal) + resp, err := c.PromptConstructor(promptOpts...).Run(ctx, chal) + return resp, trace.Wrap(err) } // CeremonyFn is a function that will carry out an MFA ceremony. diff --git a/lib/client/cluster_client.go b/lib/client/cluster_client.go index c68fa36179908..ce0a7d6485a6e 100644 --- a/lib/client/cluster_client.go +++ b/lib/client/cluster_client.go @@ -658,6 +658,7 @@ func PerformSessionMFACeremony(ctx context.Context, params PerformSessionMFACere mfaRequiredReq = nil // Already checked, don't check again at root. } + params.MFACeremony.CreateAuthenticateChallenge = rootClient.CreateAuthenticateChallenge mfaResp, err := params.MFACeremony.Run(ctx, &proto.CreateAuthenticateChallengeRequest{ Request: &proto.CreateAuthenticateChallengeRequest_ContextUser{ ContextUser: &proto.ContextUser{}, diff --git a/lib/client/cluster_client_test.go b/lib/client/cluster_client_test.go index bfcd088d34d52..70a9985853ceb 100644 --- a/lib/client/cluster_client_test.go +++ b/lib/client/cluster_client_test.go @@ -364,7 +364,7 @@ func TestIssueUserCertsWithMFA(t *testing.T) { SiteName: "test", Tracer: tracing.NoopTracer("test"), MFAPromptConstructor: func(cfg *libmfa.PromptConfig) mfa.Prompt { - return test.prompt.Prompt + return test.prompt }, }, lastPing: &webclient.PingResponse{ diff --git a/lib/client/mfa.go b/lib/client/mfa.go index ae5a4576286ce..7dc728f5d6c93 100644 --- a/lib/client/mfa.go +++ b/lib/client/mfa.go @@ -21,24 +21,25 @@ package client import ( "context" + "github.com/gravitational/trace" + "github.com/gravitational/teleport/api/client/proto" "github.com/gravitational/teleport/api/mfa" wancli "github.com/gravitational/teleport/lib/auth/webauthncli" wantypes "github.com/gravitational/teleport/lib/auth/webauthntypes" libmfa "github.com/gravitational/teleport/lib/client/mfa" - "github.com/gravitational/trace" ) // NewMFACeremony returns a new MFA ceremony configured for this client. func (tc *TeleportClient) NewMFACeremony() *mfa.Ceremony { return &mfa.Ceremony{ - CreateAuthenticateChallenge: tc.CreateAuthenticateChallenge, + CreateAuthenticateChallenge: tc.createAuthenticateChallenge, PromptConstructor: tc.NewMFAPrompt, } } -// CreateAuthenticateChallenge creates and returns MFA challenges for a users registered MFA devices. -func (tc *TeleportClient) CreateAuthenticateChallenge(ctx context.Context, req *proto.CreateAuthenticateChallengeRequest) (*proto.MFAAuthenticateChallenge, error) { +// createAuthenticateChallenge creates and returns MFA challenges for a users registered MFA devices. +func (tc *TeleportClient) createAuthenticateChallenge(ctx context.Context, req *proto.CreateAuthenticateChallengeRequest) (*proto.MFAAuthenticateChallenge, error) { clusterClient, err := tc.ConnectToCluster(ctx) if err != nil { return nil, trace.Wrap(err) diff --git a/lib/client/presence.go b/lib/client/presence.go index 46f2a31e0637e..c50fbf004ca26 100644 --- a/lib/client/presence.go +++ b/lib/client/presence.go @@ -57,7 +57,6 @@ func WithPresenceClock(clock clockwork.Clock) PresenceOption { // RunPresenceTask periodically performs and MFA ceremony to detect that a user is // still present and attentive. func RunPresenceTask(ctx context.Context, term io.Writer, maintainer PresenceMaintainer, sessionID string, mfaPrompt mfa.Prompt, opts ...PresenceOption) error { - fmt.Fprintf(term, "\r\nTeleport > MFA presence enabled\r\n") o := &presenceOptions{ diff --git a/lib/web/terminal.go b/lib/web/terminal.go index ed8ba51c56018..4839a15827d92 100644 --- a/lib/web/terminal.go +++ b/lib/web/terminal.go @@ -639,7 +639,6 @@ func mfaPrompt(stream *terminal.WSStream, codec terminal.MFACodec) mfa.Prompt { resp, err := stream.ReadChallengeResponse(codec) return resp, trace.Wrap(err) }) - } type connectWithMFAFn = func(ctx context.Context, ws terminal.WSConn, tc *client.TeleportClient, accessChecker services.AccessChecker, getAgent teleagent.Getter, signer agentless.SignerCreator) (*client.NodeClient, error) From 2e82ef316029a1724eaabe30fb59f7dee6020fff Mon Sep 17 00:00:00 2001 From: joerger Date: Tue, 1 Oct 2024 17:32:07 -0700 Subject: [PATCH 09/11] Update comments. --- api/mfa/ceremony.go | 5 ++++- lib/client/presence.go | 2 +- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/api/mfa/ceremony.go b/api/mfa/ceremony.go index a0cf22cdfd994..f3c5f88e23d65 100644 --- a/api/mfa/ceremony.go +++ b/api/mfa/ceremony.go @@ -36,7 +36,10 @@ type Ceremony struct { SolveAuthenticateChallenge func(ctx context.Context, chal *proto.MFAAuthenticateChallenge) (*proto.MFAAuthenticateResponse, error) } -// Run runs the MFA ceremony. +// Run the MFA ceremony. +// +// 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) { switch { case c.CreateAuthenticateChallenge == nil: diff --git a/lib/client/presence.go b/lib/client/presence.go index c50fbf004ca26..14a8cb5d542f6 100644 --- a/lib/client/presence.go +++ b/lib/client/presence.go @@ -116,7 +116,7 @@ func RunPresenceTask(ctx context.Context, term io.Writer, maintainer PresenceMai for { select { case <-ticker.Chan(): - mfaResp, err := mfaCeremony.Run(ctx, nil) + mfaResp, err := mfaCeremony.Run(ctx, nil /* req is not needed for MaintainSessionPresence */) if err != nil { return trace.Wrap(err) } From df669318b091754d70a05f181e55e01e49af0f0d Mon Sep 17 00:00:00 2001 From: joerger Date: Tue, 1 Oct 2024 17:51:04 -0700 Subject: [PATCH 10/11] Fix test. --- tool/tsh/common/tsh_test.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/tool/tsh/common/tsh_test.go b/tool/tsh/common/tsh_test.go index 97e4dc47ffdaa..3604f6e42e794 100644 --- a/tool/tsh/common/tsh_test.go +++ b/tool/tsh/common/tsh_test.go @@ -1407,7 +1407,7 @@ func TestSSHOnMultipleNodes(t *testing.T) { webauthnLogin: successfulChallenge("localhost"), target: "env=prod", stderrAssertion: func(tt require.TestingT, i interface{}, i2 ...interface{}) { - require.Equal(t, "error\n", i, i2...) + require.Contains(t, i, "error\n", i2...) }, stdoutAssertion: func(t require.TestingT, i interface{}, i2 ...interface{}) { require.Equal(t, "test\n", i, i2...) @@ -1502,7 +1502,7 @@ func TestSSHOnMultipleNodes(t *testing.T) { require.Equal(t, "test\n", i, i2...) }, stderrAssertion: func(tt require.TestingT, i interface{}, i2 ...interface{}) { - require.Equal(t, "error\n", i, i2...) + require.Contains(t, i, "error\n", i2...) }, mfaPromptCount: 1, errAssertion: require.NoError, @@ -1589,7 +1589,7 @@ func TestSSHOnMultipleNodes(t *testing.T) { roles: []string{perSessionMFARole.GetName()}, webauthnLogin: successfulChallenge("leafcluster"), stderrAssertion: func(tt require.TestingT, i interface{}, i2 ...interface{}) { - require.Equal(t, "error\n", i, i2...) + require.Contains(t, i, "error\n", i2...) }, stdoutAssertion: func(t require.TestingT, i interface{}, i2 ...interface{}) { require.Equal(t, "test\n", i, i2...) @@ -1637,7 +1637,7 @@ func TestSSHOnMultipleNodes(t *testing.T) { roles: []string{perSessionMFARole.GetName()}, webauthnLogin: successfulChallenge("localhost"), stderrAssertion: func(tt require.TestingT, i interface{}, i2 ...interface{}) { - require.Equal(t, "error\n", i, i2...) + require.Contains(t, i, "error\n", i2...) }, stdoutAssertion: func(t require.TestingT, i interface{}, i2 ...interface{}) { require.Equal(t, "test\n", i, i2...) From bf690e49b5cfa11a264777460bd0bdeb970ff9d9 Mon Sep 17 00:00:00 2001 From: joerger Date: Wed, 2 Oct 2024 10:32:42 -0700 Subject: [PATCH 11/11] Fix lint. --- api/mfa/ceremony_test.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/api/mfa/ceremony_test.go b/api/mfa/ceremony_test.go index ef92a4380f1d8..bb6a24b6fcdbe 100644 --- a/api/mfa/ceremony_test.go +++ b/api/mfa/ceremony_test.go @@ -21,9 +21,8 @@ import ( "errors" "testing" - "github.com/stretchr/testify/assert" - "github.com/gravitational/trace" + "github.com/stretchr/testify/assert" "github.com/gravitational/teleport/api/client/proto" mfav1 "github.com/gravitational/teleport/api/gen/proto/go/teleport/mfa/v1"