Skip to content

Commit

Permalink
add permission validation for robot creating and updating. (#19598)
Browse files Browse the repository at this point in the history 
* 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 <[email protected]>

* Fix robot testcase and update robot permission metadata (#167)

1. Fix robot testcase
2. update robot permission metadata

Signed-off-by: Yang Jiao <[email protected]>
Signed-off-by: wang yan <[email protected]>

---------

Signed-off-by: wang yan <[email protected]>
Signed-off-by: Yang Jiao <[email protected]>
Co-authored-by: Yang Jiao <[email protected]>
  • Loading branch information
@wy65701436 @YangJiao0817
wy65701436 and YangJiao0817 authored Nov 22, 2023
1 parent 43ccd2f commit 062d144
Show file tree
Hide file tree
Showing 5 changed files with 145 additions and 46 deletions.
43 changes: 22 additions & 21 deletions src/common/rbac/const.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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},
Expand Down Expand Up @@ -123,27 +123,19 @@ 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},

{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},
Expand All @@ -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},
Expand All @@ -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},
Expand Down Expand Up @@ -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},
},
}
)
32 changes: 32 additions & 0 deletions src/server/v2.0/handler/robot.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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
}

Expand Down Expand Up @@ -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
}
78 changes: 78 additions & 0 deletions src/server/v2.0/handler/robot_test.go
View file
Original file line number Diff line number Diff line change
@@ -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"
)
Expand Down Expand Up @@ -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)
}
})
}
}
18 changes: 3 additions & 15 deletions tests/apitests/python/library/robot.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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),
Expand All @@ -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")
Expand All @@ -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 = []
Expand Down
20 changes: 10 additions & 10 deletions tests/apitests/python/test_robot_account.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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)):
Expand Down Expand Up @@ -240,25 +240,25 @@ 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)

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)
Expand Down Expand Up @@ -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)
Expand Down

0 comments on commit 062d144

Please sign in to comment.