search

NuGet / Home

Repo for NuGet Client issues
Other
1.49k stars 250 forks source link

Enable lock file SHA hash validation for MSBuild Sdk downloads #12326

Open aetos382 opened 1 year ago

aetos382 commented 1 year ago

NuGet Product(s) Involved

NuGet.exe, Visual Studio Package Management UI, Visual Studio Package Manager Console, MSBuild.exe, dotnet.exe

The Elevator Pitch

When using the --use-lock-file option, the hashes for packages referenced by <PackageReference> are included in packages.lock.json, but not for packages referenced by <Sdk>.

Shouldn't the Sdk packages also be ~pinned~ SHA hash validated to make the project build fully reproducible? edited by @zivkan

Additional Context and Details

No response

erdembayar commented 1 year ago

@aetos382 Thank you for filing this issue. Could you please help us with intention of pinning the sdk version? Do you have any existing use case for pinning the sdk version? What problem we would solve?

aetos382 commented 1 year ago

@erdembayar I am sorry, I do not have such a specific example. I just thought that packages retrieved via NuGet should be able to be pinned as well as PackageReference.

zivkan commented 1 year ago

@aetos382 When I try Sdk="Microsoft.Build.NoTargets/3.2.0", MSBuild fails to evaluate the project file, because that specific version can't be found. It does not automatically use the next highest version, like PackageReference does.

Therefore, do you consider that MSBuild SDKs already support 'pinning'?

aetos382 commented 1 year ago

The following blog post states that package version pinning can also detect that the contents of NuGet.config have changed or that the author of the package has published different contents with the same version number. I think such problems can happen to SDK packages as well.

https://devblogs.microsoft.com/nuget/enable-repeatable-package-restores-using-a-lock-file

KalleOlaviNiemitalo commented 1 year ago

I would sure like cryptographic assurance that the build is using a package with the expected contents, even if the package source has been compromised somehow. Perhaps it's not so important for nuget.org, which I hope is professionally and securely maintained; but for file shares or other on-premises package sources, it would still be useful.

zivkan commented 1 year ago

Ok, I wanted to validate that the request was really about SHA validation, not version pinning, as I see them as different behaviours. I'll edit the title.

zivkan commented 1 year ago

Oh, and for anyone curious why it's not already happening, MSBuild have docs with a build process overview. The key is that MSBuild will first "evaluate" a project (loading files/SDKs imported, then checking all <PropertyGroup>s not inside a <Target>, then checking all <ItemGroup>s also not in any <Target>. Finally, once all that has happened, it executes targets.

MSBuild SDK download happens during that import evaluation, where MSBuild calls a NuGet API specifically for downloading SDKs. At this time, since the rest of the evaluation hasn't happened (it can't happen, because the props/targets files haven't been downloaded yet in order to evaluate), NuGet doesn't yet know what all the <PackageReference items are.

Restore is a target, and so happens during MSBuild's target exucution phase. So a "long time" after the SDK imports were evaluated (downloaded).

Just thinking out loud, since one of the features of lock files is detecting when the package graph has changed (and allowing to fail restore with "locked mode"), this will make the implementation of including MSBuild SDKs in the lock file challenging, since there is no single method that is an entry point to both PackageReference item restore and MSBuild SDK downloads, so NuGet will never have a complete set of packages in memory to compare against the lock file. The design will have to account for this, but it might (I really haven't thought much about this) be as simple as adding a "sdk" property to the lock file, so that PackageReference and SDKs can be evaluated separately.

KalleOlaviNiemitalo commented 1 year ago

The versions of referenced MSBuild SDKs can already be declared in a global.json file. I imagine the SDK resolver could likewise read hashes from a file (either global.json or a new msbuild-sdks.lock.json file) and verify them. However: