Vulnerability checking in PC restore #13154
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,93 @@ | ||
| # ***Vulnerabilities in packages.config restore*** | ||
|
|
||
| - [Nikolche Kolev](https://github.com/nkolev92) | ||
| - GitHub Issue <https://github.com/NuGet/Home/issues/12307> | ||
|
|
||
| ## Summary | ||
|
|
||
| This is a continuation of the [vulnerabilities in PackageReference work](../2022/vulnerabilities-in-restore.md), extending matching functionality for packages.config restore. | ||
|
|
||
| ## Motivation | ||
|
|
||
| See [vulnerabilities in PackageReference restore motivation](../2022/vulnerabilities-in-restore.md#motivation). | ||
|
|
||
| ## Explanation | ||
|
|
||
| ### Functional explanation | ||
|
|
||
| Packages.config restore combines all packages regardless of project. Instead of raising warnings and errors on a per project level, at restore time, they're raised on a per package level. | ||
|
|
||
| The configuration knobs for packages.config restore auditing functionality will be NuGet.config based. | ||
| In particular, there will be 2 new configuration keys: | ||
|
|
||
| | Key | Acceptable values | Description | Default | | ||
| |-----|-------------------|-------------|---------| | ||
| | auditForPackagesConfig | enable, disable | Enables or disables NuGet Audit for packages config projects | If not specified, the default will be `enable` | | ||
| | auditLevelForPackagesConfig | Critical, high, moderate, low | Configurations the default audit level for NuGet audit for packages config projects | If not specified, the default will be `disable` | | ||
nkolev92 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| The audit functionality for packages.config restore will be enabled by default. | ||
| To disable it, one can specify a property in the config section of the configuration file. | ||
|
There was a problem hiding this comment. This might get confusing between having this and also a MSBuild property. Is the rationale here that this would be used for legacy project system? There was a problem hiding this comment. We have places where we have duplicate configurations, one via nuget.config and another one via property. The property tends to override the nuget.config value. The reasoning here is that in packages.config world, for those customers, nuget.config makes more sense. |
||
|
|
||
| ```xml | ||
| <configuration> | ||
| <config> | ||
| <add key="audit" value="enable" /> | ||
| <add key="auditLevel" value="low" /> | ||
| </config> | ||
| </configuration> | ||
| ``` | ||
|
|
||
| Whenever a vulnerability for a package is discovered, a warning will be raised indicating the severity and advisory url. | ||
| Note that warnings as errors and no warn are not support in packages.config projects, so these warnings will not fail the build. | ||
|
|
||
| > Package 'Contoso.Service.APIs' 1.0.3 has a known critical severity vulnerability, https://github.com/advisories/GHSA-1234-5678-9012. | ||
|
|
||
| For performance considerations, vulnerability checks are only going to be performed when the packages.config restore downloads a package. | ||
nkolev92 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| ### Technical explanation | ||
|
|
||
| An [AuditUtility](https://github.com/NuGet/NuGet.Client/blob/dev/src/NuGet.Core/NuGet.PackageManagement/AuditUtility.cs) already exists for packages.config projects, written along similar lines as the AuditUtility for PackageReference. | ||
| For performance considerations, vulnerability checks are only going to be performed when the packages.config restore downloads a package. | ||
|
|
||
| When we run restore for packages.config, the following metrics will be considered: | ||
|
|
||
| - Was Audit enabled | ||
| - Was Audit Run | ||
| - Reason why audit was not run. (No package downloads for example) | ||
| - Sev0Matches | ||
| - Sev1Matches | ||
| - Sev2Matches | ||
| - Sev3Matches | ||
| - InvalidSevMatches | ||
| - List of package ids with advisories | ||
| - Severity | ||
| - DownloadDurationSeconds | ||
| - CheckPackagesDurationSeconds | ||
| - SourcesWithVulnerabilityData | ||
|
|
||
| ## Drawbacks | ||
|
|
||
| - Vulnerability checking comes with a performance hit. | ||
|
|
||
| ## Rationale and alternatives | ||
|
|
||
| - Enable vulnerability checking on demand. | ||
donnie-msft marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| - Enable vulnerability checking at installation time only. | ||
nkolev92 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| ## Prior Art | ||
|
|
||
| - [Vulnerability reporting in PackageReference restore](../2022/vulnerabilities-in-restore.md) | ||
|
|
||
| ## Unresolved Questions | ||
|
|
||
| - Should the NuGet.config configuration *affect* the PackageReference defaults as well? | ||
|
There was a problem hiding this comment. Not sure I like the idea of nuget.config config being introduced here nor affecting PR. There was a problem hiding this comment. It covers the in depth thought process. |
||
| - Should vulnerability checks be performed at every packages.config restore? | ||
| - Should we add the vulnerability metrics in the vs/nuget/restoreinformation event or a dedicated one? | ||
nkolev92 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| <!-- What parts of the proposal do you expect to resolve before this gets accepted? --> | ||
| <!-- What parts of the proposal need to be resolved before the proposal is stabilized? --> | ||
| <!-- What related issues would you consider out of scope for this proposal but can be addressed in the future? --> | ||
|
|
||
| ## Future Possibilities | ||
|
|
||
| - NuGet Audit without nuget.org as a package source - [Pull Request #12918](https://github.com/NuGet/Home/pull/12918) | ||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 disagree with this.
Firstly, as you already listed in the unanswered questions section, should PackageReference projects use this config?
Secondly, from a customer point of view, it means they'll have to duplicate the config (when not using defaults), and will have (at least) two places to update when changing values. This will have even higher impact when suppressing specific advisories is implemented, as that feature will have to design a packages.config specific alternative, and customers may need to duplicate the suppressions.
packages.config already reads MSBuild properties for lock files, so configuring and using lock files is the same, regardless if the project uses packages.config or PackageReference. I think NuGetAudit for packages.config should be the same.
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.
Usage of packages.config lock is minimal, so I'm not sure it's necessarily a well established pattern.
Maybe we make the nuget.config apply to everything?
I'm also wondering if the technical overhead of adding individual project support in packages.config is worth it in V1 of the feature.
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.
fwiw, I'll schedule a meeting to discuss this.
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.
is there any technical or feasibility issues with using the same pattern we've established earlier w/ MSBuild properties?
I like re-using the same pattern if possible.
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.
My argument is that it's not a pattern we've adopted for packages.config projects.
Technically, they're both feasible, but for packages.config projects using MSBuild property is not really a thing like it is in PackageReference.