fix(cve): cummulative fixes and improvements for CVE scanning logic (#…

…1810)

1. Only scan CVEs for images returned by graphql calls
Since pagination was refactored to account for image indexes, we had started
to run the CVE scanner before pagination was applied, resulting in
decreased ZOT performance if CVE information was requested

2. Increase in medory-cache of cve results to 1m, from 10k digests.

3. Update CVE model to use CVSS severity values in our code.
Previously we relied upon the strings returned by trivy directly,
and the sorting they implemented.
Since CVE severities are standardized, we don't need to pass around
an adapter object just for pagination and sorting purposes anymore.
This also improves our testing since we don't mock the sorting functions anymore.

4. Fix a flaky CLI test not waiting for the zot service to start.

5. Add the search build label on search/cve tests which were missing it.

6. The boltdb update method was used in a few places where view was supposed to be called.

7. Add logs for start and finish of parsing MetaDB.

8. Avoid unmarshalling twice to obtain annotations for multiarch images.

Signed-off-by: Andrei Aaron <[email protected]>

pkg/cli/client/cve_cmd_internal_test.go

@@ -59,7 +59,7 @@ func TestSearchCVECmd(t *testing.T) {
 	ctlr := api.NewController(conf)
 	cm := test.NewControllerManager(ctlr)
 
-	cm.StartServer()
+	cm.StartAndWait(port)
 	defer cm.StopServer()
 
 	Convey("Test CVE help", t, func() {
@@ -947,21 +947,10 @@ func TestCVESort(t *testing.T) {
 		panic(err)
 	}
 
-	severities := map[string]int{
-		"UNKNOWN":  0,
-		"LOW":      1,
-		"MEDIUM":   2,
-		"HIGH":     3,
-		"CRITICAL": 4,
-	}
-
 	ctlr.CveInfo = cveinfo.BaseCveInfo{
 		Log:    ctlr.Log,
 		MetaDB: mocks.MetaDBMock{},
 		Scanner: mocks.CveScannerMock{
-			CompareSeveritiesFn: func(severity1, severity2 string) int {
-				return severities[severity2] - severities[severity1]
-			},
 			ScanImageFn: func(image string) (map[string]cvemodel.CVE, error) {
 				return map[string]cvemodel.CVE{
 					"CVE-2023-1255": {
@@ -1385,15 +1374,7 @@ func TestCVECommandErrors(t *testing.T) {
 }
 
 func getMockCveInfo(metaDB mTypes.MetaDB, log log.Logger) cveinfo.CveInfo {
-	// MetaDB loaded with initial data, mock the scanner
-	severities := map[string]int{
-		"UNKNOWN":  0,
-		"LOW":      1,
-		"MEDIUM":   2,
-		"HIGH":     3,
-		"CRITICAL": 4,
-	}
-
+	// MetaDB loaded with initial data now mock the scanner
 	// Setup test CVE data in mock scanner
 	scanner := mocks.CveScannerMock{
 		ScanImageFn: func(image string) (map[string]cvemodel.CVE, error) {
@@ -1436,9 +1417,6 @@ func getMockCveInfo(metaDB mTypes.MetaDB, log log.Logger) cveinfo.CveInfo {
 			// By default the image has no vulnerabilities
 			return map[string]cvemodel.CVE{}, nil
 		},
-		CompareSeveritiesFn: func(severity1, severity2 string) int {
-			return severities[severity2] - severities[severity1]
-		},
 		IsImageFormatScannableFn: func(repo string, reference string) (bool, error) {
 			// Almost same logic compared to actual Trivy specific implementation
 			imageDir := repo

pkg/extensions/extension_search_test.go

@@ -70,7 +70,7 @@ func TestTrivyDBGenerator(t *testing.T) {
 
 		// Wait for trivy db to download
 		found, err := ReadLogFileAndCountStringOccurence(logPath,
-			"DB update completed, next update scheduled", 120*time.Second, 2)
+			"DB update completed, next update scheduled", 140*time.Second, 2)
 		So(err, ShouldBeNil)
 		So(found, ShouldBeTrue)
 	})

pkg/extensions/search/convert/annotations.go

@@ -134,9 +134,10 @@ func GetAnnotations(annotations, labels map[string]string) ImageAnnotations {
 	}
 }
 
-func GetIndexAnnotations(indexAnnotations, manifestAnnotations, manifestLabels map[string]string) ImageAnnotations {
-	annotationsFromManifest := GetAnnotations(manifestAnnotations, manifestLabels)
-
+func GetIndexAnnotations(
+	indexAnnotations map[string]string,
+	annotationsFromManifest *ImageAnnotations,
+) ImageAnnotations {
 	description := GetDescription(indexAnnotations)
 	if description == "" {
 		description = annotationsFromManifest.Description

pkg/extensions/search/convert/convert_internal_test.go

@@ -0,0 +1,302 @@
+//go:build search
+
+package convert
+
+import (
+	"context"
+	"encoding/json"
+	"errors"
+	"testing"
+
+	"github.com/99designs/gqlgen/graphql"
+	godigest "github.com/opencontainers/go-digest"
+	ispec "github.com/opencontainers/image-spec/specs-go/v1"
+	. "github.com/smartystreets/goconvey/convey"
+
+	cvemodel "zotregistry.io/zot/pkg/extensions/search/cve/model"
+	"zotregistry.io/zot/pkg/extensions/search/gql_generated"
+	"zotregistry.io/zot/pkg/log"
+	"zotregistry.io/zot/pkg/meta/boltdb"
+	mTypes "zotregistry.io/zot/pkg/meta/types"
+	"zotregistry.io/zot/pkg/test/mocks"
+)
+
+var ErrTestError = errors.New("TestError")
+
+func TestCVEConvert(t *testing.T) {
+	Convey("Test adding CVE information to Summary objects", t, func() {
+		params := boltdb.DBParameters{
+			RootDir: t.TempDir(),
+		}
+		boltDB, err := boltdb.GetBoltDriver(params)
+		So(err, ShouldBeNil)
+
+		metaDB, err := boltdb.New(boltDB, log.NewLogger("debug", ""))
+		So(err, ShouldBeNil)
+
+		configBlob, err := json.Marshal(ispec.Image{})
+		So(err, ShouldBeNil)
+
+		manifestBlob, err := json.Marshal(ispec.Manifest{
+			Layers: []ispec.Descriptor{
+				{
+					MediaType: ispec.MediaTypeImageLayerGzip,
+					Size:      0,
+					Digest:    godigest.NewDigestFromEncoded(godigest.SHA256, "digest"),
+				},
+			},
+		})
+		So(err, ShouldBeNil)
+
+		repoMeta11 := mTypes.ManifestMetadata{
+			ManifestBlob: manifestBlob,
+			ConfigBlob:   configBlob,
+		}
+
+		digest11 := godigest.FromString("abc1")
+		err = metaDB.SetManifestMeta("repo1", digest11, repoMeta11)
+		So(err, ShouldBeNil)
+		err = metaDB.SetRepoReference("repo1", "0.1.0", digest11, ispec.MediaTypeImageManifest)
+		So(err, ShouldBeNil)
+
+		reposMeta, manifestMetaMap, _, err := metaDB.SearchRepos(context.Background(), "")
+		So(err, ShouldBeNil)
+
+		ctx := graphql.WithResponseContext(context.Background(),
+			graphql.DefaultErrorPresenter, graphql.DefaultRecover)
+
+		Convey("Add CVE Summary to ImageSummary", func() {
+			var imageSummary *gql_generated.ImageSummary
+
+			So(imageSummary, ShouldBeNil)
+
+			updateImageSummaryVulnerabilities(ctx,
+				imageSummary,
+				SkipQGLField{
+					Vulnerabilities: false,
+				},
+				mocks.CveInfoMock{
+					GetCVESummaryForImageMediaFn: func(repo string, digest, mediaType string,
+					) (cvemodel.ImageCVESummary, error) {
+						return cvemodel.ImageCVESummary{}, ErrTestError
+					},
+				},
+			)
+
+			So(imageSummary, ShouldBeNil)
+			So(graphql.GetErrors(ctx), ShouldBeNil)
+
+			imageSummary, _, err = ImageManifest2ImageSummary(ctx, "repo1", "0.1.0", digest11, reposMeta[0],
+				manifestMetaMap[digest11.String()])
+			So(err, ShouldBeNil)
+
+			So(imageSummary, ShouldNotBeNil)
+			So(imageSummary.Vulnerabilities, ShouldBeNil)
+
+			updateImageSummaryVulnerabilities(ctx,
+				imageSummary,
+				SkipQGLField{
+					Vulnerabilities: true,
+				},
+				mocks.CveInfoMock{},
+			)
+
+			So(imageSummary.Vulnerabilities, ShouldNotBeNil)
+			So(*imageSummary.Vulnerabilities.Count, ShouldEqual, 0)
+			So(*imageSummary.Vulnerabilities.MaxSeverity, ShouldEqual, "")
+			So(graphql.GetErrors(ctx), ShouldBeNil)
+
+			imageSummary.Vulnerabilities = nil
+
+			updateImageSummaryVulnerabilities(ctx,
+				imageSummary,
+				SkipQGLField{
+					Vulnerabilities: false,
+				},
+				mocks.CveInfoMock{
+					GetCVESummaryForImageMediaFn: func(repo string, digest, mediaType string,
+					) (cvemodel.ImageCVESummary, error) {
+						return cvemodel.ImageCVESummary{
+							Count:       1,
+							MaxSeverity: "HIGH",
+						}, nil
+					},
+				},
+			)
+
+			So(imageSummary.Vulnerabilities, ShouldNotBeNil)
+			So(*imageSummary.Vulnerabilities.Count, ShouldEqual, 1)
+			So(*imageSummary.Vulnerabilities.MaxSeverity, ShouldEqual, "HIGH")
+			So(graphql.GetErrors(ctx), ShouldBeNil)
+			So(len(imageSummary.Manifests), ShouldEqual, 1)
+			So(imageSummary.Manifests[0].Vulnerabilities, ShouldNotBeNil)
+			So(*imageSummary.Manifests[0].Vulnerabilities.Count, ShouldEqual, 1)
+			So(*imageSummary.Manifests[0].Vulnerabilities.MaxSeverity, ShouldEqual, "HIGH")
+
+			imageSummary.Vulnerabilities = nil
+
+			updateImageSummaryVulnerabilities(ctx,
+				imageSummary,
+				SkipQGLField{
+					Vulnerabilities: false,
+				},
+				mocks.CveInfoMock{
+					GetCVESummaryForImageMediaFn: func(repo string, digest, mediaType string,
+					) (cvemodel.ImageCVESummary, error) {
+						return cvemodel.ImageCVESummary{}, ErrTestError
+					},
+				},
+			)
+
+			So(imageSummary.Vulnerabilities, ShouldNotBeNil)
+			So(*imageSummary.Vulnerabilities.Count, ShouldEqual, 0)
+			So(*imageSummary.Vulnerabilities.MaxSeverity, ShouldEqual, "")
+			So(graphql.GetErrors(ctx).Error(), ShouldContainSubstring, "unable to run vulnerability scan on tag")
+		})
+
+		Convey("Add CVE Summary to RepoSummary", func() {
+			var repoSummary *gql_generated.RepoSummary
+			So(repoSummary, ShouldBeNil)
+
+			updateRepoSummaryVulnerabilities(ctx,
+				repoSummary,
+				SkipQGLField{
+					Vulnerabilities: false,
+				},
+				mocks.CveInfoMock{
+					GetCVESummaryForImageMediaFn: func(repo string, digest, mediaType string,
+					) (cvemodel.ImageCVESummary, error) {
+						return cvemodel.ImageCVESummary{
+							Count:       1,
+							MaxSeverity: "HIGH",
+						}, nil
+					},
+				},
+			)
+
+			So(repoSummary, ShouldBeNil)
+			So(graphql.GetErrors(ctx), ShouldBeNil)
+
+			imageSummary, _, err := ImageManifest2ImageSummary(ctx, "repo1", "0.1.0", digest11, reposMeta[0],
+				manifestMetaMap[digest11.String()])
+			So(err, ShouldBeNil)
+
+			So(imageSummary, ShouldNotBeNil)
+
+			repoSummary = &gql_generated.RepoSummary{}
+			repoSummary.NewestImage = imageSummary
+
+			So(repoSummary.NewestImage.Vulnerabilities, ShouldBeNil)
+
+			updateImageSummaryVulnerabilities(ctx,
+				imageSummary,
+				SkipQGLField{
+					Vulnerabilities: false,
+				},
+				mocks.CveInfoMock{
+					GetCVESummaryForImageMediaFn: func(repo string, digest, mediaType string,
+					) (cvemodel.ImageCVESummary, error) {
+						return cvemodel.ImageCVESummary{
+							Count:       1,
+							MaxSeverity: "HIGH",
+						}, nil
+					},
+				},
+			)
+
+			So(repoSummary.NewestImage.Vulnerabilities, ShouldNotBeNil)
+			So(*repoSummary.NewestImage.Vulnerabilities.Count, ShouldEqual, 1)
+			So(*repoSummary.NewestImage.Vulnerabilities.MaxSeverity, ShouldEqual, "HIGH")
+			So(graphql.GetErrors(ctx), ShouldBeNil)
+		})
+
+		Convey("Add CVE Summary to ManifestSummary", func() {
+			var manifestSummary *gql_generated.ManifestSummary
+
+			So(manifestSummary, ShouldBeNil)
+
+			updateManifestSummaryVulnerabilities(ctx,
+				manifestSummary,
+				"repo1",
+				SkipQGLField{
+					Vulnerabilities: false,
+				},
+				mocks.CveInfoMock{
+					GetCVESummaryForImageMediaFn: func(repo string, digest, mediaType string,
+					) (cvemodel.ImageCVESummary, error) {
+						return cvemodel.ImageCVESummary{
+							Count:       1,
+							MaxSeverity: "HIGH",
+						}, nil
+					},
+				},
+			)
+
+			So(manifestSummary, ShouldBeNil)
+			So(graphql.GetErrors(ctx), ShouldBeNil)
+
+			imageSummary, _, err := ImageManifest2ImageSummary(ctx, "repo1", "0.1.0", digest11, reposMeta[0],
+				manifestMetaMap[digest11.String()])
+			So(err, ShouldBeNil)
+			manifestSummary = imageSummary.Manifests[0]
+
+			updateManifestSummaryVulnerabilities(ctx,
+				manifestSummary,
+				"repo1",
+				SkipQGLField{
+					Vulnerabilities: true,
+				},
+				mocks.CveInfoMock{},
+			)
+
+			So(manifestSummary, ShouldNotBeNil)
+			So(manifestSummary.Vulnerabilities, ShouldNotBeNil)
+			So(*manifestSummary.Vulnerabilities.Count, ShouldEqual, 0)
+			So(*manifestSummary.Vulnerabilities.MaxSeverity, ShouldEqual, "")
+
+			manifestSummary.Vulnerabilities = nil
+
+			updateManifestSummaryVulnerabilities(ctx,
+				manifestSummary,
+				"repo1",
+				SkipQGLField{
+					Vulnerabilities: false,
+				},
+				mocks.CveInfoMock{
+					GetCVESummaryForImageMediaFn: func(repo string, digest, mediaType string,
+					) (cvemodel.ImageCVESummary, error) {
+						return cvemodel.ImageCVESummary{
+							Count:       1,
+							MaxSeverity: "HIGH",
+						}, nil
+					},
+				},
+			)
+
+			So(manifestSummary.Vulnerabilities, ShouldNotBeNil)
+			So(*manifestSummary.Vulnerabilities.Count, ShouldEqual, 1)
+			So(*manifestSummary.Vulnerabilities.MaxSeverity, ShouldEqual, "HIGH")
+
+			manifestSummary.Vulnerabilities = nil
+
+			updateManifestSummaryVulnerabilities(ctx,
+				manifestSummary,
+				"repo1",
+				SkipQGLField{
+					Vulnerabilities: false,
+				},
+				mocks.CveInfoMock{
+					GetCVESummaryForImageMediaFn: func(repo string, digest, mediaType string,
+					) (cvemodel.ImageCVESummary, error) {
+						return cvemodel.ImageCVESummary{}, ErrTestError
+					},
+				},
+			)
+
+			So(manifestSummary.Vulnerabilities, ShouldNotBeNil)
+			So(*manifestSummary.Vulnerabilities.Count, ShouldEqual, 0)
+			So(*manifestSummary.Vulnerabilities.MaxSeverity, ShouldEqual, "")
+			So(graphql.GetErrors(ctx).Error(), ShouldContainSubstring, "unable to run vulnerability scan in repo")
+		})
+	})
+}