Vulnerability Scanning Implementation for container images #1489

karanibm6 · 2024-02-07T17:34:47Z

Changes

Implementation of this SHIP : https://github.com/shipwright-io/community/blob/main/ships/0033-build-output-vulnerability-scanning.md#build-output-vulnerability-scanning

Add vulnerability scanning options in build and buildrun types in v1alpha1 and v1beta1
Add vulnerable image for unit testing of vulnerability scanning feature
Implement vulnerability scanning for container images using trivy and lists vulnerabilities in buildrun output
Add e2e tests to verify options for vulnerability scanning

Fixes #1394

Submitter Checklist

Includes tests if functionality changed/was added
Includes docs if changes are user-facing
Set a kind label on this PR
Release notes block has been filled in, or marked NONE

See the contributor guide
for details on coding conventions, github and prow interactions, and the code review process.

Release Notes

Vulnerability Scanning Implementation

openshift-ci · 2024-02-07T17:34:59Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign adambkaplan for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

karanibm6 · 2024-02-07T17:39:27Z

/kind feature

cmd/image-processing/main_test.go

hack/install-trivy.sh

pkg/apis/build/v1alpha1/build_types.go

cmd/image-processing/main.go

cmd/image-processing/main_test.go

HeavyWombat · 2024-02-09T09:17:21Z

pkg/image/vulnerability_scan.go

