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

add signature VerifySigning for dotnet tools #39268

Merged
merged 21 commits into from
Aug 28, 2024
Merged
Show file tree
Hide file tree
Changes from 13 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 @@ -74,7 +74,7 @@ internal bool IsFirstParty(FilePath nupkgToVerify)
}
}

private static bool NuGetVerify(FilePath nupkgToVerify, out string commandOutput)
public static bool NuGetVerify(FilePath nupkgToVerify, out string commandOutput)
{
var args = new[] { "verify", "--all", nupkgToVerify.Value };
var command = new DotNetCommandFactory(alwaysRunOutOfProc: true)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -136,7 +136,8 @@
<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
29 changes: 17 additions & 12 deletions src/Cli/dotnet/NugetPackageDownloader/NuGetPackageDownloader.cs
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 @@ -130,8 +127,8 @@ public async Task<string> DownloadPackageAsync(PackageId packageId,
packageVersion.ToNormalizedString()));
}

VerifySigning(nupkgPath);

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

Expand All @@ -141,7 +138,7 @@ private bool verbosityGreaterThanMinimal()
&& _verbosityOptions != VerbosityOptions.minimal && _verbosityOptions != VerbosityOptions.m;
}

private void VerifySigning(string nupkgPath)
private async Task VerifySigning(string nupkgPath, SourceRepository repository)
{
if (!_verifySignatures && !_validationMessagesDisplayed)
{
Expand All @@ -157,14 +154,22 @@ private void VerifySigning(string nupkgPath)
return;
Copy link
Contributor

@kartheekp-ms kartheekp-ms May 1, 2024

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 (await repository.GetResourceAsync<RepositorySignatureResource>().ConfigureAwait(false) is var resource &&
Forgind marked this conversation as resolved.
Show resolved Hide resolved
resource.AllRepositorySigned)
{
if (!_firstPartyNuGetPackageSigningVerifier.Verify(new FilePath(nupkgPath),
out string commandOutput))
if (!_shouldUsePackageSourceMapping)
{
throw new NuGetPackageInstallerException(LocalizableStrings.FailedToValidatePackageSigning +
Environment.NewLine +
commandOutput);
if (!_firstPartyNuGetPackageSigningVerifier.Verify(new FilePath(nupkgPath), out string commandOutput))
{
throw new NuGetPackageInstallerException(string.Format(LocalizableStrings.FailedToValidatePackageSigning, commandOutput));
}
}
else
{
if (!FirstPartyNuGetPackageSigningVerifier.NuGetVerify(new FilePath(nupkgPath), out string commandOutput))
{
throw new NuGetPackageInstallerException(string.Format(LocalizableStrings.FailedToValidatePackageSigning, commandOutput));
}
}
}
}
Expand Down

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

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

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

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

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

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

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

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

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

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

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

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

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

3 changes: 2 additions & 1 deletion src/Cli/dotnet/ToolPackage/IToolPackageDownloader.cs
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,8 @@ IToolPackage InstallPackage(PackageLocation packageLocation,
VersionRange versionRange = null,
string targetFramework = null,
bool isGlobalTool = false,
bool isGlobalToolRollForward = false
bool isGlobalToolRollForward = false,
bool verifySignatures = true
);

NuGetVersion GetNuGetVersion(
Expand Down
22 changes: 7 additions & 15 deletions src/Cli/dotnet/ToolPackage/ToolPackageDownloader.cs
Original file line number Diff line number Diff line change
@@ -1,21 +1,17 @@
// Copyright (c) .NET Foundation and contributors. All rights reserved.
// Licensed under the MIT license. See LICENSE file in the project root for full license information.

using System;
using System.Collections.Generic;
using System.IO;
using System.Linq;
using System.CommandLine;
using System.Reflection;
using System.Runtime.InteropServices;
using System.Threading.Tasks;
using Microsoft.DotNet.Cli.NuGetPackageDownloader;
using Microsoft.DotNet.Cli.Utils;
using Microsoft.DotNet.ToolPackage;
using Microsoft.DotNet.Tools;
using Microsoft.Extensions.EnvironmentAbstractions;
using Microsoft.TemplateEngine.Utils;
using Newtonsoft.Json.Linq;
using NuGet.Client;
using NuGet.Common;
using NuGet.Configuration;
using NuGet.ContentModel;
using NuGet.Frameworks;
using NuGet.LibraryModel;
Expand All @@ -25,12 +21,6 @@
using NuGet.Repositories;
using NuGet.RuntimeModel;
using NuGet.Versioning;
using NuGet.Configuration;
using Microsoft.TemplateEngine.Utils;
using System.Text.Json;
using System.Xml;
using System.Text.Json.Nodes;
using Newtonsoft.Json.Linq;

namespace Microsoft.DotNet.Cli.ToolPackage
{
Expand Down Expand Up @@ -77,7 +67,8 @@ public IToolPackage InstallPackage(PackageLocation packageLocation, PackageId pa
VersionRange versionRange = null,
string targetFramework = null,
bool isGlobalTool = false,
bool isGlobalToolRollForward = false
bool isGlobalToolRollForward = false,
bool verifySignatures = true
)
{
var packageRootDirectory = _toolPackageStore.GetRootPackageDirectory(packageId);
Expand All @@ -100,7 +91,8 @@ public IToolPackage InstallPackage(PackageLocation packageLocation, PackageId pa

var toolDownloadDir = isGlobalTool ? _globalToolStageDir : _localToolDownloadDir;
var assetFileDirectory = isGlobalTool ? _globalToolStageDir : _localToolAssetDir;
var nugetPackageDownloader = new NuGetPackageDownloader.NuGetPackageDownloader(toolDownloadDir, verboseLogger: nugetLogger, shouldUsePackageSourceMapping: true, verbosityOptions: verbosity);

var nugetPackageDownloader = new NuGetPackageDownloader.NuGetPackageDownloader(toolDownloadDir, verboseLogger: nugetLogger, verifySignatures: verifySignatures, shouldUsePackageSourceMapping: true, verbosityOptions: verbosity);

var packageSourceLocation = new PackageSourceLocation(packageLocation.NugetConfig, packageLocation.RootConfigDirectory, null, packageLocation.AdditionalFeeds);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,7 @@ public ToolInstallGlobalOrToolPathCommand(
NoCache: (parseResult.GetValue(ToolCommandRestorePassThroughOptions.NoCacheOption) || parseResult.GetValue(ToolCommandRestorePassThroughOptions.NoHttpCacheOption)),
IgnoreFailedSources: parseResult.GetValue(ToolCommandRestorePassThroughOptions.IgnoreFailedSourcesOption),
Interactive: parseResult.GetValue(ToolCommandRestorePassThroughOptions.InteractiveRestoreOption));
nugetPackageDownloader ??= new NuGetPackageDownloader(tempDir, verboseLogger: new NullLogger(), restoreActionConfig: restoreAction, verbosityOptions: _verbosity);
nugetPackageDownloader ??= new NuGetPackageDownloader(tempDir, verboseLogger: new NullLogger(), restoreActionConfig: restoreAction, verbosityOptions: _verbosity, verifySignatures: true);
Copy link
Member

Choose a reason for hiding this comment

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

It looks like _verifySignatures isn't being used here. Shouldn't it be?

Suggested change
nugetPackageDownloader ??= new NuGetPackageDownloader(tempDir, verboseLogger: new NullLogger(), restoreActionConfig: restoreAction, verbosityOptions: _verbosity, verifySignatures: true);
nugetPackageDownloader ??= new NuGetPackageDownloader(tempDir, verboseLogger: new NullLogger(), restoreActionConfig: restoreAction, verbosityOptions: _verbosity, verifySignatures: _verifySignatures ?? true);

Copy link
Member

@Forgind Forgind Aug 27, 2024

Choose a reason for hiding this comment

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

This NuGetPackageDownloader is only used to find the app host, so I didn't think I needed to change this, but when I dug a little deeper just now, it does download the app host, so I think that if someone has a feed with a malicious version of the app host that returns faster than nuget.org, that could be bad, so I think you're right. Good catch.

Edit: I missed that it was changed here to unconditionally be true. Since this should always be a Microsoft package, I think it should always be signed.

Copy link
Member

Choose a reason for hiding this comment

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

Not necessarily, what happens when you're using a daily build of .NET and pulling the apphost from the dotnet-public feed? Those would neither have repository signatures and the package would also not have publisher signatures from Microsoft.

Copy link
Member

Choose a reason for hiding this comment

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

Ok. Note that verifySignatures was not available here before the test change, so if the consensus is that it is needed for that nugetPackageDownloader, then we need to keep (at least part of) the test changes as well.

_shellShimTemplateFinder = new ShellShimTemplateFinder(nugetPackageDownloader, tempDir, packageSourceLocation);
_store = store;

Expand Down
Loading
Loading