Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Skip scan in-toto sbom artifact #20415

Merged
merged 2 commits into from
May 13, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 24 additions & 1 deletion src/controller/artifact/controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,10 @@

var (
// Ctl is a global artifact controller instance
Ctl = NewController()
Ctl = NewController()
skippedContentTypes = map[string]struct{}{
"application/vnd.in-toto+json": {},
}
)

var (
Expand Down Expand Up @@ -113,6 +116,8 @@
RemoveLabel(ctx context.Context, artifactID int64, labelID int64) (err error)
// Walk walks the artifact tree rooted at root, calling walkFn for each artifact in the tree, including root.
Walk(ctx context.Context, root *Artifact, walkFn func(*Artifact) error, option *Option) error
// HasUnscannableLayer check artifact with digest if has unscannable layer
HasUnscannableLayer(ctx context.Context, dgst string) (bool, error)
}

// NewController creates an instance of the default artifact controller
Expand Down Expand Up @@ -759,3 +764,21 @@
}
art.Accessories = accs
}

// HasUnscannableLayer check if it is a in-toto sbom, if it contains any blob with a content_type is application/vnd.in-toto+json, then consider as in-toto sbom
func (c *controller) HasUnscannableLayer(ctx context.Context, dgst string) (bool, error) {
if len(dgst) == 0 {
return false, nil
}

Check warning on line 772 in src/controller/artifact/controller.go

View check run for this annotation

Codecov / codecov/patch

src/controller/artifact/controller.go#L771-L772

Added lines #L771 - L772 were not covered by tests
blobs, err := c.blobMgr.GetByArt(ctx, dgst)
if err != nil {
return false, err
}

Check warning on line 776 in src/controller/artifact/controller.go

View check run for this annotation

Codecov / codecov/patch

src/controller/artifact/controller.go#L775-L776

Added lines #L775 - L776 were not covered by tests
for _, b := range blobs {
if _, exist := skippedContentTypes[b.ContentType]; exist {
log.Debugf("the artifact with digest %v is unscannable, because it contains content type: %v", dgst, b.ContentType)
return true, nil
}
}
return false, nil
}
24 changes: 24 additions & 0 deletions src/controller/artifact/controller_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ import (
accessorymodel "github.com/goharbor/harbor/src/pkg/accessory/model"
basemodel "github.com/goharbor/harbor/src/pkg/accessory/model/base"
"github.com/goharbor/harbor/src/pkg/artifact"
"github.com/goharbor/harbor/src/pkg/blob/models"
"github.com/goharbor/harbor/src/pkg/label/model"
repomodel "github.com/goharbor/harbor/src/pkg/repository/model"
model_tag "github.com/goharbor/harbor/src/pkg/tag/model/tag"
Expand Down Expand Up @@ -678,6 +679,29 @@ func (c *controllerTestSuite) TestWalk() {
}
}

func (c *controllerTestSuite) TestIsIntoto() {
blobs := []*models.Blob{
{Digest: "sha256:00000", ContentType: "application/vnd.oci.image.manifest.v1+json"},
{Digest: "sha256:22222", ContentType: "application/vnd.oci.image.config.v1+json"},
{Digest: "sha256:11111", ContentType: "application/vnd.in-toto+json"},
}
c.blobMgr.On("GetByArt", mock.Anything, mock.Anything).Return(blobs, nil).Once()
isIntoto, err := c.ctl.HasUnscannableLayer(context.Background(), "sha256: 77777")
c.Nil(err)
c.True(isIntoto)

blobs2 := []*models.Blob{
{Digest: "sha256:00000", ContentType: "application/vnd.oci.image.manifest.v1+json"},
{Digest: "sha256:22222", ContentType: "application/vnd.oci.image.config.v1+json"},
{Digest: "sha256:11111", ContentType: "application/vnd.oci.image.layer.v1.tar+gzip"},
}

c.blobMgr.On("GetByArt", mock.Anything, mock.Anything).Return(blobs2, nil).Once()
isIntoto2, err := c.ctl.HasUnscannableLayer(context.Background(), "sha256: 8888")
c.Nil(err)
c.False(isIntoto2)
}

