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.

Sign up for GitHub

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

Update vulnerabilities-in-restore.md #12611

Merged
merged 3 commits into from
Jul 7, 2023
Merged

Update vulnerabilities-in-restore.md #12611

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2a1cbe9
Update vulnerabilities-in-restore.md
JonDouglas May 22, 2023
d232c68
Update vulnerabilities-in-restore.md
JonDouglas May 25, 2023
4e981b1
Update vulnerabilities-in-restore.md
JonDouglas Jun 27, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 19 additions & 0 deletions proposed/2022/vulnerabilities-in-restore.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,14 @@ This feature will be opt-in to start and gather feedback from developers.

To enable the feature, a developer can add `<NuGetAudit>enable</NuGetAudit>` to their project file as a MSBuild property. To disable the feature, a developer can add `<NuGetAudit>disable</NuGetAudit>` or remove the property from the project file.

#### Setting Vulnerability Auditing Modes

There will be different modes to audit vulnerabilities based on the developer's or developer's team preference. To do this, a developer will opt-in to a feature called `<NuGetAuditMode>` which will have different modes such as `direct`, and `all`.

These modes should be pretty straight-forward. `direct` will scan for any top-level vulnerabilities, and `all` will scan for both top-level and transitive-level vulnerabilities. The default will be `direct` until the experience is ready to be `all` given that transitive vulnerabilities are the majority of vulnerability notices (90%+).
Copy link
Contributor

Choose a reason for hiding this comment

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

The default will be direct until the experience is ready to be all

While I agree with this direction, it would be good to specify here what this "readiness' means. What is the decision criteria we would follow to switch the experience to "all" be default?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added some bullets in future possibilities to help answer

Copy link
Contributor

Choose a reason for hiding this comment

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

What are your thoughts on adding a message to the output along the lines of: "Only direct dependencies were scanned for vulnerabilities. To enable scanning transitive dependencies as well, turn on all."

Copy link
Member

Choose a reason for hiding this comment

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

Some customers are actively hostile against any non-actionable message that can't also be suprressed. If we has Roslyn analyzer style .editorconfig seveity levels on codes, then we could allow customers to customize their builds as they like. But currently we only have NU codes for warnings and errors, informational messages don't have codes and therefore can't be suppressed.

So, it's a tradeoff, especially in large solutions where the message could be shown hundreds of times, once for each solution.

One problem with moving fast, is that we don't have time to carefully plan, or test, or validate, before we have to ship. So, my recommendation for now is no, don't have a message, and we can wait for customer feedback while working on other improvements, and if we don't get feedback, we can do a bit of our own testing to estimate how important it is.


When a known vulnerability is found that is of the transitive level, it will include the path to the project containing the top-level package and including the name and version of the package the vulnerable transitive dependency is coming from. Transitive level known vulnerabilities should not be a warning, but rather a message/informational MSBuild severity as they should not break builds but still be brought up in the Error List as informational.
Copy link
Contributor

Choose a reason for hiding this comment

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

The text here can be understood in different ways, so consider clarifying,
Are transitive dependencies shown as message / informational even when NuGetAuditMode is set to "all"?
If NuGetAuditMode is set to "direct", will we still show transitive dependency vulnerabilities as message/informational, or we won't show them at all?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

direct -> direct dependencies

all -> direct & transitive

We will not show output for transitive dependencies unless the user sets their mode appropriately.

Copy link
Member

Choose a reason for hiding this comment

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

If there are more paths, are we gonna include them all? I think we should, as there's many interesting resolution things that can happen.


#### Setting an Audit Level

In cases where a developer only cares about a certain threshold of advisory severity, they can set a MSBuild property to set a level such as `<NuGetAuditLevel>moderate</NuGetAuditLevel>` in which auditing will fail. Possible values match the OSV format of `low`, `moderate`, `high`, and `critical`.
Expand Down Expand Up @@ -296,6 +304,17 @@ However, it is expected that such projects will have a CI build which will perfo

- Vulnerability scanning can be extended to SBOMs.
- Support can be added to automatically fix vulnerable dependencies (i.e. a fix experience in CLI / Tooling)
- Consideration of SDK/Framework scanning for implicit PackageReference that may be vulnernable.
- Readiness to enable `<NuGetAuditMode>` to `all` for .NET/VS vNext:
- Customer feedback from .NET 8.
- Satisfaction of direct dependency scanning.
- Noise ratio of transitive dependency scanning (i.e. new warnings)
- Performance/scalability impact of transitive dependency scanning.
- Version resolution to ensure proper vulnerability reporting.
- UI/UX considerations for distinguishing direct/transitive vulnerability warnings.
- Incremental scanning/caching to avoid redundant scans.
- Documentation and education resources for the functionality.
- Prioritization and suppression of severity / advisories.

Additionally, most of the [`Rationale and alternatives`](#rationale-and-alternatives) are really future possibilities on their own as they are not always exclusive to the current approach. Here's some further possibilities:

Expand Down