Skip to content

Commit

Permalink
Fix incorrect artifact and scanned artifact count issue (#19106)
Browse files Browse the repository at this point in the history 
* Fix incorrect artifact and scanned artifact count issue

  fixes #19009 #19020 #19013

Signed-off-by: stonezdj <[email protected]>

* fix issue

Signed-off-by: stonezdj <[email protected]>

---------

Signed-off-by: stonezdj <[email protected]>
  • Loading branch information
@stonezdj
stonezdj authored Aug 7, 2023
1 parent f8cf772 commit 3de778e
Show file tree
Hide file tree
Showing 6 changed files with 113 additions and 34 deletions.
21 changes: 7 additions & 14 deletions src/controller/securityhub/controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,6 @@ import (
"context"

"github.com/goharbor/harbor/src/lib/q"
"github.com/goharbor/harbor/src/pkg"
"github.com/goharbor/harbor/src/pkg/artifact"
"github.com/goharbor/harbor/src/pkg/scan/scanner"
"github.com/goharbor/harbor/src/pkg/securityhub"
secHubModel "github.com/goharbor/harbor/src/pkg/securityhub/model"
Expand Down Expand Up @@ -71,19 +69,17 @@ type Controller interface {
}

type controller struct {
artifactMgr artifact.Manager
scannerMgr scanner.Manager
secHubMgr securityhub.Manager
tagMgr tag.Manager
scannerMgr scanner.Manager
secHubMgr securityhub.Manager
tagMgr tag.Manager
}

// NewController ...
func NewController() Controller {
return &controller{
artifactMgr: pkg.ArtifactMgr,
scannerMgr: scanner.Mgr,
secHubMgr: securityhub.Mgr,
tagMgr: tag.Mgr,
scannerMgr: scanner.Mgr,
secHubMgr: securityhub.Mgr,
tagMgr: tag.Mgr,
}
}

Expand Down Expand Up @@ -129,10 +125,7 @@ func (c *controller) scannedArtifactCount(ctx context.Context, projectID int64)
}

func (c *controller) totalArtifactCount(ctx context.Context, projectID int64) (int64, error) {
if projectID == 0 {
return c.artifactMgr.Count(ctx, nil)
}
return c.artifactMgr.Count(ctx, q.New(q.KeyWords{"project_id": projectID}))
return c.secHubMgr.TotalArtifactsCount(ctx, projectID)
}

func (c *controller) ListVuls(ctx context.Context, scannerUUID string, projectID int64, withTag bool, query *q.Query) ([]*secHubModel.VulnerabilityItem, error) {
Expand Down
23 changes: 10 additions & 13 deletions src/controller/securityhub/controller_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@ import (
"github.com/goharbor/harbor/src/pkg/tag/model/tag"
htesting "github.com/goharbor/harbor/src/testing"
"github.com/goharbor/harbor/src/testing/mock"
artifactMock "github.com/goharbor/harbor/src/testing/pkg/artifact"
scannerMock "github.com/goharbor/harbor/src/testing/pkg/scan/scanner"
securityMock "github.com/goharbor/harbor/src/testing/pkg/securityhub"
tagMock "github.com/goharbor/harbor/src/testing/pkg/tag"
Expand All @@ -42,11 +41,10 @@ var sum = &model.Summary{

type ControllerTestSuite struct {
htesting.Suite
c *controller
artifactMgr *artifactMock.Manager
scannerMgr *scannerMock.Manager
secHubMgr *securityMock.Manager
tagMgr *tagMock.FakeManager
c *controller
scannerMgr *scannerMock.Manager
secHubMgr *securityMock.Manager
tagMgr *tagMock.FakeManager
}

// TestController is the entry of controller test suite
Expand All @@ -56,16 +54,14 @@ func TestController(t *testing.T) {

// SetupTest prepares env for the controller test suite
func (suite *ControllerTestSuite) SetupTest() {
suite.artifactMgr = &artifactMock.Manager{}
suite.secHubMgr = &securityMock.Manager{}
suite.scannerMgr = &scannerMock.Manager{}
suite.tagMgr = &tagMock.FakeManager{}

suite.c = &controller{
artifactMgr: suite.artifactMgr,
secHubMgr: suite.secHubMgr,
scannerMgr: suite.scannerMgr,
tagMgr: suite.tagMgr,
secHubMgr: suite.secHubMgr,
scannerMgr: suite.scannerMgr,
tagMgr: suite.tagMgr,
}
}

Expand All @@ -76,7 +72,7 @@ func (suite *ControllerTestSuite) TearDownTest() {
func (suite *ControllerTestSuite) TestSecuritySummary() {
ctx := suite.Context()

mock.OnAnything(suite.artifactMgr, "Count").Return(int64(1234), nil)
mock.OnAnything(suite.secHubMgr, "TotalArtifactsCount").Return(int64(1234), nil)
mock.OnAnything(suite.secHubMgr, "ScannedArtifactsCount").Return(int64(1000), nil)
mock.OnAnything(suite.secHubMgr, "Summary").Return(sum, nil).Twice()
mock.OnAnything(suite.scannerMgr, "DefaultScannerUUID").Return("ruuid", nil)
Expand Down Expand Up @@ -125,12 +121,13 @@ func (suite *ControllerTestSuite) TestSecuritySummary() {
func (suite *ControllerTestSuite) TestSecuritySummaryError() {
ctx := suite.Context()
mock.OnAnything(suite.scannerMgr, "DefaultScannerUUID").Return("ruuid", nil)
mock.OnAnything(suite.secHubMgr, "TotalArtifactsCount").Return(int64(0), errors.New("project not found")).Once()
mock.OnAnything(suite.secHubMgr, "ScannedArtifactsCount").Return(int64(1000), nil)
mock.OnAnything(suite.secHubMgr, "Summary").Return(nil, errors.New("invalid project")).Once()
summary, err := suite.c.SecuritySummary(ctx, 0, WithCVE(false), WithArtifact(false))
suite.Error(err)
suite.Nil(summary)
mock.OnAnything(suite.artifactMgr, "Count").Return(int64(0), errors.New("failed to connect db")).Once()
mock.OnAnything(suite.secHubMgr, "TotalArtifactsCount").Return(int64(0), errors.New("failed to connect db")).Once()
mock.OnAnything(suite.secHubMgr, "Summary").Return(sum, nil).Once()
summary, err = suite.c.SecuritySummary(ctx, 0, WithCVE(false), WithArtifact(false))
suite.Error(err)
Expand Down
51 changes: 46 additions & 5 deletions src/pkg/securityhub/dao/security.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -47,10 +47,36 @@ where a.digest = s.digest
order by s.critical_cnt desc, s.high_cnt desc, s.medium_cnt desc, s.low_cnt desc
limit 5`

// sql to query the scanned artifact count
scannedArtifactCountSQL = `select count(1)
from artifact
where exists (select 1 from scan_report s where artifact.digest = s.digest and s.registration_uuid = ?) `
// sql to query the total artifact count, exclude the artifact accessory, and child artifact in image index
totalArtifactCountSQL = `SELECT COUNT(1)
FROM artifact A
WHERE NOT EXISTS (select 1 from artifact_accessory acc WHERE acc.artifact_id = a.id)
AND (EXISTS (SELECT 1 FROM tag WHERE tag.artifact_id = a.id)
OR NOT EXISTS (SELECT 1 FROM artifact_reference ref WHERE ref.child_id = a.id))`

// sql to query the scanned artifact count, exclude the artifact accessory, and child artifact in image index,
// and include the image index artifact which at least one child artifact is scanned
scannedArtifactCountSQL = `SELECT COUNT(1)
FROM artifact a
WHERE EXISTS (SELECT 1
FROM scan_report s
WHERE a.digest = s.digest
AND s.registration_uuid = ?)
-- exclude artifact accessory
AND NOT EXISTS (SELECT 1 FROM artifact_accessory acc WHERE acc.artifact_id = a.id)
-- exclude artifact without tag and part of the image index
AND EXISTS (SELECT 1
FROM tag
WHERE tag.artifact_id = id
OR (NOT EXISTS (SELECT 1 FROM artifact_reference ref WHERE ref.child_id = a.id)))
-- include image index which is scanned
OR EXISTS (SELECT 1
FROM scan_report s,
artifact_reference ref
WHERE s.digest = ref.child_digest
AND ref.parent_id = a.id AND s.registration_uuid = ? AND NOT EXISTS (SELECT 1
FROM scan_report s
WHERE s.digest = a.digest and s.registration_uuid = ?))`

// sql to query the dangerous CVEs
dangerousCVESQL = `select vr.*
Expand Down Expand Up @@ -143,6 +169,8 @@ type SecurityHubDao interface {
DangerousCVEs(ctx context.Context, scannerUUID string, projectID int64, query *q.Query) ([]*scan.VulnerabilityRecord, error)
// DangerousArtifacts returns top 5 dangerous artifact for the given scanner. return top 5 result
DangerousArtifacts(ctx context.Context, scannerUUID string, projectID int64, query *q.Query) ([]*model.DangerousArtifact, error)
// TotalArtifactsCount return the count of total artifacts.
TotalArtifactsCount(ctx context.Context, projectID int64) (int64, error)
// ScannedArtifactsCount return the count of scanned artifacts.
ScannedArtifactsCount(ctx context.Context, scannerUUID string, projectID int64, query *q.Query) (int64, error)
// ListVulnerabilities search vulnerability record by cveID
Expand All @@ -159,6 +187,19 @@ func New() SecurityHubDao {
type dao struct {
}

func (d *dao) TotalArtifactsCount(ctx context.Context, projectID int64) (int64, error) {
if projectID != 0 {
return 0, nil
}
o, err := orm.FromContext(ctx)
if err != nil {
return 0, err
}
var count int64
err = o.Raw(totalArtifactCountSQL).QueryRow(&count)
return count, err
}

func (d *dao) Summary(ctx context.Context, scannerUUID string, projectID int64, query *q.Query) (*model.Summary, error) {
if len(scannerUUID) == 0 || projectID != 0 {
return nil, nil
Expand Down Expand Up @@ -199,7 +240,7 @@ func (d *dao) ScannedArtifactsCount(ctx context.Context, scannerUUID string, pro
if err != nil {
return cnt, err
}
err = o.Raw(scannedArtifactCountSQL, scannerUUID).QueryRow(&cnt)
err = o.Raw(scannedArtifactCountSQL, scannerUUID, scannerUUID, scannerUUID).QueryRow(&cnt)
return cnt, err
}
func (d *dao) DangerousCVEs(ctx context.Context, scannerUUID string, projectID int64, query *q.Query) ([]*scan.VulnerabilityRecord, error) {
Expand Down
22 changes: 20 additions & 2 deletions src/pkg/securityhub/dao/security_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -44,9 +44,19 @@ func (suite *SecurityDaoTestSuite) SetupSuite() {
// SetupTest prepares env for test case.
func (suite *SecurityDaoTestSuite) SetupTest() {
testDao.ExecuteBatchSQL([]string{
`delete from tag`,
`delete from artifact_accessory`,
`delete from artifact`,
`insert into scan_report(uuid, digest, registration_uuid, mime_type, critical_cnt, high_cnt, medium_cnt, low_cnt, unknown_cnt, fixable_cnt) values('uuid', 'digest1001', 'ruuid', 'application/vnd.scanner.adapter.vuln.report.harbor+json; version=1.0', 50, 50, 50, 0, 0, 20)`,
`insert into artifact (project_id, repository_name, digest, type, pull_time, push_time, repository_id, media_type, manifest_media_type, size, extra_attrs, annotations, icon)
values (1, 'library/hello-world', 'digest1001', 'IMAGE', '2023-06-02 09:16:47.838778', '2023-06-02 01:45:55.050785', 1742, 'application/vnd.docker.container.image.v1+json', 'application/vnd.docker.distribution.manifest.v2+json', 4452, '{"architecture":"amd64","author":"","config":{"Env":["PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin"],"Cmd":["/hello"]},"created":"2023-05-04T17:37:03.872958712Z","os":"linux"}', null, '');`,
`insert into artifact (id, project_id, repository_name, digest, type, pull_time, push_time, repository_id, media_type, manifest_media_type, size, extra_attrs, annotations, icon)
values (1001, 1, 'library/hello-world', 'digest1001', 'IMAGE', '2023-06-02 09:16:47.838778', '2023-06-02 01:45:55.050785', 1742, 'application/vnd.docker.container.image.v1+json', 'application/vnd.docker.distribution.manifest.v2+json', 4452, '{"architecture":"amd64","author":"","config":{"Env":["PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin"],"Cmd":["/hello"]},"created":"2023-05-04T17:37:03.872958712Z","os":"linux"}', null, '');`,
`insert into artifact (id, project_id, repository_name, digest, type, pull_time, push_time, repository_id, media_type, manifest_media_type, size, extra_attrs, annotations, icon)
values (1002, 1, 'library/hello-world', 'digest1002', 'IMAGE', '2023-06-02 09:16:47.838778', '2023-06-02 01:45:55.050785', 1742, 'application/vnd.docker.container.image.v1+json', 'application/vnd.oci.image.config.v1+json', 4452, '{"architecture":"amd64","author":"","config":{"Env":["PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin"],"Cmd":["/hello"]},"created":"2023-05-04T17:37:03.872958712Z","os":"linux"}', null, '');`,
`insert into artifact (id, project_id, repository_name, digest, type, pull_time, push_time, repository_id, media_type, manifest_media_type, size, extra_attrs, annotations, icon)
values (1003, 1, 'library/hello-world', 'digest1003', 'IMAGE', '2023-06-02 09:16:47.838778', '2023-06-02 01:45:55.050785', 1742, 'application/vnd.docker.container.image.v1+json', 'application/vnd.oci.image.config.v1+json', 4452, '{"architecture":"amd64","author":"","config":{"Env":["PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin"],"Cmd":["/hello"]},"created":"2023-05-04T17:37:03.872958712Z","os":"linux"}', null, '');`,
`insert into tag (id, repository_id, artifact_id, name, push_time, pull_time) values (1001, 1742, 1001, 'latest', '2023-06-02 01:45:55.050785', '2023-06-02 09:16:47.838778')`,
`INSERT INTO artifact_accessory (id, artifact_id, subject_artifact_id, type, size, digest, creation_time, subject_artifact_digest, subject_artifact_repo) VALUES (1001, 1002, 1, 'signature.cosign', 2109, 'sha256:08c64c0de2667abcf3974b4b75b82903f294680b81584318adc4826d0dcb7a9c', '2023-08-03 04:54:32.102928', 'sha256:a97a153152fcd6410bdf4fb64f5622ecf97a753f07dcc89dab14509d059736cf', 'library/nuxeo')`,
`INSERT INTO artifact_reference (id, parent_id, child_id, child_digest, platform, urls, annotations) VALUES (1001, 1001, 1003, 'sha256:d2b2f2980e9ccc570e5726b56b54580f23a018b7b7314c9eaff7e5e479c78657', '{"architecture":"amd64","os":"linux"}', '', null)`,
`insert into scanner_registration (name, url, uuid, auth) values('trivy', 'https://www.vmware.com', 'ruuid', 'empty')`,
`insert into vulnerability_record (id, cve_id, registration_uuid, cvss_score_v3) values (1, '2023-4567-12345', 'ruuid', 9.8)`,
`insert into report_vulnerability_record (report_uuid, vuln_record_id) VALUES ('uuid', 1)`,
Expand All @@ -66,7 +76,10 @@ values (1, 'library/hello-world', 'digest1001', 'IMAGE', '2023-06-02 09:16:47.8
func (suite *SecurityDaoTestSuite) TearDownTest() {
testDao.ExecuteBatchSQL([]string{
`delete from scan_report where uuid = 'uuid'`,
`delete from tag where id = 1001`,
`delete from artifact where digest = 'digest1001'`,
`delete from artifact_accessory where id = 1001`,
`delete from artifact_reference where id = 1001`,
`delete from scanner_registration where uuid='ruuid'`,
`delete from scanner_registration where uuid='uuid2'`,
`delete from vulnerability_record where cve_id='2023-4567-12345'`,
Expand Down Expand Up @@ -178,6 +191,11 @@ func (suite *SecurityDaoTestSuite) TestRangeFilter() {
}
}

func (suite *SecurityDaoTestSuite) TestCountArtifact() {
count, err := suite.dao.TotalArtifactsCount(suite.Context(), 0)
suite.NoError(err)
suite.Equal(int64(1), count)
}
func (suite *SecurityDaoTestSuite) TestCountVul() {
count, err := suite.dao.CountVulnerabilities(suite.Context(), "ruuid", 0, true, nil)
suite.NoError(err)
Expand Down
6 changes: 6 additions & 0 deletions src/pkg/securityhub/manager.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,8 @@ type Manager interface {
Summary(ctx context.Context, scannerUUID string, projectID int64, query *q.Query) (*model.Summary, error)
// DangerousArtifacts returns the most dangerous artifact for the given scanner.
DangerousArtifacts(ctx context.Context, scannerUUID string, projectID int64, query *q.Query) ([]*model.DangerousArtifact, error)
// TotalArtifactsCount return the count of artifacts.
TotalArtifactsCount(ctx context.Context, projectID int64) (int64, error)
// ScannedArtifactsCount return the count of scanned artifacts.
ScannedArtifactsCount(ctx context.Context, scannerUUID string, projectID int64, query *q.Query) (int64, error)
// DangerousCVEs returns the most dangerous CVEs for the given scanner.
Expand All @@ -56,6 +58,10 @@ type securityManager struct {
dao dao.SecurityHubDao
}

func (s *securityManager) TotalArtifactsCount(ctx context.Context, projectID int64) (int64, error) {
return s.dao.TotalArtifactsCount(ctx, projectID)
}

func (s *securityManager) Summary(ctx context.Context, scannerUUID string, projectID int64, query *q.Query) (*model.Summary, error) {
return s.dao.Summary(ctx, scannerUUID, projectID, query)
}
Expand Down
24 changes: 24 additions & 0 deletions src/testing/pkg/securityhub/manager.go
View file

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

0 comments on commit 3de778e

Please sign in to comment.