Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(ci): Multiple data and test issues #1737

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

fix(ci): Multiple data and test issues #1737

wants to merge 3 commits into from

Conversation

dcaravel
Copy link
Contributor

@dcaravel dcaravel commented Dec 10, 2024

Description

Fixes multiple CI issues

  • Removes CVSSv3 metadata from various test cases for Microsoft images that no longer have data avail in the NVD legacy feeds.
  • Adjusts sanity test logic to skip validating CVSSv3 fields when not provided by test case.
  • Increase test timeout to avoid panic: test timed out after 20m0s (such as this one).
  • Removes CVE-2021-26291 from the ose-jenkins image test case, it is no longer appearing in scan results (ref)
$ rctl image scan -f --image=quay.io/rhacs-eng/qa:ose-jenkins 2>/dev/null | jq -r '.scan.components[] | select(.name == "jenkins-2-plugins").vulns[].cve' 
RHSA-2022:6531
RHSA-2022:7865
RHSA-2022:9098
RHSA-2023:0560
RHSA-2023:0697
RHSA-2023:1655
RHSA-2023:1866
RHSA-2023:3362
RHSA-2023:3625

Testing

CI

Logs from CI showing CVSSv3 comparison skips.

=== RUN   TestImageSanity/mcr.microsoft.com/dotnet/core/runtime:3.1.2
=== RUN   TestImageSanity/mcr.microsoft.com/dotnet/core/runtime:3.1.2/microsoft.netcore.app/3.1.2
    sanity_test.go:40: WARN: No CVSSv3 data provided for "CVE-2021-1721", skipping CVSSv3 field validation.
    sanity_test.go:40: WARN: No CVSSv3 data provided for "CVE-2021-1723", skipping CVSSv3 field validation.
    sanity_test.go:40: WARN: No CVSSv3 data provided for "CVE-2021-24112", skipping CVSSv3 field validation.
    sanity_test.go:40: WARN: No CVSSv3 data provided for "CVE-2021-26701", skipping CVSSv3 field validation.
    sanity_test.go:40: WARN: No CVSSv3 data provided for "CVE-2021-31204", skipping CVSSv3 field validation.
=== RUN   TestImageSanity/mcr.microsoft.com/dotnet/core/sdk:3.1.100@sha256:091126a93870729f4438ee7ed682ed98639a89acebed40409af90f84302c48dd
=== RUN   TestImageSanity/mcr.microsoft.com/dotnet/core/sdk:3.1.100@sha256:091126a93870729f4438ee7ed682ed98639a89acebed40409af90f84302c48dd/microsoft.aspnetcore.app/3.1.0
    sanity_test.go:40: WARN: No CVSSv3 data provided for "CVE-2020-1045", skipping CVSSv3 field validation.
    sanity_test.go:40: WARN: No CVSSv3 data provided for "CVE-2021-1723", skipping CVSSv3 field validation.
=== RUN   TestImageSanity/mcr.microsoft.com/dotnet/core/sdk:3.1.100@sha256:091126a93870729f4438ee7ed682ed98639a89acebed40409af90f84302c48dd/microsoft.netcore.app/3.1.0
    sanity_test.go:40: WARN: No CVSSv3 data provided for "CVE-2021-1721", skipping CVSSv3 field validation.
    sanity_test.go:40: WARN: No CVSSv3 data provided for "CVE-2021-1723", skipping CVSSv3 field validation.
    sanity_test.go:40: WARN: No CVSSv3 data provided for "CVE-2021-24112", skipping CVSSv3 field validation.
    sanity_test.go:40: WARN: No CVSSv3 data provided for "CVE-2021-26701", skipping CVSSv3 field validation.
    sanity_test.go:40: WARN: No CVSSv3 data provided for "CVE-2021-31204", skipping CVSSv3 field validation.

@dcaravel dcaravel changed the title [WIP DO NOT MERGE] chore(ci): Fix CI due to NVD feed changes chore(ci): Fix CI due to NVD feed changes Dec 10, 2024
@dcaravel dcaravel changed the title chore(ci): Fix CI due to NVD feed changes chore(ci): Fix CI due to NVD legacy feed changes Dec 10, 2024
@dcaravel dcaravel changed the title chore(ci): Fix CI due to NVD legacy feed changes fix(ci): NVD legacy feed changes broke CI Dec 10, 2024
@dcaravel
Copy link
Contributor Author