func TestControllerTestSuite(t *testing.T) {
suite.Run(t, &controllerTestSuite{})
}
15 changes: 12 additions & 3 deletions src/controller/scan/base_controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,6 @@
sbomModel "github.com/goharbor/harbor/src/pkg/scan/sbom/model"
"github.com/goharbor/harbor/src/pkg/scan/vuln"
"github.com/goharbor/harbor/src/pkg/task"
"github.com/goharbor/harbor/src/testing/controller/artifact"
)

var (
Expand Down Expand Up @@ -111,8 +110,6 @@
rc robot.Controller
// Tag controller
tagCtl tag.Controller
// Artifact controller
artCtl artifact.Controller
// UUID generator
uuid uuidGenerator
// Configuration getter func
Expand Down Expand Up @@ -199,6 +196,18 @@
return nil
}

// because there are lots of in-toto sbom artifacts in dockerhub and replicated to Harbor, they are considered as image type
// when scanning these type of sbom artifact, the scanner might assume it is image layer with tgz format, and if scanner read the layer with a stream of tgz,
// it fail and close the stream abruptly and cause the pannic in the harbor core log
// to avoid pannic, skip scan the in-toto sbom artifact sbom artifact
stonezdj marked this conversation as resolved.
Show resolved Hide resolved
unscannable, err := bc.ar.HasUnscannableLayer(ctx, a.Digest)
if err != nil {
return err
}

Check warning on line 206 in src/controller/scan/base_controller.go

View check run for this annotation

Codecov / codecov/patch

src/controller/scan/base_controller.go#L205-L206

Added lines #L205 - L206 were not covered by tests
if unscannable {
return nil
}

Check warning on line 209 in src/controller/scan/base_controller.go

View check run for this annotation

Codecov / codecov/patch

src/controller/scan/base_controller.go#L208-L209

Added lines #L208 - L209 were not covered by tests

supported := hasCapability(r, a)

