✨ Support Nuget Central Package Management #4369
Conversation
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
requested review from
justaugustus and
raghavkaul
and removed request for
a team
balteravishay
temporarily deployed
to
integration-test
GitHub Actions
Inactive
balteravishay
temporarily deployed
to
integration-test
GitHub Actions
Inactive
balteravishay
temporarily deployed
to
integration-test
GitHub Actions
Inactive
balteravishay
temporarily deployed
to
integration-test
GitHub Actions
Inactive
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. |
|
@jeffkl could you advise on this PR on supporting CPM in this project? 😄 |
There was a problem hiding this comment.
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" /> |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Nevermind, I was wrong what "pinning" meant in this context.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
I've been out of the office on personal time, will take a look in the next day or two.
There was a problem hiding this comment.
I’m a bit unclear on the terminology being used here, and I’d appreciate some clarification. In the example from checks.md, it says:
If your project is producing an application, declare all your dependencies with specific versions in your package format file (e.g. package.json for npm, requirements.txt for python, packages.config for nuget)
My understanding has always been that specifying exact versions is what we consider a pinned dependency check, and this is how we refer to it. Pinning provides consistency, and we typically improve reproducibility further with lock files for added determinism.
The example given, where the package manager resolves to the nearest version when an exact match isn’t available, doesn’t seem that compelling since other package managers handle similar cases. In practice, people pin versions that actually exist, and our registry follows an immutability policy, ensuring versions aren’t removed and that these dependencies remain reliable.
It might be helpful to create documentation that explains the different levels/scenarios of "pinned" dependency checks, with clear examples for each scenario. For instance, detailing how an exact version match qualifies as "pinned," a lock file represents "locked," and so forth, to ensure clarity across all cases.
There was a problem hiding this comment.
- Day 2: Version 4.0.0 gets published. NuGet now restores version 4.0.0 as it is an exact match.
Typically, we see folks publishing higher versions of their packages as time goes by, not lower versions.
There was a problem hiding this comment.
It might be helpful to create documentation that explains the different levels/scenarios of "pinned" dependency checks, with clear examples for each scenario. For instance, detailing how an exact version match qualifies as "pinned," a lock file represents "locked," and so forth, to ensure clarity across all cases.
Most of our documentation uses "pinned" to refer to lock files, so we can certainly try to clarify "pinned" vs "locked".
I did some digging into the history of the check, as well as checking with relevant contributors. It started off as Frozen-Deps, which looked for lockfiles., before being renamed Pinned-Dependencies. Most of the check is focused on lockfiles and immutable hashes (OCI or GitHub), but there is some pinning recognized for package managers with immutable versions (Go and Nuget).
In practice, people pin versions that actually exist
Typically, we see folks publishing higher versions of their packages as time goes by, not lower versions.
I recognize this is an edge case we probably don't need to worry about. I was just highlighting one difference of lockfiles and version pinning, as I clarified the semantics of the check.
There was a problem hiding this comment.
To truly "pin" a transitive package version that you do not have a direct for, you have to enable transitive pinning
Is this because one of your direct dependencies may declare a range for one of their dependencies?
There was a problem hiding this comment.
It might be helpful to create documentation that explains the different levels/scenarios of "pinned" dependency checks, with clear examples for each scenario. For instance, detailing how an exact version match qualifies as "pinned," a lock file represents "locked," and so forth, to ensure clarity across all cases.
Most of our documentation uses "pinned" to refer to lock files, so we can certainly try to clarify "pinned" vs "locked".
I did some digging into the history of the check, as well as checking with relevant contributors. It started off as
Frozen-Deps, which looked for lockfiles., before being renamedPinned-Dependencies. Most of the check is focused on lockfiles and immutable hashes (OCI or GitHub), but there is some pinning recognized for package managers with immutable versions (Go and Nuget).In practice, people pin versions that actually exist
Typically, we see folks publishing higher versions of their packages as time goes by, not lower versions.
I recognize this is an edge case we probably don't need to worry about. I was just highlighting one difference of lockfiles and version pinning, as I clarified the semantics of the check.
thanks! idk who owns the naming but might be worth calling this out in a future meeting.
should package managers provide basic support for the definition of pinned dependencies I provided earlier and what's in the docs? (i.e. exact version)? or is the expectation to contribute to the "locked" only definition?
lock files aren't that prevalent in NuGet/.NET. this means that the .NET/NuGet ecosystem would score very low:
it seems like this check might benefit from partial points for exact versions and full points for locked/frozen/etc deps?
or perhaps a new check for "pinned" / "locked" package dependencies would be needed to take all this into account to separate from the CI/CD & docker checks?
balteravishay
temporarily deployed
to
integration-test
GitHub Actions
Inactive
| func collectPostProcessNugetCPMDependencies(unpinnedNugetDependencies []*checker.Dependency, | ||
| postProcessingData *NugetPostProcessData, | ||
| ) { | ||
| packageVersions := postProcessingData.CpmConfig.PackageVersions | ||
|
|
||
| numFixedVersions, unfixedVersions := countFixedVersions(packageVersions) | ||
| // if all dependencies are fixed to specific versions, pin all dependencies | ||
| if numFixedVersions == len(packageVersions) { | ||
| pinAllNugetDependencies(unpinnedNugetDependencies) |
There was a problem hiding this comment.
Is CPM all-or-nothing for managing (direct) dependencies, or can you manage only a subset centrally? For example, can you pin foo in a Directory.Packages.props and then have a project with packages foo (with no version because CPM) and bar (with a version range)? As written, this PR may give this "pinned" status even though bar can float.
I want to make sure I understand the feature. CPM is just a configuration feature so you don't need to specify versions in more than one place? The reason I'm asking is to see if any of the rules (pinned to specific version) should also apply to all project files?
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.
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?
-->