ROX-13627: Extend GetNodeVulnerabilities API by supporting Node Inventory #1004

vikin91 · 2022-11-04T13:47:37Z

This PR extends the GetNodeVulnerabilities API to accept a Node message that contains NodeInventory (see stackrox/stackrox#3755 and stackrox/stackrox#3757). The respective Central part of this request is still in progress and there is no PR for it yet.

This is required to support RHCOS Node scanning over the GetNodeVulnerabilities API.

Additionally, this PR includes:

A new feature flag RHCOSNodeScanning
Improvements in the Makefile that would make it usable for:
- Colleagues who have spaces in their PATH
- ~~Colleagues who do not have wget but have curl~~

How tested

Added an e2e and run it locally with:
- make image
- make deploy-local
- Open port-forward to the scanner pod 8443:8443
- Run tests go test -tags e2e -timeout=10s -v -run ^TestGRPCGetRHCOSNodeVulnerabilities$ github.com/stackrox/scanner/e2etests
Tested the changes in the Makefile by doing make clean-proto-deps and doing the steps described in https://github.com/stackrox/scanner#steps
Deploying with Central locally and seeing the vulnerabilities in the UI
CI

openshift-ci · 2022-11-04T13:47:40Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

vikin91 · 2022-11-04T13:47:50Z

Current dependencies on/for this PR:

master
- PR ROX-13627: Extend GetNodeVulnerabilities API by supporting Node Inventory #1004 👈

This comment was auto-generated by Graphite.

vikin91 · 2022-11-04T13:48:09Z

/test all

vikin91 · 2022-11-04T14:10:20Z

/test all

vikin91 · 2022-11-04T14:27:57Z

/test all

ghost · 2022-11-04T14:43:03Z

Images are ready for the commit at d26da56.

To use the images, use the tag 2.27.x-55-gd26da561cb.

vikin91 · 2022-11-07T11:38:01Z

/test all

vikin91 · 2022-11-21T13:03:13Z

/test all

vikin91 · 2022-11-21T13:45:56Z

/test all

vikin91 · 2022-11-21T14:36:41Z

/test all

RTann

some comments. Also be sure to rebase to make the ose-jenkins E2E test pass

Makefile

api/v1/nodescan/service.go

RTann · 2022-11-22T15:54:59Z

make/protogen.mk

@@ -58,7 +58,11 @@ PROTOC_FILE := $(PROTOC_DOWNLOADS_DIR)/$(PROTOC_ZIP)

 $(PROTOC_FILE): $(PROTOC_DOWNLOADS_DIR)
 	@echo "+ $@"
+ifeq (, $(shell which wget))


should this be added to the stackrox repo, too?

Not sure, it works for me in when I build stackrox without any additional changes. I will look how it is done there.

RTann · 2022-11-22T16:00:29Z

api/v1/nodescan/service.go

+		if err != nil {
+			return nil, status.Error(codes.Internal, err.Error())
+		}
+		resp.InventoryFeatures = imagescan.ConvertFeatures(layer.Features)


hm maybe we should put this in a more common package so we don't mix image and node scanning libraries

We could - we kind of intentionally planned to call the imagescan package in the design doc, so having it here is a consequence and it is visible (maybe that is good?).

I would rely here on your suggestion here if you have some particular pkg in mind.

I moved it into a separate package now. Many existing pkgs caused import cycle, so I created a separate one in e693353

e2etests/node_scan_rhcos_test.go

RTann · 2022-11-22T16:05:46Z

e2etests/node_scan_rhcos_test.go

+	Severity: "Moderate",
+}
+
+func TestGRPCGetRHCOSNodeVulnerabilities(t *testing.T) {


does this pass with the feature flag off right now? May need to enable it for tests?

I will make sure that this is being executed with the FF.

Yes, this is being executed in the CI. I know this because the test has failed :)

{Failed node_scan_rhcos_test.go:141: Error Trace: /go/src/github.com/stackrox/scanner/e2etests/node_scan_rhcos_test.go:141 Error: Should be true Test: TestGRPCGetRHCOSNodeVulnerabilities/case-0 Messages: expected to find feat 'libksba:1.3.5-7.el8.x86_64' in the reply, but got none node_scan_rhcos_test.go:141: Error Trace: /go/src/github.com/stackrox/scanner/e2etests/node_scan_rhcos_test.go:141 Error: Should be true Test: TestGRPCGetRHCOSNodeVulnerabilities/case-0 Messages: expected to find feat 'tar:1.27.1.el8.x86_64' in the reply, but got none}

