✨ Add support for Nuget restore #4157

balteravishay · 2024-06-07T15:32:13Z

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.

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.

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 · 2024-06-07T19:00:16Z

/scdiff generate Pinned-Dependencies

github-actions · 2024-06-07T19:00:29Z

Here's a link to the scdiff run

spencerschrock · 2024-06-07T19:08:08Z

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 · 2024-06-08T19:01:07Z

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 · 2024-06-10T07:58:03Z

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 · 2024-06-10T20:31:48Z

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

Signed-off-by: balteraivshay <avishay.balter@gmail.com>

Bumps [github.com/google/osv-scanner](https://github.com/google/osv-scanner) from 1.7.3 to 1.7.4. - [Release notes](https://github.com/google/osv-scanner/releases) - [Changelog](https://github.com/google/osv-scanner/blob/main/CHANGELOG.md) - [Commits](google/osv-scanner@v1.7.3...v1.7.4) --- updated-dependencies: - dependency-name: github.com/google/osv-scanner dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Signed-off-by: balteraivshay <avishay.balter@gmail.com>

…pm database (ossf#4118) * Update endpoint used when getting repo from npm to solve ossf#3166 Signed-off-by: aklevans <alexklevans@gmail.com> * Update test files to account for endpoint change when getting repo from npm Signed-off-by: aklevans <alexklevans@gmail.com> * Fix linter issues Signed-off-by: aklevans <alexklevans@gmail.com> * Added unit tests for ossf#3166 and ossf#2441 Signed-off-by: aklevans <alexklevans@gmail.com> * fix linter issues and reduce mock json output in package_manager_test to only include necessary data Signed-off-by: aklevans <alexklevans@gmail.com> * fix linter issues in package_managers.go Signed-off-by: aklevans <alexklevans@gmail.com> * convert windows line breaks to linux Signed-off-by: aklevans <alexklevans@gmail.com> * reduce test case size, still has windows line breaks Signed-off-by: aklevans <alexklevans@gmail.com> * Fix unit tests Signed-off-by: aklevans <alexklevans@gmail.com> * attempt linter fix Signed-off-by: aklevans <alexklevans@gmail.com> * Fix linter issues stemming from windows line breaks Signed-off-by: aklevans <alexklevans@gmail.com> * Remove magic number and rename variable to be more accurate Signed-off-by: aklevans <alexklevans@gmail.com> --------- Signed-off-by: aklevans <alexklevans@gmail.com> Signed-off-by: aklevans <105876795+aklevans@users.noreply.github.com> Signed-off-by: balteraivshay <avishay.balter@gmail.com>

Signed-off-by: balteraivshay <avishay.balter@gmail.com>

Signed-off-by: Spencer Schrock <sschrock@google.com> Signed-off-by: balteraivshay <avishay.balter@gmail.com>

Before this change, when running with '-o foo' the output would end with: ``` RESULTS ------- ``` This was rather confusing. There's of course many ways to make this more clear, this commit adds a log line announcing where the output is written to: ``` RESULTS ------- Writing to foo ``` Signed-off-by: Arnout Engelen <arnout@bzzt.net> Signed-off-by: balteraivshay <avishay.balter@gmail.com>

* fix unlicense detection The code previously had some special logic for handling the Unlicense SPDX identifier. While this worked for local file detection, it broke detection for SPDX identifiers provided by the forge. This change moves the logic to the part of the code concerned with local file detection, so both work now. Signed-off-by: Spencer Schrock <sschrock@google.com> * remove part of comment which is no longer relevant Signed-off-by: Spencer Schrock <sschrock@google.com> --------- Signed-off-by: Spencer Schrock <sschrock@google.com> Signed-off-by: balteraivshay <avishay.balter@gmail.com>

Signed-off-by: balteraivshay <avishay.balter@gmail.com>

* add projectpackageversions to signed releases raw results Signed-off-by: Raghav Kaul <raghavkaul+github@google.com> * finding: add NewNot* helpers, fix error msg Signed-off-by: Raghav Kaul <raghavkaul+github@google.com> * probe: releasesHaveVerifiedProvenance Signed-off-by: Raghav Kaul <raghavkaul+github@google.com> * logging Signed-off-by: Raghav Kaul <raghavkaul+github@google.com> * fix tests and lint Signed-off-by: Raghav Kaul <raghavkaul+github@google.com> * address comments Signed-off-by: Raghav Kaul <raghavkaul+github@google.com> * remove unused Signed-off-by: Raghav Kaul <raghavkaul+github@google.com> * fix merge conflict Signed-off-by: Raghav Kaul <raghavkaul+github@google.com> --------- Signed-off-by: Raghav Kaul <raghavkaul+github@google.com> Signed-off-by: balteraivshay <avishay.balter@gmail.com>

Signed-off-by: balteraivshay <avishay.balter@gmail.com>

balteravishay · 2024-06-12T08:39:00Z

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 · 2024-06-12T16:42:34Z

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 · 2024-06-12T19:31:34Z

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 · 2024-06-24T01:50:26Z

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

spencerschrock · 2024-06-26T21:25:14Z

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

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

github-actions · 2024-07-07T01:54:10Z

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

This reverts commit 9e66eb2. Signed-off-by: Spencer Schrock <sschrock@google.com>

Signed-off-by: Spencer Schrock <sschrock@google.com>

spencerschrock

Thanks for the PR! I went ahead and reverted the supported shell change to at least get partial support before our next release.

balteravishay requested a review from a team as a code owner June 7, 2024 15:32

balteravishay requested review from naveensrinivasan and spencerschrock and removed request for a team June 7, 2024 15:32

balteravishay temporarily deployed to gitlab June 7, 2024 15:32 — with GitHub Actions Inactive

balteravishay temporarily deployed to integration-test June 7, 2024 15:32 — with GitHub Actions Inactive

balteravishay force-pushed the avbalter/support-dotnet-lock branch from 73a1af8 to 282c324 Compare June 7, 2024 15:35

balteravishay temporarily deployed to gitlab June 7, 2024 15:36 — with GitHub Actions Inactive

balteravishay temporarily deployed to integration-test June 7, 2024 15:36 — with GitHub Actions Inactive

balteravishay temporarily deployed to gitlab June 7, 2024 15:36 — with GitHub Actions Inactive

balteravishay temporarily deployed to integration-test June 7, 2024 15:37 — with GitHub Actions Inactive

balteravishay temporarily deployed to gitlab June 7, 2024 15:51 — with GitHub Actions Inactive

balteravishay temporarily deployed to integration-test June 7, 2024 15:51 — with GitHub Actions Inactive

balteravishay temporarily deployed to gitlab June 8, 2024 19:01 — with GitHub Actions Inactive

balteravishay temporarily deployed to integration-test June 8, 2024 19:01 — with GitHub Actions Inactive

balteravishay temporarily deployed to gitlab June 12, 2024 08:36 — with GitHub Actions Inactive

balteravishay temporarily deployed to integration-test June 12, 2024 08:36 — with GitHub Actions Inactive

balteravishay and others added 7 commits June 12, 2024 08:36

Nuget lock file support

4302af6

Signed-off-by: balteraivshay <avishay.balter@gmail.com>

🌱 Bump golang.org/x/text from 0.15.0 to 0.16.0 (ossf#4142)

a8cb31d

Signed-off-by: balteraivshay <avishay.balter@gmail.com>

🌱 Bump github.com/rhysd/actionlint from 1.7.0 to 1.7.1 (ossf#4138)

ead508d

Signed-off-by: balteraivshay <avishay.balter@gmail.com>

🌱 Bump github.com/bradleyfalzon/ghinstallation/v2 (ossf#4137)

12f1ca5

Signed-off-by: balteraivshay <avishay.balter@gmail.com>

⚠️ remove dependencydiff functionality (ossf#4146)

8de4b87

Signed-off-by: Spencer Schrock <sschrock@google.com> Signed-off-by: balteraivshay <avishay.balter@gmail.com>

raboof and others added 5 commits June 12, 2024 08:36

fix lint

a609128

Signed-off-by: balteraivshay <avishay.balter@gmail.com>

fix shell download

9e66eb2

Signed-off-by: balteraivshay <avishay.balter@gmail.com>

balteravishay force-pushed the avbalter/support-dotnet-lock branch from 258682b to 9e66eb2 Compare June 12, 2024 08:37

balteravishay temporarily deployed to gitlab June 12, 2024 08:37 — with GitHub Actions Inactive

Merge branch 'main' into avbalter/support-dotnet-lock

bb8f301

balteravishay temporarily deployed to gitlab June 12, 2024 08:37 — with GitHub Actions Inactive

balteravishay temporarily deployed to integration-test June 12, 2024 08:38 — with GitHub Actions Inactive

Merge branch 'main' into avbalter/support-dotnet-lock

6decb95

balteravishay temporarily deployed to gitlab June 13, 2024 12:33 — with GitHub Actions Inactive

balteravishay temporarily deployed to integration-test June 13, 2024 12:33 — with GitHub Actions Inactive

github-actions bot added the Stale label Jun 24, 2024

spencerschrock removed the Stale label Jun 26, 2024

github-actions bot added the Stale label Jul 7, 2024

spencerschrock added 2 commits July 10, 2024 15:39

Revert "fix shell download"

dde4f24

This reverts commit 9e66eb2. Signed-off-by: Spencer Schrock <sschrock@google.com>

Merge branch 'main' into avbalter/support-dotnet-lock

d47c79d

Signed-off-by: Spencer Schrock <sschrock@google.com>

spencerschrock temporarily deployed to gitlab July 10, 2024 22:57 — with GitHub Actions Inactive

spencerschrock temporarily deployed to integration-test July 10, 2024 22:57 — with GitHub Actions Inactive

spencerschrock approved these changes Jul 10, 2024

View reviewed changes

spencerschrock enabled auto-merge (squash) July 10, 2024 23:01

spencerschrock merged commit 78115de into ossf:main Jul 10, 2024
36 checks passed

balteravishay mentioned this pull request Oct 15, 2024

BUG Pinned Dependency checks for nuget/.Net does not consider implicit restore #4381

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ Add support for Nuget restore #4157

✨ Add support for Nuget restore #4157

balteravishay commented Jun 7, 2024

spencerschrock commented Jun 7, 2024

github-actions bot commented Jun 7, 2024

spencerschrock commented Jun 7, 2024

balteravishay commented Jun 8, 2024

balteravishay commented Jun 10, 2024

spencerschrock commented Jun 10, 2024

balteravishay commented Jun 12, 2024

spencerschrock commented Jun 12, 2024 •

edited

Loading

balteravishay commented Jun 12, 2024

github-actions bot commented Jun 24, 2024

spencerschrock commented Jun 26, 2024

github-actions bot commented Jul 7, 2024

spencerschrock left a comment

✨ Add support for Nuget restore #4157

✨ Add support for Nuget restore #4157

Conversation

balteravishay commented Jun 7, 2024

What kind of change does this PR introduce?

What is the current behavior?

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

Which issue(s) this PR fixes

Special notes for your reviewer

Does this PR introduce a user-facing change?

spencerschrock commented Jun 7, 2024

github-actions bot commented Jun 7, 2024

spencerschrock commented Jun 7, 2024

balteravishay commented Jun 8, 2024

balteravishay commented Jun 10, 2024

spencerschrock commented Jun 10, 2024

balteravishay commented Jun 12, 2024

spencerschrock commented Jun 12, 2024 • edited Loading

balteravishay commented Jun 12, 2024

github-actions bot commented Jun 24, 2024

spencerschrock commented Jun 26, 2024

github-actions bot commented Jul 7, 2024

spencerschrock left a comment

Choose a reason for hiding this comment

spencerschrock commented Jun 12, 2024 •

edited

Loading