-
Notifications
You must be signed in to change notification settings - Fork 258
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
dotnet audit & dotnet audit fix for NuGet packages. #11549
base: dev
Are you sure you want to change the base?
Conversation
|
||
#### Vulnerabilities | ||
|
||
When vulnerable packages are detected, an error is thrown by default. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you explain why an error is thrown instead of a warning for vulnerabilities?
A reason I ask is because vulnerabilities in the Javascript ecosystem often noise or very low impact.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll add more rationale in the proposal. I believe with our lower amount of known vulnerabilities, high importance of the types of vulnerabilities issued for .NET, and the nature of shifting left, these should be treated as errors for now.
"legacy": "1", | ||
"critical-bugs": "0", | ||
"other": "0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we remove these fields? From my understanding, you can get the same information by inspecting the dependencies
's reason
property`.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think so. We want to use these as high level counts for the known reason category per dependency. Is there a way we can associate a number with the reason field?
> anurse.testing.TestDeprecatedPackage 1.0.0 2.0.0 | ||
> UmbracoForms 8.4.1 8.7.1 | ||
Found 2 top-level outdated package(s) in 36 scanned packages. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would this report transitive packages that are outdated? Would users have to hoist outdated transitive dependencies to update them to the latest version? That may be a frustrating experience for customers.
Perhaps we should only report outdated top-level dependencies. If so, consider adding a note on that limitation somewhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It currently follows the outdated experience which doesn't include transitives. I'm also not so sure what value seeing outdated transitives would have unless one wanted to promote those explicitly. Will have to circle back here.
"name": "UmbracoForms", | ||
"requestedVersion": "8.4.1", | ||
"resolvedVersion": "8.4.1", | ||
"vulnerabilitySeverity": "Moderate", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Line 109 uses property name severity
. Should this line align to that?
"vulnerabilitySeverity": "Moderate", | |
"severity": "Moderate", |
"low": "0", | ||
"moderate": "2", | ||
"high": "0", | ||
"critical": "0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we remove these fields? From my understand you can get the same information by inspecting the severity
properties?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are to aggregate the number from the audit. If there's another way to do that, we can. Wanted to give a summary of json at the top so it's clear where attention should be spent and not looking at each node.
"resolvedVersion": "8.4.1", | ||
"vulnerabilitySeverity": "Moderate", | ||
"advisoryUrl": "https://github.com/advisories/GHSA-8m73-w2r2-6xxj", | ||
"transitiveDependencies": [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What would this look like if a transitive dependency has a vulnerability but all top-level dependencies are OK?
- Should this command only audit `vulnerabilities`? Or should it audit & be proactive as to `vulnerabilities`, `deprecations`, and `outdated` packages? | ||
- How much information should be present in the --json output? | ||
|
||
## Future Possibilities |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we make dotnet pack
warn if one of your dependencies have a version range that accepts a vulnerability?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah if pack can resolve at the time of packing we should definitely do that.
## Future Possibilities | ||
|
||
<!-- What future possibilities can you think of that this proposal would help with? --> | ||
- `dotnet audit` can be run on every `restore` which can throw warnings or errors to the user to take action against a vulnerable, deprecated, or outdated software supply chain. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could customer suppress warnings/errors? It could be low impact noise. Also, even package has vulnerability and there is no alternative then customer stuck with it and took other mitigation steps (isolate network from outside world), then keep throwing error could discourage further usage.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I think we can figure out something. I am not so sure what arguments that might be right now as this experience will require an internet connection or a local cache. It might be something like --offline
, --level high
, or --features vulnerability
.
- [Vulnerability](https://docs.microsoft.com/en-us/nuget/api/registration-base-url-resource#vulnerabilities) | ||
- Outdated - No existing endpoint, will need to call a source. | ||
|
||
##### dotnet audit --json |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we're doing this, should it match the dotnet list package design?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, given that design is still open and being iterated on, whatever comes from it should be reflected here for consistency and used.
``` | ||
$ dotnet audit | ||
Fetching package metadata from: 'https://api.nuget.org/v3/index.json' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the command should consider package source mapping configuration
if available to decide which sources to fetch metadata from.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will other feeds such as AzDO
have this vulnerabilities, deprecations, or outdated information?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not believe they support these features. The MVP is designed around the central registry.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think the extra comment about the packages helps tbh.
We should have just have a a Sources used
statement and capture everything.
|
||
``` | ||
dotnet audit fix --help | ||
dotnet audit fix [<PROJECT>|<SOLUTION>|<Directory.Packages.props>] [-v|--verbosity <LEVEL>] [--dry-run] [--json] [--interactive] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider the following scenario.
- Customer runs
dotnet audit --json
- Few violations are reported
- Customer runs
dotnet audit fix
command
This may be an implementation detail how dotnet audit fix
command works but it could be great to add an option to optionally pass --audited-json
file as input to the fix
sub command so that it can act on those findings instead of finding the problems again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The dotnet audit fix
command does an implicit dotnet audit
. Thus I don't think this is needed. Perhaps we can do some caching to make it faster, but we should probably call out to the net often to ensure nothing changed.
|
||
#### dotnet audit fix | ||
|
||
The `dotnet audit fix` command will provide a remediation that is calculated with an implicit `dotnet audit` to then apply directly to a resulting package graph. It can add packages, remove packages, and update packages depending on the problem it's attempting to resolve. It does not take into consideration downgrading to a compatible version if a higher one has already been specified. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume it fails if there is no alternative, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If there's no alternative found, the tool would likely throw a manual review for the user to remove the dependency or ignore it in the future.
- Should the command be named `audit` or `check`? | ||
- `audit` is the more consistent name for package manager tooling with other ecosystems. | ||
- `check` is the more consistent name with dotnet CLI. | ||
- Should this command only audit `vulnerabilities`? Or should it audit & be proactive as to `vulnerabilities`, `deprecations`, and `outdated` packages? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think vulnerabilities and deprecations should be accounted for.
Outdated packages is the one that gives me a pause (at least by default), due to the fact updating packages to the latest is not necessarily always the right solution.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense. I'd like to see what other thoughts are about this 3-in-1 tool and then can iterate further.
## Summary | ||
|
||
<!-- One-paragraph description of the proposal. --> | ||
`dotnet audit` & `dotnet audit fix` helps you find, fix, and monitor known security vulnerabilities, deprecated packages, and outdated versions in your .NET projects & solutions. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It may be crazy but finding unused packages would be awesome.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's some a community project that does this: https://github.com/spectresystems/snitch
FYI this is a hard problem since a package may be used through reflection. But .NET trimming effort aligns well with this idea.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- 0 - The command will exit with a 0 exit code if no vulnerabilities, deprecations, or outdated packages were found. | ||
- 1 - The command will exit with a 1 exit code if a vulnerability, deprecation, or outdated package was found. | ||
- 2 - The command will exit with a 2 exit code if it unexpectedly failed. | ||
- 3 - The command will exit with a 3 exit code if an unsupported project is detected. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the value of splitting exit codes 2 and 3? It may be simpler to have a single exit code for "NuGet was unable to determine if there vulnerabilities, deprecations, or outdated packages. This may due to unexpected failure or unsupported projects"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally I think unsupported vs. failure are different scenarios. Why have someone guess?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Some comments.
<!-- What related issues would you consider out of scope for this proposal but can be addressed in the future? --> | ||
- Should the command be named `audit` or `check`? | ||
- `audit` is the more consistent name for package manager tooling with other ecosystems. | ||
- `check` is the more consistent name with dotnet CLI. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
check is taken in dotnet format --check
command
|
||
### Technical explanation | ||
|
||
<!-- Explain the proposal in sufficient detail with implementation details, interaction models, and clarification of corner cases. --> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be nice to outline how dotnet audit fix
will fix outdated, deprecated, vulnerable packages:
- Pick the latest stable?
- Pick the closest next-stable, non-flagged package?
- Pick the alternate package in deprecated package?
- Should it include preview versions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll add a flowchart concept to the proposal soon.
Found 1 top-level Moderate severity vulnerability & 1 transtive Moderate severity vulnerability package(s) in 36 scanned packages. | ||
Run 'dotnet audit fix' to fix them. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about a scenario where top-level package has no vulnerability but a transitive one has a warning? AFAIK, we can't update the version of transitive dependency unless we add it as top-level dependency.
@zivkan answered this question in an offline review that, making it a top-level dependency is the design for how to upgrade transitive packages.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we need to document this under a better heading, but we've also documented it publicly: https://docs.microsoft.com/en-us/nuget/concepts/dependency-resolution#cousin-dependencies
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I hope to answer this further with a flowchart or similar. Thanks for bringing it up.
<!-- One-paragraph description of the proposal. --> | ||
`dotnet audit` & `dotnet audit fix` helps you find, fix, and monitor known security vulnerabilities, deprecated packages, and outdated versions in your .NET projects & solutions. | ||
|
||
## Motivation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my time in the NuGet team, there have been a few times when we've been looking at some old feature and trying to understand WHY it was done a particular way. So, in the interest of helping people in the future understand, can you please add something (anywhere in the document, not necessarily here) about why we're not just adding a --fix
argument to dotnet list package --deprecated --include-transitive
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure thing.
|
||
Example: | ||
|
||
![dotnet audit](/meta/resources/dotnetaudit/dotnetaudit.png) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
personal comment, I don't find the screenshot + text below useful. It just makes the document longer for me.
- 2 - The command will exit with a 2 exit code if it unexpectedly failed. | ||
- 3 - The command will exit with a 3 exit code if an unsupported project is detected. | ||
|
||
#### dotnet audit fix |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm sure we have more duplicates of the same request, but dotnet audit fix
just isn't going to work for a whole lot of customers, including ASP.NET Core customers using an LTS (not latest) version: #11457
Unless we're planning on implementing dotnet audit fix
after implementing the above feature, I think this spec should document this known limitation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is quite a hard challenge indeed. I think we can do our best job designing this feature for the majority of packages that aren't tied directly to a .NET version in NuGet. Even then, there is value for when vulnerabilities or deprecations are found.
i.e. https://www.nuget.org/packages/System.DirectoryServices.Protocols/ may have vulnerabilities in NET5 & prompt a user to migrate to NET6 or higher. I don't think this tool will be the panacea of modernization, but it can help bring awareness to the benefit of doing so or even point users to the respective modernization tools if needed.
If we do the work in the issue that allows for such functionality, we can port it over to this tool / try our best to detect it from the package lists to be smarter in these cases.
I'll add similar notes in the proposal.
Found 1 top-level Moderate severity vulnerability & 1 transtive Moderate severity vulnerability package(s) in 36 scanned packages. | ||
Run 'dotnet audit fix' to fix them. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we need to document this under a better heading, but we've also documented it publicly: https://docs.microsoft.com/en-us/nuget/concepts/dependency-resolution#cousin-dependencies
Co-authored-by: Loïc Sharma <737941+loic-sharma@users.noreply.github.com>
Co-authored-by: Loïc Sharma <737941+loic-sharma@users.noreply.github.com>
Thank you everyone so far for the reviews! |
Any idea how that would work with a nuget feed on azure devops server for example? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few comments I've been meaning to add for a while. They're largely stylistic, but things that I think are valuable to be called out.
``` | ||
$ dotnet audit | ||
Fetching package metadata from: 'https://api.nuget.org/v3/index.json' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think the extra comment about the packages helps tbh.
We should have just have a a Sources used
statement and capture everything.
$ dotnet audit | ||
Fetching package metadata from: 'https://api.nuget.org/v3/index.json' | ||
Loaded 23 security advisories from 'https://api.nuget.org/v3/index.json' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does 23 advisories mean in this case?
23 packages in our graph had advisories? Or 23 packages were inspected? Or the source had 23 advisories.
Found 2 top-level outdated package(s) in 36 scanned packages. | ||
Run 'dotnet audit fix' to fix them. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Say there are 10 packages with advisories.
Does fix update them one by one? (this can be super slow, something we don't do in VS).
What if the updates fial to bring us to a successful restore state?
I think the answer is we our best, and if we fail, say conflicts xyz and add details about potential actions the customer can take.
Note that because of the graph flattening, failures to upgrade are more common compared to something like NPM.
|
||
NuGet will use existing endpoints to optimize the speed of audit results. | ||
|
||
- [Deprecation](https://docs.microsoft.com/en-us/nuget/api/registration-base-url-resource#package-deprecation) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does audit check nuget.org if nuget.org is not part of the list? Probably should call it out.
Thanks Jon for getting this up! I have a few comments and questions. I think the audit command should flag obsolete packages by default. This is somewhat implicit in the name: if you're auditing your code base, you care about being in compliance with some sort of application health policy. An outdated package has been explicitly marked by the maintainer as "I have abandoned this and will no longer provide actionable package health data," and such a statement is absolutely relevant to people dealing with compliance enforcement. What about people who need to suppress warnings? Let's say that for whatever reason I absolutely must use vulnerable package Foo.1.2.3.4 - not any other version. I have mitigated my own risk by reviewing every single line of code involved in my app's data processing and I have high confidence that reported vulnerabilities do not apply to my use cases. What would the recommendation be here: put some DoNotAudit flag in packages.config for this particular reference, have the audit command continue to warn but make some external reporting tool responsible for implementing the idea of "baselining" warnings, stop using the vulnerable package and copy the relevant code into my own project, or something else? Similarly, when you implement the audit fix, you may need to introduce the concept of a "weak" package dependency, which can be thought of as a version redirect without the direct dependency. For example, let's say my app depends directly on DirectDependency.2.0.0.0, which depends on TransitiveDependency.1.0.0.0. There's a vulnerability in the transitive dependency, fixed in v1.0.1.0. The fix process could make my app take a weak dependency (not a direct dependency!) on TransitiveDependency.1.0.1.0. The behavior would be that if TransitiveDependency <1.0.1.0 appears anywhere in my package dependency graph as a strong dependency, it gets updated to 1.0.1.0; but if TransitiveDependency does not appear anywhere in my package dependency graph, it does not ever get deployed alongside my app. This allows for a future where DirectDependency.2.2.0.0 completely drops its reference to TransitiveDependency. When my app eventually upgrades to DirectDependency.2.2.0.0, there remains no strong dependency to TransitiveDependency anywhere in the entire graph, and my app's "weak" dependency is completely ignored. Perhaps it's even auto-deleted from packages.config. Finally, do you need to introduce the concept of a "fork" dependency? Let's say that my package Fork is a fork of some other package Base. I fork the code, make changes, and republish under my own banner. My package does not reference Base. However, because my code is so tightly intertwined with Base, any vulnerabilities in that package should also be reflected in my package so that my package's consumers are alerted to the problem, so I publish my package with a special attribute saying "vulnerabilities in Base.1.2.3.4 also apply to this package." The big issue here is that consumers cannot self-remediate after learning of a vulnerability, since they'd need to wait for the Fork author to publish a new package version. (Apps can't simply reference the patched version of Base, since presumably they were depending on some custom behavior of Fork.) The only benefit this would confer is that consuming apps could learn about the problem early and take proactive mitigation steps - perhaps by disabling affected features of their app - while waiting for an official Fork patch. |
Thanks for the review Levi, I'll try my best to answer everything but do know that not everything has an answer today.
This should be covered by
Definitely have thought about a
Absolutely. We will have to consider all our options for resolving the graph. Even considering the potential features of a package overrides type feature to fix problems in the graph at the transitive layer.
Although this is a great feature idea and has been mentioned a number of times to our binary package ecosystem vs. source based one, we will be looking towards warning consumers with as much information as possible for them to resolve issues in their projects via upgrading, replacing, and even removing dependencies. If we can't do anything, we will give them a notice that we can't help and they will have to advise based on the problem. At this time, we are not considering extra features such as monkey patching, but using what we have today and working on-top of that. |
Others already mentioning allowlist of specific CVE's. As to running audit in a pipeline; i really like the way Trivy is setup, where you can say Another question is of the json output could support https://cyclonedx.org/ format. It seems like there is a lot of tools popping up that will use this format. if all tools can support the same format, then managing scanresults/sboms from multiple development environments will become easier. could even take a cyclonedx-bom file as input in the future to have a quick way to check an old build. |
Introduction to an auditing experience in the .NET CLI that allows one to view known vulnerabilities, deprecations, and outdated packages from a glance and a subcommand to quickly fix and resolve issues to the best of its ability or provide a message to manually intervene.
This proposal introduces two new commands to .NET CLI known as
dotnet audit
anddotnet audit fix
.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 🎉