add signature VerifySigning for dotnet tools #39268

JL03-Yue · 2024-03-06T21:34:39Z

#37469

Add signature verifications for NuGet Tools

dsplaisted

Some scenarios to test (manually):

Can you install a non-Microsoft tool from NuGet.org?
Can you install a locally-built tool (which won't be signed at all)?

src/Cli/dotnet/NugetPackageDownloader/NuGetPackageDownloader.cs

JL03-Yue · 2024-04-17T22:25:07Z

Thank you very much for the review!

For the two manual test questions:

Can you install a non-Microsoft tool from NuGet.org?

Yes, for example, Cake.tool can be installed. It sounds to me that the tool can have various signings, such as package signs and organization signs.

Can you install a locally-built tool (which won't be signed at all)?

No, from manual test, locally built tools cannot be installed because the signing is not validated. I think this is not the expected behavior and will need a work-around about this.

@dsplaisted

dsplaisted · 2024-04-18T22:47:42Z

No, from manual test, locally built tools cannot be installed because the signing is not validated. I think this is not the expected behavior and will need a work-around about this.

Yes, I think we would want to use the same type of logic that NuGet uses for PackageReferences to determine whether we need to verify whether the package is signed.

kartheekp-ms · 2024-05-01T22:08:45Z

src/Cli/dotnet/NugetPackageDownloader/NuGetPackageDownloader.cs

            {
-                if (!_firstPartyNuGetPackageSigningVerifier.Verify(new FilePath(nupkgPath),
-                    out string commandOutput))
+                if(RuntimeInformation.IsOSPlatform(OSPlatform.Windows))


Given that NuGet package signature verification is enabled by default on Linux in .NET 8 #31868 PR, I think we could enable signature verification for .NET Tools NuGet packages on Linux also.

cc @dtivel

Can the default behavior be the same as https://github.com/dotnet/sdk/pull/31868/files, driven by the same environment variable?

kartheekp-ms · 2024-05-01T22:15:34Z

src/Cli/dotnet/NugetPackageDownloader/NuGetPackageDownloader.cs

-            if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows))
+            if (!_isNuGetTool)
+            {
+                if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows))


Same comment as https://github.com/dotnet/sdk/pull/39268/files#r1586876370

kartheekp-ms · 2024-05-01T22:28:07Z

src/Cli/dotnet/NugetPackageDownloader/NuGetPackageDownloader.cs

+        private static bool NuGetVerify(FilePath nupkgToVerify, out string commandOutput)
+        {
+            var args = new[] { "verify", "--all", nupkgToVerify.Value };
+            var command = new DotNetCommandFactory(alwaysRunOutOfProc: true)
+                .Create("nuget", args);
+
+            var commandResult = command.CaptureStdOut().Execute();
+            commandOutput = commandResult.StdOut + Environment.NewLine + commandResult.StdErr;
+            return commandResult.ExitCode == 0;
+        }
+


Can we leverage the existing logic instead of code duplication?

sdk/src/Cli/dotnet/NugetPackageDownloader/FirstPartyNuGetPackageSigningVerifier.cs

Line 36 in 4544337

public bool Verify(FilePath nupkgToVerify, out string commandOutput)

kartheekp-ms · 2024-05-01T22:31:57Z

src/Cli/dotnet/NugetPackageDownloader/NuGetPackageDownloader.cs

@@ -157,18 +154,48 @@ private void VerifySigning(string nupkgPath)
                return;


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.

dtivel · 2024-05-03T21:14:55Z

src/Tests/Microsoft.DotNet.PackageInstall.Tests/ToolPackageDownloaderTests.cs

@@ -54,7 +54,8 @@ public void GivenNugetConfigInstallSucceeds(bool testMockBehaviorIsInSync)
                verbosity: TestVerbosity,
                versionRange: VersionRange.Parse(TestPackageVersion),
                targetFramework: _testTargetframework,
-                isGlobalTool: true);
+                isGlobalTool: true,
+                verifySignatures: false);


Is there a test that does verify signatures?

src/Cli/dotnet/NugetPackageDownloader/NuGetPackageDownloader.cs

This test can no longer work. The sign check only happens when a nuget package source says the nuget package should be signed, and the only way to make that happen in a test is to download the package from an https resource, which generally isn't a good idea in the middle of a test regardless and certainly isn't a good idea if we need to find a package that should be signed but isn't (i.e., may be malicious). I propose replacing this test with manual tests.

marcpopMSFT · 2024-08-20T21:20:07Z

@Forgind is this now ready for review (modulo the remaining failing leg). Added @joeloff to review.

Forgind · 2024-08-21T18:53:25Z

@Forgind is this now ready for review (modulo the remaining failing leg). Added @joeloff to review.

