From f38b21d8f1ac87d6974c456dd9ba5c272da4eaff Mon Sep 17 00:00:00 2001 From: Brian Joerger Date: Wed, 13 Mar 2024 16:24:09 -0700 Subject: [PATCH] [v15] Fix admin MFA flow for `tctl bots add` (#39269) * Make the MFA for admin actions interceptor re-prompt for MFA when a request fails due to a reusable MFA response being denied by the server. * Allow reused MFA for get/list tokens. * Update api/utils/grpc/interceptors/mfa.go Co-authored-by: rosstimothy <39066650+rosstimothy@users.noreply.github.com> --------- Co-authored-by: rosstimothy <39066650+rosstimothy@users.noreply.github.com> --- api/utils/grpc/interceptors/mfa.go | 6 +- api/utils/grpc/interceptors/mfa_test.go | 124 ++++++++++++++++++++++-- lib/auth/auth_with_roles.go | 4 +- 3 files changed, 124 insertions(+), 10 deletions(-) diff --git a/api/utils/grpc/interceptors/mfa.go b/api/utils/grpc/interceptors/mfa.go index 851ee700e5b64..ce82aa6e2e0d5 100644 --- a/api/utils/grpc/interceptors/mfa.go +++ b/api/utils/grpc/interceptors/mfa.go @@ -35,7 +35,11 @@ func WithMFAUnaryInterceptor(mfaCeremony mfa.MFACeremony) grpc.UnaryClientInterc 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 { - return invoker(ctx, method, req, reply, cc, append(opts, mfa.WithCredentials(mfaResp))...) + // If we find an MFA response passed through the context, attach it to the + // request. Note: this may still fail if the MFA response allows reuse and + // the specified endpoint doesn't allow reuse. In this case, the client + // prompts for MFA again below. + opts = append(opts, mfa.WithCredentials(mfaResp)) } else if !trace.IsNotFound(err) { return trace.Wrap(err) } diff --git a/api/utils/grpc/interceptors/mfa_test.go b/api/utils/grpc/interceptors/mfa_test.go index f483196e28589..222bf99b14787 100644 --- a/api/utils/grpc/interceptors/mfa_test.go +++ b/api/utils/grpc/interceptors/mfa_test.go @@ -31,20 +31,24 @@ import ( "github.com/gravitational/teleport/api/utils/grpc/interceptors" ) -const otpTestCode = "otp-test-code" +const ( + otpTestCode = "otp-test-code" + otpTestCodeReusable = "otp-test-code-reusable" +) type mfaService struct { + allowReuse bool proto.UnimplementedAuthServiceServer } func (s *mfaService) Ping(ctx context.Context, req *proto.PingRequest) (*proto.PingResponse, error) { - if err := verifyMFAFromContext(ctx); err != nil { + if err := s.verifyMFAFromContext(ctx); err != nil { return nil, trace.Wrap(err) } return &proto.PingResponse{}, nil } -func verifyMFAFromContext(ctx context.Context) error { +func (s *mfaService) verifyMFAFromContext(ctx context.Context) error { mfaResp, err := mfa.CredentialsFromContext(ctx) if err != nil { // (In production consider logging err, so we don't swallow it silently.) @@ -53,14 +57,20 @@ func verifyMFAFromContext(ctx context.Context) error { switch r := mfaResp.Response.(type) { case *proto.MFAAuthenticateResponse_TOTP: - if r.TOTP.Code != otpTestCode { - return trace.AccessDenied("failed MFA verification") + switch r.TOTP.Code { + case otpTestCode: + return nil + case otpTestCodeReusable: + if s.allowReuse { + return nil + } + fallthrough + default: + return trace.Wrap(&mfa.ErrAdminActionMFARequired) } default: return trace.BadParameter("unexpected mfa response type %T", r) } - - return nil } // TestGRPCErrorWrapping tests the error wrapping capability of the client @@ -169,3 +179,103 @@ func TestRetryWithMFA(t *testing.T) { }) }) } + +func TestRetryWithMFA_Reuse(t *testing.T) { + t.Parallel() + ctx := context.Background() + + mtlsConfig := mtls.NewConfig(t) + listener, err := net.Listen("tcp", "localhost:0") + require.NoError(t, err) + + mfaService := &mfaService{} + server := grpc.NewServer( + grpc.Creds(credentials.NewTLS(mtlsConfig.ServerTLS)), + grpc.ChainUnaryInterceptor(interceptors.GRPCServerUnaryErrorInterceptor), + ) + proto.RegisterAuthServiceServer(server, mfaService) + go func() { + server.Serve(listener) + }() + defer server.Stop() + + okMFACeremony := func(ctx context.Context, challengeRequest *proto.CreateAuthenticateChallengeRequest, promptOpts ...mfa.PromptOpt) (*proto.MFAAuthenticateResponse, error) { + return &proto.MFAAuthenticateResponse{ + Response: &proto.MFAAuthenticateResponse_TOTP{ + TOTP: &proto.TOTPResponse{ + Code: otpTestCode, + }, + }, + }, nil + } + + okMFACeremonyAllowReuse := func(ctx context.Context, challengeRequest *proto.CreateAuthenticateChallengeRequest, promptOpts ...mfa.PromptOpt) (*proto.MFAAuthenticateResponse, error) { + return &proto.MFAAuthenticateResponse{ + Response: &proto.MFAAuthenticateResponse_TOTP{ + TOTP: &proto.TOTPResponse{ + Code: otpTestCodeReusable, + }, + }, + }, nil + } + + t.Run("ok allow reuse", func(t *testing.T) { + mfaService.allowReuse = true + conn, err := grpc.Dial( + listener.Addr().String(), + grpc.WithTransportCredentials(credentials.NewTLS(mtlsConfig.ClientTLS)), + grpc.WithChainUnaryInterceptor( + interceptors.WithMFAUnaryInterceptor(okMFACeremonyAllowReuse), + interceptors.GRPCClientUnaryErrorInterceptor, + ), + ) + require.NoError(t, err) + defer conn.Close() + + client := proto.NewAuthServiceClient(conn) + _, err = client.Ping(ctx, &proto.PingRequest{}) + assert.NoError(t, err) + }) + + t.Run("nok disallow reuse", func(t *testing.T) { + mfaService.allowReuse = false + conn, err := grpc.Dial( + listener.Addr().String(), + grpc.WithTransportCredentials(credentials.NewTLS(mtlsConfig.ClientTLS)), + grpc.WithChainUnaryInterceptor( + interceptors.WithMFAUnaryInterceptor(okMFACeremonyAllowReuse), + interceptors.GRPCClientUnaryErrorInterceptor, + ), + ) + require.NoError(t, err) + defer conn.Close() + + client := proto.NewAuthServiceClient(conn) + _, err = client.Ping(ctx, &proto.PingRequest{}) + assert.ErrorIs(t, err, &mfa.ErrAdminActionMFARequired, "Ping error mismatch") + }) + + t.Run("ok disallow reuse, retry with one-shot mfa", func(t *testing.T) { + mfaService.allowReuse = false + conn, err := grpc.Dial( + listener.Addr().String(), + grpc.WithTransportCredentials(credentials.NewTLS(mtlsConfig.ClientTLS)), + grpc.WithChainUnaryInterceptor( + interceptors.WithMFAUnaryInterceptor(okMFACeremony), + interceptors.GRPCClientUnaryErrorInterceptor, + ), + ) + require.NoError(t, err) + defer conn.Close() + + // Pass reusable MFA through the context. The interceptor should + // catch the resulting ErrAdminActionMFARequired and retry with + // a one-shot mfa challenge. + mfaResp, _ := okMFACeremony(ctx, nil) + ctx := mfa.ContextWithMFAResponse(ctx, mfaResp) + + client := proto.NewAuthServiceClient(conn) + _, err = client.Ping(ctx, &proto.PingRequest{}) + assert.NoError(t, err) + }) +} diff --git a/lib/auth/auth_with_roles.go b/lib/auth/auth_with_roles.go index bc86172038323..4f399c37d6754 100644 --- a/lib/auth/auth_with_roles.go +++ b/lib/auth/auth_with_roles.go @@ -2018,7 +2018,7 @@ func (a *ServerWithRoles) GetTokens(ctx context.Context) ([]types.ProvisionToken return nil, trace.Wrap(err) } - if err := a.context.AuthorizeAdminAction(); err != nil { + if err := a.context.AuthorizeAdminActionAllowReusedMFA(); err != nil { return nil, trace.Wrap(err) } @@ -2034,7 +2034,7 @@ func (a *ServerWithRoles) GetToken(ctx context.Context, token string) (types.Pro } } - if err := a.context.AuthorizeAdminAction(); err != nil { + if err := a.context.AuthorizeAdminActionAllowReusedMFA(); err != nil { return nil, trace.Wrap(err) }