Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable NuGet Audit #6303

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Enable NuGet Audit #6303

wants to merge 6 commits into from

Conversation

joperezr
Copy link
Member

@joperezr joperezr commented Oct 14, 2024

Description

Enabling NuGet Audit so that our build can catch issues when one of our dependencies is marked as vulnerable, so we are able to pin to a higher version in order to lift the dependency out of the vulnerability.

Checklist

  • Is this feature complete?
    • Yes. Ready to ship.
    • No. Follow-up changes expected.
  • Are you including unit tests for the changes and scenario tests if relevant?
    • Yes
    • No
  • Did you add public API?
    • Yes
      • If yes, did you have an API Review for it?
        • Yes
        • No
      • Did you add <remarks /> and <code /> elements on your triple slash comments?
        • Yes
        • No
    • No
  • Does the change make any security assumptions or guarantees?
    • Yes
      • If yes, have you done a threat model and had a security review?
        • Yes
        • No
    • No
  • Does the change require an update in our Aspire docs?
    • Yes
      • Link to aspire-docs issue:
    • No
Microsoft Reviewers: Open in CodeFlow

@ViktorHofer
Copy link
Member

Contributes to dotnet/arcade#15019

@joperezr we recommend also setting the following WarningsNotAsErrors property so that local dev builds / PRs don't stop working on a Patch Tuesday: https://github.com/dotnet/arcade/blob/964e434191bd3ca5675743d08ff742ec0f1e79a9/Directory.Build.props#L17-L18

It's important to keep this enabled for official builds though.

Directory.Packages.props Outdated Show resolved Hide resolved
@@ -34,6 +34,8 @@

<DashboardPublishedArtifactsOutputDir>$([MSBuild]::NormalizeDirectory('$(ArtifactsDir)', 'DashboardArtifacts', '$(Configuration)'))</DashboardPublishedArtifactsOutputDir>
<WorkloadsPackageSource>$(ArtifactsShippingPackagesDir)</WorkloadsPackageSource>
<!-- Only upgrade NuGetAudit warnings to errors for official builds. -->
<WarningsNotAsErrors Condition="'$(OfficialBuild)' != 'true'">$(WarningsNotAsErrors);NU1901;NU1902;NU1903;NU1904</WarningsNotAsErrors>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have a way to alert the team, or a subset of the team, when an OfficialBuild fails?

Maybe this shouldn't be based on OfficialBuild, but instead in our rolling CI build? For sure I don't want PRs to start failing on the day a security release comes out.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Along those lines.... this PR is failing due to NU1903

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

@ViktorHofer ViktorHofer Oct 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The workaround doesn't work. I would hold this PR off for a few days until we reach consensus on how to react to the msbuild bug.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like understanding why WarningsNotAsErrors doesn't correctly work with solutions will take longer. If things work for you I would not block this PR on it. You always have the option to turn NuGet Audit on/off if something doesn't work. You can NoWarn individual nuget warnings, suppress specific vulerability reports and/or disable NuGet Audit entirely.

As this feature is enabled by default for our customers starting with .NET 8.0.2xx and .NET 9, please give this a try and give feedback back to the NuGet team and me/us in the Arcade issue.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we should merge this change until it doesn't fail PRs. Getting all our PRs (and local builds) broken when someone marks a NuGet package as vulnerable isn't a good situation to be in.

@eerhardt
Copy link
Member

eerhardt commented Oct 15, 2024

FYI - #6319 should fix the current failure in this PR - transitively referencing Microsoft.Extensions.Caching.Memory 8.0.0. I've pushed that chagne to this branch as well.

@joperezr joperezr added the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Oct 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants