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

Adding a proposal for the new CLI command to fix vulnerabilities #13961

Open
wants to merge 3 commits into
base: dev
Choose a base branch
from
Open

Adding a proposal for the new CLI command to fix vulnerabilities #13961

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e65e43b
Adding a proposal for the new CLI command to fix vulnerabilities
OliaG Nov 25, 2024
d33ee50
Updated the doc
OliaG Dec 10, 2024
2aca536
Addressed review comments
OliaG Dec 17, 2024
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
238 changes: 238 additions & 0 deletions accepted/2024/package-update-command.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,238 @@
# dotnet package update command

- [Olia Gavrysh](https://github.com/OliaG/)
- Start Date (2024-11-11)
- [#13372](https://github.com/NuGet/Home/issues/13372)
- [#4103](https://github.com/NuGet/Home/issues/4103)

## Summary

<!-- One-paragraph description of the proposal. -->
The `dotnet package update` feature is designed to enhance the user experience by enabling .NET developers to address security vulnerabilities, deprecated packages, and outdated versions of their NuGet dependencies with a single command.
This improvement will, in turn, bolster the overall security and reliability of .NET applications. The feature will build upon the existing NuGet Audit functionality.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
This improvement will, in turn, bolster the overall security and reliability of .NET applications. The feature will build upon the existing NuGet Audit functionality.
This improvement will, in turn, bolster the overall security and reliability of .NET applications.
The feature will build upon the existing NuGet Audit functionality.


## Motivation

<!-- Why are we doing this? What pain points does this solve? What is the expected outcome? -->
Even though .NET developers now have the opportunity to learn about vulnerabilities and deprecated dependencies in their NuGet packages and addressing those issues, there are gaps in user experience, especially in the CLI workflows.

### Existing capabilities

Visual Studio

- Has a means to show outdated packages
- Has a means to show deprecate/vulnerable packages
- Has a means to update all packages to latest
- Has a means to update a single package to latest (which can be challenging)
- Has a vulnerability filter, which in turn can allow you to update all vulnerable packages to latest
dotnet
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
dotnet


CLI
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 the comparison between VS and the dotnet CLI woud be easier to understand as a table. For example:

Capability VS CLI
do thing ✔️ ✔️
something else ✔️ ❌


- Has a means to show outdated packages
- Has a means to show deprecated/vulnerable packages
- Has a means to update a single package
- Does not have means to update multiple packages at once

### Gaps in CLI experience

- No means to update to all latest packages on the CLI
- No means for update heuristics, such as, update my vulnerable packages

This proposal is focused on bringing the CLI experience up to funcional parity with the experience in Visual Studio and make developers more productive by providing a single command to update all packages to the latest version and a way to update all vulnerable packages.

## User experience
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 it would be nice to have a summary of all features, similar to what you'd expect to see when you run dotnet package update -h. The sub-feature examples below are good, but I had an idea for something, and a ctrl-f can't find it in the document. Without re-reading the whole thing top to bottom, I don't have an easy way to figure out if I'm searching for the wrong keyword or if it's something I need to suggest.

And that is that I think a --dry-run option would be good. But now I feel like this was originally proposed. Did we remove it for some reason that I can't remember?


### Updating all packages to the latest version

When users want to ensure their NuGet dependencies are up to date, they can run a command in .NET CLI `dotnet package update` for a project or a solution.

```CLI
C:\> dotnet package update [<SOLUTION_PATH>|<PROJECT_PATH_>]
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
C:\> dotnet package update [<SOLUTION_PATH>|<PROJECT_PATH_>]
C:\> dotnet package update [<SOLUTION_PATH>|<PROJECT_PATH>]


Fixing outdated packages in ContosoUniversity.sln
ContosoLibrary:
Updated UmbracoForms 8.4.1 to 8.7.1
Updated Newtonsoft.Json 11.0.1 to 13.0.3

ContosoApp:
Updated UmbracoForms 8.4.0 to 8.7.1

Updated 3 packages in 36 scanned packages.
```

### Fixing vulnerabilities

When users run `dotnet build` or `dotnet restore` commands in CLI, if they see any warnings related to vulnerabilities in their project’s NuGet dependencies, they can run `dotnet package update --vulnerable` CLI command to try to remediate all the vulnerabilities. This includes both direct and transitive.

```CLI
C:\ContosoApp\> dotnet package update --vulnerable

Fixing vulnernable packages in ContosoUniversity.sln
ContosoLibrary:
Upgrading UmbracoForms 8.4.1 to 8.7.1

ContosoApp:
Updated UmbracoForms 8.4.0 to 8.7.1

Fixed 2 packages in 36 scanned packages.
```

In the future this can be extended to fix deprecated versions.

## Explanation

### Functional explanation

<!-- Explain the proposal as if it were already implemented and you're teaching it to another person. -->
<!-- Introduce new concepts, functional designs with real life examples, and low-fidelity mockups or pseudocode to show how this proposal would look. -->

#### dotnet package update

The `dotnet package update` command will update all the NuGet dependencies to the latest versions.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
The `dotnet package update` command will update all the NuGet dependencies to the latest versions.
The `dotnet package update` command without any other arguments or options will update all the NuGet dependencies to the latest versions.


```CLI
C:\> dotnet package update --help
dotnet package update [<PROJECT>|<SOLUTION>] [--vulnerable] [--mode <MODE>] [-v|--verbosity <LEVEL>] [--json]
Copy link
Member

Choose a reason for hiding this comment

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

above, the json option was --format json, but here it's just --json. As someone else commented, we already have prior art with --format json

```

#### --vulnerable

This flag will try to update only packages that have direct or transitive vulnerabilities. The remediation 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.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
This flag will try to update only packages that have direct or transitive vulnerabilities. The remediation 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.
This flag will try to update only packages that have direct or transitive vulnerabilities.
The remediation 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.

I like one sentence per line, because it makes it easier to comment on specific things.

In particular:

  1. In another comment, I wrote that if we tag packages promoted to top level, then later on we can remove it as a top level package when we detect it's no longer needed as top level. Without this feature, I'm not sure I can think of any scenarios where we will remove a package reference.
  2. I shouldn't be working this late at night, but at the moment, I don't understand the last sentence. Is this in reference to packages where new versions no longer support target frameworks that the current project is using, so while the higher package version no longer has a known vulnerability, it's not compatible with the project? The "if a higher one has already been specified" part also throws me off. At this time of night, I can't guess what this means.

The algorithm of the fixing process will have the following steps:

1. Identify vulnerabilities in the graph
1. Distinguish which are top level, transitive level, etc.,
1. Resolve any compatibility conflicts
1. Apply the patch/update/fix
1. Restore dependencies
1. Verify that issues were resolved . Provide the report to the user.
1. If the issues were not resolved or were partially resolved, we will still commit the fixes and notify users about what was and was not fixed and what are the recommended manual steps with links to documentation.

Example:

```CLI
C:\ContosoApp\> dotnet package update --vulnerable

Fixing vulnernable packages in ContosoUniversity.sln
ContosoLibrary:
Upgrading UmbracoForms 8.4.1 to 8.7.1

ContosoApp:
Updated UmbracoForms 8.4.0 to 8.7.1

Fixed 2 packages in 36 scanned packages.
```

Another option for the output with more information (will review with CLI team to decide which one to pick):

```CLI
C:\> dotnet package update --vulnerable [<SOLUTION_PATH>|<PROJECT_PATH_>]

Fetching package metadata from: 'https://api.nuget.org/v3/index.json'
Loaded 23 security advisories from 'https://api.nuget.org/v3/index.json'
Scanning ContosoUniversity.sln (36 NuGet dependencies)

error: Vulnernable packages found!
[net5.0]:
Top-level Package Requested Resolved Severity Advisory URL
> UmbracoForms 8.4.1 8.4.1 Moderate https://github.com/advisories/GHSA-8m73-w2r2-6xxj

Transitive Package Resolved Severity Advisory URL
> Microsoft.Data.OData 5.2.0 Moderate https://github.com/advisories/GHSA-mv2r-q4g5-j8q5

Found 1 top-level Moderate severity vulnerability & 1 transtive Moderate severity vulnerability package(s) in 36 scanned packages.

Fixing vulnernable packages in ContosoUniversity.sln
Upgrading UmbracoForms 8.4.1 to 8.7.1

Fixed 2 packages in 36 scanned packages.
```

#### --mode

Priority: P1 for `promote`; P2 for `closest` and switching the default to `closest`.

| Mode | Explanation |
|------|-------------|
| `promote` | the algorithm will always promote transitive packages to top level. This is an easy to implement feature that we will release in MVP. |
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure where in the document it should be written, but while talking to an internal partner, they suggested that packages that are promoted to top level in order to remove a vulnerable package should have some additional metadata to signal this. For example <PackageReference Include="SomePackage" Version="1.2.3" PromotedBecauseOfVulnerability="true" />. A much, much better name is needed though, this is just an example.

If in the future the top level package that originally caused SomePackage to be included in the package graph has a new version that removes SomePackage from the package graph, this extra metadata will allow a future improvement to dotnet update package to detect that the PackageReference can now be removed.

I think this is a great idea for projects that aren't using CPM with transitive pinning. Although it's also a signal that transitive pinning without CPM would be a useful feature, but very much out of scope for this feature 😁

| `closest` | the algorithm will try to upgrade the direct package reference, and if that doesn't work, it will walk the graph one dependency level at a time trying to upgrade that, until it gets to the package with the known vulnerability. Once this mode is implemented, it should be set to be the default one. |

#### --format json

Priority: P2

Provides a detailed report in a JSON format:

```json
"added": [
Copy link
Member

Choose a reason for hiding this comment

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

I'd like to see a complete, valid JSON, rather than a snippet of only part of a valid document. In particular, this snippet doesn't show how a solution with multiple projects would be represented. The project path/filename will need to be declared somewhere.

{
"name": "Microsoft.AspNetCore.MVC",
"version": "2.2.0"
}
],
"removed": [
{
"name": "anurse.testing.TestDeprecatedPackage",
"version": "1.0.0"
}
],
"updated": [
{
"name": "UmbracoForms",
"version": "8.7.1",
"previousVersion": "8.4.1"
}
],
"failures": [],
"warnings": []
```
Comment on lines +167 to +189
Copy link
Contributor

@martinrrm martinrrm Dec 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Since this command is going to be used in solutions too, I think we need to be more detailed in this json format. For example, let's say that dotnet audit fix resolves to different package version for different projects, or for different target frameworks, I think we need to reflect that in the JSON output.

Another edge case I can think of is when customer has project references. Let's say that ProjectA has a reference to ProjectB that contains a vulnerable package, since those packages are considered transitive will the solution be updating the package in ProjectB and install it in ProjectA?

With those considerations I was thinking something like this:

{
	"version": "string",
	"feasible": true,
	"metadata": 
	{
		"added": "number",
		"removed": "number",
		"updated": "number",
		"promotedToTopLevel": "number",
	},
	"vulnerabilities":
	{
		"projectA":
		{
			"net8.0":
			{
				"total": "number",
				"newtonsoft.json": 
				{
					"originalVersion": "12.0.1", // Could be ranges too
					"newVersion": "13.0.1",
					"promotedToTopLevel": true,
					"projectReference": true, // Could be a path to the project reference
					...
				},
				"castle.core":
				{
					...
				},
			},
			"net7.0":
			{
				...
			},	
		},
		"projectB":
		{
			...
		},
	},
}

@OliaG @zivkan What do you think about this JSON proposal?

Copy link
Member

Choose a reason for hiding this comment

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

I have a more comments than I thought I would:

  1. The proposed schema doesn't provide a place to output warning or error messages. If the command fails, the error should still be in a valid JSON, to make it easier to script. I believe that having an errors or messages array removes the need for a feasible bool, since any result without any errors is by definition feasible.
  2. Some languages make it hard to use json properties that are not valid language variable names, so property keys like "net8.0" is problematic, as is using project names. Even when your language allows you to parse it, it can be harder to use since property/key names are dynamic. In some languages it's much easier when the schema is static. For example, consider trying to use json query to search the result, or use Convert-FromJson in PowerShell.
  3. On the topic of project names, it's possible to have multiple projects in a solution have the same name, as long as they're in different subdirectories, so we have to use the full path to the project, not just the name, otherwise in edge cases they won't be unique.
  4. In the case of a package being transitive through a project reference, I think the tool should be smart enough to not update the package. In that case, since the package isn't being updated, there's no need for a "projectReference" property.
  5. dotnet package update will be used for all kinds of updates, not just vulnerable packages. I think Olia's "added":, "updated":, and "removed": properties make more sense than a vulnerabilities property key. Although in that property you have a collection of projects, so projects is a more appropriate name.
  6. I haven't been looking at JSON other than NuGet's own for years, so I'm quite out of touch with JSON best practises. But I don't understand the purpose of putting the counts within a metadata object. Is there a benefit over putting the counts in the parent object?
  7. I feel like the counts should be per-project, not at the root object level. Having it at the root object level raises questions, like if multiple projects have to promote the same package to top level, does that count as 1, or is the promote to top level count going to equal the number of projects? However, I don't understand the point of the count properties, since you can look at look at the length of the array of package updates.
  8. JSON numbers aren't quoted. Some languages/parsers might be loosey-goosey, but strict parsers will fail. I feel like NuGet had this problem when migrating one of our parsers from Newtonsoft.Json (using JToken) to System.Text.Json.
  9. While nuget.org doesn't have a package with id total, it is a valid package ID, so it should not be used in a place where package IDs are also expressed. However, I already mentioned we shouldn't have a dynamic schema, so this concern should automatically be solved when addressing where to put package ids.
  10. Similar to the "I'm not familiar with JSON best practises" comment I made earlier, I'm not sure if long property names originalVersion, newVersion, promotedToTopLevel are commonly used. from and to I think are easy enough to understand, as are original and new. Assuming we have some docs somewhere that explains what it means, I think promoted might be a good enough name. promotedToTopLevel might be guessable without docs. However, I just had the thought that since it's possible to tell the difference between an "addition" vs an "upgrade", there could be a property wasTransitive, and I think that will be more clear to people than promoted


In the "failures" and "warnings" should be the information about what projects were not updated due to failed pipeline or different reassons like no solution was found, the format of the project file is not supported, etc.

##### Exit Codes

Priority: P1

- 0 - The command will exit with a 0 exit code if no vulnerabilities or outdated packages were found and no changes were made.
- 1 - The command will exit with a 1 exit code if changes were successful and the PR is submitted.
- 2 - The command will exit with a 2 exit code if any error has occurred, PR will not be submitted.
Comment on lines +198 to +199
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
- 1 - The command will exit with a 1 exit code if changes were successful and the PR is submitted.
- 2 - The command will exit with a 2 exit code if any error has occurred, PR will not be submitted.
- 1 - The command will exit with a 1 exit code if changes were successful.
- 2 - The command will exit with a 2 exit code if any error has occurred.

dotnet package update won't automate creating pull requests, just modifying local files. The developer, or other tools that use dotnet package update, will be responsible for creating the pull request.


##### Endpoints

NuGet will use existing endpoints to optimize the speed of audit results.

- [Vulnerability](https://docs.microsoft.com/en-us/nuget/api/registration-base-url-resource#vulnerabilities)
- Outdated - no existing endpoint, will need to call a source.
- [Deprecation](https://docs.microsoft.com/en-us/nuget/api/registration-base-url-resource#package-deprecation) for future possible improvements.

## Rationale and alternatives

<!-- Why is this the best design compared to other designs? -->
<!-- What other designs have been considered and why weren't they chosen? -->
<!-- What is the impact of not doing this? -->

### Alternatives considrered
Copy link
Member

Choose a reason for hiding this comment

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

dotnet nuget fix and dotnet nuget audit fix commands were considered to fix vulnerabilities, but we believe that it's more natural and more discoverable to have a generic update command that has different "modes" to choose all or just vulnerable packages.


We could enable the option for the users to pick if they want only direct vulnerabilities to be resolved or direct and transitive. We decided to not do so and implement the resolution of all vulnerabilities as we believe that better corresponds to our goal of increasing security across all .NET applications.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
We could enable the option for the users to pick if they want only direct vulnerabilities to be resolved or direct and transitive. We decided to not do so and implement the resolution of all vulnerabilities as we believe that better corresponds to our goal of increasing security across all .NET applications.
We could enable the option for the users to pick if they want only direct vulnerabilities to be resolved or direct and transitive.
We decided to not do so and implement the resolution of all vulnerabilities as we believe that better corresponds to our goal of increasing security across all .NET applications.

One sentence per line is more useful in our docs, where we're more likely to update the file some time in the future, but I like to practise it for all markdown documents


## Prior Art

<!-- What prior art, both good and bad are related to this proposal? -->
<!-- Do other features exist in other ecosystems and what experience have their community had? -->
<!-- What lessons from other communities can we learn from? -->
<!-- Are there any resources that are relevent to this proposal? -->
- [snyk](https://snyk.io/)
- [npm audit](https://docs.npmjs.com/cli/v7/commands/npm-audit)
- [cargo audit](https://github.com/RustSec/cargo-audit)
- [dotnet outdated](https://github.com/dotnet-outdated/dotnet-outdated)
- [dotnet retire](https://github.com/retirenet/dotnet-retire)
- [NuGet Defense](https://github.com/digitalcoyote/NuGetDefense)

## Future Possibilities

<!-- What future possibilities can you think of that this proposal would help with? -->

- In the future the basic algorithm could check for breaking changes in fixed versions.
- This functionality may be reused to implement a one-click fix in the Visual Studio UI experience.
- Enable fixes for deprecations and outdated versions as well together with an option for the users to choose what they want to fix (vulnerabilities, depreciation, outdated versions).