Skip to content

Commit

Permalink
Merge branch 'main' into fix-19928
Browse files Browse the repository at this point in the history
  • Loading branch information
wy65701436 authored May 13, 2024
2 parents adf6694 + 232f9ba commit 63eef01
Show file tree
Hide file tree
Showing 12 changed files with 123 additions and 15 deletions.
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 @@ import (

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 @@ type Controller interface {
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 @@ func (c *controller) populateAccessories(ctx context.Context, art *Artifact) {
}
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
}
blobs, err := c.blobMgr.GetByArt(ctx, dgst)
if err != nil {
return false, err
}
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 @@ import (
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 @@ type basicController struct {
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 @@ func (bc *basicController) collectScanningArtifacts(ctx context.Context, r *scan
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
unscannable, err := bc.ar.HasUnscannableLayer(ctx, a.Digest)
if err != nil {
return err
}
if unscannable {
return nil
}

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 @@ func (c *checker) IsScannable(ctx context.Context, art *artifact.Artifact) (bool
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
}
if unscannable {
return nil
}

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
1 change: 1 addition & 0 deletions src/pkg/scan/job.go
Original file line number Diff line number Diff line change
Expand Up @@ -419,6 +419,7 @@ func removeScanAuthInfo(sr *v1.ScanRequest) string {
URL: sr.Registry.URL,
Authorization: "[HIDDEN]",
},
RequestType: sr.RequestType,
}

str, err := req.ToJSON()
Expand Down
2 changes: 2 additions & 0 deletions src/pkg/scan/rest/v1/models.go
Original file line number Diff line number Diff line change
Expand Up @@ -206,6 +206,8 @@ type Registry struct {
// An optional value of the HTTP Authorization header sent with each request to the Docker Registry for getting or exchanging token.
// For example, `Basic: Base64(username:password)`.
Authorization string `json:"authorization"`
// Insecure is an indicator of https or http.
Insecure bool `json:"insecure"`
}

// ScanRequest represents a structure that is sent to a Scanner Adapter to initiate artifact scanning.
Expand Down
13 changes: 7 additions & 6 deletions src/pkg/scan/sbom/sbom.go
Original file line number Diff line number Diff line change
Expand Up @@ -43,13 +43,13 @@ const (
)

func init() {
scan.RegisterScanHanlder(v1.ScanTypeSbom, &scanHandler{GenAccessoryFunc: scan.GenAccessoryArt, RegistryServer: registryFQDN})
scan.RegisterScanHanlder(v1.ScanTypeSbom, &scanHandler{GenAccessoryFunc: scan.GenAccessoryArt, RegistryServer: registry})
}

// ScanHandler defines the Handler to generate sbom
type scanHandler struct {
GenAccessoryFunc func(scanRep v1.ScanRequest, sbomContent []byte, labels map[string]string, mediaType string, robot *model.Robot) (string, error)
RegistryServer func(ctx context.Context) string
RegistryServer func(ctx context.Context) (string, bool)
}

// RequestProducesMineTypes defines the mine types produced by the scan handler
Expand Down Expand Up @@ -96,7 +96,7 @@ func (v *scanHandler) PostScan(ctx job.Context, sr *v1.ScanRequest, _ *scanModel
Artifact: sr.Artifact,
}
// the registry server url is core by default, need to replace it with real registry server url
scanReq.Registry.URL = v.RegistryServer(ctx.SystemContext())
scanReq.Registry.URL, scanReq.Registry.Insecure = v.RegistryServer(ctx.SystemContext())
if len(scanReq.Registry.URL) == 0 {
return "", fmt.Errorf("empty registry server")
}
Expand Down Expand Up @@ -139,15 +139,16 @@ func (v *scanHandler) generateReport(startTime time.Time, repository, digest, st
}

// extract server name from config, and remove the protocol prefix
func registryFQDN(ctx context.Context) string {
func registry(ctx context.Context) (string, bool) {
cfgMgr, ok := config.FromContext(ctx)
if ok {
extURL := cfgMgr.Get(context.Background(), common.ExtEndpoint).GetString()
insecure := strings.HasPrefix(extURL, "http://")
server := strings.TrimPrefix(extURL, "https://")
server = strings.TrimPrefix(server, "http://")
return server
return server, insecure
}
return ""
return "", false
}

// retrieveSBOMContent retrieves the "sbom" field from the raw report
Expand Down
4 changes: 2 additions & 2 deletions src/pkg/scan/sbom/sbom_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -89,8 +89,8 @@ func Test_scanHandler_RequestProducesMineTypes(t *testing.T) {
}
}

func mockGetRegistry(ctx context.Context) string {
return "myharbor.example.com"
func mockGetRegistry(ctx context.Context) (string, bool) {
return "myharbor.example.com", false
}

func mockGenAccessory(scanRep v1.ScanRequest, sbomContent []byte, labels map[string]string, mediaType string, robot *model.Robot) (string, error) {
Expand Down
3 changes: 3 additions & 0 deletions src/pkg/scan/util.go
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,9 @@ func GenAccessoryArt(sq v1sq.ScanRequest, accData []byte, accAnnotations map[str
return "", err
}
accRef, err := name.ParseReference(fmt.Sprintf("%s/%s@%s", sq.Registry.URL, sq.Artifact.Repository, dgst.String()))
if sq.Registry.Insecure {
accRef, err = name.ParseReference(fmt.Sprintf("%s/%s@%s", sq.Registry.URL, sq.Artifact.Repository, dgst.String()), name.Insecure)
}
if err != nil {
return "", 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.

0 comments on commit 63eef01

Please sign in to comment.