Address component government alerts #5308

erdembayar · 2023-07-12T03:41:17Z

Bug

Fixes: https://github.com/NuGet/Client.Engineering/issues/2372

Regression? Last working version:

Description

Recently #5289 upgraded System.Security.Cryptography.Pkcs to non-vulnerable version, still GC alert persists.

Problem was https://www.nuget.org/packages/Microsoft.Build.Tasks.Core/17.3.1 defined in Directory.Packages.props is still bringing same vulnerable version 6.0.1 into graph.
Added override to version into respective projects to fix this.
In NuGet.Build.Tasks.Pack.csproj we need to remove ExcludeAssets="runtime" /> because this one is doing ILmerge, without this ilmerge fails with strong name signature problem, see here.

GC alert is no longer present in CI build after this fix: https://devdiv.visualstudio.com/DevDiv/_build/results?buildId=8058016&view=logs&j=cb98efd6-7062-56ef-f642-0176540bbe0a&t=f95ba3d8-e30a-5687-3fc7-3bc14bb0710b&l=2327

PR Checklist

erdembayar · 2023-07-12T04:26:59Z

src/NuGet.Core/NuGet.Build.Tasks.Pack/NuGet.Build.Tasks.Pack.csproj

    <PackageReference Include="Microsoft.Build.Utilities.Core" ExcludeAssets="runtime" GeneratePathProperty="true" />
    <!-- System.Security.Cryptography.Xml is a dependency of Microsoft.Build.Tasks.Core. Therefore, we have assigned the same ExcludeAssets value. -->
    <PackageReference Include="System.Security.Cryptography.Xml" ExcludeAssets="Runtime" />
+    <PackageReference Include="System.Security.Cryptography.Pkcs" />


No ExcludeAssets="runtime" here otherwise IlMerge fails.

Here is the difference between current branch and dev for deps.json file.
Please note here 2 screenshots:
Left side current branch and right is dev branch.

2.

donnie-msft

I believe this looks OK, although I'm not sure why the ILMerge issue came up because of this change? I suppose the worst side effect is there's a change in nuget.exe payload with this package, but I hope someone else understands the cause and its implications better than I do.

Ideally, @zivkan would be the best to approve in case he knows more having recently looked at this package and CG.

Also, there's a lot of build links in the description, but those will all be lost in a short timeframe, as they expire. Consider putting whatever detail you want directly into the PR.

zivkan

I missed this in my own PR because I didn't expect CG to be "green" when there's an alert in the PR build. I see that in official builds that the component detection task is orange.

@erdembayar can you please figure out why official and PR builds behave differently (I'm sure it's related to some build variable), and fix the PR pipeline to also be orange, so we can more easily see when our PRs don't fix CG issues correctly.

src/NuGet.Core/NuGet.Build.Tasks.Console/NuGet.Build.Tasks.Console.csproj

src/NuGet.Core/NuGet.Build.Tasks/NuGet.Build.Tasks.csproj

as per my review comments, I think this might actually cause insertion failures in the .NET SDK

erdembayar · 2023-07-12T20:31:27Z

@erdembayar can you please figure out why official and PR builds behave differently (I'm sure it's related to some build variable), and fix the PR pipeline to also be orange, so we can more easily see when our PRs don't fix CG issues correctly.

It was env var cfsNugetWarnLevel making the difference. I copied it from official to private pipeline.
Now it's properly raising the GC warning in private pipeline with dev branch.
Env vars in both private and public pipelines looks almost identical, not sure what is making one to flag and not the other one. I'll inquire offline.

erdembayar · 2023-07-13T23:16:59Z

@erdembayar can you please figure out why official and PR builds behave differently (I'm sure it's related to some build variable), and fix the PR pipeline to also be orange, so we can more easily see when our PRs don't fix CG issues correctly.

I inquired about this, below is answer:

This is happening due to Production/Non-Production classification differences in both pipelines.  The partial task failure (orange build) will only occur if there’s a due date on the alert. 
Due dates are only applicable to production build.

That means we couldn't up light up orange build in private pipeline unless we classify as production, I believe we're not going to do it, because it would cause ton of other unnecessary compliance warnings.

erdembayar

@nkolev92
I added deps.json file comparison for each changes after latest change. Please review again.

erdembayar · 2023-07-17T15:50:27Z

src/NuGet.Core/NuGet.Build.Tasks.Console/NuGet.Build.Tasks.Console.csproj

-
+
+  <ItemGroup Condition=" '$(IsCore)' == 'true' ">
+    <PackageReference Include="System.Security.Cryptography.Pkcs" ExcludeAssets="Compile"/>


@nkolev92
Here is the difference between current branch and dev for deps.json file.
Left side current branch and right is dev branch.

erdembayar · 2023-07-17T15:53:46Z

src/NuGet.Core/NuGet.Build.Tasks.Pack/NuGet.Build.Tasks.Pack.csproj

    <PackageReference Include="Microsoft.Build.Utilities.Core" ExcludeAssets="runtime" GeneratePathProperty="true" />
    <!-- System.Security.Cryptography.Xml is a dependency of Microsoft.Build.Tasks.Core. Therefore, we have assigned the same ExcludeAssets value. -->
    <PackageReference Include="System.Security.Cryptography.Xml" ExcludeAssets="Runtime" />
+    <PackageReference Include="System.Security.Cryptography.Pkcs" />


Here is the difference between current branch and dev for deps.json file.
Please note here 2 screenshots:
Left side current branch and right is dev branch.

2.

erdembayar · 2023-07-17T15:56:18Z

src/NuGet.Core/NuGet.Build.Tasks/NuGet.Build.Tasks.csproj

    <PackageReference Include="Microsoft.Build.Utilities.Core" ExcludeAssets="runtime" />
    <!-- System.Security.Cryptography.Xml is a dependency of Microsoft.Build.Tasks.Core. Therefore, we have assigned the same ExcludeAssets value. -->
    <PackageReference Include="System.Security.Cryptography.Xml" ExcludeAssets="runtime" />
+    <PackageReference Include="System.Security.Cryptography.Pkcs" ExcludeAssets="compile" />


Here is the difference between current branch and dev for deps.json file.
Left side current branch and right is dev branch.

2.

aortiz-msft

LGTM

* Override vulnerable transitive dependency to fix GC alerts

…#5332) * Address component government alerts (#5308) * Override vulnerable transitive dependency to fix GC alerts * Update System.Security.Cryptography.Pkcs version (#5289) --------- Co-authored-by: Erick Yondon <[email protected]> Co-authored-by: Andy Zivkovic <[email protected]>

* Override vulnerable transitive dependency to fix GC alerts

…#5332) * Address component government alerts (#5308) * Override vulnerable transitive dependency to fix GC alerts * Update System.Security.Cryptography.Pkcs version (#5289) --------- Co-authored-by: Erick Yondon <[email protected]> Co-authored-by: Andy Zivkovic <[email protected]>

erdembayar added 2 commits July 11, 2023 15:29

Override vulnerable transitive dependency

d411897

Fix strong signing failure with ilmerge

f7c9d5a

erdembayar marked this pull request as ready for review July 12, 2023 03:46

erdembayar requested a review from a team as a code owner July 12, 2023 03:46

erdembayar commented Jul 12, 2023

View reviewed changes

donnie-msft reviewed Jul 12, 2023

View reviewed changes

zivkan previously approved these changes Jul 12, 2023

View reviewed changes

zivkan reviewed Jul 12, 2023

View reviewed changes

src/NuGet.Core/NuGet.Build.Tasks.Console/NuGet.Build.Tasks.Console.csproj Outdated Show resolved Hide resolved

src/NuGet.Core/NuGet.Build.Tasks/NuGet.Build.Tasks.csproj Outdated Show resolved Hide resolved

Try address PR comment

6c1ce8c

erdembayar requested review from nkolev92 and zivkan July 13, 2023 23:17

Fix runtime dep

738657c

zivkan approved these changes Jul 17, 2023

View reviewed changes

erdembayar commented Jul 17, 2023

View reviewed changes

aortiz-msft self-requested a review July 17, 2023 18:16

aortiz-msft approved these changes Jul 17, 2023

View reviewed changes

erdembayar merged commit 64b8cd1 into dev Jul 18, 2023

erdembayar deleted the dev-eryondon-fix-pkcs-gc-warning6 branch July 18, 2023 00:05

martinrrm pushed a commit that referenced this pull request Jul 25, 2023

Address component government alerts (#5308)

9ecf568

* Override vulnerable transitive dependency to fix GC alerts

jeffkl pushed a commit that referenced this pull request Aug 1, 2023

Address component government alerts (#5308)

8f7febf

* Override vulnerable transitive dependency to fix GC alerts

jeffkl pushed a commit that referenced this pull request Aug 1, 2023

Address component government alerts (#5308)

6280d60

* Override vulnerable transitive dependency to fix GC alerts



		<ItemGroup Condition=" '$(IsCore)' == 'true' ">
		<PackageReference Include="System.Security.Cryptography.Pkcs" ExcludeAssets="Compile"/>

Address component government alerts #5308

Address component government alerts #5308

Uh oh!

Conversation

erdembayar commented Jul 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Bug

Description

PR Checklist

Uh oh!

erdembayar Jul 12, 2023

Choose a reason for hiding this comment

Uh oh!

erdembayar Jul 17, 2023

Choose a reason for hiding this comment

Uh oh!

donnie-msft left a comment • 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!

zivkan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

erdembayar commented Jul 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

erdembayar commented Jul 13, 2023

Uh oh!

erdembayar left a comment • 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!

erdembayar Jul 17, 2023

Choose a reason for hiding this comment

Uh oh!

erdembayar Jul 17, 2023

Choose a reason for hiding this comment

Uh oh!

erdembayar Jul 17, 2023

Choose a reason for hiding this comment

Uh oh!

aortiz-msft left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

erdembayar commented Jul 12, 2023 •

edited

Loading

donnie-msft left a comment •

edited

Loading

erdembayar commented Jul 12, 2023 •

edited

Loading

erdembayar left a comment •

edited

Loading