Add Istio request handler and business logics for fetching Istio CVEs #984

daynewlee · 2022-10-19T00:56:24Z

Add functionalities of fetch Istio CVEs, converting Istio CVEs to responses and handling Istio CVE get requests.

ghost · 2022-10-19T01:13:02Z

Images are ready for the commit at abc1e5c.

To use the images, use the tag 2.26.x-24-gabc1e5cb92.

daynewlee · 2022-10-26T01:47:50Z

Manual test with http request

curl "https://localhost:8443/v1/orchestrator/istio/vulnerabilities/1.13.6" -k

{"scannerVersion":"2.26.x-15-g341eca297c","vulnerabilities":[{"name":"ISTIO-SECURITY-2022-006","description":"Ill-formed headers sent to Envoy in certain configurations can lead to unexpected memory access resulting in undefined behavior or crashing","link":"https://istio.io/latest/news/security/istio-security-2022-006/","metadataV2":{"publishedDateTime":"2022-07-26T00:00Z","lastModifiedDateTime":"","cvssV2":null,"cvssV3":{"vector":"CVSS:3.1/AV:N/AC:H/PR:N/UI:N/S:U/C:N/I:N/A:H","score":5.9,"exploitabilityScore":2.2,"impactScore":3.6}},"fixedBy":"1.13.7","severity":"Moderate"}]}

RTann

A few changes requested. Also, be sure to rebase to get the new *zip.Reader updates

tools/allowed-large-files

proto/scanner/api/v1/orchestrator_scan_service.proto

RTann · 2022-10-27T18:56:03Z

api/v1/orchestratorscan/service.go

