diff --git a/src/frontend/apiserver/controllers/users.go b/src/frontend/apiserver/controllers/users.go index 065c7bca..50b9942a 100644 --- a/src/frontend/apiserver/controllers/users.go +++ b/src/frontend/apiserver/controllers/users.go @@ -401,14 +401,11 @@ func (uc *UserController) DeleteRolesForUser(ctx *gin.Context) { } // check rbac - permission, _ := GetRequireResourcePermission(submitUser, "rbac:control") - if permission.DomainType != models.DomainTypeAll { - if username != submitUser { - err := errors.New("no permission") - logger.Error(err.Error()) - ReturnError(ctx, common.INVALID_PARAMS_CODE, err.Error(), nil) - return - } + if !canChangeUserRoles(submitUser) { + logger.Error("no permission to revoke user roles", zap.String("submit_user", submitUser), + zap.String("user_name", username), zap.String("role_name", rolename)) + ReturnError(ctx, common.ERROR_AUTH_CODE, errNoRoleChangePermission, nil) + return } // check whether user and role existed @@ -452,14 +449,11 @@ func (uc *UserController) AddRolesForUser(ctx *gin.Context) { } // check rbac - permission, _ := GetRequireResourcePermission(submitUser, "rbac:control") - if permission.DomainType != models.DomainTypeAll { - if req.UserName != submitUser { - err := errors.New("no permission") - logger.Error(err.Error()) - ReturnError(ctx, common.INVALID_PARAMS_CODE, err.Error(), nil) - return - } + if !canChangeUserRoles(submitUser) { + logger.Error("no permission to grant user roles", zap.String("submit_user", submitUser), + zap.String("user_name", req.UserName), zap.Strings("role_name", req.RoleName)) + ReturnError(ctx, common.ERROR_AUTH_CODE, errNoRoleChangePermission, nil) + return } roleNames := make([]string, 0, len(req.RoleName)) @@ -478,6 +472,23 @@ func (uc *UserController) AddRolesForUser(ctx *gin.Context) { return } +const errNoRoleChangePermission = "no permission: granting or revoking user roles requires rbac:control with scope all" + +// canChangeUserRoles reports whether user may grant or revoke user roles: +// only an rbac:control grant with scope all allows it, for any target user +// including itself (bug-1952). A narrower scope such as self is refused +// 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. +func canChangeUserRoles(user string) bool { + for _, p := range collectUserPermissionsWithType(user) { + if p.Permission == "rbac:control" && p.DomainType == models.DomainTypeAll { + return true + } + } + return false +} + func preCheckUserRequest(req *models.AddRoleForUserRequest) error { // validate username whether is validate req.UserName = strings.TrimSpace(req.UserName) diff --git a/src/frontend/apiserver/controllers/users_rbac_test.go b/src/frontend/apiserver/controllers/users_rbac_test.go new file mode 100644 index 00000000..d439c03c --- /dev/null +++ b/src/frontend/apiserver/controllers/users_rbac_test.go @@ -0,0 +1,122 @@ +//go:build !cgo + +package controllers + +import ( + "apiserver/store" + "apiserver/utils" + "net/http" + "net/http/httptest" + "strings" + "testing" + "time" + + "github.com/gin-gonic/gin" +) + +// RBAC fixture for bug-1952: a custom role grants rbac:control scoped to +// self; rbac_admin grants it with scope all. +const userRoleChangeRBAC = ` +permissions: + - rbac:control + - rbac:view + - host:control +roles: + cluster_admin: + permissions: + - action: host:control + domain: + - type: all + rbac_self: + permissions: + - action: rbac:control + domain: + - type: self + rbac_admin: + permissions: + - action: rbac:control + domain: + - type: all + basic_user: + permissions: + - action: rbac:view + domain: + - type: self +users: + mallory: + roles: + - name: rbac_self + root: + roles: + - name: rbac_admin + bob: + roles: + - name: basic_user +` + +func newUserRoleRouter() *gin.Engine { + r := newTestRouter() + uc := &UserController{} + r.POST("/xce/v1/users/:username/roles", RequirePermission("rbac:control"), uc.AddRolesForUser) + r.DELETE("/xce/v1/users/:username/:rolename", RequirePermission("rbac:control"), uc.DeleteRolesForUser) + return r +} + +func doRoleRequest(r *gin.Engine, method, path, user, body string) *httptest.ResponseRecorder { + req := httptest.NewRequest(method, path, strings.NewReader(body)) + req.RemoteAddr = "127.0.0.1:0" + req.Header.Set("Content-Type", "application/json") + req.Header.Set("Authorization", "Bearer "+signTestJWT(user, "", time.Now().Add(time.Hour))) + w := httptest.NewRecorder() + r.ServeHTTP(w, req) + return w +} + +// bug-1952: rbac:control scoped to self must not let a user grant itself +// cluster_admin (or any role). +func TestAddRolesForUser_SelfScopeCannotGrantItselfRoles(t *testing.T) { + setupRBACTest(t, userRoleChangeRBAC) + r := newUserRoleRouter() + + w := doRoleRequest(r, http.MethodPost, "/xce/v1/users/mallory/roles", "mallory", + `{"user_name":"mallory","role_name":["cluster_admin"]}`) + if utils.ContainsField(store.GetUserRoleNames("mallory"), "cluster_admin") { + t.Errorf("privilege escalation: mallory (rbac:control scope self) granted itself cluster_admin (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-1952: rbac:control scoped to self must not revoke roles either. +func TestDeleteRolesForUser_SelfScopeRejected(t *testing.T) { + setupRBACTest(t, userRoleChangeRBAC) + r := newUserRoleRouter() + + w := doRoleRequest(r, http.MethodDelete, "/xce/v1/users/mallory/rbac_self", "mallory", "") + if w.Code != http.StatusForbidden { + t.Errorf("status = %d, want 403 (%s)", w.Code, w.Body.String()) + } + if !utils.ContainsField(store.GetUserRoleNames("mallory"), "rbac_self") { + t.Error("role was removed although the request was rejected") + } +} + +// bug-1952: rbac:control scoped to all may still grant and revoke roles for +// any user, itself included. +func TestUserRoleChanges_AllScopeAllowed(t *testing.T) { + setupRBACTest(t, userRoleChangeRBAC) + r := newUserRoleRouter() + + w := doRoleRequest(r, http.MethodPost, "/xce/v1/users/bob/roles", "root", + `{"user_name":"bob","role_name":["rbac_self"]}`) + // ReturnSuccess always answers HTTP 200 (code 201 is in the envelope). + if w.Code != http.StatusOK || !utils.ContainsField(store.GetUserRoleNames("bob"), "rbac_self") { + t.Fatalf("root granting bob a role: status %d (%s), bob roles %v", w.Code, w.Body.String(), store.GetUserRoleNames("bob")) + } + w = doRoleRequest(r, http.MethodDelete, "/xce/v1/users/bob/rbac_self", "root", "") + if w.Code != http.StatusOK || utils.ContainsField(store.GetUserRoleNames("bob"), "rbac_self") { + t.Errorf("root revoking bob's role: status %d (%s), bob roles %v", w.Code, w.Body.String(), store.GetUserRoleNames("bob")) + } +}