Yes, I believe so. I reran the failing leg, and it failed again, so I think it's a real failure. It seems to be because a package is very out-of-date, so I think the right answer is just to update to a later package. The failing tests are for unlisted packages, though, and I'm not sure how to find them. @joeloff, is there a way to find a more up-to-date unlisted package we can use for those tests?

dsplaisted · 2024-08-26T23:06:33Z

src/Cli/dotnet/NugetPackageDownloader/NuGetPackageDownloader.cs

+                if ((!_shouldUsePackageSourceMapping && !_firstPartyNuGetPackageSigningVerifier.Verify(new FilePath(nupkgPath), out commandOutput)) ||
+                    (_shouldUsePackageSourceMapping && !FirstPartyNuGetPackageSigningVerifier.NuGetVerify(new FilePath(nupkgPath), out commandOutput, _currentWorkingDirectory)))


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.

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.

dsplaisted · 2024-08-26T23:09:03Z

src/Cli/dotnet/commands/dotnet-tool/install/ToolInstallGlobalOrToolPathCommand.cs

@@ -89,7 +94,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);


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);

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.

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.

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.

dsplaisted · 2024-08-26T23:11:59Z

test/Microsoft.DotNet.PackageInstall.Tests/ToolPackageDownloaderTests.cs

@@ -54,7 +54,8 @@ public void GivenNugetConfigInstallSucceeds(bool testMockBehaviorIsInSync)
                verbosity: TestVerbosity,
                versionRange: VersionRange.Parse(TestPackageVersion),
                targetFramework: _testTargetframework,
-                isGlobalTool: true);
+                isGlobalTool: true,
+                verifySignatures: false);


Do we need all these test changes to specify verifySignatures = false? It seems to me like in the tests there won't be a RepositorySignatureResource or it won't specify AllRepositorySigned, so the downloader won't verify the signatures anyway and we shouldn't need all these test changes.

Short answer: yes.

You can see the full list of changes needed for the test in 131cff5

You are generally correct that we won't verify signatures anyway in tests, but there are two tests that were different: the tests for unlisted packages. Since local feeds can't provide unlisted packages, JL03-Yue used the package the customer provided in writing tests to ensure that dotnet tool restore can find unlisted packages if necessary, and it turns out that package does claim to be signed, but the signature isn't valid, and it fails the test.

I have to admit that it took a lot of changes to thread through verifySignatures: false to a point where it could actually affect the test, and even after I got that part to work, the tests still needed more changes to find the right rid graph json, etc. If we don't want all those changes, the only alternative proposals I'm aware of are to either find another unlisted package that's signed correctly (but I/standup yesterday didn't know how to do that) or disable the test. marcpopMSFT sounded like he was most in favor of disabling the tests and avoiding all the other changes. If that's your preference as well, I can revert those last two commits and just disable them.

Fixes dotnet#37469 Add signature verifications for NuGet Tools

dtivel · 2024-09-04T21:27:47Z

src/Cli/dotnet/NugetPackageDownloader/LocalizableStrings.resx

+  </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>


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.

Fixes dotnet#37469 Add signature verifications for NuGet Tools

dotnet-issue-labeler bot added Area-Tools untriaged Request triage from a team member labels Mar 6, 2024

JL03-Yue changed the base branch from main to release/8.0.3xx March 6, 2024 21:35

JL03-Yue marked this pull request as draft April 8, 2024 23:39

JL03-Yue marked this pull request as ready for review April 12, 2024 22:43

JL03-Yue changed the title ~~[IP] add signature VerifySigning for dotnet tools~~ add signature VerifySigning for dotnet tools Apr 12, 2024

JL03-Yue changed the base branch from release/8.0.3xx to release/8.0.4xx April 12, 2024 22:47

JL03-Yue requested a review from dsplaisted April 12, 2024 22:51

JL03-Yue removed the untriaged Request triage from a team member label Apr 15, 2024

dsplaisted reviewed Apr 16, 2024

View reviewed changes

src/Cli/dotnet/NugetPackageDownloader/NuGetPackageDownloader.cs Outdated Show resolved Hide resolved

src/Cli/dotnet/NugetPackageDownloader/NuGetPackageDownloader.cs Outdated Show resolved Hide resolved

kartheekp-ms reviewed May 1, 2024

View reviewed changes

dtivel reviewed May 3, 2024

View reviewed changes

JL03-Yue and others added 10 commits July 31, 2024 15:29

add signature VerifySigning for dotnet tools and add tests

ac94e8c

fix tests

00331c9

fix tests

2d5f551

remove duplicate definition of verifysignature

4bca9df

fix tests

060e172

fix builds

5ade9d9

fix tests

ab30839

Resolve comments

df58e0b

Change that looks good but isn't right

89b822d

Fix NuGet signing check

aaeccf3

Forgind changed the base branch from release/8.0.4xx to main July 31, 2024 19:32

Forgind force-pushed the tool-update-signature-verification branch from c681a27 to aaeccf3 Compare July 31, 2024 19:32

