Add PackageDownload vulnerability telemetry #6180

Nigusu-Allehu · 2024-12-04T19:43:01Z

Bug

Fixes: https://github.com/NuGet/Client.Engineering/issues/3112

Description

Telemetry design https://github.com/NuGet/Client.Engineering/blob/main/designs/telemetry/telemetry-design-PackageDownload-vulnerabilities.md
This PR introduces telemetry collection for vulnerabilities in <PackageDownload> packages:

Summary of Changes

Collects the list of vulnerable package IDs at the end of the restore operation.

Goals

This telemetry will enable us to:

Determine how many restore operations generate warnings due to vulnerable <PackageDownload> packages.
Use package names to estimate the proportion of warnings attributable to SDK-managed packages.

PR Checklist

Meaningful title, helpful description and a linked NuGet/Home issue
Added tests
Link to an issue or pull request to update docs if this PR changes settings, environment variables, new feature, etc.

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

zivkan · 2024-12-06T22:27:45Z

test/NuGet.Core.Tests/NuGet.Commands.Test/RestoreCommandTests/Utility/AuditUtilityTests.cs

@@ -179,6 +179,9 @@ public async Task Check_ProjectReferencingPackageWithVulnerability_WarningLogged

        context.PackagesDependencyProvider.Package("pkga", "1.0.0").DependsOn("pkgb", "1.0.0");
        context.PackagesDependencyProvider.Package("pkgb", "1.0.0");
+        context.PackageDownloadPackages.Add(new DownloadDependency("pkgDownload", new VersionRange(new NuGetVersion("1.0.0"), maxVersion: new NuGetVersion("1.0.0"))));


more of a nitpick: these package download package lines look a lot less nice than adding the package reference packages on the two lines above, since they're using a builder pattern, but this package download is not.

I wonder if it's worthwhile creating a DownloadDependencyProvider so tests that use it are easier to read.

Having said that, half the line length could be reduced if the VersionRange was changed to VersionRange.Parse("[1.0.0]). I wouldn't be surprised if I did the new VersionRange thing in the past, since I know it will execute faster than Parse. I keep changing my mind between "OMG, NuGet's tests are so slow, we need to make them faster, if only by a little bit" and also "these tests need to be easier to read" 🤣

Thanks for pointing this out. Personally, I am indifferent on this. But VersionRange.Parse() looks cleaner to me

test/NuGet.Core.Tests/NuGet.Commands.Test/RestoreCommandTests/Utility/AuditUtilityTests.cs

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

nkolev92

Consider using a telemetry template issue: https://github.com/NuGet/Client.Engineering/issues/new?assignees=&labels=Type%3AEngineering%2CPriority%3A2&projects=&template=telemetry.md.

I think we've forgotten to use it reliable (includes all of us :) )

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

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

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

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

nkolev92 · 2024-12-20T18:47:05Z

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

@@ -115,6 +115,7 @@ private readonly Dictionary<RestoreTargetGraph, Dictionary<string, LibraryInclud
        private const string AuditTransitiveVulnerabilitySev2 = "Audit.Vulnerability.Transitive.Severity2";
        private const string AuditTransitiveVulnerabilitySev3 = "Audit.Vulnerability.Transitive.Severity3";
        private const string AuditTransitiveVulnerabilitySevInvalid = "Audit.Vulnerability.Transitive.SeverityInvalid";
+        private const string AuditPackageDownloadVulnerabilitiesPackages = "Audit.Vulnerability.PackageDownload.Packages";


Did you and @zivkan have an extra conversation that wasn't included here?

Curious why we're not adding the counts for package downloads with vulnerabilities?

Not that I can remember 🤣

I think Audit.Vulnerability.PackageDownload.* properties make sense and will make it easier for us to tell the difference between older versions of NuGet/VS that don't support this telemetry, and restores that have zero PackageDownloads with known vulnerabilities. We could do it by devenv ExeVersion, but I prefer to filter out null values.

zivkan · 2024-12-23T00:06:45Z

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

        public async Task<bool> CheckPackageVulnerabilitiesAsync(CancellationToken cancellationToken)
        {
            // Performance: Early exit if restore graph does not contain any packages.
-            if (!HasPackages())
+            if (!HasPackages() && (_targetFrameworks.Count() == 0))


never use use Linq's .Count() when the type has a .Length or .Count property.

zivkan · 2024-12-23T00:08:06Z

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

@@ -78,10 +82,27 @@ public AuditUtility(
            }
        }

+        private void CheckPackageDownloadVulnerabilities(IReadOnlyList<IReadOnlyDictionary<string, IReadOnlyList<PackageVulnerabilityInfo>>> knownVulnerabilities)
+        {
+            foreach (var targetFramework in _targetFrameworks)


Since _targetFrameworks is IList, this foreach loop will cause an allocation. And since audit runs in restore, which is our most perf-critical path, we need to do everything we can to minimize perf impacts. So, either change this to a for loop and use the indexer to get the value from the list, or use NoAllocEnumerate (or whatever that extension method/class is called).

zivkan · 2024-12-23T00:28:00Z

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

@@ -115,6 +115,7 @@ private readonly Dictionary<RestoreTargetGraph, Dictionary<string, LibraryInclud
        private const string AuditTransitiveVulnerabilitySev2 = "Audit.Vulnerability.Transitive.Severity2";
        private const string AuditTransitiveVulnerabilitySev3 = "Audit.Vulnerability.Transitive.Severity3";
        private const string AuditTransitiveVulnerabilitySevInvalid = "Audit.Vulnerability.Transitive.SeverityInvalid";
+        private const string AuditPackageDownloadVulnerabilitiesPackages = "Audit.Vulnerability.PackageDownload.Packages";


Not that I can remember 🤣

I think Audit.Vulnerability.PackageDownload.* properties make sense and will make it easier for us to tell the difference between older versions of NuGet/VS that don't support this telemetry, and restores that have zero PackageDownloads with known vulnerabilities. We could do it by devenv ExeVersion, but I prefer to filter out null values.

Add PackageDownload vulnerability telemetry

8f7baec

Nigusu-Allehu self-assigned this Dec 4, 2024

Nigusu-Allehu added 2 commits December 5, 2024 09:53

fix

32d0dfa

test

bf91b10

Nigusu-Allehu marked this pull request as ready for review December 6, 2024 00:00

Nigusu-Allehu requested a review from a team as a code owner December 6, 2024 00:00

zivkan reviewed Dec 6, 2024

View reviewed changes

nkolev92 reviewed Dec 10, 2024

View reviewed changes

jeffkl self-requested a review December 10, 2024 23:09

jeffkl reviewed Dec 12, 2024

View reviewed changes

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

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

Nigusu-Allehu marked this pull request as draft December 17, 2024 22:15

Nigusu-Allehu added 2 commits December 17, 2024 16:19

Address comments

6c9a8e9

fix

be776ab

Nigusu-Allehu marked this pull request as ready for review December 19, 2024 18:15

Nigusu-Allehu requested review from jeffkl, zivkan and nkolev92 December 19, 2024 18:15

nkolev92 reviewed Dec 20, 2024

View reviewed changes

zivkan reviewed Dec 23, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add PackageDownload vulnerability telemetry #6180

Add PackageDownload vulnerability telemetry #6180

Nigusu-Allehu commented Dec 4, 2024 •

edited

Loading

zivkan Dec 6, 2024

Nigusu-Allehu Dec 19, 2024

nkolev92 left a comment

nkolev92 Dec 20, 2024

zivkan Dec 23, 2024

zivkan Dec 23, 2024

zivkan Dec 23, 2024

zivkan Dec 23, 2024

Add PackageDownload vulnerability telemetry #6180

Are you sure you want to change the base?

Add PackageDownload vulnerability telemetry #6180

Conversation

Nigusu-Allehu commented Dec 4, 2024 • edited Loading

Bug

Description

Summary of Changes

Goals

PR Checklist

zivkan Dec 6, 2024

Choose a reason for hiding this comment

Nigusu-Allehu Dec 19, 2024

Choose a reason for hiding this comment

nkolev92 left a comment

Choose a reason for hiding this comment

nkolev92 Dec 20, 2024

Choose a reason for hiding this comment

zivkan Dec 23, 2024

Choose a reason for hiding this comment

zivkan Dec 23, 2024

Choose a reason for hiding this comment

zivkan Dec 23, 2024

Choose a reason for hiding this comment

zivkan Dec 23, 2024

Choose a reason for hiding this comment

Nigusu-Allehu commented Dec 4, 2024 •

edited

Loading