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

⚠️ OSV scanner integration #2509

Merged
merged 7 commits into from
Dec 13, 2022
Merged

Conversation

another-rex
Copy link
Contributor

@another-rex another-rex commented Dec 2, 2022

What kind of change does this PR introduce?

Feature

What is the current behavior?

Currently the vulnerability check only checks if the HEAD commit hash has any vulnerability specified in OSV.dev. Most of the time this will return 10/10.

What is the new behavior (if this is a feature change)?**

This integrates the OSV-Scanner library, which will scan for manifest and SBOM files, retrieve the dependencies, and match those dependencies with OSV.dev database as well. (The commit hash check is still being done, though through the osvscanner library rather than directly setting up a client to use the OSV API).

  • Tests for the changes have been added (for bug fixes/features)

Which issue(s) this PR fixes

Fixes #2162

Special notes for your reviewer

Currently the e2e tests fail because the Scorecard score has changed since osvscanner found 3 vulnerabilities. I can change the expected results to match, but this might cause more issues in the future as new vulnerabilities are discovered, causing tests to break even though there are no code changes. Not sure what the best way to resolve this.

Does this PR introduce a user-facing change?

Yes, TODO.

TODO!

Copy link
Contributor

@laurentsimon laurentsimon left a comment

Choose a reason for hiding this comment

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

Thanks for this PR. I left one major comment about keeping clients.Vulnerabilities field.

checker/raw_result.go Outdated Show resolved Hide resolved
@azeemshaikh38
Copy link
Contributor

a challenge to adding the unit tests for this is /that it will be picked up as a vulnerability, even though it's part of the test data in the repository. Any ideas for a quick fix? Eventually we want to have project level ignore files for osv-scanner.

unit tests should be hermetic and should not be making any network calls. You can use the mocks generated for the interfaces to write any unit tests and it shouldn't affect any vulnerability finding.

checker/client.go Show resolved Hide resolved
clients/repo_client.go Show resolved Hide resolved
@another-rex
Copy link
Contributor Author

unit tests should be hermetic and should not be making any network calls. You can use the mocks generated for the interfaces to write any unit tests and it shouldn't affect any vulnerability finding.

That makes sense, what about e2e tests? It looks like a some repositories are set up in ossf-tests, can I set up a project with some lockfiles that contain known vulnerable dependencies?

@azeemshaikh38
Copy link
Contributor

unit tests should be hermetic and should not be making any network calls. You can use the mocks generated for the interfaces to write any unit tests and it shouldn't affect any vulnerability finding.

That makes sense, what about e2e tests? It looks like a some repositories are set up in ossf-tests, can I set up a project with some lockfiles that contain known vulnerable dependencies?

Yes that's possible. Curious, does OSV-Scanner look at direct dependencies only or transitive dependencies too?

@laurentsimon
Copy link
Contributor

unit tests should be hermetic and should not be making any network calls. You can use the mocks generated for the interfaces to write any unit tests and it shouldn't affect any vulnerability finding.

That makes sense, what about e2e tests? It looks like a some repositories are set up in ossf-tests, can I set up a project with some lockfiles that contain known vulnerable dependencies?

Yes that's possible. Curious, does OSV-Scanner look at direct dependencies only or transitive dependencies too?

https://github.com/ossf-tests/scorecard-check-osv-e2e

Send a PR and cc me on it - I don't receive notifications automatically for these repos

@codecov
Copy link

codecov bot commented Dec 6, 2022

Codecov Report

Merging #2509 (cd4440e) into main (7206a2b) will decrease coverage by 0.73%.
The diff coverage is 34.61%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2509      +/-   ##
==========================================
- Coverage   40.69%   39.95%   -0.74%     
==========================================
  Files         115      122       +7     
  Lines        9609     9815     +206     
==========================================
+ Hits         3910     3922      +12     
- Misses       5419     5612     +193     
- Partials      280      281       +1     

@inferno-chromium
Copy link
Contributor

@another-rex - Please try to finish this soon, so a scorecard release can be cut by end of week or coming Monday.

@another-rex
Copy link
Contributor Author

unit tests should be hermetic and should not be making any network calls. You can use the mocks generated for the interfaces to write any unit tests and it shouldn't affect any vulnerability finding.

That makes sense, what about e2e tests? It looks like a some repositories are set up in ossf-tests, can I set up a project with some lockfiles that contain known vulnerable dependencies?

Yes that's possible. Curious, does OSV-Scanner look at direct dependencies only or transitive dependencies too?

OSV-Scanner also looks at transitive dependencies, since it scans the lockfile, which generally lists the transitive dependencies.

@naveensrinivasan
Copy link
Member