Merge branch 'main' into tool-update-signature-verification

66fbe72

Forgind added 2 commits July 31, 2024 15:43

Change name

3a22067

Add paramName:

50378d1

Forgind reviewed Jul 31, 2024

View reviewed changes

src/Cli/dotnet/NugetPackageDownloader/NuGetPackageDownloader.cs Outdated Show resolved Hide resolved

Update src/Cli/dotnet/NugetPackageDownloader/NuGetPackageDownloader.cs

87520e5

Forgind reviewed Aug 1, 2024

View reviewed changes

src/Cli/dotnet/NugetPackageDownloader/NuGetPackageDownloader.cs Outdated Show resolved Hide resolved

Update src/Cli/dotnet/NugetPackageDownloader/NuGetPackageDownloader.cs

2b449fc

build-analysis bot mentioned this pull request Aug 1, 2024

Test failure - AllWebProjectsRestoreAndBuild fails with Unbalanced Condition for F# MVC templates #40073

Open

Forgind added 3 commits August 1, 2024 14:28

little tweak

deabd72

Add messages

196ab12

marcpopMSFT requested a review from joeloff August 20, 2024 21:19

Forgind added 2 commits August 23, 2024 16:17

Make test skip sign check

131cff5

Make other test pass (and merge into theory)

9e4910b

This was referenced Aug 24, 2024

dotnet-watch tests failing #40006

Open

Darwin Build_Release failure on Helix tests - Tests timing out #40074

Open

dsplaisted approved these changes Aug 26, 2024

View reviewed changes

Comments

24658d5

Forgind merged commit f7ddf14 into dotnet:main Aug 28, 2024
37 checks passed

baronfel added the Backport when open Items to backport to the Milestone's branch when the branch is created label Aug 28, 2024

Forgind pushed a commit to Forgind/sdk that referenced this pull request Aug 28, 2024

add signature VerifySigning for dotnet tools (dotnet#39268)

35ec022

Fixes dotnet#37469 Add signature verifications for NuGet Tools

dtivel reviewed Sep 4, 2024

View reviewed changes

kartheekp-ms mentioned this pull request Sep 12, 2024

Discover plugins installed using Net tools NuGet/NuGet.Client#5990

Merged

3 tasks

marcpopMSFT pushed a commit to Forgind/sdk that referenced this pull request Oct 21, 2024

add signature VerifySigning for dotnet tools (dotnet#39268)

a042aee

Fixes dotnet#37469 Add signature verifications for NuGet Tools

marcpopMSFT added a commit that referenced this pull request Oct 29, 2024

Backport: add signature VerifySigning for dotnet tools (#39268) (#43060)

6cd883c

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add signature VerifySigning for dotnet tools #39268

add signature VerifySigning for dotnet tools #39268

JL03-Yue commented Mar 6, 2024

dsplaisted left a comment

JL03-Yue commented Apr 17, 2024 •

edited

Loading

dsplaisted commented Apr 18, 2024

kartheekp-ms May 1, 2024

dtivel May 3, 2024

kartheekp-ms May 1, 2024 •

edited

Loading

kartheekp-ms May 1, 2024

kartheekp-ms May 1, 2024 •

edited

Loading

dtivel May 3, 2024

marcpopMSFT commented Aug 20, 2024

Forgind commented Aug 21, 2024

dsplaisted Aug 26, 2024

Forgind Aug 27, 2024

dsplaisted Aug 26, 2024

Forgind Aug 27, 2024 •

edited

Loading

joeloff Aug 27, 2024

Forgind Aug 27, 2024

dsplaisted Aug 26, 2024

Forgind Aug 27, 2024

dtivel Sep 4, 2024

		@@ -157,18 +154,48 @@ private void VerifySigning(string nupkgPath)
		return;

		if ((!_shouldUsePackageSourceMapping && !_firstPartyNuGetPackageSigningVerifier.Verify(new FilePath(nupkgPath), out commandOutput)) \|\|
		(_shouldUsePackageSourceMapping && !FirstPartyNuGetPackageSigningVerifier.NuGetVerify(new FilePath(nupkgPath), out commandOutput, _currentWorkingDirectory)))

	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);

add signature VerifySigning for dotnet tools #39268

add signature VerifySigning for dotnet tools #39268

Conversation

JL03-Yue commented Mar 6, 2024

dsplaisted left a comment

Choose a reason for hiding this comment

JL03-Yue commented Apr 17, 2024 • edited Loading

dsplaisted commented Apr 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kartheekp-ms May 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kartheekp-ms May 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcpopMSFT commented Aug 20, 2024

Forgind commented Aug 21, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Forgind Aug 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JL03-Yue commented Apr 17, 2024 •

edited

Loading

kartheekp-ms May 1, 2024 •

edited

Loading

kartheekp-ms May 1, 2024 •

edited

Loading

Forgind Aug 27, 2024 •

edited

Loading