diff --git a/api/mfa/ceremony.go b/api/mfa/ceremony.go index 8159cf34d29f3..ca7a8640f62bc 100644 --- a/api/mfa/ceremony.go +++ b/api/mfa/ceremony.go @@ -36,8 +36,7 @@ type MFACeremonyClient interface { // 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. A nil response will be returned if an MFA required check -// is provided and MFA is not required. +// 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") @@ -53,13 +52,19 @@ func PerformMFACeremony(ctx context.Context, clt MFACeremonyClient, challengeReq chal, err := clt.CreateAuthenticateChallenge(ctx, challengeRequest) if err != nil { + // CreateAuthenticateChallenge returns a bad parameter error when the the client + // user is not a Teleport user. For example, the AdminRole. Treat this as a false + // MFA required check if a check was requested. + if trace.IsBadParameter(err) { + return nil, &ErrMFANotRequired + } return nil, trace.Wrap(err) } // If an MFA required check was provided, and the client discovers MFA is not required, // skip the MFA prompt and return an empty response. if chal.MFARequired == proto.MFARequired_MFA_REQUIRED_NO { - return nil, nil + return nil, &ErrMFANotRequired } return clt.PromptMFA(ctx, chal, promptOpts...) @@ -67,7 +72,6 @@ func PerformMFACeremony(ctx context.Context, clt MFACeremonyClient, challengeReq // 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. -// An empty response will be returned if MFA is not required for the given admin action. func PerformAdminActionMFACeremony(ctx context.Context, clt MFACeremonyClient, allowReuse bool) (*proto.MFAAuthenticateResponse, error) { allowReuseExt := mfav1.ChallengeAllowReuse_CHALLENGE_ALLOW_REUSE_NO if allowReuse { diff --git a/api/mfa/ceremony_test.go b/api/mfa/ceremony_test.go index 31f4d4aa8202d..5e9df622534b7 100644 --- a/api/mfa/ceremony_test.go +++ b/api/mfa/ceremony_test.go @@ -61,7 +61,7 @@ func TestPerformMFACeremony(t *testing.T) { mfaRequired: proto.MFARequired_MFA_REQUIRED_NO, }, assertCeremonyResponse: func(t *testing.T, mr *proto.MFAAuthenticateResponse, err error, i ...interface{}) { - assert.NoError(t, err) + assert.Error(t, err, mfa.ErrMFANotRequired) assert.Nil(t, mr) }, }, { diff --git a/api/mfa/mfa.go b/api/mfa/mfa.go index 85636d666920f..fc52da18fb609 100644 --- a/api/mfa/mfa.go +++ b/api/mfa/mfa.go @@ -33,9 +33,16 @@ import ( // ResponseMetadataKey is the context metadata key for an MFA response in a gRPC request. const ResponseMetadataKey = "mfa_challenge_response" -// ErrAdminActionMFARequired is an error indicating that an admin-level -// API request failed due to missing MFA verification. -var ErrAdminActionMFARequired = trace.AccessDeniedError{Message: "admin-level API request requires MFA verification"} +var ( + // ErrAdminActionMFARequired is an error indicating that an admin-level + // API request failed due to missing MFA verification. + ErrAdminActionMFARequired = trace.AccessDeniedError{Message: "admin-level API request requires MFA verification"} + + // ErrMFANotRequired is returned by MFA ceremonies when it is discovered or + // inferred that the MFA ceremony is not necessary. This is usually because + // the server does require/support MFA for the user. + ErrMFANotRequired = trace.BadParameterError{Message: "re-authentication with MFA is not required"} +) // WithCredentials can be called on a GRPC client request to attach // MFA credentials to the GRPC metadata for requests that require MFA, diff --git a/tool/tctl/common/admin_action_test.go b/tool/tctl/common/admin_action_test.go index 558981e3fc0a0..be44091c48d4b 100644 --- a/tool/tctl/common/admin_action_test.go +++ b/tool/tctl/common/admin_action_test.go @@ -995,6 +995,12 @@ func newAdminActionTestSuite(t *testing.T) *adminActionTestSuite { Credentials: []client.Credentials{ client.LoadProfile(tshHome, ""), }, + MFAPromptConstructor: func(po ...mfa.PromptOpt) mfa.Prompt { + return mfa.PromptFunc(func(ctx context.Context, chal *proto.MFAAuthenticateChallenge) (*proto.MFAAuthenticateResponse, error) { + // return MFA not required to gracefully skip the MFA prompt. + return nil, &mfa.ErrMFANotRequired + }) + }, }) require.NoError(t, err) diff --git a/tool/tctl/common/bots_command.go b/tool/tctl/common/bots_command.go index d092fffd9747a..38a75a5526387 100644 --- a/tool/tctl/common/bots_command.go +++ b/tool/tctl/common/bots_command.go @@ -21,6 +21,7 @@ package common import ( "context" "encoding/json" + "errors" "fmt" "maps" "os" @@ -39,6 +40,7 @@ import ( "github.com/gravitational/teleport/api/constants" headerv1 "github.com/gravitational/teleport/api/gen/proto/go/teleport/header/v1" machineidv1pb "github.com/gravitational/teleport/api/gen/proto/go/teleport/machineid/v1" + "github.com/gravitational/teleport/api/mfa" "github.com/gravitational/teleport/api/types" "github.com/gravitational/teleport/lib/asciitable" "github.com/gravitational/teleport/lib/auth" @@ -300,6 +302,14 @@ func (c *BotsCommand) addBotLegacy(ctx context.Context, client auth.ClientI) err // AddBot adds a new certificate renewal bot to the cluster. func (c *BotsCommand) AddBot(ctx context.Context, client auth.ClientI) error { + // Prompt for admin action MFA if required, allowing reuse for UpsertToken and CreateBot. + mfaResponse, err := mfa.PerformAdminActionMFACeremony(ctx, client, true /*allowReuse*/) + if err == nil { + ctx = mfa.ContextWithMFAResponse(ctx, mfaResponse) + } else if !errors.Is(err, &mfa.ErrMFANotRequired) { + return trace.Wrap(err) + } + // Jankily call the endpoint invalidly. This lets us version check and use // the legacy version of this CLI tool if we are talking to an older // server. @@ -318,7 +328,6 @@ func (c *BotsCommand) AddBot(ctx context.Context, client auth.ClientI) error { log.Warning("No roles specified. The bot will not be able to produce outputs until a role is added to the bot.") } var token types.ProvisionToken - var err error if c.tokenID == "" { // If there's no token specified, generate one tokenName, err := utils.CryptoRandomHex(16) diff --git a/tool/tctl/common/user_command.go b/tool/tctl/common/user_command.go index 52bfcf6baa7ce..8437d4a93fd71 100644 --- a/tool/tctl/common/user_command.go +++ b/tool/tctl/common/user_command.go @@ -21,6 +21,7 @@ package common import ( "context" "encoding/json" + "errors" "fmt" "net/url" "os" @@ -299,13 +300,11 @@ func (u *UserCommand) Add(ctx context.Context, client auth.ClientI) error { user.SetRoles(u.allowedRoles) // Prompt for admin action MFA if required, allowing reuse for CreateResetPasswordToken. - if u.config.Auth.Preference.IsAdminActionMFAEnforced() { - mfaResponse, err := mfa.PerformAdminActionMFACeremony(ctx, client, true /*allowReuse*/) - if err != nil { - return trace.Wrap(err) - } else if mfaResponse != nil { - ctx = mfa.ContextWithMFAResponse(ctx, mfaResponse) - } + mfaResponse, err := mfa.PerformAdminActionMFACeremony(ctx, client, true /*allowReuse*/) + if err == nil { + ctx = mfa.ContextWithMFAResponse(ctx, mfaResponse) + } else if !errors.Is(err, &mfa.ErrMFANotRequired) { + return trace.Wrap(err) } if _, err := client.CreateUser(ctx, user); err != nil {