Skip to content

Commit

Permalink
Fixes for various bugs/issues logged as a part of the test day. (goha…
Browse files Browse the repository at this point in the history 
…rbor#17232)

Closes:
* CVE Data Export API IDOR issue
* goharbor#17199
* goharbor#17193
* goharbor#17188
* goharbor#17184

Signed-off-by: prahaladdarkin <prahaladd@vmware.com>
  • Loading branch information
@prahaladdarkin @sluetze
prahaladdarkin authored and sluetze committed Oct 29, 2022
1 parent ca2218e commit 331260c
Show file tree
Hide file tree
Showing 6 changed files with 133 additions and 36 deletions.
9 changes: 6 additions & 3 deletions src/jobservice/job/impl/scandataexport/scan_data_export.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@ import (
"github.com/opencontainers/go-digest"

"github.com/goharbor/harbor/src/jobservice/job"
"github.com/goharbor/harbor/src/jobservice/logger"
"github.com/goharbor/harbor/src/lib/errors"
"github.com/goharbor/harbor/src/pkg/project"
"github.com/goharbor/harbor/src/pkg/scan/export"
Expand Down Expand Up @@ -76,12 +75,13 @@ func (sde *ScanDataExport) Run(ctx job.Context, params job.Parameters) error {
}

mode := params[export.JobModeKey].(string)
logger := ctx.GetLogger()
logger.Infof("Scan data export job started in mode : %v", mode)
sde.init()
fileName := fmt.Sprintf("%s/scandata_export_%v.csv", sde.scanDataExportDirPath, params["JobId"])

// ensure that CSV files are cleared post the completion of the Run.
defer sde.cleanupCsvFile(fileName, params)
defer sde.cleanupCsvFile(ctx, fileName, params)
err := sde.writeCsvFile(ctx, params, fileName)
if err != nil {
logger.Errorf("error when writing data to CSV: %v", err)
Expand Down Expand Up @@ -133,6 +133,7 @@ func (sde *ScanDataExport) Run(ctx job.Context, params job.Parameters) error {
func (sde *ScanDataExport) updateExecAttributes(ctx job.Context, params job.Parameters, err error, hash digest.Digest) error {
execID := int64(params["JobId"].(float64))
exec, err := sde.execMgr.Get(ctx.SystemContext(), execID)
logger := ctx.GetLogger()
if err != nil {
logger.Errorf("Export Job Id = %v. Error when fetching execution record for update : %v", params["JobId"], err)
return err
Expand All @@ -153,6 +154,7 @@ func (sde *ScanDataExport) writeCsvFile(ctx job.Context, params job.Parameters,
systemContext := ctx.SystemContext()
defer csvFile.Close()

logger := ctx.GetLogger()
if err != nil {
logger.Errorf("Failed to create CSV export file %s. Error : %v", fileName, err)
return err
Expand Down Expand Up @@ -329,7 +331,8 @@ func (sde *ScanDataExport) init() {
}
}

func (sde *ScanDataExport) cleanupCsvFile(fileName string, params job.Parameters) {
func (sde *ScanDataExport) cleanupCsvFile(ctx job.Context, fileName string, params job.Parameters) {
logger := ctx.GetLogger()
if _, err := os.Stat(fileName); os.IsNotExist(err) {
logger.Infof("Export Job Id = %v, CSV Export File = %s does not exist. Nothing to do", params["JobId"], fileName)
return
Expand Down
49 changes: 32 additions & 17 deletions src/pkg/scan/export/manager.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ const (
VulnScanReportQuery = `select row_number() over() as result_row_id, project.project_id as project_id, project."name" as project_name, harbor_user.user_id as user_id, harbor_user.username as project_owner, repository.repository_id, repository.name as repository_name,
scanner_registration.id as scanner_id, scanner_registration."name" as scanner_name,
vulnerability_record.cve_id, vulnerability_record.package, vulnerability_record.severity,
vulnerability_record.cvss_score_v3, vulnerability_record.cvss_score_v2, vulnerability_record.cvss_vector_v3, vulnerability_record.cvss_vector_v2, vulnerability_record.cwe_ids from report_vulnerability_record inner join scan_report on report_vulnerability_record.report_uuid = scan_report.uuid
vulnerability_record.cvss_score_v3, vulnerability_record.cvss_score_v2, vulnerability_record.cvss_vector_v3, vulnerability_record.cvss_vector_v2, vulnerability_record.cwe_ids, vulnerability_record.package_version, vulnerability_record.fixed_version from report_vulnerability_record inner join scan_report on report_vulnerability_record.report_uuid = scan_report.uuid
inner join artifact on scan_report.digest = artifact.digest
left outer join artifact_reference on artifact.id = artifact_reference.child_id
inner join vulnerability_record on report_vulnerability_record.vuln_record_id = vulnerability_record.id
Expand All @@ -29,19 +29,20 @@ inner join repository on artifact.repository_id = repository.repository_id
inner join tag on tag.repository_id = repository.repository_id
inner join harbor_user on project.owner_id = harbor_user.user_id
inner join scanner_registration on scan_report.registration_uuid = scanner_registration.uuid `
ArtifactBylabelQueryTemplate = "select distinct artifact.id from artifact inner join label_reference on artifact.id = label_reference.artifact_id inner join harbor_label on label_reference.label_id = harbor_label.id and harbor_label.id in (%s)"
SQLAnd = " and "
SQLOr = " or "
RepositoryIDColumn = "repository.repository_id"
ProjectIDColumn = "project.project_id"
TagIDColumn = "tag.id"
ArtifactParentIDColumn = "artifact_reference.parent_id"
ArtifactIDColumn = "artifact.id"
GroupBy = " group by "
GroupByCols = `package, vulnerability_record.severity, vulnerability_record.cve_id, project.project_id, harbor_user.user_id ,
ArtifactBylabelQueryTemplate = "select distinct artifact.id from artifact inner join label_reference on artifact.id = label_reference.artifact_id inner join harbor_label on label_reference.label_id = harbor_label.id and harbor_label.id in (%s)"
ArtifactByLabelRepoFilterTemplate = " and artifact.repository_id in (%s)"
SQLAnd = " and "
SQLOr = " or "
RepositoryIDColumn = "repository.repository_id"
ProjectIDColumn = "project.project_id"
TagIDColumn = "tag.id"
ArtifactParentIDColumn = "artifact_reference.parent_id"
ArtifactIDColumn = "artifact.id"
GroupBy = " group by "
GroupByCols = `package, vulnerability_record.severity, vulnerability_record.cve_id, project.project_id, harbor_user.user_id ,
repository.repository_id, scanner_registration.id, vulnerability_record.cvss_score_v3,
vulnerability_record.cvss_score_v2, vulnerability_record.cvss_vector_v3, vulnerability_record.cvss_vector_v2,
vulnerability_record.cwe_ids`
vulnerability_record.cwe_ids, vulnerability_record.package_version, vulnerability_record.fixed_version`
JobModeExport = "export"
JobModeKey = "mode"
)
Expand Down Expand Up @@ -107,7 +108,7 @@ func NewManager() Manager {

func (em *exportManager) Fetch(ctx context.Context, params Params) ([]Data, error) {
exportData := make([]Data, 0)
artifactIdsWithLabel, err := em.getArtifactsWithLabel(ctx, params.Labels)
artifactIdsWithLabel, err := em.getArtifactsWithLabel(ctx, params.Labels, params.Repositories)
if err != nil {
return nil, err
}
Expand Down Expand Up @@ -226,24 +227,38 @@ func (em *exportManager) buildIDFilterFragmentWithInForMultipleCols(ids []int64,
// the specified label ids.
// Within Harbor, labels are attached to the root artifact whereas scan results
// are associated with the child artifact.
func (em *exportManager) getArtifactsWithLabel(ctx context.Context, ids []int64) ([]int64, error) {
func (em *exportManager) getArtifactsWithLabel(ctx context.Context, ids []int64, repositoryIds []int64) ([]int64, error) {
artifactIds := make([]int64, 0)

if len(ids) == 0 {
return artifactIds, nil
}
artifactQueryBuilder := strings.Builder{}

strIds := make([]string, 0)
for _, id := range ids {
strIds = append(strIds, strconv.FormatInt(id, 10))
}
artifactQuery := fmt.Sprintf(ArtifactBylabelQueryTemplate, strings.Join(strIds, ","))
logger.Infof("Triggering artifact query : %s", artifactQuery)
artifactQueryFragment := fmt.Sprintf(ArtifactBylabelQueryTemplate, strings.Join(strIds, ","))
artifactQueryBuilder.WriteString(artifactQueryFragment)

// append any repository filters
strRepoIds := make([]string, 0)
for _, repoID := range repositoryIds {
strRepoIds = append(strRepoIds, strconv.FormatInt(repoID, 10))
}
if len(repositoryIds) > 0 {
repoFilterFragment := fmt.Sprintf(ArtifactByLabelRepoFilterTemplate, strings.Join(strRepoIds, ","))
artifactQueryBuilder.WriteString(repoFilterFragment)
}

logger.Infof("Triggering artifact query : %s", artifactQueryBuilder.String())

ormer, err := orm.FromContext(ctx)
if err != nil {
return nil, err
}
numRows, err := ormer.Raw(artifactQuery).QueryRows(&artifactIds)
numRows, err := ormer.Raw(artifactQueryFragment).QueryRows(&artifactIds)
if err != nil {
return nil, err
}
Expand Down
2 changes: 1 addition & 1 deletion src/pkg/scan/export/manager_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -217,7 +217,7 @@ func (suite *ExportManagerSuite) generateVulnerabilityRecordsForReport(registrat
vulnV2 := new(daoscan.VulnerabilityRecord)
vulnV2.CVEID = fmt.Sprintf("CVE-ID%d", i)
vulnV2.Package = fmt.Sprintf("Package%d", i)
vulnV2.PackageVersion = "NotAvailable"
vulnV2.PackageVersion = "Package-0.9.0"
vulnV2.PackageType = "Unknown"
vulnV2.Fix = "1.0.0"
vulnV2.URLs = "url1"
Expand Down
29 changes: 15 additions & 14 deletions src/pkg/scan/export/model.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,20 +9,21 @@ import (
// Data models a single row of the exported scan vulnerability data

type Data struct {
ID int64 `orm:"column(result_row_id)" csv:"RowId"`
ProjectName string `orm:"column(project_name)" csv:"Project"`
ProjectOwner string `orm:"column(project_owner)" csv:"Owner"`
ScannerName string `orm:"column(scanner_name)" csv:"Scanner"`
Repository string `orm:"column(repository_name)" csv:"Repository"`
ArtifactDigest string `orm:"column(artifact_digest)" csv:"Artifact Digest"`
CVEId string `orm:"column(cve_id)" csv:"CVE"`
Package string `orm:"column(package)" csv:"Package"`
Severity string `orm:"column(severity)" csv:"Severity"`
CVSSScoreV3 string `orm:"column(cvss_score_v3)" csv:"CVSS V3 Score"`
CVSSScoreV2 string `orm:"column(cvss_score_v2)" csv:"CVSS V2 Score"`
CVSSVectorV3 string `orm:"column(cvss_vector_v3)" csv:"CVSS V3 Vector"`
CVSSVectorV2 string `orm:"column(cvss_vector_v2)" csv:"CVSS V2 Vector"`
CWEIds string `orm:"column(cwe_ids)" csv:"CWE Ids"`
ID int64 `orm:"column(result_row_id)" csv:"RowId"`
ProjectName string `orm:"column(project_name)" csv:"Project"`
ProjectOwner string `orm:"column(project_owner)" csv:"Owner"`
ScannerName string `orm:"column(scanner_name)" csv:"Scanner"`
Repository string `orm:"column(repository_name)" csv:"Repository"`
CVEId string `orm:"column(cve_id)" csv:"CVE"`
Package string `orm:"column(package)" csv:"Package"`
Version string `orm:"column(package_version)" csv:"Current Version"`
FixVersion string `orm:"column(fixed_version)" csv:"Fixed in version"`
Severity string `orm:"column(severity)" csv:"Severity"`
CVSSScoreV3 string `orm:"column(cvss_score_v3)" csv:"CVSS V3 Score"`
CVSSScoreV2 string `orm:"column(cvss_score_v2)" csv:"CVSS V2 Score"`
CVSSVectorV3 string `orm:"column(cvss_vector_v3)" csv:"CVSS V3 Vector"`
CVSSVectorV2 string `orm:"column(cvss_vector_v2)" csv:"CVSS V2 Vector"`
CWEIds string `orm:"column(cwe_ids)" csv:"CWE Ids"`
}

// Request encapsulates the filters to be provided when exporting the data for a scan.
Expand Down
16 changes: 16 additions & 0 deletions src/server/v2.0/handler/scanexport.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@ import (
"net/http"
"strings"

"github.com/goharbor/harbor/src/lib/errors"

"github.com/go-openapi/runtime"
"github.com/go-openapi/runtime/middleware"
"github.com/go-openapi/strfmt"
Expand Down Expand Up @@ -107,6 +109,20 @@ func (se *scanDataExportAPI) GetScanDataExportExecution(ctx context.Context, par
return se.SendError(ctx, err)
}
execution, err := se.scanDataExportCtl.GetExecution(ctx, params.ExecutionID)
if err != nil {
return se.SendError(ctx, err)
}

// check if the execution being fetched is owned by the current user
secContext, err := se.GetSecurityContext(ctx)
if err != nil {
return se.SendError(ctx, err)
}
if secContext.GetUsername() != execution.UserName {
err = errors.New(nil).WithCode(errors.ForbiddenCode)
return se.SendError(ctx, err)
}

if err != nil {
return se.SendError(ctx, err)
}
Expand Down
64 changes: 63 additions & 1 deletion src/server/v2.0/handler/scanexport_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -252,7 +252,7 @@ func (suite *ScanExportTestSuite) TestExportScanDataNoPrivileges() {
}

func (suite *ScanExportTestSuite) TestGetScanDataExportExecution() {

suite.Security.On("GetUsername").Return("test-user")
suite.Security.On("IsAuthenticated").Return(true).Once()
suite.Security.On("Can", mock.Anything, mock.Anything, mock.Anything).Return(true).Once()
url := "/export/cve/execution/100"
Expand Down Expand Up @@ -284,6 +284,33 @@ func (suite *ScanExportTestSuite) TestGetScanDataExportExecution() {

}

func (suite *ScanExportTestSuite) TestGetScanDataExportExecutionUserNotOwnerOfExport() {
suite.Security.On("GetUsername").Return("test-user")
suite.Security.On("IsAuthenticated").Return(true).Once()
suite.Security.On("Can", mock.Anything, mock.Anything, mock.Anything).Return(true).Once()
url := "/export/cve/execution/100"
endTime := time.Now()
startTime := endTime.Add(-10 * time.Minute)

execution := &export.Execution{
ID: 100,
UserID: 3,
Status: "Success",
StatusMessage: "",
Trigger: "MANUAL",
StartTime: startTime,
EndTime: endTime,
ExportDataDigest: "datadigest",
UserName: "test-user1",
JobName: "test-job",
FilePresent: false,
}
mock.OnAnything(suite.scanExportCtl, "GetExecution").Return(execution, nil).Once()
res, err := suite.DoReq(http.MethodGet, url, nil)
suite.Equal(http.StatusForbidden, res.StatusCode)
suite.Equal(nil, err)
}

func (suite *ScanExportTestSuite) TestDownloadScanData() {
suite.Security.On("GetUsername").Return("test-user")
suite.Security.On("IsAuthenticated").Return(true).Once()
Expand Down Expand Up @@ -476,6 +503,41 @@ func (suite *ScanExportTestSuite) TestGetScanDataExportExecutionList() {
suite.Equal(int64(100), respData.Items[0].ID)
}

func (suite *ScanExportTestSuite) TestGetScanDataExportExecutionListFilterNotOwned() {
suite.Security.On("GetUsername").Return("test-user")
suite.Security.On("IsAuthenticated").Return(true).Once()
suite.Security.On("Can", mock.Anything, mock.Anything, mock.Anything).Return(true).Once()
url, err := url2.Parse("/export/cve/executions")
params := url2.Values{}
params.Add("user_name", "test-user")
url.RawQuery = params.Encode()
endTime := time.Now()
startTime := endTime.Add(-10 * time.Minute)

executionOwned := &export.Execution{
ID: 100,
UserID: 3,
Status: "Success",
StatusMessage: "",
Trigger: "MANUAL",
StartTime: startTime,
EndTime: endTime,
ExportDataDigest: "datadigest",
JobName: "test-job",
UserName: "test-user",
}

fmt.Println("URL string : ", url.String())
mock.OnAnything(suite.scanExportCtl, "ListExecutions").Return([]*export.Execution{executionOwned}, nil).Once()
res, err := suite.DoReq(http.MethodGet, url.String(), nil)
suite.Equal(200, res.StatusCode)
suite.Equal(nil, err)
respData := models.ScanDataExportExecutionList{}
json.NewDecoder(res.Body).Decode(&respData)
suite.Equal(1, len(respData.Items))
suite.Equal(int64(100), respData.Items[0].ID)
}

func TestScanExportTestSuite(t *testing.T) {
suite.Run(t, &ScanExportTestSuite{})
}

0 comments on commit 331260c

Please sign in to comment.