diff --git a/src/frontend/apiserver/controllers/roles.go b/src/frontend/apiserver/controllers/roles.go index 62457ec9..1b387789 100644 --- a/src/frontend/apiserver/controllers/roles.go +++ b/src/frontend/apiserver/controllers/roles.go @@ -507,6 +507,10 @@ func (rc *RoleController) AddPermissionsForRole(ctx *gin.Context) { return } + if !checkRoleChangePermission(ctx, "add permissions to", req.RoleName) { + return + } + err := preCheckRoleRequest(&req, false) if err != nil { logger.Error("Failed to create role request", zap.Error(err)) @@ -549,6 +553,11 @@ func (rc *RoleController) DeleteRoleAndPermissions(ctx *gin.Context) { ReturnError(ctx, common.ERROR_CODE, err.Error(), nil) return } + + if !checkRoleChangePermission(ctx, "delete", req_rolename) { + return + } + // step 1: role name whether exists allRoleNames := store.GetAllRoleNames(false) found := false @@ -602,6 +611,10 @@ func (rc *RoleController) CreateRole(ctx *gin.Context) { return } + if !checkRoleChangePermission(ctx, "create", req.RoleName) { + return + } + // precheck the request parameter err := preCheckRoleRequest(&req, true) if err != nil { @@ -632,6 +645,30 @@ func (rc *RoleController) CreateRole(ctx *gin.Context) { return } +const errNoRoleDefinitionPermission = "no permission: creating, changing or deleting roles requires rbac:control with scope all" + +// checkRoleChangePermission enforces that only an rbac:control grant with +// scope all may create, extend or delete roles (bug-1958): with a narrower +// scope such as self a user could add any permission, cluster:control +// included, to a role it already holds. The rule is the one canChangeUserRoles +// applies to user-role assignment (bug-1952). On refusal it writes the error +// response and returns false; the caller must return. +func checkRoleChangePermission(ctx *gin.Context, action string, roleName string) bool { + token := ctx.GetHeader("Authorization") + submitUser, err := GetUsernameFromToken(token) + if err != nil { + ReturnError(ctx, common.ERROR_CODE, err.Error(), nil) + return false + } + if !canChangeUserRoles(submitUser) { + logger.Error("no permission to "+action+" role", zap.String("submit_user", submitUser), + zap.String("role_name", roleName)) + ReturnError(ctx, common.ERROR_AUTH_CODE, errNoRoleDefinitionPermission, nil) + return false + } + return true +} + func preCheckRole(role_name string) error { return nil diff --git a/src/frontend/apiserver/controllers/roles_rbac_test.go b/src/frontend/apiserver/controllers/roles_rbac_test.go new file mode 100644 index 00000000..ffcd6d43 --- /dev/null +++ b/src/frontend/apiserver/controllers/roles_rbac_test.go @@ -0,0 +1,122 @@ +//go:build !cgo + +package controllers + +import ( + "apiserver/store" + "apiserver/utils" + "net/http" + "testing" + + "github.com/gin-gonic/gin" +) + +// RBAC fixture for bug-1958: mallory holds rbac:control scoped to self, +// root holds it scoped to all. spare_role is a custom role nobody holds. +const roleChangeRBAC = ` +permissions: + - rbac:control + - rbac:view + - cluster:control +roles: + rbac_self: + permissions: + - action: rbac:control + domain: + - type: self + rbac_admin: + permissions: + - action: rbac:control + domain: + - type: all + spare_role: + permissions: + - action: rbac:view + domain: + - type: self +users: + mallory: + roles: + - name: rbac_self + root: + roles: + - name: rbac_admin +` + +func newRoleChangeRouter() *gin.Engine { + r := newTestRouter() + rc := &RoleController{} + r.POST("/xce/v1/roles/create", RequirePermission("rbac:control"), rc.CreateRole) + r.POST("/xce/v1/roles/:roleid/permissions", RequirePermission("rbac:control"), rc.AddPermissionsForRole) + r.DELETE("/xce/v1/roles/:rolename", RequirePermission("rbac:control"), rc.DeleteRoleAndPermissions) + return r +} + +const clusterControlPerm = `{"permissions":[{"permission_name":"cluster:control","domain_type":"all"}]}` + +// bug-1958: rbac:control scoped to self must not add cluster:control (or +// anything) to a role the user holds. +func TestAddPermissionsForRole_SelfScopeCannotExtendOwnRole(t *testing.T) { + setupRBACTest(t, roleChangeRBAC) + r := newRoleChangeRouter() + + w := doRoleRequest(r, http.MethodPost, "/xce/v1/roles/rbac_self/permissions", "mallory", clusterControlPerm) + if utils.ContainsField(store.GetRoleAllPermissions("rbac_self"), "cluster:control") { + t.Errorf("privilege escalation: mallory (rbac:control scope self) added cluster:control to its own role (status %d, %s)", + w.Code, w.Body.String()) + } + if w.Code != http.StatusForbidden { + t.Errorf("status = %d, want 403 (%s)", w.Code, w.Body.String()) + } +} + +// bug-1958: rbac:control scoped to self must not create roles. +func TestCreateRole_SelfScopeRejected(t *testing.T) { + setupRBACTest(t, roleChangeRBAC) + r := newRoleChangeRouter() + + w := doRoleRequest(r, http.MethodPost, "/xce/v1/roles/create", "mallory", + `{"role_name":"evil","permissions":[{"permission_name":"cluster:control","domain_type":"all"}]}`) + if w.Code != http.StatusForbidden { + t.Errorf("status = %d, want 403 (%s)", w.Code, w.Body.String()) + } + if utils.ContainsField(store.GetAllRoleNames(false), "evil") { + t.Error("role was created although the request was rejected") + } +} + +// bug-1958: rbac:control scoped to self must not delete roles. +func TestDeleteRole_SelfScopeRejected(t *testing.T) { + setupRBACTest(t, roleChangeRBAC) + r := newRoleChangeRouter() + + w := doRoleRequest(r, http.MethodDelete, "/xce/v1/roles/spare_role", "mallory", "") + if w.Code != http.StatusForbidden { + t.Errorf("status = %d, want 403 (%s)", w.Code, w.Body.String()) + } + if !utils.ContainsField(store.GetAllRoleNames(false), "spare_role") { + t.Error("role was deleted although the request was rejected") + } +} + +// bug-1958: rbac:control scoped to all may still create, extend and delete +// roles. +func TestRoleChanges_AllScopeAllowed(t *testing.T) { + setupRBACTest(t, roleChangeRBAC) + r := newRoleChangeRouter() + + // ReturnSuccess always answers HTTP 200 (code 201 is in the envelope). + w := doRoleRequest(r, http.MethodPost, "/xce/v1/roles/create", "root", + `{"role_name":"ops","permissions":[{"permission_name":"rbac:view","domain_type":"all"}]}`) + if w.Code != http.StatusOK || !utils.ContainsField(store.GetAllRoleNames(false), "ops") { + t.Fatalf("root creating role: status %d (%s)", w.Code, w.Body.String()) + } + w = doRoleRequest(r, http.MethodPost, "/xce/v1/roles/ops/permissions", "root", clusterControlPerm) + if w.Code != http.StatusOK || !utils.ContainsField(store.GetRoleAllPermissions("ops"), "cluster:control") { + t.Fatalf("root extending role: status %d (%s), perms %v", w.Code, w.Body.String(), store.GetRoleAllPermissions("ops")) + } + w = doRoleRequest(r, http.MethodDelete, "/xce/v1/roles/ops", "root", "") + if w.Code != http.StatusOK || utils.ContainsField(store.GetAllRoleNames(false), "ops") { + t.Errorf("root deleting role: status %d (%s)", w.Code, w.Body.String()) + } +} diff --git a/src/frontend/apiserver/controllers/users.go b/src/frontend/apiserver/controllers/users.go index 50b9942a..e79bf0eb 100644 --- a/src/frontend/apiserver/controllers/users.go +++ b/src/frontend/apiserver/controllers/users.go @@ -480,6 +480,8 @@ const errNoRoleChangePermission = "no permission: granting or revoking user role // outright: letting it change its own roles would let it grant itself any // role, cluster_admin included. All grants are checked before // deduplication, which would otherwise prefer a narrower scope over all. +// Role definitions (create, add permissions, delete) follow the same rule +// (bug-1958, see canChangeRoles). func canChangeUserRoles(user string) bool { for _, p := range collectUserPermissionsWithType(user) { if p.Permission == "rbac:control" && p.DomainType == models.DomainTypeAll {