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.

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

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

Merged
merged 22 commits into from
Feb 26, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -190,6 +190,8 @@ private async Task PerformV2RestoreAsync(string packagesConfigFilePath, string i
packageRestoreFailedEvent: (sender, args) => { failedEvents.Enqueue(args); },
sourceRepositories: packageSources.Select(sourceRepositoryProvider.CreateRepository),
maxNumberOfParallelTasks: DisableParallelProcessing ? 1 : PackageManagementConstants.DefaultMaxDegreeOfParallelism,
enableNuGetAudit: true,
restoreAuditProperties: new(),
logger: Console);

var packageSaveMode = Packaging.PackageSaveMode.Defaultv2;
Expand Down
57 changes: 48 additions & 9 deletions src/NuGet.Clients/NuGet.CommandLine/Commands/RestoreCommand.cs
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@
using System.Globalization;
using System.IO;
using System.Linq;
using System.Runtime.Remoting;
using System.Threading;
using System.Threading.Tasks;
using NuGet.Commands;
Expand Down Expand Up @@ -273,6 +272,7 @@ private async Task<IReadOnlyList<RestoreSummary>> PerformNuGetV2RestoreAsync(Pac

List<PackageRestoreData> packageRestoreData = new();
bool areAnyPackagesMissing = false;
Dictionary<string, RestoreAuditProperties> restoreAuditProperties = null;

if (packageRestoreInputs.RestoringWithSolutionFile)
{
Expand All @@ -283,7 +283,7 @@ private async Task<IReadOnlyList<RestoreSummary>> PerformNuGetV2RestoreAsync(Pac
{
foreach (PackageReference packageReference in GetInstalledPackageReferences(configFile))
{
if (configToProjectPath.TryGetValue(configFile, out string projectPath))
if (!configToProjectPath.TryGetValue(configFile, out string projectPath))
nkolev92 marked this conversation as resolved.
Show resolved Hide resolved
{
projectPath = configFile;
}
Expand All @@ -303,6 +303,7 @@ private async Task<IReadOnlyList<RestoreSummary>> PerformNuGetV2RestoreAsync(Pac
packageRestoreData.Add(new PackageRestoreData(package.Key, package.Value, !exists));
areAnyPackagesMissing |= !exists;
}
restoreAuditProperties = GetRestoreAuditProperties(packageRestoreInputs);

}
else if (packageRestoreInputs.PackagesConfigFiles.Count > 0)
Expand All @@ -324,15 +325,34 @@ private async Task<IReadOnlyList<RestoreSummary>> PerformNuGetV2RestoreAsync(Pac

throw new InvalidOperationException(message);
}
restoreAuditProperties = new(PathUtility.GetStringComparerBasedOnOS());

string referenceFile = packageReferenceFile;
// If restoring with a csproj directly, ensure we read the audit configuration.
if (packageRestoreInputs.ProjectFiles.Count > 0)
{
var packageSpec = packageRestoreInputs.ProjectReferenceLookup.GetProjectSpec(packageRestoreInputs.ProjectFiles.First());
if (packageSpec != null)
{
referenceFile = packageSpec.FilePath;
restoreAuditProperties.Add(referenceFile, packageSpec.RestoreMetadata.RestoreAuditProperties);
}
}

foreach (PackageReference packageReference in GetInstalledPackageReferences(packageReferenceFile))
{
bool exists = nuGetPackageManager.PackageExistsInPackagesFolder(packageReference.PackageIdentity, packageSaveMode);
packageRestoreData.Add(new PackageRestoreData(packageReference, [packageReferenceFile], !exists));
packageRestoreData.Add(new PackageRestoreData(packageReference, [referenceFile], !exists));
areAnyPackagesMissing |= !exists;
}
}

var packageSources = GetPackageSources(Settings);

var repositories = packageSources
.Select(sourceRepositoryProvider.CreateRepository)
.ToList();

if (!areAnyPackagesMissing)
{
var message = string.Format(
Expand All @@ -350,6 +370,15 @@ private async Task<IReadOnlyList<RestoreSummary>> PerformNuGetV2RestoreAsync(Pac
packagesFolderPath,
restoreSummaries);

using SourceCacheContext cacheContext = new();
nkolev92 marked this conversation as resolved.
Show resolved Hide resolved

var auditUtility = new AuditChecker(
repositories,
cacheContext,
Console);

await auditUtility.CheckPackageVulnerabilitiesAsync(packageRestoreData, restoreAuditProperties, CancellationToken.None);

if (restoreSummaries.Count == 0)
{
restoreSummaries.Add(new RestoreSummary(success: true));
Expand All @@ -358,12 +387,6 @@ private async Task<IReadOnlyList<RestoreSummary>> PerformNuGetV2RestoreAsync(Pac
return restoreSummaries;
}

var packageSources = GetPackageSources(Settings);

var repositories = packageSources
.Select(sourceRepositoryProvider.CreateRepository)
.ToArray();

var installCount = 0;
var failedEvents = new ConcurrentQueue<PackageRestoreFailedEventArgs>();
var collectorLogger = new RestoreCollectorLogger(Console);
Expand All @@ -378,6 +401,8 @@ private async Task<IReadOnlyList<RestoreSummary>> PerformNuGetV2RestoreAsync(Pac
maxNumberOfParallelTasks: DisableParallelProcessing
? 1
: PackageManagementConstants.DefaultMaxDegreeOfParallelism,
enableNuGetAudit: true,
restoreAuditProperties,
logger: collectorLogger);

CheckRequireConsent();
Expand Down Expand Up @@ -458,6 +483,20 @@ private static Dictionary<string, string> GetPackagesConfigToProjectPath(Package
return configToProjectPath;
}

private static Dictionary<string, RestoreAuditProperties> GetRestoreAuditProperties(PackageRestoreInputs packageRestoreInputs)
{
Dictionary<string, RestoreAuditProperties> restoreAuditProperties = new(PathUtility.GetStringComparerBasedOnOS());
foreach (PackageSpec project in packageRestoreInputs.ProjectReferenceLookup.Projects)
nkolev92 marked this conversation as resolved.
Show resolved Hide resolved
{
if (project.RestoreMetadata?.ProjectStyle == ProjectStyle.PackagesConfig)
{
restoreAuditProperties.Add(project.FilePath, project.RestoreMetadata.RestoreAuditProperties);
}
}

return restoreAuditProperties;
}

/// <summary>
/// Processes List of PackageRestoreFailedEventArgs into a List of RestoreLogMessages.
/// </summary>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -849,7 +849,8 @@ private PackageSpec GetPackageSpec(IMSBuildProject project, IReadOnlyDictionary<
restoreMetadata = new PackagesConfigProjectRestoreMetadata
{
PackagesConfigPath = packagesConfigFilePath,
RepositoryPath = GetRepositoryPath(project, settings)
RepositoryPath = GetRepositoryPath(project, settings),
RestoreAuditProperties = auditProperties,
};
}
else
Expand Down
19 changes: 17 additions & 2 deletions src/NuGet.Core/NuGet.Build.Tasks/BuildTasksUtility.cs
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
using NuGet.Shared;
using static NuGet.Shared.XmlUtility;
using System.Globalization;
using System.Collections;
#endif

namespace NuGet.Build.Tasks
Expand Down Expand Up @@ -422,6 +423,7 @@ private static async Task<RestoreSummary> PerformNuGetV2RestoreAsync(Common.ILog
ISettings settings = null;

Dictionary<PackageReference, List<string>> packageReferenceToProjects = new(PackageReferenceComparer.Instance);
Dictionary<string, RestoreAuditProperties> restoreAuditProperties = new(PathUtility.GetStringComparerBasedOnOS());

foreach (PackageSpec packageSpec in dgFile.Projects.Where(i => i.RestoreMetadata.ProjectStyle == ProjectStyle.PackagesConfig))
{
Expand Down Expand Up @@ -454,8 +456,9 @@ private static async Task<RestoreSummary> PerformNuGetV2RestoreAsync(Common.ILog
value ??= new();
packageReferenceToProjects.Add(packageReference, value);
}
value.Add(packagesConfigPath);
value.Add(pcRestoreMetadata.PackagesConfigPath);
}
restoreAuditProperties.Add(packageSpec.FilePath, packageSpec.RestoreMetadata.RestoreAuditProperties);
}

if (string.IsNullOrEmpty(repositoryPath))
Expand All @@ -482,12 +485,22 @@ private static async Task<RestoreSummary> PerformNuGetV2RestoreAsync(Common.ILog
areAnyPackagesMissing |= !exists;
}

var repositories = sourceRepositoryProvider.GetRepositories().ToList();

if (!areAnyPackagesMissing)
{
using SourceCacheContext cacheContext = new();

var auditUtility = new AuditChecker(
repositories,
cacheContext,
log);

await auditUtility.CheckPackageVulnerabilitiesAsync(packageRestoreData, restoreAuditProperties, CancellationToken.None);

return new RestoreSummary(true);
}

var repositories = sourceRepositoryProvider.GetRepositories().ToArray();

var installCount = 0;
var failedEvents = new ConcurrentQueue<PackageRestoreFailedEventArgs>();
Expand All @@ -503,6 +516,8 @@ private static async Task<RestoreSummary> PerformNuGetV2RestoreAsync(Common.ILog
maxNumberOfParallelTasks: disableParallel
? 1
: PackageManagementConstants.DefaultMaxDegreeOfParallelism,
enableNuGetAudit: true,
restoreAuditProperties,
logger: collectorLogger);

// TODO: Check require consent?
Expand Down
2 changes: 2 additions & 0 deletions src/NuGet.Core/NuGet.Build.Tasks/NuGet.targets
Original file line number Diff line number Diff line change
Expand Up @@ -908,6 +908,8 @@ Copyright (c) .NET Foundation. All rights reserved.
<ConfigFilePaths>$(_OutputConfigFilePaths)</ConfigFilePaths>
<PackagesPath>$(_OutputPackagesPath)</PackagesPath>
<TargetFrameworks>@(_RestoreTargetFrameworksOutputFiltered)</TargetFrameworks>
<NuGetAudit>$(NuGetAudit)</NuGetAudit>
<NuGetAuditLevel>$(NuGetAuditLevel)</NuGetAuditLevel>
</_RestoreGraphEntry>
</ItemGroup>

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -305,7 +305,7 @@ await _logger.LogAsync(RestoreLogMessage.CreateWarning(NuGetLogCode.NU1803,
}

bool auditEnabled = AuditUtility.ParseEnableValue(
_request.Project.RestoreMetadata?.RestoreAuditProperties?.EnableAudit,
_request.Project.RestoreMetadata?.RestoreAuditProperties,
_request.Project.FilePath,
_logger);
telemetry.TelemetryEvent[AuditEnabled] = auditEnabled ? "enabled" : "disabled";
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
using NuGet.DependencyResolver;
using NuGet.LibraryModel;
using NuGet.Packaging.Core;
using NuGet.ProjectModel;
using NuGet.Protocol;
using NuGet.Protocol.Model;
using NuGet.Versioning;
Expand Down Expand Up @@ -363,27 +364,15 @@ private PackageVulnerabilitySeverity ParseAuditLevel()
return PackageVulnerabilitySeverity.Low;
}

if (string.Equals("low", auditLevel, StringComparison.OrdinalIgnoreCase))
if (_restoreAuditProperties!.TryParseAuditLevel(out PackageVulnerabilitySeverity result))
{
return PackageVulnerabilitySeverity.Low;
}
if (string.Equals("moderate", auditLevel, StringComparison.OrdinalIgnoreCase))
{
return PackageVulnerabilitySeverity.Moderate;
}
if (string.Equals("high", auditLevel, StringComparison.OrdinalIgnoreCase))
{
return PackageVulnerabilitySeverity.High;
}
if (string.Equals("critical", auditLevel, StringComparison.OrdinalIgnoreCase))
{
return PackageVulnerabilitySeverity.Critical;
return result;
}

string messageText = string.Format(Strings.Error_InvalidNuGetAuditLevelValue, auditLevel, "low, moderate, high, critical");
RestoreLogMessage message = RestoreLogMessage.CreateError(NuGetLogCode.NU1014, messageText);
_logger.Log(message);
return 0;
return PackageVulnerabilitySeverity.Low;
}

internal enum NuGetAuditMode { Unknown, Direct, All }
Expand Down Expand Up @@ -414,27 +403,22 @@ private NuGetAuditMode ParseAuditMode()
}

// Enum parsing and ToString are a magnitude of times slower than a naive implementation.
public static bool ParseEnableValue(string? value, string projectFullPath, ILogger logger)
public static bool ParseEnableValue(RestoreAuditProperties? value, string projectFullPath, ILogger logger)
{
// Earlier versions allowed "enable" and "default" to opt-in
if (string.IsNullOrEmpty(value)
|| string.Equals(value, bool.TrueString, StringComparison.OrdinalIgnoreCase)
|| string.Equals(value, "enable", StringComparison.OrdinalIgnoreCase)
|| string.Equals(value, "default", StringComparison.OrdinalIgnoreCase))
if (value == null)
{
return true;
}
if (string.Equals(value, bool.FalseString, StringComparison.OrdinalIgnoreCase)
|| string.Equals(value, "disable", StringComparison.OrdinalIgnoreCase))

if (!value.TryParseEnableAudit(out bool result))
{
return false;
string messageText = string.Format(Strings.Error_InvalidNuGetAuditValue, value, "true, false");
RestoreLogMessage message = RestoreLogMessage.CreateError(NuGetLogCode.NU1014, messageText);
message.ProjectPath = projectFullPath;
logger.Log(message);
}

string messageText = string.Format(Strings.Error_InvalidNuGetAuditValue, value, "true, false");
RestoreLogMessage message = RestoreLogMessage.CreateError(NuGetLogCode.NU1014, messageText);
message.ProjectPath = projectFullPath;
logger.Log(message);
return true;
return result;
}

// Enum parsing and ToString are a magnitude of times slower than a naive implementation.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -285,7 +285,7 @@ public static PackageSpec GetPackageSpec(IEnumerable<IMSBuildItem> items)
);
}
pcRestoreMetadata.RestoreLockProperties = GetRestoreLockProperties(specItem);

pcRestoreMetadata.RestoreAuditProperties = GetRestoreAuditProperties(specItem);
}

if (restoreType == ProjectStyle.ProjectJson)
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,89 @@
// Copyright (c) .NET Foundation. All rights reserved.
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

#nullable enable

using System;
using System.Collections.Generic;
using NuGet.Common;
using NuGet.Packaging.Core;

namespace NuGet.PackageManagement
{
public record AuditCheckResult
{
public IReadOnlyList<ILogMessage> Warnings { get; }
internal bool IsAuditEnabled { get; set; } = true;
zivkan marked this conversation as resolved.
Show resolved Hide resolved

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; }
Comment on lines +16 to +26
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

@zivkan zivkan Feb 26, 2024

Choose a reason for hiding this comment

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

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.


private const string AuditVulnerabilitiesStatus = "PackagesConfig.Audit.Enabled";
private const string AuditVulnerabilitiesCount = "PackagesConfig.Audit.Vulnerability.Count";
private const string AuditVulnerabilitiesSev0Count = "PackagesConfig.Audit.Vulnerability.Severity0.Count";
private const string AuditVulnerabilitiesSev1Count = "PackagesConfig.Audit.Vulnerability.Severity1.Count";
private const string AuditVulnerabilitiesSev2Count = "PackagesConfig.Audit.Vulnerability.Severity2.Count";
private const string AuditVulnerabilitiesSev3Count = "PackagesConfig.Audit.Vulnerability.Severity3.Count";
private const string AuditVulnerabilitiesInvalidSeverityCount = "PackagesConfig.Audit.Vulnerability.SeverityInvalid.Count";
private const string AuditDurationDownload = "PackagesConfig.Audit.Duration.Download";
private const string AuditDurationCheck = "PackagesConfig.Audit.Duration.Check";
private const string SourcesWithVulnerabilitiesCount = "PackagesConfig.Audit.DataSources.Count";
private const string AuditVulnerabilitiesPackages = "PackagesConfig.Audit.Vulnerability.Packages";


internal AuditCheckResult(IReadOnlyList<ILogMessage> warnings)
{
if (warnings is null)
{
throw new ArgumentNullException(nameof(warnings));
}

Warnings = warnings;
}

public void AddMetricsToTelemetry(TelemetryEvent telemetryEvent)
{
telemetryEvent[AuditVulnerabilitiesStatus] = IsAuditEnabled;
telemetryEvent[AuditVulnerabilitiesSev0Count] = Severity0VulnerabilitiesFound;
telemetryEvent[AuditVulnerabilitiesSev1Count] = Severity1VulnerabilitiesFound;
telemetryEvent[AuditVulnerabilitiesSev2Count] = Severity2VulnerabilitiesFound;
telemetryEvent[AuditVulnerabilitiesSev3Count] = Severity3VulnerabilitiesFound;
telemetryEvent[AuditVulnerabilitiesInvalidSeverityCount] = InvalidSeverityVulnerabilitiesFound;
telemetryEvent[AuditVulnerabilitiesCount] = Packages?.Count ?? 0;

if (DownloadDurationInSeconds.HasValue)
{
telemetryEvent[AuditDurationDownload] = DownloadDurationInSeconds;
}
if (CheckPackagesDurationInSeconds.HasValue)
{
telemetryEvent[AuditDurationCheck] = CheckPackagesDurationInSeconds;
}

if (SourcesWithVulnerabilities.HasValue)
{
telemetryEvent[SourcesWithVulnerabilitiesCount] = SourcesWithVulnerabilities;
}

if (Packages is not null)
{
List<TelemetryEvent> result = new List<TelemetryEvent>(Packages.Count);
foreach (var package in Packages)
{
TelemetryEvent packageData = new TelemetryEvent(eventName: string.Empty);
packageData.AddPiiData("id", package.Id.ToLowerInvariant());
packageData["version"] = package.Version;
result.Add(packageData);
}
telemetryEvent.ComplexData[AuditVulnerabilitiesPackages] = result;
}
}
}
}
Loading