+
+// GetIstioVulnerabilities returns Istio vulnerabilities for requested Kubernetes version.
+func (s *serviceImpl) GetIstioVulnerabilities(_ context.Context, req *v1.GetIstioVulnerabilitiesRequest) (*v1.GetIstioVulnerabilitiesResponse, error) {
+	var err error


nit: declare this closer to where it's used

api/v1/orchestratorscan/service.go

RTann · 2022-10-27T19:02:23Z

api/v1/orchestratorscan/service.go

+	}
+	version, err := convert.TruncateVersion(version)
+	if err != nil {
+		log.Warnf("Unable to convert version of %s - %v. Skipping...", version, err)


Unable to convert Istio version etc

api/v1/convert/istio.go

RTann · 2022-10-27T19:40:48Z

api/v1/convert/istio.go

+	"github.com/stackrox/rox/pkg/stringutils"
+	v1 "github.com/stackrox/scanner/generated/scanner/api/v1"
+	stackroxTypes "github.com/stackrox/scanner/pkg/types"
+


remove newline

RTann · 2022-10-27T19:41:47Z

api/v1/convert/istio.go

+)
+
+// IstioVulnerabilities converts istio cve schema to vulnerability.
+func IstioVulnerabilities(version string, istioVulns []types.Vuln) ([]*v1.Vulnerability, error) {


This doesn't seem to ever return an error, so no need for that return type

pkg/types/types.go

RTann

Almost there!

RTann · 2022-11-07T23:45:32Z

api/v1/orchestratorscan/service.go

+		ScannerVersion: s.version,
+	}
+
+	getIstioVuln := func(version string) ([]*v1.Vulnerability, error) {


I don't think this needs to be a closure. Why not just run these steps and then eventually set resp.Vulnerabilities = filterInvalidVulns(converted)

RTann · 2022-11-10T23:50:07Z

e2etests/orchestrator_scan_test.go

@@ -20,6 +20,34 @@ var (
 	patchRegex = regexp.MustCompile(`^[0-9]+\.[0-9]+\.([0-9]+)$`)
 )

+func TestGRPCGetIstioVulnerabilities(t *testing.T) {


I think it may be more worthwhile to give an Istio version and ensure we find specific vulnerabilities. For example, we know Istio verison 1.13.6 is affected by ISTIO-SECURITY-2022-007, which has a specific description and CVSS and is fixed by 1.13.9

RTann · 2022-11-10T23:51:58Z

pkg/istioUtil/util.go

@@ -0,0 +1,26 @@
+package istioUtil


nit the directory and package name should be all lowercase: istioutil

RTann · 2022-11-10T23:52:32Z

pkg/istioUtil/util.go

+)
+
+// IstioIsAffected gets the fixed-by version for vStr in Istion vuln.
+func IstioIsAffected(vStr string, vuln types.Vuln) (bool, string, error) {


can probably just rename to IsAffected since it's clear this is about Istio from the package name

can we pass in a version.Version instead of a string?

RTann · 2022-11-10T23:58:19Z

istio/cache/db.go

+
+	var vulns []types.Vuln
+	for _, vuln := range c.cache {
+		isAffected, _, _ := istioUtil.IstioIsAffected(version, vuln)


can you add a comment why we ignore errors here?

api/v1/orchestratorscan/service.go

openshift-ci · 2022-11-14T17:08:59Z

@daynewlee: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/e2e-tests	`abc1e5c`	link	false	`/test e2e-tests`

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

RTann

a few minor nits. Going to approve to avoid having to do another review cycle, but please take a look at the suggestions

RTann · 2022-11-14T17:54:25Z

api/v1/orchestratorscan/service.go

@@ -107,6 +110,29 @@ func (s *serviceImpl) GetKubeVulnerabilities(_ context.Context, req *v1.GetKubeV
 	return resp, nil
 }

+// GetIstioVulnerabilities returns Istio vulnerabilities for requested Kubernetes version.


nit: Istio version

RTann · 2022-11-14T17:57:14Z

pkg/istioutil/util.go

+	"github.com/stackrox/istio-cves/types"
+)
+
+// IsAffected gets the fixed-by version for vStr in Istion vuln.


// IsAffected returns whether the given version of Istio is affected by the given vulnerability. // If it is, then the fixed-by version is returned as well.

RTann · 2022-11-14T18:00:48Z

istio/cache/db.go

+		return nil
+	}
+	for _, vuln := range c.cache {
+		isAffected, _, error := istioutil.IsAffected(v, vuln)


missed this before, sorry. Let's call this err instead, as error is the type

RTann · 2022-11-14T18:01:58Z

istio/cache/db.go

+	c.cacheRWLock.RLock()
+	defer c.cacheRWLock.RUnlock()
+
+	var vulns []types.Vuln
+	v, err := version.NewVersion(vStr)
+	if err != nil {
+		log.Infof("Failed to get version: %s", vStr)
+		return nil
+	}


Suggested change

c.cacheRWLock.RLock()

defer c.cacheRWLock.RUnlock()

var vulns []types.Vuln

v, err := version.NewVersion(vStr)

if err != nil {

log.Infof("Failed to get version: %s", vStr)

return nil

}

var vulns []types.Vuln

v, err := version.NewVersion(vStr)

if err != nil {

log.Infof("Failed to get version: %s", vStr)

return nil

}

c.cacheRWLock.RLock()

defer c.cacheRWLock.RUnlock()

This way, we only lock when we have to

RTann · 2022-11-14T18:03:46Z

api/v1/convert/istio.go

+		}
+
+		link := stringutils.OrDefault(istioVuln.Link, "https://istio.io/latest/news/security/")
+		_, fixedBy, err := istioutil.IsAffected(v, istioVuln)


Can we leave a comment saying we know this version is affected, which is why we can ignore that return value?

RTann · 2022-11-14T18:04:11Z

api/v1/convert/istio.go

+		link := stringutils.OrDefault(istioVuln.Link, "https://istio.io/latest/news/security/")
+		_, fixedBy, err := istioutil.IsAffected(v, istioVuln)
+		if err != nil {
+			log.Errorf("unable to get fixedBy for %s: %istioVuln", istioVuln.Name, err)


I think you meant %v at the end?

daynewlee added 4 commits October 18, 2022 19:52

First commit

0da7c8e

Added istio load to cache and cache test

1190612

Added istio vuln response and request

95c7bb3

Minor update

0672f36

daynewlee added 2 commits October 18, 2022 20:22

Added Istio convertor and types

7f0e4ad

Removed extra changes

8002d1c

daynewlee force-pushed the personal/yli3/addIstioService branch 2 times, most recently from 8193288 to f07e6ce Compare October 19, 2022 16:50

Added GetIstioVulnerabilities

8b08c6e

daynewlee force-pushed the personal/yli3/addIstioService branch 4 times, most recently from f18bb0b to 8b08c6e Compare October 25, 2022 02:51

daynewlee added 2 commits October 24, 2022 21:51

CI

39f9200

Merge branch 'master' into personal/yli3/addIstioService

de7a831

daynewlee force-pushed the personal/yli3/addIstioService branch 5 times, most recently from a93bbcb to 8038896 Compare October 26, 2022 00:44

Added istio request handler and e2e test

b48d8ad

daynewlee force-pushed the personal/yli3/addIstioService branch from 8038896 to b48d8ad Compare October 26, 2022 01:45

daynewlee requested review from c-du and RTann October 26, 2022 01:46

daynewlee changed the title ~~Personal/yli3/add istio service~~ Add Istio request handler and business logics for fetching Istio CVEs Oct 27, 2022

RTann requested changes Oct 27, 2022

View reviewed changes

Merge branch 'master' into personal/yli3/addIstioService

0254d0c

daynewlee force-pushed the personal/yli3/addIstioService branch from 231ed03 to 95bcca4 Compare October 28, 2022 16:13

daynewlee force-pushed the personal/yli3/addIstioService branch 3 times, most recently from f54d492 to 96b5f2a Compare November 7, 2022 20:04

c-du approved these changes Nov 7, 2022

View reviewed changes

daynewlee force-pushed the personal/yli3/addIstioService branch from 4059e5b to 9301d72 Compare November 7, 2022 21:31

Removed MetadataIstio and Added util.IstioIsAffected

807c953

daynewlee force-pushed the personal/yli3/addIstioService branch from 9301d72 to 807c953 Compare November 7, 2022 23:02

daynewlee enabled auto-merge (squash) November 8, 2022 16:34

daynewlee requested review from RTann and removed request for RTann November 8, 2022 16:58

RTann reviewed Nov 10, 2022

View reviewed changes

daynewlee force-pushed the personal/yli3/addIstioService branch from 6c24339 to 8c49906 Compare November 11, 2022 17:09

daynewlee requested a review from RTann November 11, 2022 17:11

RTann reviewed Nov 11, 2022

View reviewed changes

api/v1/orchestratorscan/service.go Show resolved Hide resolved

daynewlee force-pushed the personal/yli3/addIstioService branch 9 times, most recently from 3b56631 to 27f2d4e Compare November 13, 2022 18:28

daynewlee requested a review from RTann November 13, 2022 23:24

Updated e2e test

abc1e5c

daynewlee force-pushed the personal/yli3/addIstioService branch from 1d1e63f to abc1e5c Compare November 14, 2022 16:25

RTann approved these changes Nov 14, 2022

View reviewed changes

daynewlee merged commit 4f2407b into master Nov 14, 2022

daynewlee deleted the personal/yli3/addIstioService branch November 14, 2022 18:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Istio request handler and business logics for fetching Istio CVEs #984

Add Istio request handler and business logics for fetching Istio CVEs #984

daynewlee commented Oct 19, 2022 •

edited

Loading

ghost commented Oct 19, 2022 •

edited by ghost

Loading

daynewlee commented Oct 26, 2022 •

edited

Loading

RTann left a comment

RTann Oct 27, 2022

RTann Nov 4, 2022

RTann Oct 27, 2022

RTann Oct 27, 2022

RTann Oct 27, 2022

RTann left a comment

RTann Nov 7, 2022

RTann Nov 10, 2022

RTann Nov 10, 2022

RTann Nov 10, 2022

RTann Nov 10, 2022

RTann Nov 10, 2022

openshift-ci bot commented Nov 14, 2022

RTann left a comment

RTann Nov 14, 2022

RTann Nov 14, 2022

RTann Nov 14, 2022

RTann Nov 14, 2022

RTann Nov 14, 2022

RTann Nov 14, 2022

RTann Nov 14, 2022

Add Istio request handler and business logics for fetching Istio CVEs #984

Add Istio request handler and business logics for fetching Istio CVEs #984

Conversation

daynewlee commented Oct 19, 2022 • edited Loading

ghost commented Oct 19, 2022 • edited by ghost Loading

daynewlee commented Oct 26, 2022 • edited Loading

RTann 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

RTann 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

openshift-ci bot commented Nov 14, 2022

RTann 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

daynewlee commented Oct 19, 2022 •

edited

Loading

ghost commented Oct 19, 2022 •

edited by ghost

Loading

daynewlee commented Oct 26, 2022 •

edited

Loading