/test e2e-tests

@dcaravel
Copy link
Contributor Author

dcaravel commented Dec 11, 2024

Last failure appears to be a flake:

=== RUN   TestStackroxVulnImages/quay.io/rhacs-eng/qa:appdynamics
Handling connection for 8443
    utils.go:49: 
        	Error Trace:	/go/src/github.com/stackrox/scanner/e2etests/utils.go:49
        	            				/go/src/github.com/stackrox/scanner/e2etests/utils.go:65
        	            				/go/src/github.com/stackrox/scanner/e2etests/vuln_test.go:39
        	            				/go/src/github.com/stackrox/scanner/e2etests/vuln_test.go:265
        	Error:      	Received unexpected error:
        	            	rpc error: code = Unknown desc = could not advance in the tar archive: archive/tar: invalid tar header
        	Test:       	TestStackroxVulnImages/quay.io/rhacs-eng/qa:appdynamics

@dcaravel dcaravel requested review from a team, jvdm, BradLugo, daynewlee and RTann December 11, 2024 02:16
@dcaravel dcaravel changed the title fix(ci): NVD legacy feed changes broke CI fix(ci): Multiple data and test issues Dec 11, 2024
Comment on lines +30 to +31
// When expected vuln has no CVSSv3 data, do not try to compare it.
// This was added when NVD stopped returning CVSSv3 data for some vulns which we had test cases for.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you confirm your intention is to continue ignoring V3 if/when NVD bring it back?

Copy link
Contributor Author

@dcaravel dcaravel Dec 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good Q. Main intention was to get CI green again 😉 . Given that this scanner is on 'life support' I didn't spend too much time considering far into the future.

As it's currently written it would indeed continue ignoring V3 if/when NVD brings it back. I did consider reversing the condition such that if the NVD data has V3 then do the compare, otherwise ignore - I changed that assuming we would want CI failures when the data we're expecting to exist goes away so that we could investigate further (like we did with these).

I suppose we could change the condition to fail the test if NVD V3 data was provided but our test case isn't comparing it so that if/when the data comes back we'll have failures again.

Any preference?

@dcaravel dcaravel requested a review from jvdm December 11, 2024 22:35
"Score": 6.5,
"Vectors": "CVSS:3.1/AV:N/AC:L/PR:N/UI:R/S:U/C:N/I:N/A:H",
},
// NVD stopped returning this
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you create a ticket to track this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I may instead change the sanity test to fail if this data comes back so that we will not need a ticket and can rely on CI failures instead.

@@ -3887,31 +3899,6 @@ Applications using RegexRequestMatcher with '.' in the regular expression are po
AddedBy: "sha256:3fa3f612bdcb92746bf76be1b9c9e1c1c80de777aedaf48b7068f4a129ded3c2",
FixedBy: "4.10.1685679861-1.el8",
Vulnerabilities: []apiV1.Vulnerability{
{
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You know I have seen this flap sometimes, and I have yet to dig into why this happens. Any ideas? I recall this was a key vulnerability we wanted to track because it's an unfixed OpenShift-related vulnerability which we did not track for some time. It'd be nice to keep this, but since we are focused more on Scanner V4, I'm ok with commenting this out with a ticket to go with it to track it

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hrm, interesting, I'll see if I have an old copy of the data for comparing to see what changed - I did check and none of the RHSA's listed in the current scan results for this image include this particular CVE.

I did notice CI uses a new 'genesis dump' each run - and if I remember correctly scanner currently does not handle 'deleted' vulns when processing a diff. So this would only appear in scanners that include a new genesis dump as of whatever date when this vuln was seemingly removed from the feeds.

}

// When expected vuln has no CVSSv3 data, do not try to compare it.
// This was added when NVD stopped returning CVSSv3 data for some vulns which we had test cases for.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe another ticket here while we're at it :D

Copy link
Contributor Author

@dcaravel dcaravel Dec 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, what would the ticket be for? Rechecking the vulns to see if NVD added the data back?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants