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

Jump to bottom

add signature VerifySigning for dotnet tools #39268

Merged
merged 21 commits into from
Aug 28, 2024
Merged

add signature VerifySigning for dotnet tools #39268

Show file tree
Hide file tree
Changes from 20 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
ac94e8c
add signature VerifySigning for dotnet tools and add tests
JL03-Yue Mar 6, 2024
00331c9
fix tests
JL03-Yue Apr 12, 2024
2d5f551
fix tests
JL03-Yue Apr 15, 2024
4bca9df
remove duplicate definition of verifysignature
JL03-Yue Apr 17, 2024
060e172
fix tests
JL03-Yue Apr 17, 2024
5ade9d9
fix builds
JL03-Yue Apr 17, 2024
ab30839
fix tests
JL03-Yue Apr 17, 2024
df58e0b
Resolve comments
Forgind Jul 23, 2024
89b822d
Change that looks good but isn't right
Forgind Jul 23, 2024
aaeccf3
Fix NuGet signing check
Forgind Jul 31, 2024
66fbe72
Merge branch 'main' into tool-update-signature-verification
Forgind Jul 31, 2024
3a22067
Change name
Forgind Jul 31, 2024
50378d1
Add paramName:
Forgind Jul 31, 2024
87520e5
Update src/Cli/dotnet/NugetPackageDownloader/NuGetPackageDownloader.cs
Forgind Jul 31, 2024
2b449fc
Update src/Cli/dotnet/NugetPackageDownloader/NuGetPackageDownloader.cs
Forgind Aug 1, 2024
deabd72
little tweak
Forgind Aug 1, 2024
a7b9ae4
Delete test that can no longer work
Forgind Aug 16, 2024
196ab12
Add messages
Forgind Aug 19, 2024
131cff5
Make test skip sign check
Forgind Aug 23, 2024
9e4910b
Make other test pass (and merge into theory)
Forgind Aug 24, 2024
24658d5
Comments
Forgind Aug 27, 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
18 changes: 12 additions & 6 deletions src/Cli/dotnet/CommandFactory/CommandFactoryUsingResolver.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,12 +15,14 @@ public static Command CreateDotNet(
string commandName,
IEnumerable<string> args,
NuGetFramework framework = null,
string configuration = Constants.DefaultConfiguration)
string configuration = Constants.DefaultConfiguration,
string currentWorkingDirectory = null)
{
return Create("dotnet",
new[] { commandName }.Concat(args),
framework,
configuration: configuration);
configuration: configuration,
currentWorkingDirectory);
}

/// <summary>
Expand All @@ -35,7 +37,8 @@ public static Command Create(
NuGetFramework framework = null,
string configuration = Constants.DefaultConfiguration,
string outputPath = null,
string applicationName = null)
string applicationName = null,
string currentWorkingDirectory = null)
{
return Create(
new DefaultCommandResolverPolicy(),
Expand All @@ -44,7 +47,8 @@ public static Command Create(
framework,
configuration,
outputPath,
applicationName);
applicationName,
currentWorkingDirectory);
}

