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

Auditing projects for package vulnerabilities during restore #12310

Merged
merged 15 commits into from
Mar 16, 2023

Conversation

JonDouglas
Copy link
Contributor

Introduction to an auditing experience at package restore time where known vulnerabilities will be reported.

This proposal introduces new MSBuild warnings and output to the NuGet restore operation.

Rendered Spec

Please 👍 or 👎 this comment to help us with the direction of this feature & leave as much feedback/questions/concerns as you'd like on this issue itself and we will get back to you shortly.

Thank You 🎉

@JonDouglas JonDouglas requested a review from a team as a code owner December 7, 2022 19:31
@mattzink
Copy link

mattzink commented Dec 7, 2022

We use Nexus repo manager and Artifactory as local Nuget.org package mirrors, so it would be great if there was a way in the nuget.config file to specify that Nuget.org is used only for vulnerability lookup, not for package hosting.

| NU1904 | critical |

```bash
Found 2 vulnerabilities (0 low, 1 moderate, 0 high, 1 critical) in 2 package(s)
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the value in reporting severities with 0 hits? Would it be better to exclude those instead?

Suggested change
Found 2 vulnerabilities (0 low, 1 moderate, 0 high, 1 critical) in 2 package(s)
Found 2 vulnerabilities (1 moderate, 1 critical) in 2 package(s)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea, let's keep this open for discussion!

Co-authored-by: Loïc Sharma <737941+loic-sharma@users.noreply.github.com>
@CamiloTerevinto
Copy link

This sounds very interesting, thanks @JonDouglas. I have a few comments:

  • CLI switches to run/skip auditing

It's quite common for an entire build to be run twice (feature branch and dev/main branch) for a change in environments with CI/CD. I think that for this to be useful, we should have an easy way of telling dotnet restore to run or skip the audit (for example, run the audit on the PR build, skip it in the dev build).

  • Fail on X level

It would be useful to have a way to make dotnet restore exit as failure when a package with a particular vulnerability level is found (such as treating NU1904 as error instead of warning).
However, this might be doable already through something like <WarningsAsErrors>NU1904</WarningsAsErrors>.

@nkolev92
Copy link
Member

CLI switches to run/skip auditing

Any CLI switch basically doubles as an MSBuild property. We're proposing MSBuild properties for opting into auditing, so that automatically makes it available as a CLI switch. Example: dotnet restore -p:NuGetAudit=disable


It is **very important* that if the upstream sources link to the nuget.org vulnerability, the actual nuget.org urls are used wherever possible, as this would allow the client to easily deduplicate the vulnerability information.

### Command Line Restore output
Copy link
Member

Choose a reason for hiding this comment

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

@JonDouglas FYI, I moved the vulnerability summary output on command line restores under "Future Possibilities" for two reasons. 1. So we can get a V1 of this feature out sooner. 2. I don't believe this proposal has been specced out well. I have a bunch of questions if we want to implement it.

For example, in the sample output, if there are non-vulerability warnings or errors, where in the output do they go? Before the vulnerability summary, or below with the sample vulerability warnings? If before, should vulerability warnings/errors be output twice, once between the summary once after?

What's the expected order of output when restoring a solution with many projects? Wait until all projects finish restoring, and then output all their summaries? Or output project summaries as each project finishes restoring, even if it means that the vulnerability summary for that project will be somewhere much earlier in the restore log?

When restoring a solution where the same package version is in multiple projects, should we de-duplicate the output?

Anyway, as I said, I think we can take this on in the future if we still think it's useful. Let's get a V1 of the feature done without it, and see how we and customers feel about it.

Copy link
Member

Choose a reason for hiding this comment

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

I think some details, like the ones about the handling of multiple vulnerability for a package can still be in the main part as that's likely going to be done.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let's see what this shapes to be while working to answer the unknown. Thanks for being so thorough with questions to be answered.

Copy link
Member

@nkolev92 nkolev92 left a comment

Choose a reason for hiding this comment

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

Great updates @zivkan

proposed/2022/vulnerabilities-in-restore.md Show resolved Hide resolved
proposed/2022/vulnerabilities-in-restore.md Outdated Show resolved Hide resolved

It is **very important* that if the upstream sources link to the nuget.org vulnerability, the actual nuget.org urls are used wherever possible, as this would allow the client to easily deduplicate the vulnerability information.

### Command Line Restore output
Copy link
Member

Choose a reason for hiding this comment

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

I think some details, like the ones about the handling of multiple vulnerability for a package can still be in the main part as that's likely going to be done.

proposed/2022/vulnerabilities-in-restore.md Outdated Show resolved Hide resolved
@JonDouglas
Copy link
Contributor Author

We have approval from @nkolev92, but @zivkan did you want to approve before merge?

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.

7 participants