Enable NuGet Audit and fix issues #107639
Conversation
Microsoft.NET.HostModel can reference the live builds of the packages it depends on. These will be deployed by the SDK.� Most other audit alerts were due to tasks pulling in old dependencies that aren't even used by the task. Avoid these by cherry-picking just the assemblies needed by the tasks and provided by MSBuild / SDK. This prevents NuGet from downloading the package closure with the vulnerable packages. We don't need those packages since the tasks aren't responsible for deploying them. A better solution in the future would be a targeting pack for MSBuild and the .NET SDK - so that components that contribute to these hosts have a surface area they can target without taking on responsibility for servicing. There is once case where we have a test that references NuGet.* packages which also bring in stale dependencies that overlap with framework assemblies. Avoid these by cherry-picking the NuGet packages in the same way.
ericstj
requested review from
jaredpar,
carlossanlop,
jkoritzinsky,
rainersigwald,
zivkan and
mmitche
dotnet-issue-labeler
bot
added
the
needs-area-label
ericstj
added
area-Infrastructure-libraries
and removed
needs-area-label
|
Tagging subscribers to this area: @dotnet/area-infrastructure-libraries |
| <ProjectReference Include="$(LibrariesProjectRoot)System.Reflection.Metadata\src\System.Reflection.Metadata.csproj" /> | ||
| <ProjectReference Include="$(LibrariesProjectRoot)System.Text.Json\src\System.Text.Json.csproj" /> |
There was a problem hiding this comment.
These references need to not move newer than MSBuild as this library is consumed by MSBuild tasks in the SDK (including for the .NET Framework build of the SDK targets). We can't reference live builds as a result.
There was a problem hiding this comment.
The SDK is already on the live version of System.Text.Json for its tasks, but it seems it is not on the live version of System.Reflection.Metadata. We have another library (Microsoft.Extensions.DependencyModel) that SDK consumes with live JSON as well. I only needed to update to live STJ to fix the audit warning so I could undo the SRM change, but it feels wrong.
I'll file a follow up issue in the SDK for that. dotnet/sdk#43325
There was a problem hiding this comment.
The SDK is already on the live version of System.Text.Json for its tasks
Would you mind elaborating? Last time I looked, NETFramework tasks in the SDK were using an 8.0.x version of STJ because MSBuild's binding redirects required that.
There was a problem hiding this comment.
The SDK tasks themselves ('Microsoft.NET.Build.Tasks.dll`) reference the latest. Because it's an exact match they don't need the redirects. Other assemblies loaded by the SDK (NuGet, for example) do reference lower versions, and they rely on MSBuild's redirects, but they don't exchange types with the SDK tasks. The SDK also ships the latest STJ along side its tasks.
For example:
"C:\Program Files\dotnet\sdk\9.0.100-preview.7.24407.12\Sdks\Microsoft.NET.Sdk\tools\net472\System.Text.Json.dll" => 9.0.0.0
"C:\Program Files\dotnet\sdk\8.0.400\Sdks\Microsoft.NET.Sdk\tools\net472\System.Text.Json.dll" => 8.0.0.4
The SDK's MSBuild SDK resolver does need to stay on the VS copy of STJ because it doesn't carry a copy next to itself.
CC @marcpopMSFT
SDK pins S.R.M and a few others, so don't make them upgrade yet.
| <PackageDownload Include="@(PackageDownloadAndReference)" /> | ||
| <PackageDownload Update="@(PackageDownloadAndReference)" Version="[%(Version)]"/> | ||
| <PackageDownloadAndReference Update="@(PackageDownloadAndReference)" PackageFolder="$([System.String]::new(%(Identity)).ToLowerInvariant())" /> | ||
| <Reference Include="@(PackageDownloadAndReference->'$(NuGetPackageRoot)%(PackageFolder)/%(Version)/%(Folder)/%(AssemblyName).dll')" /> |
There was a problem hiding this comment.
Won't this approach break the ability to use CPM and transitive pinning?
There was a problem hiding this comment.
This doesn't pull any transitive dependencies at all. That's the point. It's a stand-in for a targeting pack. We pull down just single package and use it only for reference. It's very manual, but it avoids accidentally lifting dependencies that aren't in the control of the plugin (or pulling down dependencies just to drop them in favor of a framework library). It should be used sparingly as it is bypassing nuget resolution. It also doesn't persist into a packed project, so it shouldn't be used in "normal" libraries. It's for tasks, analyzers, and private assemblies intended for use in similar circumstances.
There was a problem hiding this comment.
Sure but this means it's a different kind of package reference that you can't manage with CPM correct?
There was a problem hiding this comment.
We'd just use the same properties when setting these that we'd feed into CPM. It's a temporary solution. We need an official package or SDK from MSBuild (and roslyn) for targeting hosts.
Nonetheless I added a note to NuGet/Home#8476 that would improve PackageDownload if it didn't need to mention versions.
If you felt strongly about making this hack honor CPM I could add that functionality by merging the version defined in @(PackageVersion) when not specified.
Consolidate representation of msbuild-provided task dependencies
| <!-- we need to explicitly reference System.Reflection.MetadataLoadContext 8.0 or newer for function pointer support, | ||
| because the default version pulled in from MSBuild is lower. --> | ||
| <!-- TODO: this can be removed once MSBuild brings in a new enough version --> | ||
| <PackageReference Include="System.Reflection.MetadataLoadContext" Version="$(SystemReflectionMetadataLoadContextToolsetVersion)" /> |
There was a problem hiding this comment.
So I tried to remove this and the build failed with
Error when creating nuget packed package from D:\a\_work\1\s\artifacts\packages\Release\specs\Microsoft.NET.Runtime.WebAssembly.Wasi.Sdk.nuspec. NuGet.Packaging.Core.PackagingException: File not found: 'D:\a\_work\1\s\artifacts\bin\WasmAppBuilder\Release\net9.0\System.Reflection.MetadataLoadContext.dll'.
That means we were previously redistributing a non-live version of this dependency. I think the fix would be to stop redistributing here (rather than continue redistributing the pre-built binary).
There was a problem hiding this comment.
I checked the .NET SDK and it includes the latest 9.0 version of MetadataLoadContext. Desktop MSBuild, however, only includes 8.0.0.
That's OK though, because the version we're referencing is 8.0.0 -> https://github.com/dotnet/runtime/pull/107639/files#diff-1ea18ff65faa2ae6fed570b83747086d0317f5e4bc325064f6c14319a9c4ff67L139
|
The build errors here look like double writes, probably my change to reference libs from hostmodel needs a fix. edit: found the problem -- it's due to different Configuration value being used for the reference. Fix is to pass LibrariesConfiguration. |
Microsoft.NET.HostModel can reference the live builds of the packages it depends on. These will be deployed by the SDK.
Most other audit alerts were due to tasks pulling in old dependencies that aren't even used by the task. Avoid these by cherry-picking just the assemblies needed by the tasks and provided by MSBuild / SDK. This prevents NuGet from downloading the package closure with the vulnerable packages. We don't need those packages since the tasks aren't responsible for deploying them. A better solution in the future would be a targeting pack for MSBuild and the .NET SDK - so that components that contribute to these hosts have a surface area they can target without taking on responsibility for servicing.
There is once case where we have a test that references NuGet.* packages which also bring in stale dependencies that overlap with framework assemblies. Avoid these by cherry-picking the NuGet packages in the same way.
Keeping this draft as I want to test some more and add some docs / comments - but sharing this now since a lot of folks were working on this and I wanted to share the techniques I'm using.