MFA for Admin Actions: users

api/mfa/mfa.go

@@ -128,6 +128,9 @@ func MFAResponseFromContext(ctx context.Context) (*proto.MFAAuthenticateResponse
 		if !ok {
 			return nil, trace.BadParameter("unexpected context value type %T", val)
 		}
+		if mfaResp == nil {
+			return nil, trace.NotFound("mfa response not found in the context")
+		}
 		return mfaResp, nil
 	}
 	return nil, trace.NotFound("mfa response not found in the context")

lib/auth/users/usersv1/service.go

@@ -218,6 +218,10 @@ func (s *Service) CreateUser(ctx context.Context, req *userspb.CreateUserRequest
 		return nil, trace.Wrap(err)
 	}
 
+	if err := authz.AuthorizeAdminAction(ctx, authzCtx); err != nil {
+		return nil, trace.Wrap(err)
+	}
+
 	if err = okta.CheckOrigin(authzCtx, req.User); err != nil {
 		return nil, trace.Wrap(err)
 	}
@@ -280,6 +284,10 @@ func (s *Service) UpdateUser(ctx context.Context, req *userspb.UpdateUserRequest
 		return nil, trace.Wrap(err)
 	}
 
+	if err := authz.AuthorizeAdminAction(ctx, authzCtx); err != nil {
+		return nil, trace.Wrap(err)
+	}
+
 	if err = okta.CheckOrigin(authzCtx, req.User); err != nil {
 		return nil, trace.Wrap(err)
 	}
@@ -346,6 +354,10 @@ func (s *Service) UpsertUser(ctx context.Context, req *userspb.UpsertUserRequest
 		return nil, trace.Wrap(err)
 	}
 
+	if err := authz.AuthorizeAdminAction(ctx, authzCtx); err != nil {
+		return nil, trace.Wrap(err)
+	}
+
 	if createdBy := req.User.GetCreatedBy(); createdBy.IsEmpty() {
 		req.User.SetCreatedBy(types.CreatedBy{
 			User: types.UserRef{Name: authzCtx.User.GetName()},
@@ -418,6 +430,10 @@ func (s *Service) DeleteUser(ctx context.Context, req *userspb.DeleteUserRequest
 		return nil, trace.Wrap(err)
 	}
 
+	if err := authz.AuthorizeAdminAction(ctx, authzCtx); err != nil {
+		return nil, trace.Wrap(err)
+	}
+
 	prevUser, err := s.cache.GetUser(ctx, req.Name, false)
 	var omitEditorEvent bool
 	if err != nil && !trace.IsNotFound(err) {

lib/auth/users/usersv1/service_test.go

@@ -73,7 +73,8 @@ func (a fakeAuthorizer) Authorize(ctx context.Context) (*authz.Context, error) {
 					},
 				},
 			},
-			Identity: identity,
+			Identity:              identity,
+			AdminActionAuthorized: true,
 		}, nil
 	}
 
@@ -102,8 +103,8 @@ func (a fakeAuthorizer) Authorize(ctx context.Context) (*authz.Context, error) {
 				Username: "alice",
 			},
 		},
+		AdminActionAuthorized: true,
 	}, nil
-
 }
 
 type fakeChecker struct {
@@ -866,7 +867,6 @@ func TestRBAC(t *testing.T) {
 
 	for _, test := range tests {
 		t.Run(test.desc, func(t *testing.T) {
-
 			env, err := newTestEnv(withAuthorizer(&fakeAuthorizer{authzContext: &authz.Context{
 				User:    llama,
 				Checker: test.checker,
@@ -876,6 +876,7 @@ func TestRBAC(t *testing.T) {
 						Groups: []string{"dev"},
 					},
 				},
+				AdminActionAuthorized: true,
 			}}))
 			require.NoError(t, err, "creating test service")
 
@@ -888,5 +889,4 @@ func TestRBAC(t *testing.T) {
 			require.ElementsMatch(t, test.expectChecks, test.checker.checks)
 		})
 	}
-
 }

lib/authz/permissions.go

@@ -193,7 +193,7 @@ type Context struct {
 	// AdminActionVerified is whether this auth request is verified for admin actions. This
 	// either means that the request was MFA verified through the context or Hardware Key support,
 	// or the identity does not require admin MFA (built in roles, bot impersonated user, etc).
-	adminActionAuthorized bool
+	AdminActionAuthorized bool
 }
 
 // GetUserMetadata returns information about the authenticated identity
@@ -384,7 +384,7 @@ func (a *authorizer) checkAdminActionVerification(ctx context.Context, authConte
 		return trace.Wrap(err)
 	}
 
-	authContext.adminActionAuthorized = true
+	authContext.AdminActionAuthorized = true
 	return nil
 }
 
@@ -1101,6 +1101,7 @@ func ContextForBuiltinRole(r BuiltinRole, recConfig types.SessionRecordingConfig
 		Identity:              r,
 		UnmappedIdentity:      r,
 		disableDeviceRoleMode: true, // Builtin roles skip device trust.
+		AdminActionAuthorized: true, // builtin roles skip mfa for admin actions.
 	}, nil
 }
 
@@ -1324,7 +1325,7 @@ func AuthorizeContextWithVerbs(ctx context.Context, log logrus.FieldLogger, auth
 
 // AuthorizeAdminAction will ensure that the user is authorized to perform admin actions.
 func AuthorizeAdminAction(ctx context.Context, authCtx *Context) error {
-	if !authCtx.adminActionAuthorized {
+	if !authCtx.AdminActionAuthorized {
 		return trace.Wrap(&mfa.ErrAdminActionMFARequired)
 	}
 	return nil

lib/service/service.go

@@ -1620,7 +1620,6 @@ func (process *TeleportProcess) initAuthExternalAuditLog(auditConfig types.Clust
 	if len(loggers) < 1 {
 		if externalAuditStorage.IsUsed() {
 			return nil, externalAuditMissingAthenaError
-
 		}
 		return nil, nil
 	}

lib/web/users.go

@@ -27,6 +27,7 @@ import (
 	"github.com/julienschmidt/httprouter"
 
 	"github.com/gravitational/teleport/api/client/proto"
+	"github.com/gravitational/teleport/api/mfa"
 	"github.com/gravitational/teleport/api/types"
 	wantypes "github.com/gravitational/teleport/lib/auth/webauthntypes"
 	"github.com/gravitational/teleport/lib/httplib"
@@ -155,7 +156,11 @@ func updateUser(r *http.Request, m userAPIGetter, createdBy string) (*ui.User, e
 		return nil, trace.Wrap(err)
 	}
 
-	user, err := m.GetUser(r.Context(), req.Name, false)
+	// Remove the MFA resp from the context before getting the user.
+	// Otherwise, it will be consumed before the Update which actually
+	// requires the MFA.
+	getUserCtx := mfa.ContextWithMFAResponse(r.Context(), nil)
+	user, err := m.GetUser(getUserCtx, req.Name, false)
 	if err != nil {
 		return nil, trace.Wrap(err)
 	}