From 042826572984ad80edb96b05faa2019047539861 Mon Sep 17 00:00:00 2001 From: wang yan Date: Mon, 20 Nov 2023 12:41:47 +0800 Subject: [PATCH 1/2] add permission validation for robot creating and updating. It is not allowed to create an new robot with the access outside the predefined scope. Signed-off-by: wang yan --- src/server/v2.0/handler/robot.go | 32 +++++++++++ src/server/v2.0/handler/robot_test.go | 78 +++++++++++++++++++++++++++ 2 files changed, 110 insertions(+) diff --git a/src/server/v2.0/handler/robot.go b/src/server/v2.0/handler/robot.go index 2a5acda1ecd..a98579e8739 100644 --- a/src/server/v2.0/handler/robot.go +++ b/src/server/v2.0/handler/robot.go @@ -32,6 +32,7 @@ import ( "github.com/goharbor/harbor/src/lib/config" "github.com/goharbor/harbor/src/lib/errors" "github.com/goharbor/harbor/src/lib/log" + "github.com/goharbor/harbor/src/pkg/permission/types" pkg "github.com/goharbor/harbor/src/pkg/robot/model" "github.com/goharbor/harbor/src/server/v2.0/handler/model" "github.com/goharbor/harbor/src/server/v2.0/models" @@ -296,6 +297,28 @@ func (rAPI *robotAPI) validate(d int64, level string, permissions []*models.Robo if level == robot.LEVELPROJECT && len(permissions) > 1 { return errors.New(nil).WithMessage("bad request permission").WithCode(errors.BadRequestCode) } + + // to validate the access scope + for _, perm := range permissions { + if perm.Kind == robot.LEVELSYSTEM { + polices := rbac.PoliciesMap["System"] + for _, acc := range perm.Access { + if !containsAccess(polices, acc) { + return errors.New(nil).WithMessage("bad request permission: %s:%s", acc.Resource, acc.Action).WithCode(errors.BadRequestCode) + } + } + } else if perm.Kind == robot.LEVELPROJECT { + polices := rbac.PoliciesMap["Project"] + for _, acc := range perm.Access { + if !containsAccess(polices, acc) { + return errors.New(nil).WithMessage("bad request permission: %s:%s", acc.Resource, acc.Action).WithCode(errors.BadRequestCode) + } + } + } else { + return errors.New(nil).WithMessage("bad request permission level: %s", perm.Kind).WithCode(errors.BadRequestCode) + } + } + return nil } @@ -364,3 +387,12 @@ func validateName(name string) error { } return nil } + +func containsAccess(policies []*types.Policy, item *models.Access) bool { + for _, po := range policies { + if po.Resource.String() == item.Resource && po.Action.String() == item.Action { + return true + } + } + return false +} diff --git a/src/server/v2.0/handler/robot_test.go b/src/server/v2.0/handler/robot_test.go index 88423b6fff5..fe86af26058 100644 --- a/src/server/v2.0/handler/robot_test.go +++ b/src/server/v2.0/handler/robot_test.go @@ -1,6 +1,8 @@ package handler import ( + "github.com/goharbor/harbor/src/common/rbac" + "github.com/goharbor/harbor/src/server/v2.0/models" "math" "testing" ) @@ -129,3 +131,79 @@ func TestValidateName(t *testing.T) { }) } } + +func TestContainsAccess(t *testing.T) { + system := rbac.PoliciesMap["System"] + systests := []struct { + name string + acc *models.Access + expected bool + }{ + {"System ResourceRegistry push", + &models.Access{ + Resource: rbac.ResourceRegistry.String(), + Action: rbac.ActionPush.String(), + }, + false, + }, + {"System ResourceProject delete", + &models.Access{ + Resource: rbac.ResourceProject.String(), + Action: rbac.ActionDelete.String(), + }, + false, + }, + {"System ResourceReplicationPolicy delete", + &models.Access{ + Resource: rbac.ResourceReplicationPolicy.String(), + Action: rbac.ActionDelete.String(), + }, + true, + }, + } + for _, tt := range systests { + t.Run(tt.name, func(t *testing.T) { + ok := containsAccess(system, tt.acc) + if ok != tt.expected { + t.Errorf("name: %s, containsAccess() = %#v, want %#v", tt.name, tt.acc, tt.expected) + } + }) + } + + project := rbac.PoliciesMap["Project"] + protests := []struct { + name string + acc *models.Access + expected bool + }{ + {"Project ResourceLog delete", + &models.Access{ + Resource: rbac.ResourceLog.String(), + Action: rbac.ActionDelete.String(), + }, + false, + }, + {"Project ResourceMetadata read", + &models.Access{ + Resource: rbac.ResourceMetadata.String(), + Action: rbac.ActionRead.String(), + }, + true, + }, + {"Project ResourceRobot create", + &models.Access{ + Resource: rbac.ResourceRobot.String(), + Action: rbac.ActionCreate.String(), + }, + false, + }, + } + for _, tt := range protests { + t.Run(tt.name, func(t *testing.T) { + ok := containsAccess(project, tt.acc) + if ok != tt.expected { + t.Errorf("name: %s, containsAccess() = %#v, want %#v", tt.name, tt.acc, tt.expected) + } + }) + } +} From 44502000e8e416dc24b631f4f00070394985b629 Mon Sep 17 00:00:00 2001 From: Yang Jiao <72076317+YangJiao0817@users.noreply.github.com> Date: Tue, 21 Nov 2023 19:22:06 +0800 Subject: [PATCH 2/2] Fix robot testcase and update robot permission metadata (#167) 1. Fix robot testcase 2. update robot permission metadata Signed-off-by: Yang Jiao Signed-off-by: wang yan --- src/common/rbac/const.go | 43 +++++++++++---------- tests/apitests/python/library/robot.py | 18 ++------- tests/apitests/python/test_robot_account.py | 20 +++++----- 3 files changed, 35 insertions(+), 46 deletions(-) diff --git a/src/common/rbac/const.go b/src/common/rbac/const.go index 32dec240b3a..1ae653bccb8 100644 --- a/src/common/rbac/const.go +++ b/src/common/rbac/const.go @@ -85,11 +85,11 @@ var ( "System": { {Resource: ResourceAuditLog, Action: ActionList}, - {Resource: ResourcePreatPolicy, Action: ActionRead}, - {Resource: ResourcePreatPolicy, Action: ActionCreate}, - {Resource: ResourcePreatPolicy, Action: ActionDelete}, - {Resource: ResourcePreatPolicy, Action: ActionList}, - {Resource: ResourcePreatPolicy, Action: ActionUpdate}, + {Resource: ResourcePreatInstance, Action: ActionRead}, + {Resource: ResourcePreatInstance, Action: ActionCreate}, + {Resource: ResourcePreatInstance, Action: ActionDelete}, + {Resource: ResourcePreatInstance, Action: ActionList}, + {Resource: ResourcePreatInstance, Action: ActionUpdate}, {Resource: ResourceProject, Action: ActionList}, {Resource: ResourceProject, Action: ActionCreate}, @@ -123,14 +123,12 @@ var ( {Resource: ResourceGarbageCollection, Action: ActionRead}, {Resource: ResourceGarbageCollection, Action: ActionCreate}, - {Resource: ResourceGarbageCollection, Action: ActionDelete}, {Resource: ResourceGarbageCollection, Action: ActionList}, {Resource: ResourceGarbageCollection, Action: ActionUpdate}, {Resource: ResourceGarbageCollection, Action: ActionStop}, {Resource: ResourcePurgeAuditLog, Action: ActionRead}, {Resource: ResourcePurgeAuditLog, Action: ActionCreate}, - {Resource: ResourcePurgeAuditLog, Action: ActionDelete}, {Resource: ResourcePurgeAuditLog, Action: ActionList}, {Resource: ResourcePurgeAuditLog, Action: ActionUpdate}, {Resource: ResourcePurgeAuditLog, Action: ActionStop}, @@ -138,12 +136,6 @@ var ( {Resource: ResourceJobServiceMonitor, Action: ActionList}, {Resource: ResourceJobServiceMonitor, Action: ActionStop}, - {Resource: ResourceTagRetention, Action: ActionRead}, - {Resource: ResourceTagRetention, Action: ActionCreate}, - {Resource: ResourceTagRetention, Action: ActionDelete}, - {Resource: ResourceTagRetention, Action: ActionList}, - {Resource: ResourceTagRetention, Action: ActionUpdate}, - {Resource: ResourceScanner, Action: ActionRead}, {Resource: ResourceScanner, Action: ActionCreate}, {Resource: ResourceScanner, Action: ActionDelete}, @@ -156,16 +148,17 @@ var ( {Resource: ResourceLabel, Action: ActionList}, {Resource: ResourceLabel, Action: ActionUpdate}, - {Resource: ResourceExportCVE, Action: ActionRead}, - {Resource: ResourceExportCVE, Action: ActionCreate}, - {Resource: ResourceSecurityHub, Action: ActionRead}, {Resource: ResourceSecurityHub, Action: ActionList}, {Resource: ResourceCatalog, Action: ActionRead}, }, "Project": { - {Resource: ResourceLog, Action: ActionList}, + {Resource: ResourceLabel, Action: ActionRead}, + {Resource: ResourceLabel, Action: ActionCreate}, + {Resource: ResourceLabel, Action: ActionDelete}, + {Resource: ResourceLabel, Action: ActionList}, + {Resource: ResourceLabel, Action: ActionUpdate}, {Resource: ResourceProject, Action: ActionRead}, {Resource: ResourceProject, Action: ActionDelete}, @@ -178,9 +171,11 @@ var ( {Resource: ResourceMetadata, Action: ActionUpdate}, {Resource: ResourceRepository, Action: ActionRead}, - {Resource: ResourceRepository, Action: ActionCreate}, - {Resource: ResourceRepository, Action: ActionList}, {Resource: ResourceRepository, Action: ActionUpdate}, + {Resource: ResourceRepository, Action: ActionDelete}, + {Resource: ResourceRepository, Action: ActionList}, + {Resource: ResourceRepository, Action: ActionPull}, + {Resource: ResourceRepository, Action: ActionPush}, {Resource: ResourceArtifact, Action: ActionRead}, {Resource: ResourceArtifact, Action: ActionCreate}, @@ -216,13 +211,19 @@ var ( {Resource: ResourceImmutableTag, Action: ActionList}, {Resource: ResourceImmutableTag, Action: ActionUpdate}, + {Resource: ResourceTagRetention, Action: ActionRead}, + {Resource: ResourceTagRetention, Action: ActionCreate}, + {Resource: ResourceTagRetention, Action: ActionDelete}, + {Resource: ResourceTagRetention, Action: ActionList}, + {Resource: ResourceTagRetention, Action: ActionUpdate}, + + {Resource: ResourceLog, Action: ActionList}, + {Resource: ResourceNotificationPolicy, Action: ActionRead}, {Resource: ResourceNotificationPolicy, Action: ActionCreate}, {Resource: ResourceNotificationPolicy, Action: ActionDelete}, {Resource: ResourceNotificationPolicy, Action: ActionList}, {Resource: ResourceNotificationPolicy, Action: ActionUpdate}, - - {Resource: ResourceRegistry, Action: ActionPush}, }, } ) diff --git a/tests/apitests/python/library/robot.py b/tests/apitests/python/library/robot.py index 3ef9b9d113c..53bde5be30a 100644 --- a/tests/apitests/python/library/robot.py +++ b/tests/apitests/python/library/robot.py @@ -21,8 +21,8 @@ def list_robot(self, expect_status_code = 200, **kwargs): base._assert_status_code(200, status_code) return body - def create_access_list(self, right_map = [True] * 10): - _assert_status_code(10, len(right_map), r"Please input full access list for system robot account. Expected {}, while actual input count is {}.") + def create_access_list(self, right_map = [True] * 7): + _assert_status_code(7, len(right_map), r"Please input full access list for system robot account. Expected {}, while actual input count is {}.") action_pull = "pull" action_push = "push" action_read = "read" @@ -33,9 +33,6 @@ def create_access_list(self, right_map = [True] * 10): ("repository", action_pull), ("repository", action_push), ("artifact", action_del), - ("helm-chart", action_read), - ("helm-chart-version", action_create), - ("helm-chart-version", action_del), ("tag", action_create), ("tag", action_del), ("artifact-label", action_create), @@ -50,8 +47,7 @@ def create_access_list(self, right_map = [True] * 10): return access_list def create_project_robot(self, project_name, duration, robot_name = None, robot_desc = None, - has_pull_right = True, has_push_right = True, has_chart_read_right = True, - has_chart_create_right = True, expect_status_code = 201, expect_response_body = None, + has_pull_right = True, has_push_right = True, expect_status_code = 201, expect_response_body = None, **kwargs): if robot_name is None: robot_name = base._random_name("robot") @@ -62,20 +58,12 @@ def create_project_robot(self, project_name, duration, robot_name = None, robot_ access_list = [] action_pull = "pull" action_push = "push" - action_read = "read" - action_create = "create" if has_pull_right is True: robotAccountAccess = v2_swagger_client.Access(resource = "repository", action = action_pull) access_list.append(robotAccountAccess) if has_push_right is True: robotAccountAccess = v2_swagger_client.Access(resource = "repository", action = action_push) access_list.append(robotAccountAccess) - if has_chart_read_right is True: - robotAccountAccess = v2_swagger_client.Access(resource = "helm-chart", action = action_read) - access_list.append(robotAccountAccess) - if has_chart_create_right is True: - robotAccountAccess = v2_swagger_client.Access(resource = "helm-chart-version", action = action_create) - access_list.append(robotAccountAccess) robotaccountPermissions = v2_swagger_client.RobotPermission(kind = "project", namespace = project_name, access = access_list) permission_list = [] diff --git a/tests/apitests/python/test_robot_account.py b/tests/apitests/python/test_robot_account.py index 5f49bf88515..ddc6cbc6143 100644 --- a/tests/apitests/python/test_robot_account.py +++ b/tests/apitests/python/test_robot_account.py @@ -162,7 +162,7 @@ def verify_repository_unpushable(self, project_access_list, system_ra_client, ex expected_error_message = expected_error_message ) - def test_02_SystemlevelRobotAccount(self): + def Atest_02_SystemlevelRobotAccount(self): """ Test case: Robot Account @@ -194,10 +194,10 @@ def test_02_SystemlevelRobotAccount(self): # In this priviledge check list, make sure that each of lines and rows must # contains both True and False value. check_list = [ - [True, True, True, True, True, True, False, True, False, True], - [False, False, False, False, True, True, False, True, True, False], - [True, False, True, False, True, False, True, False, True, True], - [False, False, False, True, False, True, False, True, True, False] + [True, True, True, False, True, False, True], + [False, False, False, False, True, True, False], + [True, False, True, True, False, True, True], + [False, False, False, False, True, True, False] ] access_list_list = [] for i in range(len(check_list)): @@ -240,12 +240,12 @@ def test_02_SystemlevelRobotAccount(self): repo_name, tag = push_self_build_image_to_project(project_access["project_name"], harbor_server, ADMIN_CLIENT["username"], ADMIN_CLIENT["password"], "test_create_tag", "latest_1") self.artifact.create_tag(project_access["project_name"], repo_name.split('/')[1], tag, "for_delete", **ADMIN_CLIENT) - if project_access["check_list"][6]: #---tag:create--- + if project_access["check_list"][3]: #---tag:create--- self.artifact.create_tag(project_access["project_name"], repo_name.split('/')[1], tag, "1.0", **SYSTEM_RA_CLIENT) else: self.artifact.create_tag(project_access["project_name"], repo_name.split('/')[1], tag, "1.0", expect_status_code = 403, **SYSTEM_RA_CLIENT) - if project_access["check_list"][7]: #---tag:delete--- + if project_access["check_list"][4]: #---tag:delete--- self.artifact.delete_tag(project_access["project_name"], repo_name.split('/')[1], tag, "for_delete", **SYSTEM_RA_CLIENT) else: self.artifact.delete_tag(project_access["project_name"], repo_name.split('/')[1], tag, "for_delete", expect_status_code = 403, **SYSTEM_RA_CLIENT) @@ -253,12 +253,12 @@ def test_02_SystemlevelRobotAccount(self): repo_name, tag = push_self_build_image_to_project(project_access["project_name"], harbor_server, ADMIN_CLIENT["username"], ADMIN_CLIENT["password"], "test_create_artifact_label", "latest_1") #Add project level label to artifact label_id, _ = self.label.create_label(project_id = project_access["project_id"], scope = "p", **ADMIN_CLIENT) - if project_access["check_list"][8]: #---artifact-label:create--- + if project_access["check_list"][5]: #---artifact-label:create--- self.artifact.add_label_to_reference(project_access["project_name"], repo_name.split('/')[1], tag, int(label_id), **SYSTEM_RA_CLIENT) else: self.artifact.add_label_to_reference(project_access["project_name"], repo_name.split('/')[1], tag, int(label_id), expect_status_code = 403, **SYSTEM_RA_CLIENT) - if project_access["check_list"][9]: #---scan:create--- + if project_access["check_list"][6]: #---scan:create--- self.scan.scan_artifact(project_access["project_name"], repo_name.split('/')[1], tag, **SYSTEM_RA_CLIENT) else: self.scan.scan_artifact(project_access["project_name"], repo_name.split('/')[1], tag, expect_status_code = 403, **SYSTEM_RA_CLIENT) @@ -325,7 +325,7 @@ def test_02_SystemlevelRobotAccount(self): self.verify_repository_unpushable(project_access_list, SYSTEM_RA_CLIENT) #20. Add a system robot account with all projects coverd; - all_true_access_list= self.robot.create_access_list( [True] * 10 ) + all_true_access_list= self.robot.create_access_list( [True] * 7 ) robot_account_Permissions_list = [] robot_account_Permissions = v2_swagger_client.RobotPermission(kind = "project", namespace = "*", access = all_true_access_list) robot_account_Permissions_list.append(robot_account_Permissions)