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

feat(vulnerabilities): add feature-flagged support for OSV #20226

Merged
merged 23 commits into from Feb 10, 2023

Conversation

Churro
Copy link
Collaborator

@Churro Churro commented Feb 4, 2023

Changes

Adds integration for vulnerability alerts based on data renovate finds in the osv.dev database. This PR continues the work @JamieMagee initiated in #15159 and further augments it by addressing previous TODOs, test coverage and support for a larger set of ecosystems and versioning schemes.

There are 3 building blocks realized in this PR:

  • Check if packages are affected by vulnerabilities
    • Done by implementing the evaluation algorithm, as included in the OSV schema
      • The intention here is also to make it easier maintainable in renovate in case of future changes
      • Side-note: The check for limit is missing on purpose, as it is only used with "versions" that are Git hashes
  • If vulnerable, find a version that remediates the vulnerability
  • Create a package rule that introduces version constraints and vulnerability information in PR notes
    • As OSV entries describe the severity only using the CVSS vector, vuln-vects is used to infer the CVSS base score.

Inline with #15159, the feature is opt-in and needs to be enabled using the osvVulnerabilityAlerts config flag.

Context

Documentation (please check one with an [x])

  • I have updated the documentation, or
  • No documentation update is required

How I've tested my work (please select one)

I have verified these changes via:

  • Code inspection only, or
  • Newly added/modified unit tests, or
  • No unit tests but ran on a real repository, or
  • Both unit tests + ran on a real repository

Test repos (split because GH enforces secondary rate limits when creating > 10 PRs):

Copy link
Collaborator

@secustor secustor left a comment

Choose a reason for hiding this comment

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

Really like to see this implemented.

lib/workers/repository/process/vulnerabilities.ts Outdated Show resolved Hide resolved
lib/workers/repository/process/vulnerabilities.ts Outdated Show resolved Hide resolved
lib/workers/repository/process/vulnerabilities.ts Outdated Show resolved Hide resolved
lib/workers/repository/process/vulnerabilities.ts Outdated Show resolved Hide resolved
Churro and others added 2 commits February 5, 2023 01:17
Co-authored-by: Sebastian Poxhofer <secustor@users.noreply.github.com>
Co-authored-by: Sebastian Poxhofer <secustor@users.noreply.github.com>
Churro and others added 7 commits February 5, 2023 12:10
Co-authored-by: Sebastian Poxhofer <secustor@users.noreply.github.com>
…ed on last_affected.

This is only relevant if a package rule is based on last_affected, e.g. "> 29.0", for regular fixed versions the list is already condensed to just one item at this point.

The version of the first entry here is compatible version-wise. It may hold a version suffix though which could be incompatible, e.g. 30.0-android, whereas 30.0-jre would be correct.
@JamieMagee
Copy link
Contributor

Thanks for getting this across the line 🎉 Can't wait to see this in Renovate.

docs/usage/configuration-options.md Outdated Show resolved Hide resolved
docs/usage/configuration-options.md Outdated Show resolved Hide resolved
docs/usage/configuration-options.md Outdated Show resolved Hide resolved
docs/usage/configuration-options.md Outdated Show resolved Hide resolved
Churro and others added 6 commits February 6, 2023 11:57
Co-authored-by: HonkingGoose <34918129+HonkingGoose@users.noreply.github.com>
Co-authored-by: HonkingGoose <34918129+HonkingGoose@users.noreply.github.com>
Co-authored-by: HonkingGoose <34918129+HonkingGoose@users.noreply.github.com>
Co-authored-by: HonkingGoose <34918129+HonkingGoose@users.noreply.github.com>
Copy link
Member

@viceice viceice left a comment

Choose a reason for hiding this comment

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

otherwise LGTM.

@Churro Churro requested a review from viceice February 9, 2023 19:52
@secustor secustor enabled auto-merge (squash) February 10, 2023 09:51
@secustor secustor merged commit a91ca62 into renovatebot:main Feb 10, 2023
@renovate-release
Copy link
Collaborator

🎉 This PR is included in version 34.129.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@icanhazstring
Copy link

icanhazstring commented Feb 14, 2023

@JamieMagee @Churro

You mentioned testing it with composer.
The documentation however states only the following:

crate
go
hex
maven
npm
nuget
packagist
pypi
rubygems

So, is it even getting information for composer now?

@viceice
Copy link
Member

viceice commented Feb 14, 2023

packagist is the composer datasource 😉

@icanhazstring
Copy link

packagist is the composer datasource 😉

Haha lol. Yes ofc. I totally overlooked that one. Nevermind. Maybe my brain was not functioning well at the time 😂🙈

@HonkingGoose
Copy link
Collaborator

@rarkins and @JamieMagee

The docs for osvVulnerabilityAlerts has this admonition:

This feature is flagged as experimental
Experimental features might be changed or even removed at any time.
To track this feature visit the following GitHub issue #6562.

The admonition links to this issue, which we resolved. The source for the admonition:

{
name: 'osvVulnerabilityAlerts',
description: 'Use vulnerability alerts from `osv.dev`.',
type: 'boolean',
default: false,
experimental: true,
experimentalIssues: [6562],
},

Should we drop these lines?

-    experimental: true,
-    experimentalIssues: [6562],

@JamieMagee
Copy link
Contributor

@HonkingGoose I'd still consider the feature experimental for a little while, and would like to keep the feature flag. What about opening a new issue to centralise feedback collection?

@icanhazstring
Copy link

@JamieMagee If you mean "feature flag". Does it mean it has to be set to true while vulnerabilityAlerts have to be enabled as well?

Because I can't figure that out from the docs.

@JamieMagee
Copy link
Contributor

@icanhazstring this feature only requires osvVulnerabilityAlerts to be set to true. The vulnerabilityAlerts option controls GitHub-only vulnerability alerts. I expect we'll merge them both under the vulnerabilityAlerts option in the near future.

@HonkingGoose
Copy link
Collaborator

@JamieMagee If you mean "feature flag". Does it mean it has to be set to true while vulnerabilityAlerts have to be enabled as well?

The "feature flag" comment means this experimental: true flag in our code:

 { 
   name: 'osvVulnerabilityAlerts', 
   description: 'Use vulnerability alerts from `osv.dev`.', 
   type: 'boolean', 
   default: false, 
   experimental: true, // <--- This is the "feature flag"
   experimentalIssues: [6562],
 }, 

We look for the experimental: true flag in our docs generating code to generate the warning message that the feature is "flagged as experimental". This way the code and the docs always match.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 19, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants