search

dotnet / arcade

Tools that provide common build infrastructure for multiple .NET Foundation projects.
MIT License
673 stars 347 forks source link

Enable NuGet Audit in .NET repositories #15019

Open ViktorHofer opened 2 months ago

ViktorHofer commented 2 months ago

NuGet Audit docs: https://devblogs.microsoft.com/nuget/nugetaudit-2-0-elevating-security-and-trust-in-package-management/ & https://learn.microsoft.com/en-us/nuget/concepts/auditing-packages

NuGet Audit flags vulnerable dependencies at restore and build time so that we don’t have to exclusively rely on post-build scanners like Component Governance. NuGet Audit is part of the .NET 8 and .NET 9 SDKs and now enabled by default.

NuGetAuditMode defaulted to direct when it was introduced in the .NET 8.0.100 SDK and VS 17.8. In .NET 9.0.100 SDK and VS 17.12 the default changed to all.

Our .NET repositories need to opt into that security feature as we don’t use nuget.org as our package repository. Aside from resolving potential vulnerable packages in the build, all that's needed to turn it on is: https://github.com/dotnet/arcade/pull/15018

Requirement

This depends on a NuGet feature that got added with the .NET 9 Preview 7 SDK. If your repository doesn't yet use that, that's fine and enabling NuGetAudit can be revisited at a later point.

Status

List below are repositories that are part of the VMR. The list can be extended.

cc @zivkan @ericstj @JonDouglas

zivkan commented 2 months ago

NuGet.Client added auditSources, and therefore will get NuGetAudit warnings during restore, at the beginning of the month: https://github.com/NuGet/NuGet.Client/pull/5939/files#diff-b54ee602750f150b3d4986e0334cc3ec901d47510560e26e792e8d294ab6f37a

(I also added the 4 NU codes to WarningsNotAsErrors to prevent it from failing CI builds, or blocking people from working on their features)

ericstj commented 1 month ago

@mmitche @jaredpar @aortiz-msft - how do you think we should drive enforcement of this?

@jaredpar suggested forcing folks to emit a binlog then presumably adding some validation somewhere in the common templates or scripts to ensure they ran the build with audit enabled. Seems possible but feels like a lot of things to line up just to enforce this in the timeline we're trying to drive it.

I was thinking we could add a check in arcade for this. @aortiz-msft @zivkan suppose we wanted Arcade to enforce this, could we set a property that would make NuGet enforce that, or could we observe if NuGet ran with an audit source? If we did so, then we could make arcade check for that and emit an error if it's absent.

mmitche commented 1 month ago

Binlog checking seems rather fragile and I don't love the idea of adding additional steps to builds just to do this checking. Is it possible to detect whether NuGet Audit is enabled via msbuild targets? We have plenty of injection points in arcade. What properties can we check?

mmitche commented 1 month ago

I checked some binlogs from before and after Viktor's change to arcade to enable auditing. There's not much to go on. No real indication that actual auditing happened. The thing is that auditing is enabled in both, just only functional in the one with nuget.org as part of the audit sources. The only binlog entry differentiating the two is that searching for "nuget.org" shows a CACHE entry for nuget.org's vulnerability endpoint if auditing is working. But I wouldn't base detection on that.

Instead, it's probably better for arcade to include a target that reads the nuget.config file and verifies the audit sources are available. @zivkan @aortiz-msft Are the audit sources controllable with an itemgroup, or only through nuget.config?

jaredpar commented 1 month ago

suggested forcing folks to emit a binlog then presumably adding some validation somewhere in the common templates or scripts to ensure they ran the build with audit enabled.

I think that if pipelines / actions published binlogs to a well known location there is a lot of standard analysis that we could do. I understand it's a bit of cost initially but need to pay that at some point.

Instead, it's probably better for arcade to include a target that reads the nuget.config file and verifies the audit sources are available.