Do you know why the vulns are being found when running this e2e test locally but not on CI?

Hoping to see the test being run in CI after applying 229e12d

Apparently adding the feature-flag env to the chart did not enable it and the test was skipped. Do we have any other ideas how to make the test run on PRs and master?

Sorry, I should have mentioned adding the env var to the chart will just enable the feature in the Scanner deployment. The tests run on CI (not in the deployment), so we'd have to set the var there as well. We never really had a need to skip a test due to a feature flag before, so we do not really have that infrastructure as of yet. I'm curious if the stackrox repo has this. I know we used to with CircleCI by updating the config.yml. Maybe @gavin-stackrox knows and there exists a solution which, preferably, avoids OpenShift CI configuration changes. Perhaps just a bash script in the ci/scripts directory which sets environment variables at the beginning of each step?

I decided to remove the feature-flag. The feature will be used only if the arriving request to Scanner has non-empty node inventory. This does the same job that the feature flag is doing.

e2etests/node_scan_rhcos_test.go

RTann · 2022-11-22T16:08:45Z

e2etests/node_scan_rhcos_test.go

+					Version: "0.0.1",
+				},
+				NodeInventory: &v1.Components{
+					Namespace: "Red Hat CoreOS",


just curious I wonder what this value actually would be

Ups, confused it with the OS name. Not sure to be honest what is the correct value - should it be the cluster name or Openshift project name?

I finally decided to put rhcos:4.11 here.

vikin91 · 2022-11-22T16:59:50Z

I will rebase as well as suggested.

vikin91 · 2022-11-22T17:58:27Z

/retest

vikin91 · 2022-11-23T13:58:26Z

/retest

vikin91 · 2022-11-24T08:15:15Z

After enabling the feature flag in code (setting default to true), the test executes properly:

(from https://gcsweb-ci.apps.ci.l2s4.p1.openshiftapps.com/gcs/origin-ci-test/pr-logs/pull/stackrox_scanner/1004/pull-ci-stackrox-scanner-master-e2e-tests/1595484736086609920/artifacts/e2e-tests/e2e-tests/build-log.txt)

=== RUN   TestGRPCGetRHCOSNodeVulnerabilities
=== RUN   TestGRPCGetRHCOSNodeVulnerabilities/case-0
Handling connection for 8443
    node_scan_rhcos_test.go:141: gotFeat.InventoryFeatures: [name:"libksba" version:"1.3.5-7.el8.x86_64" feature_type:"rpm" vulnerabilities:<name:"RHSA-2022:7089" description:"KSBA (pronounced Kasbah) is a library to make X.509 certificates as well as the CMS easily accessible by other applications.  Both specifications are building blocks of S/MIME and TLS.\n\nSecurity Fix(es):\n\n* libksba: integer overflow may lead to remote code execution (CVE-2022-3515)\n\nFor more details about the security issue(s), including the impact, a CVSS score, acknowledgments, and other related information, refer to the CVE page(s) listed in the References section." link:"https://access.redhat.com/errata/RHSA-2022:7089" metadata_v2:<published_date_time:"2022-10-24T00:00Z" last_modified_date_time:"2022-10-24T00:00Z" cvss_v3:<vector:"CVSS:3.1/AV:N/AC:H/PR:N/UI:N/S:U/C:H/I:H/A:H" score:8.1 exploitability_score:2.2 impact_score:5.9 > > fixed_by:"0:1.3.5-8.el8_6" severity:"Important" > fixed_by:"1.3.5-8.el8_6"  name:"tar" version:"1.27.1.el8.x86_64" feature_type:"rpm" vulnerabilities:<name:"CVE-2005-2541" description:"DOCUMENTATION: The MITRE CVE dictionary describes this issue as: Tar 1.15.1 does not properly warn the user when extracting setuid or setgid files, which may allow local users or remote attackers to gain privileges. \n            STATEMENT: This CVE was assigned to what is the documented and expected behaviour of tar.  There are currently no plans to change tar behaviour to strip setuid and setgid bits when extracting archives." link:"https://access.redhat.com/security/cve/CVE-2005-2541" metadata_v2:<last_modified_date_time:"2021-06-28T00:00Z" cvss_v3:<vector:"CVSS:3.1/AV:L/AC:H/PR:N/UI:R/S:U/C:H/I:H/A:H" score:7 exploitability_score:1 impact_score:5.9 > > severity:"Moderate" > vulnerabilities:<name:"CVE-2019-9923" description:"DOCUMENTATION: The MITRE CVE dictionary describes this issue as: pax_decode_header in sparse.c in GNU Tar before 1.32 had a NULL pointer dereference when parsing certain archives that have malformed extended headers." link:"https://access.redhat.com/security/cve/CVE-2019-9923" metadata_v2:<last_modified_date_time:"2021-10-27T00:00Z" cvss_v3:<vector:"CVSS:3.0/AV:L/AC:L/PR:N/UI:R/S:U/C:N/I:N/A:L" score:3.3 exploitability_score:1.8 impact_score:1.4 > > severity:"Low" > vulnerabilities:<name:"CVE-2021-20193" description:"DOCUMENTATION: A flaw was found in the src/list.c of tar. This flaw allows an attacker who can submit a crafted input file to tar to cause uncontrolled consumption of memory. The highest threat from this vulnerability is to system availability." link:"https://access.redhat.com/security/cve/CVE-2021-20193" metadata_v2:<last_modified_date_time:"2022-04-17T00:00Z" cvss_v3:<vector:"CVSS:3.1/AV:L/AC:L/PR:N/UI:R/S:U/C:N/I:N/A:L" score:3.3 exploitability_score:1.8 impact_score:1.4 > > severity:"Low" > ]
    node_scan_rhcos_test.go:141: gotFeat.InventoryFeatures: [name:"libksba" version:"1.3.5-7.el8.x86_64" feature_type:"rpm" vulnerabilities:<name:"RHSA-2022:7089" description:"KSBA (pronounced Kasbah) is a library to make X.509 certificates as well as the CMS easily accessible by other applications.  Both specifications are building blocks of S/MIME and TLS.\n\nSecurity Fix(es):\n\n* libksba: integer overflow may lead to remote code execution (CVE-2022-3515)\n\nFor more details about the security issue(s), including the impact, a CVSS score, acknowledgments, and other related information, refer to the CVE page(s) listed in the References section." link:"https://access.redhat.com/errata/RHSA-2022:7089" metadata_v2:<published_date_time:"2022-10-24T00:00Z" cvss_v3:<vector:"CVSS:3.1/AV:N/AC:H/PR:N/UI:N/S:U/C:H/I:H/A:H" score:8.1 exploitability_score:2.2 impact_score:5.9 > > fixed_by:"0:1.3.5-8.el8_6" severity:"Important" > fixed_by:"1.3.5-8.el8_6"  name:"tar" version:"1.27.1.el8.x86_64" feature_type:"rpm" vulnerabilities:<name:"CVE-2005-2541" description:"DOCUMENTATION: The MITRE CVE dictionary describes this issue as: Tar 1.15.1 does not properly warn the user when extracting setuid or setgid files, which may allow local users or remote attackers to gain privileges. \n            STATEMENT: This CVE was assigned to what is the documented and expected behaviour of tar.  There are currently no plans to change tar behaviour to strip setuid and setgid bits when extracting archives." link:"https://access.redhat.com/security/cve/CVE-2005-2541" metadata_v2:<last_modified_date_time:"2021-06-28T00:00Z" cvss_v3:<vector:"CVSS:3.1/AV:L/AC:H/PR:N/UI:R/S:U/C:H/I:H/A:H" score:7 exploitability_score:1 impact_score:5.9 > > severity:"Moderate" > vulnerabilities:<name:"CVE-2019-9923" description:"DOCUMENTATION: The MITRE CVE dictionary describes this issue as: pax_decode_header in sparse.c in GNU Tar before 1.32 had a NULL pointer dereference when parsing certain archives that have malformed extended headers." link:"https://access.redhat.com/security/cve/CVE-2019-9923" metadata_v2:<last_modified_date_time:"2021-10-27T00:00Z" cvss_v3:<vector:"CVSS:3.0/AV:L/AC:L/PR:N/UI:R/S:U/C:N/I:N/A:L" score:3.3 exploitability_score:1.8 impact_score:1.4 > > severity:"Low" > vulnerabilities:<name:"CVE-2021-20193" description:"DOCUMENTATION: A flaw was found in the src/list.c of tar. This flaw allows an attacker who can submit a crafted input file to tar to cause uncontrolled consumption of memory. The highest threat from this vulnerability is to system availability." link:"https://access.redhat.com/security/cve/CVE-2021-20193" metadata_v2:<last_modified_date_time:"2022-04-17T00:00Z" cvss_v3:<vector:"CVSS:3.1/AV:L/AC:L/PR:N/UI:R/S:U/C:N/I:N/A:L" score:3.3 exploitability_score:1.8 impact_score:1.4 > > severity:"Low" > ]
--- PASS: TestGRPCGetRHCOSNodeVulnerabilities (4.51s)
    --- PASS: TestGRPCGetRHCOSNodeVulnerabilities/case-0 (4.51s)

Now I would be interested how can we enable the feature flags for e2e tests in the PRs and master, but keep it disabled (if the default is false) for releases.

RTann · 2022-11-29T00:19:56Z

After enabling the feature flag in code (setting default to true), the test executes properly:

(from https://gcsweb-ci.apps.ci.l2s4.p1.openshiftapps.com/gcs/origin-ci-test/pr-logs/pull/stackrox_scanner/1004/pull-ci-stackrox-scanner-master-e2e-tests/1595484736086609920/artifacts/e2e-tests/e2e-tests/build-log.txt)

=== RUN   TestGRPCGetRHCOSNodeVulnerabilities
=== RUN   TestGRPCGetRHCOSNodeVulnerabilities/case-0
Handling connection for 8443
    node_scan_rhcos_test.go:141: gotFeat.InventoryFeatures: [name:"libksba" version:"1.3.5-7.el8.x86_64" feature_type:"rpm" vulnerabilities:<name:"RHSA-2022:7089" description:"KSBA (pronounced Kasbah) is a library to make X.509 certificates as well as the CMS easily accessible by other applications.  Both specifications are building blocks of S/MIME and TLS.\n\nSecurity Fix(es):\n\n* libksba: integer overflow may lead to remote code execution (CVE-2022-3515)\n\nFor more details about the security issue(s), including the impact, a CVSS score, acknowledgments, and other related information, refer to the CVE page(s) listed in the References section." link:"https://access.redhat.com/errata/RHSA-2022:7089" metadata_v2:<published_date_time:"2022-10-24T00:00Z" last_modified_date_time:"2022-10-24T00:00Z" cvss_v3:<vector:"CVSS:3.1/AV:N/AC:H/PR:N/UI:N/S:U/C:H/I:H/A:H" score:8.1 exploitability_score:2.2 impact_score:5.9 > > fixed_by:"0:1.3.5-8.el8_6" severity:"Important" > fixed_by:"1.3.5-8.el8_6"  name:"tar" version:"1.27.1.el8.x86_64" feature_type:"rpm" vulnerabilities:<name:"CVE-2005-2541" description:"DOCUMENTATION: The MITRE CVE dictionary describes this issue as: Tar 1.15.1 does not properly warn the user when extracting setuid or setgid files, which may allow local users or remote attackers to gain privileges. \n            STATEMENT: This CVE was assigned to what is the documented and expected behaviour of tar.  There are currently no plans to change tar behaviour to strip setuid and setgid bits when extracting archives." link:"https://access.redhat.com/security/cve/CVE-2005-2541" metadata_v2:<last_modified_date_time:"2021-06-28T00:00Z" cvss_v3:<vector:"CVSS:3.1/AV:L/AC:H/PR:N/UI:R/S:U/C:H/I:H/A:H" score:7 exploitability_score:1 impact_score:5.9 > > severity:"Moderate" > vulnerabilities:<name:"CVE-2019-9923" description:"DOCUMENTATION: The MITRE CVE dictionary describes this issue as: pax_decode_header in sparse.c in GNU Tar before 1.32 had a NULL pointer dereference when parsing certain archives that have malformed extended headers." link:"https://access.redhat.com/security/cve/CVE-2019-9923" metadata_v2:<last_modified_date_time:"2021-10-27T00:00Z" cvss_v3:<vector:"CVSS:3.0/AV:L/AC:L/PR:N/UI:R/S:U/C:N/I:N/A:L" score:3.3 exploitability_score:1.8 impact_score:1.4 > > severity:"Low" > vulnerabilities:<name:"CVE-2021-20193" description:"DOCUMENTATION: A flaw was found in the src/list.c of tar. This flaw allows an attacker who can submit a crafted input file to tar to cause uncontrolled consumption of memory. The highest threat from this vulnerability is to system availability." link:"https://access.redhat.com/security/cve/CVE-2021-20193" metadata_v2:<last_modified_date_time:"2022-04-17T00:00Z" cvss_v3:<vector:"CVSS:3.1/AV:L/AC:L/PR:N/UI:R/S:U/C:N/I:N/A:L" score:3.3 exploitability_score:1.8 impact_score:1.4 > > severity:"Low" > ]
    node_scan_rhcos_test.go:141: gotFeat.InventoryFeatures: [name:"libksba" version:"1.3.5-7.el8.x86_64" feature_type:"rpm" vulnerabilities:<name:"RHSA-2022:7089" description:"KSBA (pronounced Kasbah) is a library to make X.509 certificates as well as the CMS easily accessible by other applications.  Both specifications are building blocks of S/MIME and TLS.\n\nSecurity Fix(es):\n\n* libksba: integer overflow may lead to remote code execution (CVE-2022-3515)\n\nFor more details about the security issue(s), including the impact, a CVSS score, acknowledgments, and other related information, refer to the CVE page(s) listed in the References section." link:"https://access.redhat.com/errata/RHSA-2022:7089" metadata_v2:<published_date_time:"2022-10-24T00:00Z" cvss_v3:<vector:"CVSS:3.1/AV:N/AC:H/PR:N/UI:N/S:U/C:H/I:H/A:H" score:8.1 exploitability_score:2.2 impact_score:5.9 > > fixed_by:"0:1.3.5-8.el8_6" severity:"Important" > fixed_by:"1.3.5-8.el8_6"  name:"tar" version:"1.27.1.el8.x86_64" feature_type:"rpm" vulnerabilities:<name:"CVE-2005-2541" description:"DOCUMENTATION: The MITRE CVE dictionary describes this issue as: Tar 1.15.1 does not properly warn the user when extracting setuid or setgid files, which may allow local users or remote attackers to gain privileges. \n            STATEMENT: This CVE was assigned to what is the documented and expected behaviour of tar.  There are currently no plans to change tar behaviour to strip setuid and setgid bits when extracting archives." link:"https://access.redhat.com/security/cve/CVE-2005-2541" metadata_v2:<last_modified_date_time:"2021-06-28T00:00Z" cvss_v3:<vector:"CVSS:3.1/AV:L/AC:H/PR:N/UI:R/S:U/C:H/I:H/A:H" score:7 exploitability_score:1 impact_score:5.9 > > severity:"Moderate" > vulnerabilities:<name:"CVE-2019-9923" description:"DOCUMENTATION: The MITRE CVE dictionary describes this issue as: pax_decode_header in sparse.c in GNU Tar before 1.32 had a NULL pointer dereference when parsing certain archives that have malformed extended headers." link:"https://access.redhat.com/security/cve/CVE-2019-9923" metadata_v2:<last_modified_date_time:"2021-10-27T00:00Z" cvss_v3:<vector:"CVSS:3.0/AV:L/AC:L/PR:N/UI:R/S:U/C:N/I:N/A:L" score:3.3 exploitability_score:1.8 impact_score:1.4 > > severity:"Low" > vulnerabilities:<name:"CVE-2021-20193" description:"DOCUMENTATION: A flaw was found in the src/list.c of tar. This flaw allows an attacker who can submit a crafted input file to tar to cause uncontrolled consumption of memory. The highest threat from this vulnerability is to system availability." link:"https://access.redhat.com/security/cve/CVE-2021-20193" metadata_v2:<last_modified_date_time:"2022-04-17T00:00Z" cvss_v3:<vector:"CVSS:3.1/AV:L/AC:L/PR:N/UI:R/S:U/C:N/I:N/A:L" score:3.3 exploitability_score:1.8 impact_score:1.4 > > severity:"Low" > ]
--- PASS: TestGRPCGetRHCOSNodeVulnerabilities (4.51s)
    --- PASS: TestGRPCGetRHCOSNodeVulnerabilities/case-0 (4.51s)

Now I would be interested how can we enable the feature flags for e2e tests in the PRs and master, but keep it disabled (if the default is false) for releases.

Scanner has never really differentiated much between release and dev before for this kind of thing. Something like this, for example, cannot even be used unless Central requests for it anyway, as the network policy dictates only Central can make requests to Scanner. Scanner does not even enforce use of the default, only, when it's a release vs dev (like the stackrox repo does) because it never really had such a need before

vikin91 · 2022-11-29T13:01:49Z

Now I would be interested how can we enable the feature flags for e2e tests in the PRs and master, but keep it disabled (if the default is false) for releases.

Scanner has never really differentiated much between release and dev before for this kind of thing. Something like this, for example, cannot even be used unless Central requests for it anyway, as the network policy dictates only Central can make requests to Scanner. Scanner does not even enforce use of the default, only, when it's a release vs dev (like the stackrox repo does) because it never really had such a need before

Would this suggest to use the feature flag in central and remove it from scanner?

RTann · 2022-12-01T21:34:49Z

Now I would be interested how can we enable the feature flags for e2e tests in the PRs and master, but keep it disabled (if the default is false) for releases.

Scanner has never really differentiated much between release and dev before for this kind of thing. Something like this, for example, cannot even be used unless Central requests for it anyway, as the network policy dictates only Central can make requests to Scanner. Scanner does not even enforce use of the default, only, when it's a release vs dev (like the stackrox repo does) because it never really had such a need before

Would this suggest to use the feature flag in central and remove it from scanner?

I'd say for now just enable the flag, and we can disable it before next release, if needed

… way

- Fix assertions when 0 vulns or 0 features are expected - Rename functions as suggested - Add comments when dummy values are used - Add comments in tents when expected & got params are switched in place (assert.Len)

e2etests/node_scan_rhcos_test.go

msugakov · 2023-01-03T16:17:40Z

e2etests/node_scan_rhcos_test.go

 			name:    "Uncertified scan is unsupported for RHCOS and returns no features",
 			request: buildRequestCase([]v1.Note{v1.Note_CERTIFIED_RHEL_SCAN_UNAVAILABLE}),
 			responseContains: &v1.GetNodeVulnerabilitiesResponse{
-				Features: []*v1.Feature{},
+				Features:  []*v1.Feature{},


Well, the problem is that grep which currently serves as an example package without vulnerabilities may get vulnerabilities detected in the future. The comparison with 0 will start failing then.
We can either:

do nothing now and address the issue by changing the package when tests start breaking,

find a different package that does so little that it will not have vulnerabilities (e.g. in Debian there are meta-packages that have empty contents but bring many transitive dependencies once installed),

try see how to use a snapshot of vulnerability data in test so that you can be confident that detected vulnerabilities don't change.

I'm fine with any of these options.

Co-authored-by: Misha Sugakov <[email protected]>

vikin91 · 2023-01-04T11:40:22Z

I wanted to answer to this comment, but cannot find a way to do so :/

Well, the problem is that grep which currently serves as an example package without vulnerabilities may get vulnerabilities detected in the future. The comparison with 0 will start failing then.
We can either:

do nothing now and address the issue by changing the package when tests start breaking,

find a different package that does so little that it will not have vulnerabilities (e.g. in Debian there are meta-packages that have empty contents but bring many transitive dependencies once installed),

see how to use a snapshot of vulnerability data in test so that you can be confident that detected vulnerabilities don't change.
I'm fine with any of these options.

I acknowledge these options. While I tend to move on with the first option (keep as it is), I will add a comment stating that these tests may start failing in the future when new vulnerabilities are discovered for grep and the test would need to be adapted - see 5216a20.

Regarding the different package than grep - I am open for suggestions. Running rpm -qa did not return any virtual packages as known from Debian, but I see that people ask in the Internet how to build a virtual package on their own using rpm.

…uture

openshift-ci · 2023-01-04T13:07:37Z

@vikin91: 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	`d26da56`	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.

vikin91 · 2023-01-05T12:58:09Z

The e2e test failure is consistently appearing but unrelated to this PR:

TestGRPCGetImageVulnerabilities/quay.io/rhacs-eng/qa:ose-jenkins/jenkins-2-plugins/4.10.1650890594-1.el8.noarch
 Messages:   	Expected to find CVE-2022-30945 in scan results

jvdm

LGTM.

msugakov · 2023-01-06T11:19:23Z

Regarding the different package than grep - I am open for suggestions. Running rpm -qa did not return any virtual packages as known from Debian, but I see that people ask in the Internet how to build a virtual package on their own using rpm.

The answer was couple more attempts away (10 minutes).

$ docker run --rm -it --entrypoint=/bin/bash registry.access.redhat.com/ubi8/ubi:8.7
$ dnf install less
$ dnf search '*' | grep noarch | sort | less

The ones which seemed most innocent:

git-core-doc.noarch
golang-docs.noarch
iso-codes.noarch
tzdata.noarch

msugakov

Looks good to ship. Feel free to treat new comments as optional suggestions.

msugakov · 2023-01-06T11:32:08Z

e2etests/node_scan_rhcos_test.go

+
+	for _, c := range cases {
+		t.Run(c.name, func(t *testing.T) {
+			c := c


The page fails to call out explicitly is that this matters when you start goroutines (or the code that's called starts goroutines). We don't do start goroutines here and can get by without c := c.

msugakov · 2023-01-06T11:32:40Z

e2etests/node_scan_rhcos_test.go

@@ -136,16 +134,18 @@ func TestGRPCGetRHCOSNodeVulnerabilities(t *testing.T) {
 						Vulnerabilities: []*v1.Vulnerability{vulnTar},
 					},
 					{
-						Name:            "grep",
-						Version:         "3.1-6.el8.x86_64",
+						Name:    "grep",


See my .noarch suggestion #1004 (comment) . Optionally, up to you.

Unfortunately golang-docs.noarch returns a vulnerability (the same that can be found for golang-docs:1.19.2.noarch).

Error Trace: /Users/prygiels/go/src/github.com/stackrox/scanner/e2etests/node_scan_rhcos_test.go:192 Error: "[name:"CVE-2022-23806" description:"DOCUMENTATION: A flaw was found in the elliptic package of the crypto library in golang (...)" metadata_v2:<last_modified_date_time:"2022-10-24T00:00Z" cvss_v3:<vector:"CVSS:3.1/AV:N/AC:L/PR:N/UI:N/S:U/C:N/I:L/A:H" score:8.2 exploitability_score:3.9 impact_score:4.2 > > severity:"Moderate" ]" should have 0 item(s), but has 1 Test: TestGRPCGetRHCOSNodeVulnerabilities/Selected_vulnerabilities_should_be_returned_by_the_certified_scan Messages: Expected to find 0 vulnerabilities for feature 'golang-docs:.noarch'

It looks like the CVE is for golang and not for the golang-docs, but this is what our scanner returns 🤷

How about these?

git-core-doc.noarch iso-codes.noarch tzdata.noarch

Following up in https://srox.slack.com/archives/C04213W844U/p1673027151155069

openshift-ci bot added the do-not-merge/work-in-progress label Nov 4, 2022

vikin91 changed the title ~~Extend GetNodeVulnerabilities by supporting Node Inventory~~ ROX-12975: Extend GetNodeVulnerabilities API by supporting Node Inventory Nov 4, 2022

vikin91 force-pushed the pr/expand-GetNodeVulnerabilities-API branch 2 times, most recently from 61bf04c to 38050ee Compare November 21, 2022 13:02

vikin91 force-pushed the pr/expand-GetNodeVulnerabilities-API branch from f634efe to a778e72 Compare November 22, 2022 14:45

vikin91 marked this pull request as ready for review November 22, 2022 14:54

openshift-ci bot removed the do-not-merge/work-in-progress label Nov 22, 2022

vikin91 requested a review from RTann November 22, 2022 14:55

RTann reviewed Nov 22, 2022

View reviewed changes

vikin91 force-pushed the pr/expand-GetNodeVulnerabilities-API branch from 0af4f77 to 5952462 Compare November 22, 2022 17:00

vikin91 changed the title ~~ROX-12975: Extend GetNodeVulnerabilities API by supporting Node Inventory~~ ROX-13627: Extend GetNodeVulnerabilities API by supporting Node Inventory Nov 24, 2022

vikin91 mentioned this pull request Nov 30, 2022

ROX-13722: Move NodeInventory into Node stackrox/stackrox#3757

Closed

8 tasks

msugakov self-requested a review December 2, 2022 11:49

vikin91 added 14 commits January 2, 2023 13:15

Handle uncertifiedRHEL. Move hasUncertifiedRHEL to common pkg

45be13f

Add test case for uncertifiedRHEL scan

f6c9742

Apply renaming in e2e test

7c8416a

Fix expected result for uncertifiedRHEL scan

f75f7d3

Add testcase for feat with 0 vulnerabilities. Fix issues found on the…

e0a6003

… way

Add --fail to curl

0070416

Add assertion on number of features in the API response

de9c59c

Remove curl support

5fe7d4f

Correct leftovers

c1eee55

Extract ConvertFeatures to a separate pkg

687f46c

Add comment to meet style reqs

875c69d

Simplify the test assertions

e2e5d30

Add comments about spot-checking of vulns

fe94c74

Address review comments

6692db4

- Fix assertions when 0 vulns or 0 features are expected - Rename functions as suggested - Add comments when dummy values are used - Add comments in tents when expected & got params are switched in place (assert.Len)

vikin91 force-pushed the pr/expand-GetNodeVulnerabilities-API branch from 1e07c0e to 6692db4 Compare January 2, 2023 15:55

msugakov reviewed Jan 3, 2023

View reviewed changes

vikin91 and others added 2 commits January 4, 2023 12:03

Remove nil check in test code

fe869a5

Co-authored-by: Misha Sugakov <[email protected]>

Remove accidentally added blank lines

1e3bd9d

vikin91 added 2 commits January 4, 2023 12:45

Add comment to the grep test case stating that this may fail in the f…

5216a20

…uture

Use map for storing test cases

d26da56

vikin91 requested a review from daynewlee January 5, 2023 16:27

jvdm approved these changes Jan 5, 2023

View reviewed changes

msugakov approved these changes Jan 6, 2023

View reviewed changes

vikin91 merged commit 4672ccf into master Jan 6, 2023

vikin91 deleted the pr/expand-GetNodeVulnerabilities-API branch January 6, 2023 13:37

vikin91 mentioned this pull request Jan 10, 2023

Improve e2e testcase for RHCOS #1045

Merged

ROX-13627: Extend GetNodeVulnerabilities API by supporting Node Inventory #1004

ROX-13627: Extend GetNodeVulnerabilities API by supporting Node Inventory #1004

Conversation

vikin91 commented Nov 4, 2022 • edited Loading

How tested

openshift-ci bot commented Nov 4, 2022

vikin91 commented Nov 4, 2022

vikin91 commented Nov 4, 2022

vikin91 commented Nov 4, 2022

vikin91 commented Nov 4, 2022

ghost commented Nov 4, 2022 • edited by ghost Loading

vikin91 commented Nov 7, 2022

vikin91 commented Nov 21, 2022

vikin91 commented Nov 21, 2022

vikin91 commented Nov 21, 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

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

vikin91 Nov 22, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vikin91 commented Nov 22, 2022

vikin91 commented Nov 22, 2022

vikin91 commented Nov 23, 2022

vikin91 commented Nov 24, 2022

RTann commented Nov 29, 2022

vikin91 commented Nov 29, 2022

RTann commented Dec 1, 2022

Choose a reason for hiding this comment

vikin91 commented Jan 4, 2023 • edited Loading

openshift-ci bot commented Jan 4, 2023

vikin91 commented Jan 5, 2023

jvdm left a comment

Choose a reason for hiding this comment

msugakov commented Jan 6, 2023

msugakov 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

vikin91 commented Nov 4, 2022 •

edited

Loading

ghost commented Nov 4, 2022 •

edited by ghost

Loading

vikin91 Nov 22, 2022 •

edited

Loading

vikin91 commented Jan 4, 2023 •

edited

Loading