if !supported && a.IsImageIndex() {
Expand Down
8 changes: 6 additions & 2 deletions src/controller/scan/base_controller_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -350,6 +350,7 @@ func (suite *ControllerTestSuite) TestScanControllerScan() {
{
// artifact not provieded
suite.Require().Error(suite.c.Scan(context.TODO(), nil))
mock.OnAnything(suite.ar, "HasUnscannableLayer").Return(false, nil).Times(3)
}

{
Expand Down Expand Up @@ -448,6 +449,7 @@ func (suite *ControllerTestSuite) TestScanControllerStop() {

// TestScanControllerGetReport ...
func (suite *ControllerTestSuite) TestScanControllerGetReport() {
mock.OnAnything(suite.ar, "HasUnscannableLayer").Return(false, nil).Once()
ctx := orm.NewContext(nil, &ormtesting.FakeOrmer{})
mock.OnAnything(suite.ar, "Walk").Return(nil).Run(func(args mock.Arguments) {
walkFn := args.Get(2).(func(*artifact.Artifact) error)
Expand All @@ -466,20 +468,21 @@ func (suite *ControllerTestSuite) TestScanControllerGetReport() {
// TestScanControllerGetSummary ...
func (suite *ControllerTestSuite) TestScanControllerGetSummary() {
ctx := orm.NewContext(nil, &ormtesting.FakeOrmer{})
mock.OnAnything(suite.ar, "HasUnscannableLayer").Return(false, nil).Once()
mock.OnAnything(suite.accessoryMgr, "List").Return([]accessoryModel.Accessory{}, nil).Once()
mock.OnAnything(suite.ar, "Walk").Return(nil).Run(func(args mock.Arguments) {
walkFn := args.Get(2).(func(*artifact.Artifact) error)
walkFn(suite.artifact)
}).Once()
mock.OnAnything(suite.taskMgr, "ListScanTasksByReportUUID").Return(nil, nil).Once()

sum, err := suite.c.GetSummary(ctx, suite.artifact, []string{v1.MimeTypeNativeReport})
require.NoError(suite.T(), err)
assert.Equal(suite.T(), 1, len(sum))
}

// TestScanControllerGetScanLog ...
func (suite *ControllerTestSuite) TestScanControllerGetScanLog() {
mock.OnAnything(suite.ar, "HasUnscannableLayer").Return(false, nil).Once()
ctx := orm.NewContext(nil, &ormtesting.FakeOrmer{})
mock.OnAnything(suite.taskMgr, "ListScanTasksByReportUUID").Return([]*task.Task{
{
Expand All @@ -500,6 +503,7 @@ func (suite *ControllerTestSuite) TestScanControllerGetScanLog() {

func (suite *ControllerTestSuite) TestScanControllerGetMultiScanLog() {
ctx := orm.NewContext(nil, &ormtesting.FakeOrmer{})
mock.OnAnything(suite.ar, "HasUnscannableLayer").Return(false, nil).Times(4)
suite.taskMgr.On("ListScanTasksByReportUUID", ctx, "rp-uuid-001").Return([]*task.Task{
{
ID: 1,
Expand Down Expand Up @@ -562,7 +566,7 @@ func (suite *ControllerTestSuite) TestScanAll() {
{
// no artifacts found when scan all
executionID := int64(1)

mock.OnAnything(suite.ar, "HasUnscannableLayer").Return(false, nil).Once()
suite.execMgr.On(
"Create", mock.Anything, "SCAN_ALL", int64(0), "SCHEDULE",
mock.Anything).Return(executionID, nil).Once()
Expand Down
12 changes: 12 additions & 0 deletions src/controller/scan/checker.go
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,18 @@
return artifact.ErrBreak
}

// because there are lots of in-toto sbom artifacts in dockerhub and replicated to Harbor, they are considered as image type
// when scanning these type of sbom artifact, the scanner might assume it is image layer with tgz format, and if scanner read the layer with a stream of tgz,
// it fail and close the stream abruptly and cause the pannic in the harbor core log
// to avoid pannic, skip scan the in-toto sbom artifact sbom artifact
unscannable, err := c.artifactCtl.HasUnscannableLayer(ctx, a.Digest)
if err != nil {
return err
}

Check warning on line 96 in src/controller/scan/checker.go

View check run for this annotation

Codecov / codecov/patch

src/controller/scan/checker.go#L95-L96

Added lines #L95 - L96 were not covered by tests
if unscannable {
return nil
}

Check warning on line 99 in src/controller/scan/checker.go

View check run for this annotation

Codecov / codecov/patch

src/controller/scan/checker.go#L98-L99

Added lines #L98 - L99 were not covered by tests

return nil
}

Expand Down
3 changes: 2 additions & 1 deletion src/controller/scan/checker_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ func (suite *CheckerTestSuite) TestIsScannable() {
walkFn := args.Get(2).(func(*artifact.Artifact) error)
walkFn(art)
})

mock.OnAnything(c.artifactCtl, "HasUnscannableLayer").Return(false, nil).Once()
isScannable, err := c.IsScannable(context.TODO(), art)
suite.Nil(err)
suite.False(isScannable)
Expand All @@ -97,6 +97,7 @@ func (suite *CheckerTestSuite) TestIsScannable() {
walkFn := args.Get(2).(func(*artifact.Artifact) error)
walkFn(art)
})
mock.OnAnything(c.artifactCtl, "HasUnscannableLayer").Return(false, nil).Once()

isScannable, err := c.IsScannable(context.TODO(), art)
suite.Nil(err)
Expand Down
28 changes: 28 additions & 0 deletions src/testing/controller/artifact/controller.go

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

Loading