That approach though limits our ability to say look at what warnings are suppressed, how globally are they suppressed, etc ...

zivkan commented 1 month ago

suppose we wanted Arcade to enforce this, could we set a property that would make NuGet enforce that, or could we observe if NuGet ran with an audit source? If we did so, then we could make arcade check for that and emit an error if it's absent.

When <auditSources> is not empty, but a vulnerability database couldn't be downloaded, NuGet will raise a NU1905 warning. This can be elevated to an error. However, NuGetAudit will not run if the MSBuild property NuGetAudit is set to false. So, you could enforce by 1. checking that nuget.config has at least one audit source (no easy way, needs xml parsing, or use dotnet nuget config get all, and parse the output) and 2. make sure NuGetAudit isn't set to false (dotnet sln list and dotnet msbuild -getProperty:NuGetAudit, or search all project.asset.json files for $.project.restore.restoreAuditProperties.enableAudit)

Are the audit sources controllable with an itemgroup, or only through nuget.config?

Only through nuget.config. We have a request to make it available via msbuild, but it has zero upvotes so far.

mmitche commented 1 month ago

Arcade can check whether NuGetAudit = false (it isn't). We would need to implement the additional check on the nuget.config. I don't think the binlog is the way to go here as there was no obvious way to say auditing happened. For instance, the list of audit sources is not even outputted into the binlog.

We could go in the direction of the binlog being more checkable, but IMO:

jaredpar commented 1 month ago

I don't think we should rely on the existence of the binlog for auditing. Some repos turn this off specifically (aspnetcore) for perf purposes. It seems pretty fragile to me.

You can argue "they can turn it off" for any option here. The question is whether you can detect the situation or not. The abscence of a published file is pretty easy to detect 😄

ericstj commented 1 month ago

Could we force a layered NuGet.config with the audit source in official builds? NuGet.config's are all about layering.

mmitche commented 1 month ago

I don't think layering would work (not sure where we would put the layer, but we could modify Maestro to augment the nuget.config. It already can do so for other things. Not sure whether all repos would love that approach, though.

AndriySvyryd commented 1 month ago

There should also be an option for arcade to set NuGetAudit to false when the branch is locked and a release is underway. Since it can start failing at any point and delay the builds. Setting WarningsNotAsErrors is not enough, the build will still fail.

marcpopMSFT commented 1 week ago

What is the guidance here? If I turn it on and leave them as warnings (as errors), my builds will get broken the moment there is a release. We've already seen that break VMR builds because of older arcade dependencies and the warnings were downgraded back to warnings. However, if they are left as warnings, will anyone ever see them?

We already have CG running so what additional value does this provide over that?

ViktorHofer commented 1 week ago

We recommend and implemented the following in most repos: https://github.com/dotnet/arcade/blob/ad64a76fd00a641116e79f77d48b42c6e7ed9b0a/Directory.Build.props#L18

This means that we block official builds but not dev and CI builds. We must not release new bits without NuGet Audit enabled but we don't want to nuke all builds on a Patch Tuesday.

jaredpar commented 1 week ago

This means that we block official builds but not dev and CI builds.

I think the best approach is:

The rationale for not wanting error on official builds is that it will lead to us failing official builds for NuGet audit warnings that aren't actually blocking us shipping. NuGet audit is always going to have a degree of false positive or real positive but test assets only. Having us fail official builds for that feels wrong. Instead I think it's better to warn on official builds and then before shipping audit the warnings and decide if they are critical enough to block shipping.

I also think we need to think about the policy for meta repos like source build or VMR. That is taking all of the friction with NuGet audit and multiplying it by the number of repositories there are. Further in the case of VMR we're getting the warning in a place (asynchronously) but we will be fixing the warning in another place (the original repo). Having NuGet audit run in PR / local dev seems like it will lead to an incredible amount of noise. For that rolling / official seems a better course with checks to ensure the teams are tracking the issues in the originating repository.