✨ Add support for Nuget restore

balteravishay commented 3 weeks ago

What kind of change does this PR introduce?

Add support for checking Nuget repeatable builds through the Pinned-Dependency checks. This supports nuget cli, dotnet cli and msbuild through an explicit restore command with the locked mode feature such as:

nuget restore -LockedMode
dotnet restore --locked-mode
msbuild.exe /t:restore /p:RestoreLockedMode=true'

It does not support implicit restores through build command such as

dotnet build

and does not support csproj definition of RestoreLockedMode attribute.

[x] PR title follows the guidelines defined in our pull request documentation

What is the current behavior?

dotnet/nuget restore commands are not being evaluated when calculating the Pinned-Dependency score.

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

dotnet, nuget and msbuild restore commands are validated for using the "locked mode" flag which assumes that the repository has a lock file and that it cannot be changed during a CI build.

[x] Tests for the changes have been added (for bug fixes/features)

Which issue(s) this PR fixes

Fixes #2865

Special notes for your reviewer

Does this PR introduce a user-facing change?

For user-facing changes, please add a concise, human-readable release note to the release-note

scorecard checks that nuget restore action that is accessible either via nuget cli, dotnet cli or msbuild is applied with a lock file and using the locked mode flag, thus making sure that the builds are repeatable and that the hashes are pinned and validated.

spencerschrock commented 3 weeks ago

/scdiff generate Pinned-Dependencies

github-actions[bot] commented 3 weeks ago

Here's a link to the scdiff run

spencerschrock commented 3 weeks ago

I'll preface this by saying I'm not familiar with the C# ecosystem, but the changes seem reasonable, given the documentation:

The implementation seems fine, and the tests match expectations. I'm not sure any of our scdiff repos are affected by the change. Do you happen to know of any repos that I can manually compare the results for?

balteravishay commented 3 weeks ago

Thank you for the review @spencerschrock ! I am sorry that I did not provide enough information for folks who are unfamiliar with the dotnet/nuget ecosystem. Here is another solid recommendation for how to lock dependencies in that ecosystem, that was the foundation for this PR: https://learn.microsoft.com/en-us/nuget/concepts/security-best-practices (specifically this linked content under the "Lock files" section of this document that leads to this document: https://learn.microsoft.com/en-us/nuget/consume-packages/package-references-in-project-files#locking-dependencies)

Here are some repos I found that could be used for testing:

https://github.com/PlexRipper/PlexRipper (locked with dotnet) https://github.com/PULSAR-Modders/pulsar-mod-loader (locked with nuget) https://github.com/SeriaWei/ZKEACMS (unpinned with dotnet) https://github.com/caphindsight/TrulyQuantumChess (unpinned with nuget)

I'll try to get someone from the nuget/dotnet tool to review this as well. maybe @nkolev92 @joelverhagen can comment on this?

balteravishay commented 3 weeks ago

Follow up question: is there a way that I can search for other files in the repo during the dependency-check? The reason I am asking is because the current implementation does not support locking the dependency by using the .csproj file attribute "RestoreLockedMode" (which is being used as far as I can tell: https://github.com/search?q=path%3A**%2F*.csproj%20RestoreLockedMode&type=code). So the way to check that is during the check for "nuget resotre" or "dotnet restore", if the locked-mode flag is not present, the code would have to search for csproj files in the repo, and then parse the csproj XML looking for that attribute. probably this is a matter for a different PR, but wanted to get your thoughts on that.

spencerschrock commented 2 weeks ago

Thanks for the info!

https://github.com/PULSAR-Modders/pulsar-mod-loader (locked with nuget)

Note: I'm not seeing this one detected.

go run main.go --repo PULSAR-Modders/pulsar-mod-loader --checks Pinned-Dependencies --format json --show-details | jq

is there a way that I can search for other files in the repo during the dependency-check

Generally this is done via fileparser.OnMatchingFileContentDo or fileparser.OnMatchingFileReaderDo. But I don't think we should do that nested inside of collectUnpinnedPackageManagerDownload.

the code would have to search for csproj files in the repo, and then parse the csproj XML looking for that attribute.

I'm curious how we would tie a given csproj file to a specific nuget/dotnet/msbuild invocation.

probably this is a matter for a different PR, but wanted to get your thoughts on that.

agreed for different PR

balteravishay commented 2 weeks ago

Thanks for the info!

https://github.com/PULSAR-Modders/pulsar-mod-loader (locked with nuget)

Note: I'm not seeing this one detected.

Sorry, forgot to push a fix in the supportedShells. please pull and try again. still trying to get folks from the nuget ecosystem to review this change as well

spencerschrock commented 2 weeks ago

Sorry, forgot to push a fix in the supportedShells. please pull and try again.

Ah I didn't look too closely at why it wasn't parsing. We may need to back this out (haven't had time to look at it closely yet).

// supportedShells is the list of shells that are supported by mvdan.cc/sh/v3/syntax.

And powershell isn't supported by mvdan.cc/sh/v3/syntax. We handle ParseErrors gracefully though, so it might be ok? But the nuget detection would only work for powershell scripts that also parse as valid Bash, which happens to be the case for this repo, but I'm not sure how wide spread that would be.

balteravishay commented 2 weeks ago

yes, i see your point. here are a couple of other examples of bash-like powershell commands that would be hit by the new check (though I would have to also add "pwsh" to the supportedShells for them to run, since they run on windows agents: repo: d2dyno1/ClipboardCanvas repo: ividyon/WitchyBND

but TBH, it is not the "common" case. thoughts?

github-actions[bot] commented 1 week ago

This pull request has been marked stale because it has been open for 10 days with no activity

spencerschrock commented 4 days ago

but TBH, it is not the "common" case. thoughts?

Let's undo 9e66eb2280d5f7f57768d610841f3ca5c9df0141 for now, and we can merge this in with partial support.

ossf / scorecard