Enable vulnerability checking for packages.config projects during commandline restore #5639

nkolev92 · 2024-02-21T22:25:00Z

Bug

Fixes: NuGet/Home#13253
Fixes: https://github.com/NuGet/Client.Engineering/issues/2698

Regression? Last working version:

Description

See NuGet/Home#13154 for the main design.

The Visual Studio side requires a lot of changes, and as such to make the review easier, I've split up the enablement of audit checking for VS scenarios. Despite that, there are changes/ideas here that are driven by that. Note that even the public APIs here aren't fully committed, but simply a good enough version as of right now.
PackageRestoreManager is where the main work for packages.config restore happens.
It's a short lived service based on https://github.com/NuGet/NuGet.Client/blob/dev/src/NuGet.Core/NuGet.PackageManagement/IDE/IPackageRestoreManager.cs.
Counterintuitively the service part is used more frequently in non-VS scenarios, which is why I have this length explanation.
There are 4 methods in this class, 2 for solution based, 2 for package driven restoring, I have deprecated the 2 unused methods as they just add complexity to adding any changes like vulnerability checking, since I now need to check many inputs.
Given that in the design we decided to use properties to enable/disable audit, I have reused the RestoreAuditProperties class.
Extract 2 methods for parsing the restore audit properties, in particular the 2 that we need for packages.config restore.
Create a public API for vulnerability running.
The check returns a type that can be "stored" and used for no-op in Visual Studio scenarios. In particular, in a future version, if the restore in VS has no-op, we'll just "replay" the warnings raised by the previous restore if any.
The type knows how to make additions to a telemetry event. Note that this isn't final, as I don't have the complete Visual Studio implementation yet.
CLI scenarios, such as restore, install from nuget.exe and msbuild /t:restore will run audit by default, even if packages are downloaded.

PR Checklist

PR has a meaningful title
PR has a linked issue.
Described changes
Tests
- Automated tests added
- OR
- Test exception
- OR
- N/A
Documentation
- Documentation PR or issue filled - https://github.com/NuGet/docs.microsoft.com-nuget/issues/3247
- OR
- N/A

…LI scenarios

zivkan

I only looked at the product code so far, not tests, but generally looks good. I have a meeting now, so don't have time to finish reviewing today.

src/NuGet.Clients/NuGet.CommandLine/Commands/InstallCommand.cs

zivkan · 2024-02-23T16:31:45Z

src/NuGet.Core/NuGet.PackageManagement/Audit/AuditCheckResult.cs

+        internal bool IsAuditEnabled { get; set; } = true;
+
+        internal int Severity0VulnerabilitiesFound { get; set; }
+        internal int Severity1VulnerabilitiesFound { get; set; }
+        internal int Severity2VulnerabilitiesFound { get; set; }
+        internal int Severity3VulnerabilitiesFound { get; set; }
+        internal int InvalidSeverityVulnerabilitiesFound { get; set; }
+        internal List<PackageIdentity>? Packages { get; set; }
+        internal double? DownloadDurationInSeconds { get; set; }
+        internal double? CheckPackagesDurationInSeconds { get; set; }
+        internal int? SourcesWithVulnerabilities { get; set; }


Considering this type is a record, and therefore a struct, I think it's important to make all these properties get; init;, rather than get; set;.

