NuGetPackageDownloader: Stop verifying the package signature on Mac and queue off of the environment on Linux by edvilme · Pull Request #47321 · dotnet/sdk

edvilme · 2025-03-06T18:15:28Z

Fixes #46857
Customer Scenario
dotnet tool install/restore is checking the package signature on Mac with 9.0.200. However, we shouldn't be doing that as macos doesn't fully support the signature store that nuget uses to verify signatures. This is already disabled for nuget restore.

Fix
Match the logic NuGet uses where we are always on for windows, default off for Mac, default on for Linux, and honor the env variable to override the values.

Justification
There are half a dozen customer comments on the linked issues saying they are blocked in 9.0.200.

Copilot

PR Overview

This PR updates the NuGetPackageDownloader to disable signature verification on macOS and Linux by ensuring that the _verifySignatures flag is only true when running on Windows.

Added documentation comments to describe the temporary disabling of signature verification on non-Windows OSes.
Updated the constructor to combine the verifySignatures parameter with an OperatingSystem check.

Reviewed Changes

File	Description
src/Cli/dotnet/NugetPackageDownloader/NuGetPackageDownloader.cs	Modified _verifySignatures initialization to disable signature verification on macOS and Linux; added documentation comments explaining the change.

Copilot reviewed 1 out of 1 changed files in this pull request and generated no comments.

src/Cli/dotnet/NugetPackageDownloader/NuGetPackageDownloader.cs

marcpopMSFT · 2025-03-06T23:53:39Z

src/Cli/dotnet/NugetPackageDownloader/NuGetPackageDownloader.cs

            _retryTimer = timer;
            _sourceRepositories = new();
-            _verifySignatures = verifySignatures;
+            _verifySignatures = OperatingSystem.IsWindows() && verifySignatures; // This is temporarily disabled for macOS and Linux.


@dtivel should signature verification on linux be kept and only Mac disabled?

It looks like we should enable it on Linux based on this doc https://learn.microsoft.com/en-us/dotnet/core/tools/nuget-signed-package-verification#linux. However, there is also an environment variable which seems to control this, so maybe we should base the check off the environment variable. I assume this is OFF by default on Mac, but that doesn't seem to be the case based on the code below. If we don't respect DOTNET_NUGET_SIGNATURE_VERIFICATION, this may be a breaking change since it impacts more than dotnet tool.

We have some logic for this

sdk/src/Cli/dotnet/NuGetSignatureVerificationEnabler.cs

Line 14 in b1d1a0d

internal static readonly string DotNetNuGetSignatureVerification = "DOTNET_NUGET_SIGNATURE_VERIFICATION";

but that just forwards the environment variable to NuGet. I assume they do most of the processing. @dtivel Should this logic to turn it on by default

sdk/src/Cli/dotnet/NuGetSignatureVerificationEnabler.cs

Line 48 in b1d1a0d

return string.Equals(bool.FalseString, value, StringComparison.OrdinalIgnoreCase)

only be true on Linux and Windows, which also might fix this bug?

Ok this code ignores the env var if its not linux.
Its possible that A - did this environment variable never work correctly? And nobody realized because nobody is using it as true on mac because it doesn't work anyway, and maybe its enabled by default somewhere else on Windows.

B- the point of that class is not to be a source of truth for the verification enablement but just a way to enable it by default on Linux without touching other stuff

We'd still have to respect it on Linux, though

Matching NuGet's signed package verification support is sensible.

Always enabled on Windows. DOTNET_NUGET_SIGNATURE_VERIFICATION is ignored.

Enabled on Linux by default. Users can opt out by setting DOTNET_NUGET_SIGNATURE_VERIFICATION to false.

Disabled on macOS by default. Users can opt in by setting DOTNET_NUGET_SIGNATURE_VERIFICATION to true, but this is neither recommended nor supported.

https://github.com/dotnet/sdk/blob/main/src/Cli/dotnet/NuGetSignatureVerificationEnabler.cs does the work of enabling on Linux by default.

https://github.com/NuGet/NuGet.Client/blob/594bfe4554893a7364d9ec1d31a22574fba3115c/src/NuGet.Core/NuGet.Packaging/PackageArchiveReader.cs#L519-L557 is where NuGet respects the environment variable. The problem is that FirstPartyNuGetPackageSigningVerifier opens a package and verifies its signature without checking if it's safe to verify signatures.

You should perform this "can verify signatures" check before deciding that you to verify signatures or you risk deviating from what NuGet supports.

Thank you for the context. I think for this change, a reasonable option would then be to enable it if on windows || by checking the NuGetSignatureVerificationEnabler.cs
on linux to see if it should be turned on, on linux.

Then in the future we could improve the rest of this like mentioned above.

Yes, that seems reasonable.

PackageArchiveReader.CanVerifySignedPackages(...) is a bit inconvenient. It doesn't access instance members and could have been a static method somewhere. You could open an issue on NuGet/Home for a more convenient "is signed package verification enabled" method.

Sounds good. I just didn't want to disable this check on Linux if it were working as that would be a security takeback.

Forgind

...other than the linux thing

src/Cli/dotnet/NugetPackageDownloader/NuGetPackageDownloader.cs

Co-authored-by: Noah Gilson <OTAKUPENGUINOP@GMAIL.COM>

Co-authored-by: Forgind <12969783+Forgind@users.noreply.github.com>

Forgind

Looks good! 🚢

edvilme · 2025-03-11T17:49:36Z

/backport to release/10.0.xx-preview2

github-actions · 2025-03-11T17:49:49Z

Started backporting to release/10.0.xx-preview2: https://github.com/dotnet/sdk/actions/runs/13794782769

github-actions · 2025-03-11T17:50:08Z

@edvilme an error occurred while backporting to "release/10.0.xx-preview2", please check the run log for details!

Error: The specified backport target branch "release/10.0.xx-preview2" wasn't found in the repo.

edvilme · 2025-03-11T17:50:34Z

/backport to release/10.0.1xx-preview2

github-actions · 2025-03-11T17:50:47Z

Started backporting to release/10.0.1xx-preview2: https://github.com/dotnet/sdk/actions/runs/13794799185

github-actions · 2025-03-11T17:51:09Z

@edvilme backporting to "release/10.0.1xx-preview2" failed, the patch most likely resulted in conflicts:

$ git am --3way --empty=keep --ignore-whitespace --keep-non-patch changes.patch

Applying: NuGetPackageDownloader: Only verify signing on windows
Applying: Update comments
error: sha1 information is lacking or useless (src/Cli/dotnet/NugetPackageDownloader/NuGetPackageDownloader.cs).
error: could not build fake ancestor
hint: Use 'git am --show-current-patch=diff' to see the failed patch
hint: When you have resolved this problem, run "git am --continue".
hint: If you prefer to skip this patch, run "git am --skip" instead.
hint: To restore the original branch and stop patching, run "git am --abort".
hint: Disable this message with "git config set advice.mergeConflict false"
Patch failed at 0002 Update comments
Error: The process '/usr/bin/git' failed with exit code 128

Please backport manually!

…tnet#47321) Co-authored-by: Noah Gilson <OTAKUPENGUINOP@GMAIL.COM> Co-authored-by: Forgind <12969783+Forgind@users.noreply.github.com>