+)
+
+const (
+	VulnerabilityCountLimit = 50 // Number of vulnerabilities to be added to buildrun output.


Nit: Would be nice to have it configurable for downstream projects to adapt to their needs.

@HeavyWombat Shall I add it as another field for vulnerabilityScanOptions ? or Can I set it as an environment variable in some config or in the image-processing container ?

My initial feeling would be an environment variable.

Should be in config.go which reads from environment variables.

pkg/image/vulnerability_scan.go

pkg/image/vulnerability_scan_test.go

HeavyWombat

Again, thanks for the PR. I am generally speaking fine with it, but have a couple of detail questions.

pkg/apis/build/v1alpha1/build_types.go

HeavyWombat · 2024-02-20T09:35:11Z

pkg/image/vulnerability_scan.go

+)
+
+const (
+	VulnerabilityCountLimit = 50 // Number of vulnerabilities to be added to buildrun output.


My initial feeling would be an environment variable.

pkg/image/vulnerability_scan.go

test/data/images/vuln-single-image/vuln-image.tar

test/e2e/v1alpha1/e2e_vulnerability_scanning_test.go

cmd/image-processing/main.go

pkg/reconciler/buildrun/resources/image_processing.go

SaschaSchwarze0

Nice work.

SaschaSchwarze0 · 2024-02-22T15:30:45Z

pkg/image/vulnerability_scan.go

+)
+
+const (
+	VulnerabilityCountLimit = 50 // Number of vulnerabilities to be added to buildrun output.


Should be in config.go which reads from environment variables.

pkg/image/vulnerability_scan.go

SaschaSchwarze0 · 2024-02-22T15:38:41Z

pkg/reconciler/buildrun/buildrun.go

+						return reconcile.Result{}, resources.UpdateConditionWithFalseStatus(ctx, r.client, buildRun, fmt.Sprintf("Vulnerabilities have been found in the output image. For detailed information, see kubectl --namespace %s logs %s --container step-image-processing",
+							buildRun.Namespace,
+							lastTaskRun.Status.PodName,
+						), "VulnerabilitiesFound")


In the message, should we say "check the BuildRun status, or see kubectl ..." ?

Can we put VulnerabilitiesFound into a constant ? Would be nearby here:

build/pkg/apis/build/v1beta1/buildrun_types.go

Line 108 in 6ee1cd8

BuildRunStatePodEvicted = "PodEvicted"

SaschaSchwarze0 · 2024-02-22T15:40:27Z

pkg/apis/build/v1beta1/buildrun_types.go

@@ -145,6 +145,12 @@ type GitSourceResult struct {
 	BranchName string `json:"branchName,omitempty"`
 }

+// Vulnerability defines a vulnerability by its ID and severity
+type Vulnerability struct {
+	VulnerabilityID string `json:"vulnerabilityID,omitempty"`


The section is called vulnerabilities, then no nead to call it vulnerabilityID insight of it.

Suggested change

VulnerabilityID string `json:"vulnerabilityID,omitempty"`

ID string `json:"id,omitempty"`

I did that because it was easy to map it with the result from Trivy scan. I will create a different struct for trivy result.

test/data/images/vuln-single-image/vuln-image.tar

test/e2e/v1alpha1/e2e_vulnerability_scanning_test.go

SaschaSchwarze0 · 2024-02-22T15:56:52Z

test/e2e/v1beta1/validators_test.go


 	// Ensure the BuildRun has been created
-	err := testBuild.CreateBR(testBuildRun)
-	Expect(err).ToNot(HaveOccurred(), "Failed to create BuildRun")
+	if _, err := testBuild.GetBR(testBuildRun.Name); err != nil {


I do not understand that check. Why do we need to check if the BuildRun exists ?

Because in my tests, I have already created the buildrun, otherwise I had to create a new function for running buildruns.
This condition is also present in validateBuildRunToSucceed function.

HeavyWombat

Again, thanks for the PR. I only found some small things we should change, but it looks mostly very good to me already.

HeavyWombat · 2024-04-22T13:08:00Z

test/integration/vulnerability_scan_test.go

+	test "github.com/shipwright-io/build/test/v1beta1_samples"
+)
+
+var _ = Describe("Integration tests Build and BuildRuns", func() {


Suggested change

var _ = Describe("Integration tests Build and BuildRuns", func() {

var _ = Describe("Vulnerability scan tests", func() {

Since it is in the integration package, I would say we do not need to repeat that in the name of the tests here.

HeavyWombat · 2024-04-22T13:11:05Z

test/e2e/v1beta1/common_cbs_test.go

+func (c *clusterBuildStrategyPrototype) TestMe(f func(clusterBuildStrategy *buildv1beta1.ClusterBuildStrategy)) {
+	cbs, err := c.Create()
+	Expect(err).ToNot(HaveOccurred())


Nit: Assuming the Expect in this helper function are not causing issues a lot, I would say that it qualifies as a Ginkgo helper function and we should declare it as such.

Suggested change

func (c *clusterBuildStrategyPrototype) TestMe(f func(clusterBuildStrategy *buildv1beta1.ClusterBuildStrategy)) {

cbs, err := c.Create()

Expect(err).ToNot(HaveOccurred())

func (c *clusterBuildStrategyPrototype) TestMe(f func(clusterBuildStrategy *buildv1beta1.ClusterBuildStrategy)) {

GinkgoHelper()

cbs, err := c.Create()

Expect(err).ToNot(HaveOccurred())

HeavyWombat · 2024-04-22T13:20:57Z

test/e2e/v1beta1/common_cbs_test.go

+
+	. "github.com/onsi/gomega"
+
+	meta "k8s.io/apimachinery/pkg/apis/meta/v1"


(Super)Nit: Can we rename it metav1? I think this is the more widely used import name.

HeavyWombat · 2024-04-22T13:24:39Z

pkg/validate/output_test.go

+			}
+		})
+
+		It("should fail for invvalid severities", func() {


Suggested change

It("should fail for invvalid severities", func() {

It("should fail for invalid severities", func() {

HeavyWombat · 2024-04-22T13:27:54Z

pkg/reconciler/buildrun/resources/results_test.go

 				})

 			resources.UpdateBuildRunUsingTaskResults(ctx, br, tr.Status.Results, taskRunRequest)

 			Expect(br.Status.Output.Digest).To(Equal(imageDigest))
 			Expect(br.Status.Output.Size).To(Equal(int64(230)))
+			Expect(len(br.Status.Output.Vulnerabilities)).To(Equal(2))


Nit: I recently learned there is a Ginkgo/Gomega idiomatic way to write that (didn't check the syntax again, but I think it is like this):

Suggested change

Expect(len(br.Status.Output.Vulnerabilities)).To(Equal(2))

Expect(br.Status.Output.Vulnerabilities).To(HaveLen(2))

SaschaSchwarze0 · 2024-05-05T20:13:50Z

cmd/image-processing/main_test.go

+						"--vuln-count-limit", "10",
+					)).ToNot(HaveOccurred())
+					output := filecontent(filename)
+					Expect(strings.Contains(output, "CVE-2019-8457")).To(BeTrue())


Will produce a better output in case substring is not found:

Suggested change

Expect(strings.Contains(output, "CVE-2019-8457")).To(BeTrue())

Expect(output).To(ContainSubstring("CVE-2019-8457"))

SaschaSchwarze0 · 2024-05-05T20:15:35Z

cmd/image-processing/main_test.go

+						"--vuln-settings", vulnSettings.String(),
+						"--result-file-image-vulnerabilities", filename,
+					)).ToNot(HaveOccurred())
+					Expect(strings.Contains(filecontent(filename), "CVE-2019-8457")).To(BeTrue())


Suggested change

Expect(strings.Contains(filecontent(filename), "CVE-2019-8457")).To(BeTrue())

output := filecontent(filename)

Expect(output).To(ContainSubstring("CVE-2019-8457"))

SaschaSchwarze0 · 2024-05-05T20:15:54Z

cmd/image-processing/main_test.go

+						"--vuln-settings", vulnSettings.String(),
+						"--result-file-image-vulnerabilities", filename,
+					)).To(HaveOccurred())
+					Expect(strings.Contains(filecontent(filename), "CVE-2019-8457")).To(BeTrue())


Suggested change

Expect(strings.Contains(filecontent(filename), "CVE-2019-8457")).To(BeTrue())

output := filecontent(filename)

Expect(output).To(ContainSubstring("CVE-2019-8457"))

SaschaSchwarze0 · 2024-05-05T20:16:22Z

cmd/image-processing/main_test.go

+			})
+		})
+
+		It("should run vulnerability scanning on a image if it is already pushed by the strategy", func() {


Suggested change

It("should run vulnerability scanning on a image if it is already pushed by the strategy", func() {

It("should run vulnerability scanning on an image that is already pushed by the strategy", func() {

SaschaSchwarze0 · 2024-05-05T20:16:58Z

cmd/image-processing/main_test.go

+						"--vuln-settings", vulnSettings.String(),
+						"--result-file-image-vulnerabilities", filename,
+					)).ToNot(HaveOccurred())
+					Expect(strings.Contains(filecontent(filename), "CVE-2019-12900")).To(BeTrue())


Suggested change

Expect(strings.Contains(filecontent(filename), "CVE-2019-12900")).To(BeTrue())

output := filecontent(filename)

Expect(output).To(ContainSubstring("CVE-2019-12900"))

SaschaSchwarze0 · 2024-05-05T20:27:09Z

pkg/config/config.go

@@ -103,6 +106,7 @@ type Config struct {
 	Controllers                      Controllers
 	KubeAPIOptions                   KubeAPIOptions
 	GitRewriteRule                   bool
+	VulnerabilityCountLimit          string


This should be a number. The error returned by SetConfigFromEnv will cause the controller to panic and that's the desired behavior for invalid values.

SaschaSchwarze0 · 2024-05-05T20:37:14Z

pkg/image/vulnerability_scan.go

+	cmd := exec.CommandContext(ctx, "trivy", trivyArgs...)
+
+	// Print the command to be executed
+	log.Println(cmd.String())


This would print the password, correct? If so, is a no-go.

SaschaSchwarze0 · 2024-05-05T20:43:19Z

pkg/reconciler/buildrun/resources/conditions.go

+
+				if failedContainer.Name == "step-image-processing" && exitCode == 22 {
+					reason = buildv1beta1.BuildRunStateVulnerabilitiesFound
+					message = fmt.Sprintf("Vulnerabilities have been found in the image which can be seen in the buildrun status. For detailed information,see kubectl --namespace %s logs %s --container=%s",


Will there be reasonable output in the container logs?

Not really, just the vulnerabilities list. In case trivy command fails, it will print an error, but in that case exit code won't be 22.

SaschaSchwarze0 · 2024-05-05T20:44:26Z

pkg/reconciler/buildrun/resources/conditions_test.go

+			// Generate a pod with the status to be evicted
+			failedTaskRunEvictedPod := corev1.Pod{


SaschaSchwarze0 · 2024-05-05T20:48:07Z

pkg/validate/output.go

+				severityStr := *b.Build.Spec.Output.VulnerabilityScan.Ignore.Severity
+				if !isValidSeverity(severityStr) {
+					b.Build.Status.Reason = build.BuildReasonPtr(build.VulnerabilityScanSeverityNotValid)
+					b.Build.Status.Message = pointer.String("output vulnerability scan severity is invalid, must be a comma separated combination of these values: low | medium | high | critical")


Message is not correct.

- Add vulnerability scanning options in build and buildrun types in v1alpha1 and v1beta1 - Changes for conversion to v1beta1

- implement vulnerability scanning using trivy - list vulnerabilities in buildrun output fix failure details

…ed handling

SaschaSchwarze0 · 2024-05-31T20:44:51Z

Looks mostly good @karanibm6. I put a few small changes in karanibm6#37. Please check.

…scanning Sascha add vulnerability scanning

openshift-ci bot added the do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. label Feb 7, 2024

openshift-ci bot requested review from otaviof and SaschaSchwarze0 February 7, 2024 17:34

pull-request-size bot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Feb 7, 2024

openshift-ci bot added the kind/feature Categorizes issue or PR as related to a new feature. label Feb 7, 2024

karanibm6 force-pushed the add-vulnerability-scanning branch 8 times, most recently from c5ea814 to a8fd62f Compare February 7, 2024 20:18

qu1queee requested review from qu1queee and removed request for otaviof February 8, 2024 08:29

HeavyWombat requested changes Feb 9, 2024

View reviewed changes

karanibm6 force-pushed the add-vulnerability-scanning branch 3 times, most recently from eaddddc to 476489d Compare February 17, 2024 12:28

openshift-ci bot added release-note Label for when a PR has specified a release note and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Feb 17, 2024

karanibm6 force-pushed the add-vulnerability-scanning branch from 476489d to 204320b Compare February 18, 2024 18:00

HeavyWombat self-requested a review February 20, 2024 09:30

HeavyWombat requested changes Feb 20, 2024

View reviewed changes

pkg/reconciler/buildrun/resources/image_processing.go Show resolved Hide resolved

qu1queee mentioned this pull request Feb 22, 2024

February 26th, 2024 Community Meeting shipwright-io/community#191

Closed

SaschaSchwarze0 requested changes Feb 22, 2024

View reviewed changes

karanibm6 force-pushed the add-vulnerability-scanning branch 3 times, most recently from 03ae3cd to fea1096 Compare April 17, 2024 17:26

HeavyWombat requested changes Apr 22, 2024

View reviewed changes

karanibm6 force-pushed the add-vulnerability-scanning branch from fea1096 to 6ddd690 Compare April 24, 2024 07:33

karanibm6 requested a review from HeavyWombat April 24, 2024 08:29

qu1queee added this to the release-v0.14.0 milestone May 2, 2024

karanibm6 force-pushed the add-vulnerability-scanning branch from 6ddd690 to 313ea2d Compare May 5, 2024 15:19

SaschaSchwarze0 requested changes May 5, 2024

View reviewed changes

karanibm6 force-pushed the add-vulnerability-scanning branch from 313ea2d to 9badf96 Compare May 15, 2024 19:31

karanibm6 added 2 commits May 16, 2024 01:17

Add options for vulnerability scanning

6b170ec

- Add vulnerability scanning options in build and buildrun types in v1alpha1 and v1beta1 - Changes for conversion to v1beta1

Add vulnerable image for unit testing of vulnerability scanning

9981fd3

karanibm6 force-pushed the add-vulnerability-scanning branch from 9badf96 to 6aaee06 Compare May 15, 2024 19:56

karanibm6 added 7 commits May 16, 2024 11:04

Implement vulnerability scanning for container images

961c40d

- implement vulnerability scanning using trivy - list vulnerabilities in buildrun output fix failure details

Add e2e tests to verify vulnerability scanning

f602c18

Install Trivy for vulnerability scanning unit-testing

e86b321

add docs for vulnerability scanning

afa1acd

add validation for vulnerability scan options

78ab308

add integration tests for vulnerability scanning

d095dec

add build sample for vulnerability scanning

4cdc36e

karanibm6 force-pushed the add-vulnerability-scanning branch from 6aaee06 to 4cdc36e Compare May 16, 2024 05:35

karanibm6 requested a review from SaschaSchwarze0 May 16, 2024 05:46

SaschaSchwarze0 added 5 commits May 31, 2024 22:34

Make severity enumerations

ca43301

Add VULNERABILITY_COUNT_LIMIT default 50

4a3e7c7

Small corrections in documentation

96950a4

Ensure message is set even without failedContainerStatus

d271be4

Remove insecure from authentication parameters, simplify ignore unfix…

92aca87

…ed handling

karanibm6 added 2 commits June 2, 2024 23:16

Merge pull request #37 from SaschaSchwarze0/sascha-add-vulnerability-…

8f17ad3

…scanning Sascha add vulnerability scanning

remove unused function

477214a

	VulnerabilityID string `json:"vulnerabilityID,omitempty"`
	ID string `json:"id,omitempty"`

	var _ = Describe("Integration tests Build and BuildRuns", func() {
	var _ = Describe("Vulnerability scan tests", func() {


		. "github.com/onsi/gomega"

		meta "k8s.io/apimachinery/pkg/apis/meta/v1"

	It("should fail for invvalid severities", func() {
	It("should fail for invalid severities", func() {

	Expect(len(br.Status.Output.Vulnerabilities)).To(Equal(2))
	Expect(br.Status.Output.Vulnerabilities).To(HaveLen(2))

	Expect(strings.Contains(output, "CVE-2019-8457")).To(BeTrue())
	Expect(output).To(ContainSubstring("CVE-2019-8457"))

	Expect(strings.Contains(filecontent(filename), "CVE-2019-8457")).To(BeTrue())
	output := filecontent(filename)
	Expect(output).To(ContainSubstring("CVE-2019-8457"))

	It("should run vulnerability scanning on a image if it is already pushed by the strategy", func() {
	It("should run vulnerability scanning on an image that is already pushed by the strategy", func() {

		// Generate a pod with the status to be evicted
		failedTaskRunEvictedPod := corev1.Pod{

Vulnerability Scanning Implementation for container images #1489

Are you sure you want to change the base?

Vulnerability Scanning Implementation for container images #1489

Conversation

karanibm6 commented Feb 7, 2024 • edited

Changes

Submitter Checklist

Release Notes

openshift-ci bot commented Feb 7, 2024

karanibm6 commented Feb 7, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HeavyWombat left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SaschaSchwarze0 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HeavyWombat left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

karanibm6 May 15, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SaschaSchwarze0 commented May 31, 2024

karanibm6 commented Feb 7, 2024 •

edited

karanibm6 May 15, 2024 •

edited