The problem with get; set; in a struct, is if one is passed in as a method parameter (but not as a ref, changing the value of properties will cause the values to be lost when the method returns. So, it's significant risk of bugs if people use it wrong.

Making it only possible to set at initialization time reduces the risk that it gets set wrong.

These are not always known/available in every audit situation.

I have tests ensuring things get set in the expected scenarios. Obviously that doesn't guard against future additions.

I can have a bunch of internal constructors I guess if you'd prefer that.

init does not mean required, it just means that the type is read only, and needs to have a new "instance" created to change a value. Using the with expression makes this very easy. Therefore multiple constructors are not needed, unless you want to validate that when "property 1" is set "property 2" must be as well.

And when I reviewed this PR last week, I didn't see any examples of the type having any values changed after initialization. At least in the product code, I only saw properties being set on init.

Obviously that doesn't guard against future additions.

This is the point of my suggestion. It does prevent future incorrect usage that might otherwise go unnoticed.

src/NuGet.Core/NuGet.PackageManagement/Audit/AuditCheckResult.cs

src/NuGet.Core/NuGet.PackageManagement/Audit/AuditChecker.cs

src/NuGet.Clients/NuGet.CommandLine/Commands/RestoreCommand.cs

nkolev92 · 2024-02-23T18:12:57Z

@zivkan Thanks for taking a look!
I've addressed or responded too all of your feedback and it's ready for another look.

src/NuGet.Clients/NuGet.CommandLine/Commands/InstallCommand.cs

src/NuGet.Clients/NuGet.CommandLine/Commands/RestoreCommand.cs

src/NuGet.Core/NuGet.Commands/RestoreCommand/Utility/AuditUtility.cs

src/NuGet.Core/NuGet.PackageManagement/Audit/AuditChecker.cs

src/NuGet.Core/NuGet.PackageManagement/IDE/PackageRestoreContext.cs

src/NuGet.Clients/NuGet.CommandLine/Commands/RestoreCommand.cs

src/NuGet.Core/NuGet.Commands/RestoreCommand/Utility/AuditUtility.cs

src/NuGet.Core/NuGet.PackageManagement/Audit/AuditChecker.cs

zivkan

Two opportunities to follow best perf practises, but otherwise LGTM.

src/NuGet.Clients/NuGet.CommandLine/Commands/RestoreCommand.cs

zivkan · 2024-02-26T14:29:25Z

src/NuGet.Core/NuGet.PackageManagement/Audit/AuditChecker.cs

+                CreateWarnings(packagesWithKnownVulnerabilities, auditSettings, ref Sev0Matches, ref Sev1Matches, ref Sev2Matches, ref Sev3Matches, ref InvalidSevMatches, ref packagesWithReportedAdvisories) :
+                Array.Empty<ILogMessage>();
+
+            foreach (var warning in warnings)


Suggested change

foreach (var warning in warnings)

foreach (var warning in warnings.NoAllocEnumerate())

warnings is IReadOnlyList<T>, hence foreach will do a heap alloc by default.

nkolev92 · 2024-02-26T17:49:41Z

thanks @zivkan

I'll address these in the next PR. Merging the PR allows me to unblock my VS work.

Enable vulnerability checking for packages.config based projects in C…

35a7083

…LI scenarios

nkolev92 force-pushed the dev-nkolev92-PCVulnerabilityInRestore branch from 7dba399 to 35a7083 Compare February 22, 2024 00:21

nkolev92 added 5 commits February 21, 2024 17:28

Add more tests

5c958d8

cleanup, add even more tests

4d95b9b

Fix up tests

1c9f719

Complete unit tests

ab1a9a2

Clenaup

1f929b1

nkolev92 marked this pull request as ready for review February 22, 2024 21:24

nkolev92 requested a review from a team as a code owner February 22, 2024 21:24

nkolev92 added 10 commits February 22, 2024 17:45

Fix static graph restore

fc2b65b

Add e2e tests

04b44de

cleanup

9f085f2

add more tests

b062e19

cleanup

10f0db8

cleanup

854d71e

more tests

81a159c

cleanup

9ce4ae5

Fix final test

5dbc2ed

Cleanup

41f0379

zivkan reviewed Feb 23, 2024

View reviewed changes

nkolev92 mentioned this pull request Feb 23, 2024

Should NuGetAudit respect NoCache NuGet/Home#13261

Open

address feedback

437c307

nkolev92 requested a review from zivkan February 23, 2024 18:12

jeffkl reviewed Feb 23, 2024

View reviewed changes

nkolev92 added 3 commits February 23, 2024 10:48

Cleanup

0b53e4c

address even more feedback

6fd9d75

use named tuples...sort of

7b476bf

nkolev92 requested a review from jeffkl February 23, 2024 19:03

jeffkl previously approved these changes Feb 23, 2024

View reviewed changes

src/NuGet.Clients/NuGet.CommandLine/Commands/RestoreCommand.cs Show resolved Hide resolved

src/NuGet.Core/NuGet.Commands/RestoreCommand/Utility/AuditUtility.cs Outdated Show resolved Hide resolved

src/NuGet.Core/NuGet.PackageManagement/Audit/AuditChecker.cs Show resolved Hide resolved

revert unnecessary changes

003708f

nkolev92 dismissed jeffkl’s stale review via 003708f February 23, 2024 21:48

nkolev92 requested a review from jeffkl February 23, 2024 21:49

jeffkl previously approved these changes Feb 23, 2024

View reviewed changes

cleanup

9d62ce5

nkolev92 dismissed jeffkl’s stale review via 9d62ce5 February 24, 2024 05:23

zivkan approved these changes Feb 26, 2024

View reviewed changes

nkolev92 merged commit 5ecd0be into dev Feb 26, 2024
16 checks passed

nkolev92 deleted the dev-nkolev92-PCVulnerabilityInRestore branch February 26, 2024 17:49

nkolev92 mentioned this pull request Feb 27, 2024

Enable vulnerability checking for packages.config projects in Visual Studio #5646

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable vulnerability checking for packages.config projects during commandline restore #5639

Enable vulnerability checking for packages.config projects during commandline restore #5639

nkolev92 commented Feb 21, 2024 •

edited

Loading

zivkan left a comment

zivkan Feb 23, 2024

nkolev92 Feb 23, 2024

zivkan Feb 26, 2024 •

edited

Loading

nkolev92 commented Feb 23, 2024

zivkan left a comment

zivkan Feb 26, 2024

nkolev92 commented Feb 26, 2024

	foreach (var warning in warnings)
	foreach (var warning in warnings.NoAllocEnumerate())

Enable vulnerability checking for packages.config projects during commandline restore #5639

Enable vulnerability checking for packages.config projects during commandline restore #5639

Conversation

nkolev92 commented Feb 21, 2024 • edited Loading

Bug

Description

PR Checklist

zivkan left a comment

Choose a reason for hiding this comment

zivkan Feb 23, 2024

Choose a reason for hiding this comment

nkolev92 Feb 23, 2024

Choose a reason for hiding this comment

zivkan Feb 26, 2024 • edited Loading

Choose a reason for hiding this comment

nkolev92 commented Feb 23, 2024

zivkan left a comment

Choose a reason for hiding this comment

zivkan Feb 26, 2024

Choose a reason for hiding this comment

nkolev92 commented Feb 26, 2024

nkolev92 commented Feb 21, 2024 •

edited

Loading

zivkan Feb 26, 2024 •

edited

Loading