…ws by default (#47321) NuGetPackageDownloader: Only verify signing on windows by default (#47321) Co-authored-by: Noah Gilson <OTAKUPENGUINOP@GMAIL.COM> Co-authored-by: Forgind <12969783+Forgind@users.noreply.github.com> ---- #### AI description (iteration 1) #### PR Classification Bug fix to ensure package signing verification is only performed on Windows by default. #### PR Summary This pull request modifies the `NuGetPackageDownloader` to verify package signing only on Windows by default, with an option to enable it on other operating systems via an environment variable. - `src/Cli/dotnet/NugetPackageDownloader/NuGetPackageDownloader.cs`: Added logic to conditionally verify package signatures based on the operating system and environment variable. Introduced error handling to delete the package file if verification fails.

ghost added Area-Workloads untriaged Request triage from a team member labels Mar 6, 2025

edvilme force-pushed the edvilme-tool-mac-sign branch from 023840e to 2cd84a8 Compare March 6, 2025 23:34

edvilme changed the base branch from main to release/9.0.2xx March 6, 2025 23:34

edvilme marked this pull request as ready for review March 6, 2025 23:37

Copilot AI review requested due to automatic review settings March 6, 2025 23:37

Copilot AI reviewed Mar 6, 2025

View reviewed changes

joeloff reviewed Mar 6, 2025

View reviewed changes

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

joeloff approved these changes Mar 6, 2025

View reviewed changes

marcpopMSFT reviewed Mar 6, 2025

View reviewed changes

Forgind approved these changes Mar 7, 2025

View reviewed changes

nagilson approved these changes Mar 7, 2025

View reviewed changes

dtivel approved these changes Mar 7, 2025

View reviewed changes

edvilme enabled auto-merge (squash) March 7, 2025 23:39

Forgind disabled auto-merge March 7, 2025 23:55

This was referenced Mar 8, 2025

The active test run was aborted. Reason: Test host process crashed dotnet/dnceng#451

Closed

dotnet-watch tests failing #40006

Open

[dotnet watch] Cannot open project Lib.fsproj because extension fsproj is not associated with a language #46378

Closed

edvilme changed the title ~~NuGetPackageDownloader: Only verify signing on windows~~ NuGetPackageDownloader: Only verify signing on windows by default Mar 8, 2025

edvilme requested a review from Forgind March 8, 2025 00:46

nagilson approved these changes Mar 8, 2025

View reviewed changes

nagilson reviewed Mar 8, 2025

View reviewed changes

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

build-analysis bot mentioned this pull request Mar 8, 2025

Template engine tests hitting known parallelism issue in GetVisualStudioInstances #44878

Open

Forgind reviewed Mar 8, 2025

View reviewed changes

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

edvilme added 6 commits March 7, 2025 22:02

NuGetPackageDownloader: Only verify signing on windows

fceba3e

Update comments

81e5168

Delete file if signature verification fails

d1f3e2b

Add spacing

9348f34

Consider env variable

ec411c1

Update logic to include linux

78b9a72

edvilme and others added 2 commits March 7, 2025 22:02

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

889dec5

Co-authored-by: Noah Gilson <OTAKUPENGUINOP@GMAIL.COM>

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

8b6ec3a

Co-authored-by: Forgind <12969783+Forgind@users.noreply.github.com>

edvilme force-pushed the edvilme-tool-mac-sign branch from 7e3beb0 to 8b6ec3a Compare March 8, 2025 04:02

Forgind approved these changes Mar 9, 2025

View reviewed changes

edvilme merged commit f6c2a6e into dotnet:release/9.0.2xx Mar 9, 2025
30 checks passed

Forgind mentioned this pull request Mar 10, 2025

dotnet tool restore started failing on macOS with NU3037 and NU3028 errors after 11th February #46857

Closed

marcpopMSFT changed the title ~~NuGetPackageDownloader: Only verify signing on windows by default~~ NuGetPackageDownloader: Stop verifying the package signature on Mac and queue off of the environment on Linux Mar 11, 2025

marcpopMSFT added the Servicing-approved label Mar 11, 2025

edvilme mentioned this pull request Mar 11, 2025

NuGetPackageDownloader: Stop verifying the package signature on Mac and queue off of the environment on Linux #47463

Merged

marcpopMSFT added this to the 9.0.4 milestone Mar 11, 2025

jamescrosswell mentioned this pull request Apr 14, 2025

CI Builds failing due to SSL Errors getsentry/sentry-dotnet#3915

Closed

Conversation

edvilme commented Mar 6, 2025 • edited by marcpopMSFT Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

PR Overview

Reviewed Changes

Uh oh!

Uh oh!

marcpopMSFT Mar 6, 2025

Choose a reason for hiding this comment

Uh oh!

nagilson Mar 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nagilson Mar 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dtivel Mar 7, 2025

Choose a reason for hiding this comment

Uh oh!

nagilson Mar 7, 2025

Choose a reason for hiding this comment

Uh oh!

dtivel Mar 7, 2025

Choose a reason for hiding this comment

Uh oh!

marcpopMSFT Mar 7, 2025

Choose a reason for hiding this comment

Uh oh!

Forgind left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Forgind left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

edvilme commented Mar 11, 2025

Uh oh!

github-actions bot commented Mar 11, 2025

Uh oh!

github-actions bot commented Mar 11, 2025

Uh oh!

edvilme commented Mar 11, 2025

Uh oh!

github-actions bot commented Mar 11, 2025

Uh oh!

github-actions bot commented Mar 11, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

edvilme commented Mar 6, 2025 •

edited by marcpopMSFT

Loading

nagilson Mar 7, 2025 •

edited

Loading

nagilson Mar 7, 2025 •

edited

Loading