From 9cbdb8cca1d943ce4058ddf890cdccc9740a3220 Mon Sep 17 00:00:00 2001 From: Wang Yan Date: Wed, 27 Jul 2022 01:11:43 +0800 Subject: [PATCH] [cherry-pick] Fix scan log mismatch issue (#17241) Fix scan log mismatch issue 1, add check in label 2, check robot account update 3, validate scan log Signed-off-by: Wang Yan Co-authored-by: stonezdj(Daojun Zhang) --- src/common/rbac/project/rbac_role.go | 3 ++ src/controller/scan/base_controller.go | 35 ++++++++++++++-- src/controller/scan/base_controller_test.go | 39 +++++++++--------- src/controller/scan/controller.go | 2 +- src/core/api/chart_label.go | 3 ++ src/server/v2.0/handler/artifact.go | 44 +++++++++++++++------ src/server/v2.0/handler/robot.go | 27 +++++++++++-- src/server/v2.0/handler/scan.go | 5 +-- src/testing/controller/scan/controller.go | 14 +++---- 9 files changed, 125 insertions(+), 47 deletions(-) diff --git a/src/common/rbac/project/rbac_role.go b/src/common/rbac/project/rbac_role.go index 8d289f665..86e7c6c63 100644 --- a/src/common/rbac/project/rbac_role.go +++ b/src/common/rbac/project/rbac_role.go @@ -80,6 +80,7 @@ var ( {Resource: rbac.ResourceHelmChartVersionLabel, Action: rbac.ActionCreate}, {Resource: rbac.ResourceHelmChartVersionLabel, Action: rbac.ActionDelete}, + {Resource: rbac.ResourceHelmChartVersionLabel, Action: rbac.ActionList}, {Resource: rbac.ResourceConfiguration, Action: rbac.ActionRead}, {Resource: rbac.ResourceConfiguration, Action: rbac.ActionUpdate}, @@ -176,6 +177,7 @@ var ( {Resource: rbac.ResourceHelmChartVersionLabel, Action: rbac.ActionCreate}, {Resource: rbac.ResourceHelmChartVersionLabel, Action: rbac.ActionDelete}, + {Resource: rbac.ResourceHelmChartVersionLabel, Action: rbac.ActionList}, {Resource: rbac.ResourceConfiguration, Action: rbac.ActionRead}, @@ -234,6 +236,7 @@ var ( {Resource: rbac.ResourceHelmChartVersionLabel, Action: rbac.ActionCreate}, {Resource: rbac.ResourceHelmChartVersionLabel, Action: rbac.ActionDelete}, + {Resource: rbac.ResourceHelmChartVersionLabel, Action: rbac.ActionList}, {Resource: rbac.ResourceConfiguration, Action: rbac.ActionRead}, diff --git a/src/controller/scan/base_controller.go b/src/controller/scan/base_controller.go index f6b902ea3..875f7dc86 100644 --- a/src/controller/scan/base_controller.go +++ b/src/controller/scan/base_controller.go @@ -619,11 +619,23 @@ func (bc *basicController) GetSummary(ctx context.Context, artifact *ar.Artifact } // GetScanLog ... -func (bc *basicController) GetScanLog(ctx context.Context, uuid string) ([]byte, error) { +func (bc *basicController) GetScanLog(ctx context.Context, artifact *ar.Artifact, uuid string) ([]byte, error) { if len(uuid) == 0 { return nil, errors.New("empty uuid to get scan log") } + r, err := bc.sc.GetRegistrationByProject(ctx, artifact.ProjectID) + if err != nil { + return nil, err + } + artifacts, _, err := bc.collectScanningArtifacts(ctx, r, artifact) + if err != nil { + return nil, err + } + artifactMap := map[int64]interface{}{} + for _, a := range artifacts { + artifactMap[a.ID] = struct{}{} + } reportUUIDs := vuln.ParseReportIDs(uuid) tasks, err := bc.listScanTasks(ctx, reportUUIDs) if err != nil { @@ -635,9 +647,12 @@ func (bc *basicController) GetScanLog(ctx context.Context, uuid string) ([]byte, } reportUUIDToTasks := map[string]*task.Task{} - for _, task := range tasks { - for _, reportUUID := range getReportUUIDs(task.ExtraAttrs) { - reportUUIDToTasks[reportUUID] = task + for _, t := range tasks { + if !scanTaskForArtifacts(t, artifactMap) { + return nil, errors.NotFoundError(nil).WithMessage("scan log with uuid: %s not found", uuid) + } + for _, reportUUID := range getReportUUIDs(t.ExtraAttrs) { + reportUUIDToTasks[reportUUID] = t } } @@ -705,6 +720,18 @@ func (bc *basicController) GetScanLog(ctx context.Context, uuid string) ([]byte, return b.Bytes(), nil } +func scanTaskForArtifacts(task *task.Task, artifactMap map[int64]interface{}) bool { + if task == nil { + return false + } + artifactID := int64(task.GetNumFromExtraAttrs(artifactIDKey)) + if artifactID == 0 { + return false + } + _, exist := artifactMap[artifactID] + return exist +} + // DeleteReports ... func (bc *basicController) DeleteReports(ctx context.Context, digests ...string) error { if err := bc.manager.DeleteByDigests(ctx, digests...); err != nil { diff --git a/src/controller/scan/base_controller_test.go b/src/controller/scan/base_controller_test.go index ac56105d0..780d90584 100644 --- a/src/controller/scan/base_controller_test.go +++ b/src/controller/scan/base_controller_test.go @@ -29,6 +29,7 @@ import ( "github.com/goharbor/harbor/src/lib/config" "github.com/goharbor/harbor/src/lib/orm" "github.com/goharbor/harbor/src/lib/q" + art "github.com/goharbor/harbor/src/pkg/artifact" _ "github.com/goharbor/harbor/src/pkg/config/db" _ "github.com/goharbor/harbor/src/pkg/config/inmemory" "github.com/goharbor/harbor/src/pkg/permission/types" @@ -85,7 +86,7 @@ func (suite *ControllerTestSuite) SetupSuite() { suite.artifactCtl = &artifacttesting.Controller{} artifact.Ctl = suite.artifactCtl - suite.artifact = &artifact.Artifact{} + suite.artifact = &artifact.Artifact{Artifact: art.Artifact{ID: 1}} suite.artifact.Type = "IMAGE" suite.artifact.ProjectID = 1 suite.artifact.RepositoryName = "library/photon" @@ -314,7 +315,7 @@ func (suite *ControllerTestSuite) TestScanControllerScan() { }).Once() mock.OnAnything(suite.taskMgr, "List").Return([]*task.Task{ - {ExtraAttrs: suite.makeExtraAttrs("rp-uuid-001"), Status: "Success"}, + {ExtraAttrs: suite.makeExtraAttrs(int64(1), "rp-uuid-001"), Status: "Success"}, }, nil).Once() mock.OnAnything(suite.reportMgr, "Delete").Return(nil).Once() @@ -335,7 +336,7 @@ func (suite *ControllerTestSuite) TestScanControllerScan() { }).Once() mock.OnAnything(suite.taskMgr, "List").Return([]*task.Task{ - {ExtraAttrs: suite.makeExtraAttrs("rp-uuid-001"), Status: "Success"}, + {ExtraAttrs: suite.makeExtraAttrs(int64(1), "rp-uuid-001"), Status: "Success"}, }, nil).Once() mock.OnAnything(suite.reportMgr, "Delete").Return(fmt.Errorf("delete failed")).Once() @@ -351,7 +352,7 @@ func (suite *ControllerTestSuite) TestScanControllerScan() { }).Once() mock.OnAnything(suite.taskMgr, "List").Return([]*task.Task{ - {ExtraAttrs: suite.makeExtraAttrs("rp-uuid-001"), Status: "Running"}, + {ExtraAttrs: suite.makeExtraAttrs(int64(1), "rp-uuid-001"), Status: "Running"}, }, nil).Once() suite.Require().Error(suite.c.Scan(context.TODO(), suite.artifact)) @@ -368,7 +369,7 @@ func (suite *ControllerTestSuite) TestScanControllerStop() { { // success mock.OnAnything(suite.execMgr, "List").Return([]*task.Execution{ - {ExtraAttrs: suite.makeExtraAttrs("rp-uuid-001"), Status: "Running"}, + {ExtraAttrs: suite.makeExtraAttrs(int64(1), "rp-uuid-001"), Status: "Running"}, }, nil).Once() mock.OnAnything(suite.execMgr, "Stop").Return(nil).Once() @@ -405,9 +406,8 @@ func (suite *ControllerTestSuite) TestScanControllerGetReport() { }).Once() mock.OnAnything(suite.taskMgr, "List").Return([]*task.Task{ - {ExtraAttrs: suite.makeExtraAttrs("rp-uuid-001")}, + {ExtraAttrs: suite.makeExtraAttrs(int64(1), "rp-uuid-001")}, }, nil).Once() - rep, err := suite.c.GetReport(context.TODO(), suite.artifact, []string{v1.MimeTypeNativeReport}) require.NoError(suite.T(), err) assert.Equal(suite.T(), 1, len(rep)) @@ -431,13 +431,13 @@ func (suite *ControllerTestSuite) TestScanControllerGetScanLog() { mock.OnAnything(suite.taskMgr, "List").Return([]*task.Task{ { ID: 1, - ExtraAttrs: suite.makeExtraAttrs("rp-uuid-001"), + ExtraAttrs: suite.makeExtraAttrs(int64(1), "rp-uuid-001"), }, }, nil).Once() mock.OnAnything(suite.taskMgr, "GetLog").Return([]byte("log"), nil).Once() - bytes, err := suite.c.GetScanLog(context.TODO(), "rp-uuid-001") + bytes, err := suite.c.GetScanLog(context.TODO(), &artifact.Artifact{Artifact: art.Artifact{ID: 1, ProjectID: 1}}, "rp-uuid-001") require.NoError(suite.T(), err) assert.Condition(suite.T(), func() (success bool) { success = len(bytes) > 0 @@ -450,23 +450,25 @@ func (suite *ControllerTestSuite) TestScanControllerGetMultiScanLog() { suite.taskMgr.On("List", context.TODO(), q.New(kw1)).Return([]*task.Task{ { ID: 1, - ExtraAttrs: suite.makeExtraAttrs("rp-uuid-001"), + ExtraAttrs: suite.makeExtraAttrs(int64(1), "rp-uuid-001"), }, }, nil).Times(4) - + mock.OnAnything(suite.ar, "Walk").Return(nil).Run(func(args mock.Arguments) { + walkFn := args.Get(2).(func(*artifact.Artifact) error) + walkFn(suite.artifact) + }) kw2 := q.KeyWords{"extra_attrs.report:rp-uuid-002": "1"} suite.taskMgr.On("List", context.TODO(), q.New(kw2)).Return([]*task.Task{ { ID: 2, - ExtraAttrs: suite.makeExtraAttrs("rp-uuid-002"), + ExtraAttrs: suite.makeExtraAttrs(int64(1), "rp-uuid-002"), }, }, nil).Times(4) - { // Both success mock.OnAnything(suite.taskMgr, "GetLog").Return([]byte("log"), nil).Twice() - bytes, err := suite.c.GetScanLog(context.TODO(), base64.StdEncoding.EncodeToString([]byte("rp-uuid-001|rp-uuid-002"))) + bytes, err := suite.c.GetScanLog(context.TODO(), &artifact.Artifact{Artifact: art.Artifact{ID: 1, ProjectID: 1}}, base64.StdEncoding.EncodeToString([]byte("rp-uuid-001|rp-uuid-002"))) suite.Nil(err) suite.NotEmpty(bytes) suite.Contains(string(bytes), "Logs of report rp-uuid-001") @@ -478,7 +480,7 @@ func (suite *ControllerTestSuite) TestScanControllerGetMultiScanLog() { suite.taskMgr.On("GetLog", context.TODO(), int64(1)).Return([]byte("log"), nil).Once() suite.taskMgr.On("GetLog", context.TODO(), int64(2)).Return(nil, fmt.Errorf("failed")).Once() - bytes, err := suite.c.GetScanLog(context.TODO(), base64.StdEncoding.EncodeToString([]byte("rp-uuid-001|rp-uuid-002"))) + bytes, err := suite.c.GetScanLog(context.TODO(), &artifact.Artifact{Artifact: art.Artifact{ID: 1, ProjectID: 1}}, base64.StdEncoding.EncodeToString([]byte("rp-uuid-001|rp-uuid-002"))) suite.Nil(err) suite.NotEmpty(bytes) suite.NotContains(string(bytes), "Logs of report rp-uuid-001") @@ -488,7 +490,7 @@ func (suite *ControllerTestSuite) TestScanControllerGetMultiScanLog() { // Both failed mock.OnAnything(suite.taskMgr, "GetLog").Return(nil, fmt.Errorf("failed")).Twice() - bytes, err := suite.c.GetScanLog(context.TODO(), base64.StdEncoding.EncodeToString([]byte("rp-uuid-001|rp-uuid-002"))) + bytes, err := suite.c.GetScanLog(context.TODO(), &artifact.Artifact{Artifact: art.Artifact{ID: 1, ProjectID: 1}}, base64.StdEncoding.EncodeToString([]byte("rp-uuid-001|rp-uuid-002"))) suite.Error(err) suite.Empty(bytes) } @@ -497,7 +499,7 @@ func (suite *ControllerTestSuite) TestScanControllerGetMultiScanLog() { // Both empty mock.OnAnything(suite.taskMgr, "GetLog").Return(nil, nil).Twice() - bytes, err := suite.c.GetScanLog(context.TODO(), base64.StdEncoding.EncodeToString([]byte("rp-uuid-001|rp-uuid-002"))) + bytes, err := suite.c.GetScanLog(context.TODO(), &artifact.Artifact{Artifact: art.Artifact{ID: 1, ProjectID: 1}}, base64.StdEncoding.EncodeToString([]byte("rp-uuid-001|rp-uuid-002"))) suite.Nil(err) suite.Empty(bytes) } @@ -565,11 +567,12 @@ func (suite *ControllerTestSuite) TestDeleteReports() { suite.Error(suite.c.DeleteReports(context.TODO(), "digest")) } -func (suite *ControllerTestSuite) makeExtraAttrs(reportUUIDs ...string) map[string]interface{} { +func (suite *ControllerTestSuite) makeExtraAttrs(artifactID int64, reportUUIDs ...string) map[string]interface{} { b, _ := json.Marshal(map[string]interface{}{reportUUIDsKey: reportUUIDs}) extraAttrs := map[string]interface{}{} json.Unmarshal(b, &extraAttrs) + extraAttrs[artifactIDKey] = float64(artifactID) return extraAttrs } diff --git a/src/controller/scan/controller.go b/src/controller/scan/controller.go index 773e83688..47b47ccdb 100644 --- a/src/controller/scan/controller.go +++ b/src/controller/scan/controller.go @@ -93,7 +93,7 @@ type Controller interface { // Returns: // []byte : the log text stream // error : non nil error if any errors occurred - GetScanLog(ctx context.Context, uuid string) ([]byte, error) + GetScanLog(ctx context.Context, art *artifact.Artifact, uuid string) ([]byte, error) // Delete the reports related with the specified digests // diff --git a/src/core/api/chart_label.go b/src/core/api/chart_label.go index c29116bd4..0e1b1edf7 100644 --- a/src/core/api/chart_label.go +++ b/src/core/api/chart_label.go @@ -107,5 +107,8 @@ func (cla *ChartLabelAPI) RemoveLabel() { // GetLabels gets labels for the specified chart version. func (cla *ChartLabelAPI) GetLabels() { + if !cla.requireAccess(rbac.ActionList) { + return + } cla.getLabelsOfResource(common.ResourceTypeChart, cla.chartFullName) } diff --git a/src/server/v2.0/handler/artifact.go b/src/server/v2.0/handler/artifact.go index 89a8f73d1..7ed4b250b 100644 --- a/src/server/v2.0/handler/artifact.go +++ b/src/server/v2.0/handler/artifact.go @@ -25,6 +25,7 @@ import ( "github.com/docker/distribution/reference" "github.com/go-openapi/runtime" "github.com/go-openapi/runtime/middleware" + "github.com/goharbor/harbor/src/common" "github.com/goharbor/harbor/src/common/rbac" "github.com/goharbor/harbor/src/common/utils" "github.com/goharbor/harbor/src/controller/artifact" @@ -35,6 +36,7 @@ import ( "github.com/goharbor/harbor/src/controller/tag" "github.com/goharbor/harbor/src/lib" "github.com/goharbor/harbor/src/lib/errors" + "github.com/goharbor/harbor/src/pkg/label" "github.com/goharbor/harbor/src/pkg/notification" "github.com/goharbor/harbor/src/pkg/scan/report" "github.com/goharbor/harbor/src/server/v2.0/handler/assembler" @@ -46,21 +48,23 @@ import ( func newArtifactAPI() *artifactAPI { return &artifactAPI{ - artCtl: artifact.Ctl, - proCtl: project.Ctl, - repoCtl: repository.Ctl, - scanCtl: scan.DefaultController, - tagCtl: tag.Ctl, + artCtl: artifact.Ctl, + proCtl: project.Ctl, + repoCtl: repository.Ctl, + scanCtl: scan.DefaultController, + tagCtl: tag.Ctl, + labelMgr: label.Mgr, } } type artifactAPI struct { BaseAPI - artCtl artifact.Controller - proCtl project.Controller - repoCtl repository.Controller - scanCtl scan.Controller - tagCtl tag.Controller + artCtl artifact.Controller + proCtl project.Controller + repoCtl repository.Controller + scanCtl scan.Controller + tagCtl tag.Controller + labelMgr label.Manager } func (a *artifactAPI) Prepare(ctx context.Context, operation string, params interface{}) middleware.Responder { @@ -397,7 +401,14 @@ func (a *artifactAPI) GetAddition(ctx context.Context, params operation.GetAddit } func (a *artifactAPI) AddLabel(ctx context.Context, params operation.AddLabelParams) middleware.Responder { - if err := a.RequireProjectAccess(ctx, params.ProjectName, rbac.ActionCreate, rbac.ResourceArtifactLabel); err != nil { + projectID, err := getProjectID(ctx, params.ProjectName) + if err != nil { + return a.SendError(ctx, err) + } + if err := a.RequireProjectAccess(ctx, projectID, rbac.ActionCreate, rbac.ResourceArtifactLabel); err != nil { + return a.SendError(ctx, err) + } + if err := a.RequireLabelInProject(ctx, projectID, params.Label.ID); err != nil { return a.SendError(ctx, err) } art, err := a.artCtl.GetByReference(ctx, fmt.Sprintf("%s/%s", params.ProjectName, params.RepositoryName), params.Reference, nil) @@ -424,6 +435,17 @@ func (a *artifactAPI) RemoveLabel(ctx context.Context, params operation.RemoveLa return operation.NewRemoveLabelOK() } +func (a *artifactAPI) RequireLabelInProject(ctx context.Context, projectID, labelID int64) error { + l, err := a.labelMgr.Get(ctx, labelID) + if err != nil { + return err + } + if l.Scope == common.LabelScopeProject && l.ProjectID != projectID { + return errors.NotFoundError(nil).WithMessage("project id %d, label %d not found", projectID, labelID) + } + return nil +} + func option(withTag, withImmutableStatus, withLabel, withSignature *bool) *artifact.Option { option := &artifact.Option{ WithTag: true, // return the tag by default diff --git a/src/server/v2.0/handler/robot.go b/src/server/v2.0/handler/robot.go index bbd3ac851..f6cb84f98 100644 --- a/src/server/v2.0/handler/robot.go +++ b/src/server/v2.0/handler/robot.go @@ -1,3 +1,17 @@ +// Copyright Project Harbor Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + package handler import ( @@ -92,7 +106,7 @@ func (rAPI *robotAPI) DeleteRobot(ctx context.Context, params operation.DeleteRo } if err := rAPI.robotCtl.Delete(ctx, params.RobotID); err != nil { - // for the version 1 robot account, has to ignore the no permissions error. + // for the version 1 robot account, has to ignore the no permission error. if !r.Editable && errors.IsNotFoundErr(err) { return operation.NewDeleteRobotOK() } @@ -284,7 +298,14 @@ func (rAPI *robotAPI) updateV2Robot(ctx context.Context, params operation.Update return err } - if err := rAPI.requireAccess(ctx, params.Robot.Level, params.Robot.Permissions[0].Namespace, rbac.ActionUpdate); err != nil { + projectID, err := getProjectID(ctx, params.Robot.Permissions[0].Namespace) + if err != nil { + return err + } + if r.Level != robot.LEVELSYSTEM && r.ProjectID != projectID { + return errors.BadRequestError(nil).WithMessage("cannot update the project id of robot") + } + if err := rAPI.requireAccess(ctx, params.Robot.Level, projectID, rbac.ActionUpdate); err != nil { return err } @@ -329,7 +350,7 @@ func isValidDuration(d int64) bool { func isValidSec(sec string) bool { hasLower := regexp.MustCompile(`[a-z]`) hasUpper := regexp.MustCompile(`[A-Z]`) - hasNumber := regexp.MustCompile(`[0-9]`) + hasNumber := regexp.MustCompile(`\d`) if len(sec) >= 8 && hasLower.MatchString(sec) && hasUpper.MatchString(sec) && hasNumber.MatchString(sec) { return true } diff --git a/src/server/v2.0/handler/scan.go b/src/server/v2.0/handler/scan.go index e1f566550..6f5c7fa20 100644 --- a/src/server/v2.0/handler/scan.go +++ b/src/server/v2.0/handler/scan.go @@ -93,14 +93,13 @@ func (s *scanAPI) GetReportLog(ctx context.Context, params operation.GetReportLo if err := s.RequireProjectAccess(ctx, params.ProjectName, rbac.ActionRead, rbac.ResourceScan); err != nil { return s.SendError(ctx, err) } - repository := fmt.Sprintf("%s/%s", params.ProjectName, params.RepositoryName) - _, err := s.artCtl.GetByReference(ctx, repository, params.Reference, nil) + a, err := s.artCtl.GetByReference(ctx, repository, params.Reference, nil) if err != nil { return s.SendError(ctx, err) } - bytes, err := s.scanCtl.GetScanLog(ctx, params.ReportID) + bytes, err := s.scanCtl.GetScanLog(ctx, a, params.ReportID) if err != nil { return s.SendError(ctx, err) } diff --git a/src/testing/controller/scan/controller.go b/src/testing/controller/scan/controller.go index 1805e10f8..47e5032a0 100644 --- a/src/testing/controller/scan/controller.go +++ b/src/testing/controller/scan/controller.go @@ -65,13 +65,13 @@ func (_m *Controller) GetReport(ctx context.Context, _a1 *artifact.Artifact, mim return r0, r1 } -// GetScanLog provides a mock function with given fields: ctx, uuid -func (_m *Controller) GetScanLog(ctx context.Context, uuid string) ([]byte, error) { - ret := _m.Called(ctx, uuid) +// GetScanLog provides a mock function with given fields: ctx, art, uuid +func (_m *Controller) GetScanLog(ctx context.Context, art *artifact.Artifact, uuid string) ([]byte, error) { + ret := _m.Called(ctx, art, uuid) var r0 []byte - if rf, ok := ret.Get(0).(func(context.Context, string) []byte); ok { - r0 = rf(ctx, uuid) + if rf, ok := ret.Get(0).(func(context.Context, *artifact.Artifact, string) []byte); ok { + r0 = rf(ctx, art, uuid) } else { if ret.Get(0) != nil { r0 = ret.Get(0).([]byte) @@ -79,8 +79,8 @@ func (_m *Controller) GetScanLog(ctx context.Context, uuid string) ([]byte, erro } var r1 error - if rf, ok := ret.Get(1).(func(context.Context, string) error); ok { - r1 = rf(ctx, uuid) + if rf, ok := ret.Get(1).(func(context.Context, *artifact.Artifact, string) error); ok { + r1 = rf(ctx, art, uuid) } else { r1 = ret.Error(1) }