From e3a353d8ae9189b8348c5e1a65a7e2434c3b9de9 Mon Sep 17 00:00:00 2001 From: Wang Yan Date: Thu, 3 Dec 2020 18:41:00 +0800 Subject: [PATCH] fix robot name conflicate issue add project name into project level robot account name Signed-off-by: Wang Yan --- src/controller/robot/controller.go | 29 +++++++++---------- src/controller/robot/controller_test.go | 2 +- src/server/middleware/security/robot2.go | 17 +++++++---- src/server/middleware/security/robot2_test.go | 10 +------ src/server/v2.0/handler/robot.go | 14 +++++++++ 5 files changed, 40 insertions(+), 32 deletions(-) diff --git a/src/controller/robot/controller.go b/src/controller/robot/controller.go index 099210ac0..4f40dbf59 100644 --- a/src/controller/robot/controller.go +++ b/src/controller/robot/controller.go @@ -76,7 +76,7 @@ func (d *controller) Count(ctx context.Context, query *q.Query) (int64, error) { // Create ... func (d *controller) Create(ctx context.Context, r *Robot) (int64, string, error) { - if err := d.setProjectID(ctx, r); err != nil { + if err := d.setProject(ctx, r); err != nil { return 0, "", err } @@ -95,8 +95,13 @@ func (d *controller) Create(ctx context.Context, r *Robot) (int64, string, error salt := utils.GenerateRandomString() secret := utils.Encrypt(pwd, salt, utils.SHA256) + name := r.Name + // for the project level robot, set the name pattern as projectname+robotname, and + is a illegal character. + if r.Level == LEVELPROJECT { + name = fmt.Sprintf("%s+%s", r.ProjectName, r.Name) + } robotID, err := d.robotMgr.Create(ctx, &model.Robot{ - Name: r.Name, + Name: name, Description: r.Description, ProjectID: r.ProjectID, ExpiresAt: expiresAt, @@ -208,10 +213,7 @@ func (d *controller) populate(ctx context.Context, r *model.Robot, option *Optio robot.Name = fmt.Sprintf("%s%s", config.RobotPrefix(), r.Name) robot.setLevel() robot.setEditable() - if option == nil { - return robot, nil - } - if option.WithPermission { + if option != nil && option.WithPermission { if err := d.populatePermissions(ctx, robot); err != nil { return nil, err } @@ -273,24 +275,19 @@ func (d *controller) populatePermissions(ctx context.Context, r *Robot) error { return nil } -func (d *controller) setProjectID(ctx context.Context, r *Robot) error { +// set the project info if it's a project level robot +func (d *controller) setProject(ctx context.Context, r *Robot) error { if r == nil { return nil } - var projectID int64 - switch r.Level { - case LEVELSYSTEM: - projectID = 0 - case LEVELPROJECT: + if r.Level == LEVELPROJECT { pro, err := d.proMgr.Get(ctx, r.Permissions[0].Namespace) if err != nil { return err } - projectID = pro.ProjectID - default: - return errors.New(nil).WithMessage("unknown robot account level").WithCode(errors.BadRequestCode) + r.ProjectName = pro.Name + r.ProjectID = pro.ProjectID } - r.ProjectID = projectID return nil } diff --git a/src/controller/robot/controller_test.go b/src/controller/robot/controller_test.go index ce6176df3..57ed2c5e6 100644 --- a/src/controller/robot/controller_test.go +++ b/src/controller/robot/controller_test.go @@ -34,7 +34,7 @@ func (suite *ControllerTestSuite) TestGet() { ctx := context.TODO() projectMgr.On("Get", mock.Anything, mock.Anything).Return(&models.Project{ProjectID: 1, Name: "library"}, nil) robotMgr.On("Get", mock.Anything, mock.Anything).Return(&model.Robot{ - Name: "test", + Name: "library+test", Description: "test get method", ProjectID: 1, Secret: utils.RandStringBytes(10), diff --git a/src/server/middleware/security/robot2.go b/src/server/middleware/security/robot2.go index e8422bb44..0d4fe01ac 100644 --- a/src/server/middleware/security/robot2.go +++ b/src/server/middleware/security/robot2.go @@ -10,10 +10,11 @@ import ( "github.com/goharbor/harbor/src/lib/log" "github.com/goharbor/harbor/src/lib/q" "github.com/goharbor/harbor/src/pkg/permission/types" + "strings" + "time" "github.com/goharbor/harbor/src/pkg/robot2/model" "net/http" - "strings" ) type robot2 struct{} @@ -27,10 +28,10 @@ func (r *robot2) Generate(req *http.Request) security.Context { if !strings.HasPrefix(name, config.RobotPrefix()) { return nil } - - // TODO use the naming pattern to avoid the permission boundary crossing. + name = strings.TrimPrefix(name, config.RobotPrefix()) + // The robot name can be used as the unique identifier to locate robot as it contains the project name. robots, err := robot_ctl.Ctl.List(req.Context(), q.New(q.KeyWords{ - "name": strings.TrimPrefix(name, config.RobotPrefix()), + "name": name, }), &robot_ctl.Option{ WithPermission: true, }) @@ -51,7 +52,11 @@ func (r *robot2) Generate(req *http.Request) security.Context { log.Errorf("failed to authenticate disabled robot account: %s", name) return nil } - // add the expiration check + now := time.Now().Unix() + if robot.ExpiresAt != -1 && robot.ExpiresAt <= now { + log.Errorf("the robot account is expired: %s", name) + return nil + } var accesses []*types.Policy for _, p := range robot.Permissions { @@ -65,7 +70,7 @@ func (r *robot2) Generate(req *http.Request) security.Context { } modelRobot := &model.Robot{ - Name: strings.TrimPrefix(name, config.RobotPrefix()), + Name: name, } log.Infof("a robot2 security context generated for request %s %s", req.Method, req.URL.Path) return robotCtx.NewSecurityContext(modelRobot, robot.Level == robot_ctl.LEVELSYSTEM, accesses) diff --git a/src/server/middleware/security/robot2_test.go b/src/server/middleware/security/robot2_test.go index 8bad2485f..614ba67dd 100644 --- a/src/server/middleware/security/robot2_test.go +++ b/src/server/middleware/security/robot2_test.go @@ -2,24 +2,16 @@ package security import ( "github.com/goharbor/harbor/src/common" - "github.com/goharbor/harbor/src/common/utils/test" core_cfg "github.com/goharbor/harbor/src/core/config" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "net/http" - "os" "testing" ) func TestRobot2(t *testing.T) { - secretKeyPath := "/tmp/secretkey" - _, err := test.GenerateKey(secretKeyPath) - assert.Nil(t, err) - defer os.Remove(secretKeyPath) - os.Setenv("KEY_PATH", secretKeyPath) - conf := map[string]interface{}{ - common.RobotPrefix: "robot@", + common.RobotNamePrefix: "robot@", } core_cfg.InitWithSettings(conf) diff --git a/src/server/v2.0/handler/robot.go b/src/server/v2.0/handler/robot.go index 2bf5ac860..0240a0c94 100644 --- a/src/server/v2.0/handler/robot.go +++ b/src/server/v2.0/handler/robot.go @@ -32,6 +32,10 @@ type robotAPI struct { } func (rAPI *robotAPI) CreateRobot(ctx context.Context, params operation.CreateRobotParams) middleware.Responder { + if err := validateName(params.Robot.Name); err != nil { + return rAPI.SendError(ctx, err) + } + if err := rAPI.validate(params.Robot.Duration, params.Robot.Level, params.Robot.Permissions); err != nil { return rAPI.SendError(ctx, err) } @@ -306,3 +310,13 @@ func isValidSec(sec string) bool { } return false } + +// validateName validates the robot name, especially '+' cannot be a valid character +func validateName(name string) error { + robotNameReg := `^[a-z0-9]+(?:[._-][a-z0-9]+)*$` + legal := regexp.MustCompile(robotNameReg).MatchString(name) + if !legal { + return errors.BadRequestError(nil).WithMessage("robot name is not in lower case or contains illegal characters") + } + return nil +}