public static Command Create(
Expand All @@ -54,7 +58,8 @@ public static Command Create(
NuGetFramework framework = null,
string configuration = Constants.DefaultConfiguration,
string outputPath = null,
string applicationName = null)
string applicationName = null,
string currentWorkingDirectory = null)
{
var commandSpec = CommandResolver.TryResolveCommandSpec(
commandResolverPolicy,
Expand All @@ -63,7 +68,8 @@ public static Command Create(
framework,
configuration: configuration,
outputPath: outputPath,
applicationName: applicationName);
applicationName: applicationName,
currentWorkingDirectory: currentWorkingDirectory);

if (commandSpec == null)
{
Expand Down
14 changes: 8 additions & 6 deletions src/Cli/dotnet/CommandFactory/CommandResolution/DefaultCommandResolverPolicy.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,12 +7,12 @@ namespace Microsoft.DotNet.CommandFactory
{
public class DefaultCommandResolverPolicy : ICommandResolverPolicy
{
public CompositeCommandResolver CreateCommandResolver()
public CompositeCommandResolver CreateCommandResolver(string currentWorkingDirectory = null)
{
return Create();
return Create(currentWorkingDirectory);
}

public static CompositeCommandResolver Create()
public static CompositeCommandResolver Create(string currentWorkingDirectory = null)
{
var environment = new EnvironmentProvider();
var packagedCommandSpecFactory = new PackagedCommandSpecFactoryWithCliRuntime();
Expand All @@ -32,20 +32,22 @@ public static CompositeCommandResolver Create()
environment,
packagedCommandSpecFactory,
platformCommandSpecFactory,
publishedPathCommandSpecFactory);
publishedPathCommandSpecFactory,
currentWorkingDirectory);
}

public static CompositeCommandResolver CreateDefaultCommandResolver(
IEnvironmentProvider environment,
IPackagedCommandSpecFactory packagedCommandSpecFactory,
IPlatformCommandSpecFactory platformCommandSpecFactory,
IPublishedPathCommandSpecFactory publishedPathCommandSpecFactory)
IPublishedPathCommandSpecFactory publishedPathCommandSpecFactory,
string currentWorkingDirectory = null)
{
var compositeCommandResolver = new CompositeCommandResolver();

compositeCommandResolver.AddCommandResolver(new MuxerCommandResolver());
compositeCommandResolver.AddCommandResolver(new DotnetToolsCommandResolver());
compositeCommandResolver.AddCommandResolver(new LocalToolsCommandResolver());
compositeCommandResolver.AddCommandResolver(new LocalToolsCommandResolver(currentWorkingDirectory: currentWorkingDirectory));
compositeCommandResolver.AddCommandResolver(new RootedCommandResolver());
compositeCommandResolver.AddCommandResolver(
new ProjectToolsCommandResolver(packagedCommandSpecFactory, environment));
Expand Down
2 changes: 1 addition & 1 deletion src/Cli/dotnet/CommandFactory/CommandResolution/ICommandResolverPolicy.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,6 @@ namespace Microsoft.DotNet.CommandFactory
{
public interface ICommandResolverPolicy
{
CompositeCommandResolver CreateCommandResolver();
CompositeCommandResolver CreateCommandResolver(string currentWorkingDirectory = null);
}
}
5 changes: 3 additions & 2 deletions src/Cli/dotnet/CommandFactory/CommandResolution/LocalToolsCommandResolver.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,9 +21,10 @@ internal class LocalToolsCommandResolver : ICommandResolver
public LocalToolsCommandResolver(
ToolManifestFinder toolManifest = null,
ILocalToolsResolverCache localToolsResolverCache = null,
IFileSystem fileSystem = null)
IFileSystem fileSystem = null,
string currentWorkingDirectory = null)
{
_toolManifest = toolManifest ?? new ToolManifestFinder(new DirectoryPath(Directory.GetCurrentDirectory()));
_toolManifest = toolManifest ?? new ToolManifestFinder(new DirectoryPath(currentWorkingDirectory ?? Directory.GetCurrentDirectory()));
_localToolsResolverCache = localToolsResolverCache ?? new LocalToolsResolverCache();
_fileSystem = fileSystem ?? new FileSystemWrapper();
}
Expand Down
2 changes: 1 addition & 1 deletion src/Cli/dotnet/CommandFactory/CommandResolution/PathCommandResolverPolicy.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ namespace Microsoft.DotNet.CommandFactory
{
public class PathCommandResolverPolicy : ICommandResolverPolicy
{
public CompositeCommandResolver CreateCommandResolver()
public CompositeCommandResolver CreateCommandResolver(string CurrentWorkingDirectory = null)
{
return Create();
}
Expand Down
7 changes: 4 additions & 3 deletions src/Cli/dotnet/CommandFactory/CommandResolver.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,20 +33,21 @@ public static CommandSpec TryResolveCommandSpec(
NuGetFramework framework = null,
string configuration = Constants.DefaultConfiguration,
string outputPath = null,
string applicationName = null)
string applicationName = null,
string currentWorkingDirectory = null)
{
var commandResolverArgs = new CommandResolverArguments
{
CommandName = commandName,
CommandArguments = args,
Framework = framework,
ProjectDirectory = Directory.GetCurrentDirectory(),
ProjectDirectory = currentWorkingDirectory ?? Directory.GetCurrentDirectory(),
Configuration = configuration,
OutputPath = outputPath,
ApplicationName = applicationName
};

var defaultCommandResolver = commandResolverPolicy.CreateCommandResolver();
var defaultCommandResolver = commandResolverPolicy.CreateCommandResolver(currentWorkingDirectory);

return defaultCommandResolver.Resolve(commandResolverArgs);
}
Expand Down
6 changes: 4 additions & 2 deletions src/Cli/dotnet/DotNetCommandFactory.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,10 +12,12 @@ namespace Microsoft.DotNet.Cli
public class DotNetCommandFactory : ICommandFactory
{
private bool _alwaysRunOutOfProc;
private readonly string _currentWorkingDirectory;

public DotNetCommandFactory(bool alwaysRunOutOfProc = false)
public DotNetCommandFactory(bool alwaysRunOutOfProc = false, string currentWorkingDirectory = null)
{
_alwaysRunOutOfProc = alwaysRunOutOfProc;
_currentWorkingDirectory = currentWorkingDirectory;
}

public ICommand Create(
Expand All @@ -32,7 +34,7 @@ public ICommand Create(
return new BuiltInCommand(commandName, args, builtInCommand);
}

return CommandFactoryUsingResolver.CreateDotNet(commandName, args, framework, configuration);
return CommandFactoryUsingResolver.CreateDotNet(commandName, args, framework, configuration, _currentWorkingDirectory);
}

private bool TryGetBuiltInCommand(string commandName, out Func<string[], int> commandFunc)
Expand Down
4 changes: 2 additions & 2 deletions src/Cli/dotnet/NugetPackageDownloader/FirstPartyNuGetPackageSigningVerifier.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,10 +74,10 @@ internal bool IsFirstParty(FilePath nupkgToVerify)
}
}

private static bool NuGetVerify(FilePath nupkgToVerify, out string commandOutput)
public static bool NuGetVerify(FilePath nupkgToVerify, out string commandOutput, string currentWorkingDirectory = null)
{
var args = new[] { "verify", "--all", nupkgToVerify.Value };
var command = new DotNetCommandFactory(alwaysRunOutOfProc: true)
var command = new DotNetCommandFactory(alwaysRunOutOfProc: true, currentWorkingDirectory)
.Create("nuget", args);

var commandResult = command.CaptureStdOut().Execute();
Expand Down
9 changes: 8 additions & 1 deletion src/Cli/dotnet/NugetPackageDownloader/LocalizableStrings.resx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,11 +132,18 @@
<data name="NuGetPackageSignatureVerificationSkipped" xml:space="preserve">
<value>Skipping NuGet package signature verification.</value>
</data>
<data name="VerifyingNuGetPackageSignature" xml:space="preserve">
<value>Verified that the NuGet package "{0}" has a valid signature.</value>
</data>
<data name="NuGetPackageShouldNotBeSigned" xml:space="preserve">
<value>Skipping signature verification for NuGet package "{0}" because it comes from a source that does not require signature validation.</value>
</data>
<data name="SkipNuGetpackageSigningValidationmacOSLinux" xml:space="preserve">
<value>Skip NuGet package signing validation. NuGet signing validation is not available on Linux or macOS https://aka.ms/workloadskippackagevalidation .</value>
Copy link
Contributor

Choose a reason for hiding this comment

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

This aka.ms link points to an announcement which doesn't apply to more recent versions of .NET.

Signed package verification is supported on Linux. See https://learn.microsoft.com/en-us/dotnet/core/tools/nuget-signed-package-verification#linux.

</data>
<data name="FailedToValidatePackageSigning" xml:space="preserve">
<value>Failed to validate package signing.</value>
<value>Failed to validate package signing.
{0}</value>
</data>
<data name="FailedToFindSourceUnderPackageSourceMapping" xml:space="preserve">
<value>Package Source Mapping is enabled, but no source found under the specified package ID: {0}. See the documentation for Package Source Mapping at https://aka.ms/nuget-package-source-mapping for more details.</value>
Expand Down
50 changes: 30 additions & 20 deletions src/Cli/dotnet/NugetPackageDownloader/NuGetPackageDownloader.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,10 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using System.Text.RegularExpressions;
using System.Threading;
using Microsoft.DotNet.Cli.Utils;
using Microsoft.DotNet.ToolPackage;
using Microsoft.DotNet.Tools;
using Microsoft.Extensions.EnvironmentAbstractions;
using Microsoft.TemplateEngine.Abstractions;
using NuGet.Common;
using NuGet.Configuration;
using NuGet.Credentials;
Expand Down Expand Up @@ -43,6 +40,7 @@ internal class NuGetPackageDownloader : INuGetPackageDownloader

private bool _verifySignatures;
private VerbosityOptions _verbosityOptions;
private readonly string _currentWorkingDirectory;

public NuGetPackageDownloader(
DirectoryPath packageInstallDir,
Expand All @@ -54,8 +52,10 @@ public NuGetPackageDownloader(
Func<IEnumerable<Task>> timer = null,
bool verifySignatures = false,
bool shouldUsePackageSourceMapping = false,
VerbosityOptions verbosityOptions = VerbosityOptions.normal)
VerbosityOptions verbosityOptions = VerbosityOptions.normal,
string currentWorkingDirectory = null)
{
_currentWorkingDirectory = currentWorkingDirectory;
_packageInstallDir = packageInstallDir;
_reporter = reporter ?? Reporter.Output;
_verboseLogger = verboseLogger ?? new NuGetConsoleLogger();
Expand Down Expand Up @@ -130,22 +130,22 @@ public async Task<string> DownloadPackageAsync(PackageId packageId,
packageVersion.ToNormalizedString()));
}

VerifySigning(nupkgPath);

await VerifySigning(nupkgPath, repository);
return nupkgPath;
}

