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

Merged
merged 9 commits into from
Dec 11, 2024

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 <contact@liammoat.com>
Co-authored-by: Ioana A <Ioana37@users.noreply.github.com>
Co-authored-by: Mélanie Guittet <meguittet@users.noreply.github.com>

Signed-off-by: balteravishay <avishay.balter@gmail.com>
@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 <avishay.balter@gmail.com>
@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? 😄

@balteravishay
Copy link
Contributor Author

hey @spencerschrock, thanks for your feedback! any other questions about CPM or can we start the review of this PR?

@spencerschrock
Copy link
Member

hey @spencerschrock, thanks for your feedback! any other questions about CPM or can we start the review of this PR?

Yep my questions are answered (thanks everyone), I'll get started on the review when I get some time.

checks/raw/pinned_dependencies.go Outdated Show resolved Hide resolved
checks/raw/pinned_dependencies.go Outdated Show resolved Hide resolved
checks/raw/pinned_dependencies.go Show resolved Hide resolved
checks/raw/pinned_dependencies.go Outdated Show resolved Hide resolved
checks/raw/pinned_dependencies.go Outdated Show resolved Hide resolved
internal/dotnet/properties/properties_test.go Outdated Show resolved Hide resolved
internal/dotnet/properties/properties_test.go Outdated Show resolved Hide resolved
Copy link

codecov bot commented Dec 10, 2024

Codecov Report

Attention: Patch coverage is 81.35593% with 22 lines in your changes missing coverage. Please review.

Project coverage is 68.63%. Comparing base (353ed60) to head (36d08c1).
Report is 73 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4369      +/-   ##
==========================================
+ Coverage   66.80%   68.63%   +1.82%     
==========================================
  Files         230      242      +12     
  Lines       16602    18031    +1429     
==========================================
+ Hits        11091    12375    +1284     
- Misses       4808     4848      +40     
- Partials      703      808     +105     

Signed-off-by: balteraivshay <avishay.balter@gmail.com>
@spencerschrock
Copy link
Member

/scdiff generate Pinned-Dependencies

Copy link

Copy link
Member

@spencerschrock spencerschrock left a comment

Choose a reason for hiding this comment

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

Thanks all!

(ignore any unit test failures when I update/merge, we had a slight flake introduced in a previous PR that should be fixed soon)

@spencerschrock spencerschrock enabled auto-merge (squash) December 11, 2024 17:08
@spencerschrock spencerschrock merged commit 213bae3 into ossf:main Dec 11, 2024
38 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

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