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

✨ Support Nuget Central Package Management #4369

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

Conversation

balteravishay
Copy link
Contributor

What kind of change does this PR introduce?

Support nuget central package management in detecting pinned dependencies

What is the current behavior?

Today, scorecard only supported nuget pinning dependencies with lockfiles. However, nuget provides package-manager-level assurance that when enabling CPM AND declaring specific versions for all direct dependencies the same level of pinning is provided to the end-user.

What is the new behavior (if this is a feature change)?**

Scorecard will check if Directory.*.props file is present and both:
a. ManagePackageVersionsCentrally attribute is set to true
b. all direct dependencies have a specific version declared

if both are true, all unpinned dependencies will be marked as pinned.

  • Tests for the changes have been added (for bug fixes/features)

Which issue(s) this PR fixes

Fixes #4252

Special notes for your reviewer

This was discussed in a community call with @spencerschrock and it continues the work that was started in PR #4351

Here is a repo that would be found as pinned by this new checks: foesmm/WolvenKit since it has a github workflow with dotnet restore (without lockfile flag) but declares CPM and all dependencies are pinned to specific versions in the properties file

Does this PR introduce a user-facing change?

-->

NONE

Co-authored-by: Liam Moat <[email protected]>
Co-authored-by: Ioana A <[email protected]>
Co-authored-by: Mélanie Guittet <[email protected]>

Signed-off-by: balteravishay <[email protected]>
@balteravishay balteravishay requested a review from a team as a code owner October 6, 2024 18:59
@balteravishay balteravishay requested review from justaugustus and raghavkaul and removed request for a team October 6, 2024 18:59
Signed-off-by: balteravishay <[email protected]>
@spencerschrock
Copy link
Member

all direct dependencies have a specific version declared

I seem to remember listing specific versions was considered pinned due to some immutable property of the nuget package manager? (Similar to how the Go ecosystem has a checksum database?) Is there a good link for that? I found https://devblogs.microsoft.com/nuget/nuget-package-signing/, but it seems to be a few years old, and didn't know what the default policy of the tool was these days.

@JonDouglas
Copy link

@jeffkl could you advise on this PR on supporting CPM in this project? 😄

Copy link

Choose a reason for hiding this comment

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

I'm not sure if it matters in this context, but be aware that the official name for the CPM file is Directory.Packages.props and on case sensitive file systems, it won't get imported if the case of the file name is incorrect. All of these files in testdata have a lowercase p. I'm not familiar enough with this codebase to say for sure if it will matter.

<ManagePackageVersionsCentrally>true</ManagePackageVersionsCentrally>
</PropertyGroup>
<ItemGroup>
<PackageVersion Include="SomePackage" Version="4.4.5" />
Copy link

Choose a reason for hiding this comment

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

Package versions are not pinned just because a version is declared. NuGet just uses this version specified when it comes across a <PackageReference Include="SomePackage" /> and emits an error if you specified a version in a project.

To truly "pin" a transitive package version that you do not have a direct <PackageReference /> for, you have to enable transitive pinning: https://learn.microsoft.com/nuget/consume-packages/Central-Package-Management#transitive-pinning

Copy link

Choose a reason for hiding this comment

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

Nevermind, I was wrong what "pinning" meant in this context.

Copy link
Member

Choose a reason for hiding this comment

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

We define "pinned" in a reproducibility sense: "a dependency that is explicitly set to a specific hash instead of
allowing a mutable version or range of versions". Which Avishay added support for via nuget --locked-mode, or the RestoreLockedMode property .

The NuGet claim in the PR description is that:

Today, scorecard only supported nuget pinning dependencies with lockfiles. However, nuget provides package-manager-level assurance that when enabling CPM AND declaring specific versions for all direct dependencies the same level of pinning is provided to the end-user.

Which to my limited understanding, is based on the package immutability nuget.org provides. But I'm not sure that fully covers all scenarios mentioned in the lockfile blog post.

Copy link
Contributor Author

@balteravishay balteravishay Oct 21, 2024

Choose a reason for hiding this comment

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

@spencerschrock, could you please elaborate which scenarios mentioned in the lockfile blog post you are referring to as missing from this PR:

  • nuget.config mismatch: is covered with CPM, since CPM suggests that packages are managed centrally and the user gets warnings when using multiple package sources
  • Intermediate versions and Floating versions - This PR checks for specifying specific versions and will deem floating versions as "unpinned"
  • Package deletion is granted by nuget
  • Package content mismatch is granted by Package Immutability/signing

Maybe, indeed, something that is not provided via CPM and direct dependency version check, as proposed by this PR, is transitive dependency pinning (as proposed by @jeffkl ). I can add that to check to the PR if you feel that would make it make all the requirements.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

BUG: .Net pinned dependency should support Central Package Management
4 participants