private bool verbosityGreaterThanMinimal()
{
return _verbosityOptions != VerbosityOptions.quiet && _verbosityOptions != VerbosityOptions.q
&& _verbosityOptions != VerbosityOptions.minimal && _verbosityOptions != VerbosityOptions.m;
}
private bool VerbosityGreaterThanMinimal() =>
_verbosityOptions != VerbosityOptions.quiet && _verbosityOptions != VerbosityOptions.q &&
_verbosityOptions != VerbosityOptions.minimal && _verbosityOptions != VerbosityOptions.m;

private void VerifySigning(string nupkgPath)
private bool DiagnosticVerbosity() => _verbosityOptions == VerbosityOptions.diag || _verbosityOptions == VerbosityOptions.diagnostic;

private async Task VerifySigning(string nupkgPath, SourceRepository repository)
{
if (!_verifySignatures && !_validationMessagesDisplayed)
{
if (verbosityGreaterThanMinimal())
if (VerbosityGreaterThanMinimal())
{
_reporter.WriteLine(LocalizableStrings.NuGetPackageSignatureVerificationSkipped);
}
Expand All @@ -157,15 +157,25 @@ private void VerifySigning(string nupkgPath)
return;
Copy link
Contributor

@kartheekp-ms kartheekp-ms May 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I am not sure if this is the correct location to add the comment. It could be possible that I don't have a good understanding of the code in this repo. I think we should set verifySignatures to true when trying to execute the dotnet tool install command.

https://github.com/JL03-Yue/sdk/blob/684d26597df5b1e06a799ce7481d97e6670fdd79/src/Cli/dotnet/commands/dotnet-tool/install/ToolInstallGlobalOrToolPathCommand.cs#L84

If we don't set verifySignatures = true, then I think this if block will cause the control to return immediately, thereby skipping signature verification.

nugetPackageDownloader ??= new NuGetPackageDownloader(tempDir, verboseLogger: new NullLogger(), restoreActionConfig: restoreAction, verbosityOptions: _verbosity);

Suggestion is to pass the optional parameter `verifySignatures = true` here also.

}

if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows))
if (repository is not null &&
await repository.GetResourceAsync<RepositorySignatureResource>().ConfigureAwait(false) is RepositorySignatureResource resource &&
resource.AllRepositorySigned)
{
if (!_firstPartyNuGetPackageSigningVerifier.Verify(new FilePath(nupkgPath),
out string commandOutput))
string commandOutput;
if ((!_shouldUsePackageSourceMapping && !_firstPartyNuGetPackageSigningVerifier.Verify(new FilePath(nupkgPath), out commandOutput)) ||
(_shouldUsePackageSourceMapping && !FirstPartyNuGetPackageSigningVerifier.NuGetVerify(new FilePath(nupkgPath), out commandOutput, _currentWorkingDirectory)))
Comment on lines +168 to +169
Copy link
Member

Choose a reason for hiding this comment

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

What's the difference between _firstPartyNuGetPackageSigningVerifier.Verify and FirstPartyNuGetPackageSigningVerifier.NuGetVerify? That's not obvious to me and it seems like a comment explaining would be helpful.

Copy link
Member

Choose a reason for hiding this comment

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

They certainly look the same. I can add a comment. It's whether or not it should additionally check that it's a first-party binary or not.

{
throw new NuGetPackageInstallerException(LocalizableStrings.FailedToValidatePackageSigning +
Environment.NewLine +
commandOutput);
throw new NuGetPackageInstallerException(string.Format(LocalizableStrings.FailedToValidatePackageSigning, commandOutput));
}

if (DiagnosticVerbosity())
{
_reporter.WriteLine(LocalizableStrings.VerifyingNuGetPackageSignature, Path.GetFileNameWithoutExtension(nupkgPath));
}
}
else if (DiagnosticVerbosity())
{
_reporter.WriteLine(LocalizableStrings.NuGetPackageShouldNotBeSigned, Path.GetFileNameWithoutExtension(nupkgPath));
}
}

Expand Down Expand Up @@ -308,7 +318,7 @@ private static bool PackageIsInAllowList(IEnumerable<string> files)
private IEnumerable<PackageSource> LoadNuGetSources(PackageId packageId, PackageSourceLocation packageSourceLocation = null, PackageSourceMapping packageSourceMapping = null)
{
List<PackageSource> defaultSources = new List<PackageSource>();
string currentDirectory = Directory.GetCurrentDirectory();
string currentDirectory = _currentWorkingDirectory ?? Directory.GetCurrentDirectory();
ISettings settings;
if (packageSourceLocation?.NugetConfig != null)
{
Expand Down
16 changes: 14 additions & 2 deletions src/Cli/dotnet/NugetPackageDownloader/xlf/LocalizableStrings.cs.xlf
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

16 changes: 14 additions & 2 deletions src/Cli/dotnet/NugetPackageDownloader/xlf/LocalizableStrings.de.xlf
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading
Loading