@another-rex - Please try to finish this soon, so a scorecard release can be cut by end of week or coming Monday.

Is there a specific reason we want to cut a release?

@inferno-chromium
Copy link
Contributor

@another-rex - Please try to finish this soon, so a scorecard release can be cut by end of week or coming Monday.

Is there a specific reason we want to cut a release?

I mean this is a very strong feature where scorecard was not looking at transitive deps before and can now can look at vulns transitively. By release, i just meant a minor release (not major announcement or anything for scorecard). OSV-scanner will go out next week, so it will be nice for people to try in scorecard too, not a big deal if you need more review time.

@naveensrinivasan
Copy link
Member

@another-rex - Please try to finish this soon, so a scorecard release can be cut by end of week or coming Monday.

Is there a specific reason we want to cut a release?

I mean this is a very strong feature where scorecard was not looking at transitive deps before and can now can look at vulns transitively. By release, i just meant a minor release (not major announcement or anything for scorecard). OSV-scanner will go out next week, so it will be nice for people to try in scorecard too, not a big deal if you need more review time.

Yes, that makes sense! 👍

Thanks for the update. We need to get better at releases. To be honest, we need to improve our release process by calling out new features and breaking changes in the release notes.

@another-rex another-rex temporarily deployed to integration-test December 9, 2022 04:23 — with GitHub Actions Inactive
@github-actions
Copy link

github-actions bot commented Dec 9, 2022

Integration tests success for
[f405955]
(https://github.com/ossf/scorecard/actions/runs/3654448698)

@another-rex another-rex temporarily deployed to integration-test December 11, 2022 21:46 — with GitHub Actions Inactive
@github-actions
Copy link

Integration tests success for
[578624d]
(https://github.com/ossf/scorecard/actions/runs/3670958619)

@naveensrinivasan
Copy link
Member

Integration tests success for
[f405955]
(https://github.com/ossf/scorecard/actions/runs/3654448698)

Can you please rebase with the main? Also, DCO is missing. Thanks

@another-rex another-rex temporarily deployed to integration-test December 12, 2022 02:29 — with GitHub Actions Inactive
@github-actions
Copy link

Integration tests success for
[61e4be7]
(https://github.com/ossf/scorecard/actions/runs/3672092646)

docs/checks/internal/checks.yaml Outdated Show resolved Hide resolved
docs/checks/internal/checks.yaml Outdated Show resolved Hide resolved
docs/checks/internal/checks.yaml Outdated Show resolved Hide resolved
clients/osv.go Outdated Show resolved Hide resolved
docs/checks.md Outdated Show resolved Hide resolved
@another-rex another-rex temporarily deployed to integration-test December 12, 2022 21:49 — with GitHub Actions Inactive
@another-rex another-rex temporarily deployed to integration-test December 12, 2022 21:51 — with GitHub Actions Inactive
@github-actions
Copy link

Integration tests success for
[981b438]
(https://github.com/ossf/scorecard/actions/runs/3680087135)

@github-actions
Copy link

Integration tests success for
[3e71603]
(https://github.com/ossf/scorecard/actions/runs/3680095638)

@laurentsimon laurentsimon enabled auto-merge (squash) December 12, 2022 22:20
auto-merge was automatically disabled December 13, 2022 00:23

Head branch was pushed to by a user without write access

@another-rex another-rex temporarily deployed to integration-test December 13, 2022 00:23 — with GitHub Actions Inactive
@github-actions
Copy link

Integration tests success for
[cd4440e]
(https://github.com/ossf/scorecard/actions/runs/3681023522)

@laurentsimon laurentsimon enabled auto-merge (squash) December 13, 2022 00:36
@laurentsimon laurentsimon self-requested a review December 13, 2022 00:46
@laurentsimon laurentsimon merged commit f983480 into ossf:main Dec 13, 2022
raghavkaul pushed a commit to raghavkaul/scorecard that referenced this pull request Feb 9, 2023
* Improve OSV scanning integration (squashed)

Signed-off-by: Rex P <[email protected]>

* Add support for grouping vulnerabilities and aliases

Signed-off-by: Rex P <[email protected]>

* Updated documentation, spit vulnerability output to multiple warnings

Signed-off-by: Rex P <[email protected]>

* Updated documentation, spit vulnerability output to multiple warnings

Signed-off-by: Rex P <[email protected]>

* Add its own codebase into docs

Signed-off-by: Rex P <[email protected]>

* Update scorecard test to not prevent known vulns

Signed-off-by: Rex P <[email protected]>

Signed-off-by: Rex P <[email protected]>
Co-authored-by: laurentsimon <[email protected]>
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.

Improve Score Reporting: OSV coverage is limited
6 participants