🐛 Dependency-Pinning: only score detected ecosystems#3436
🐛 Dependency-Pinning: only score detected ecosystems#3436spencerschrock merged 47 commits intoossf:mainfrom
Conversation
Add a field Pinned to Dependency structure. Update to save Dependencies pinned and unpinned. Not only unpinned ones. All download then run executions are considered unpinned. Because there is no remediation to pin them. For package manager downloads: add early return if there are no commands, separate package manager identification (go, npm, choco, pip) from decision if installation is pinned or unpinned. Change Go case "go get -d -v" considered pinned, to any Go installations containing "-d" to be considered pinned. Signed-off-by: Gabriela Gutierrez <[email protected]>
We need to add a new conversion of boolean to pointer. Currently, we had string and int conversions named asPointer but not used in the same file. In order to know when we are using which conversion and considering bool and string would have to be used in the same file, it was needed to differentiate the method names. New method names are asIntPointer, asStringPointer and soon asBoolPointer. Signed-off-by: Gabriela Gutierrez <[email protected]>
Field needs to be a pointer to work when accessing values on evaluation. Signed-off-by: Gabriela Gutierrez <[email protected]>
We're changing the ecossystems result structure. The result structure previously stored if the ecossystem is fully pinned or not. The new result structure can tell how many dependencies of that ecossystem were found and how many were pinned. This change is necessary to ignore not applicable ecossystems on the final aggregated score. When iterating the dependencies, now we go through pinned and unpinned dependencies, not only unpinned, and in each iteration we update the result. We kept the behavior of only log warnings for unpinned dependencies. Signed-off-by: Gabriela Gutierrez <[email protected]>
If no dependencies of an ecossystem are found, it results in an inconclusive score (-1). As in other checks, this means here that the ecossystem scoring is not applicable in this case. At the same time, we are keep the scoring criteria the same. If all dependencies are pinned, it results in maximum score (10) and if 1 or more dependencies are unpinned, it results in a minimum score (0) for that ecossystem. GitHub workflow cases are handled differently but the idea is the same. We are also adding a log to know when an ecossystem was not found. Signed-off-by: Gabriela Gutierrez <[email protected]>
Signed-off-by: Gabriela Gutierrez <[email protected]>
Signed-off-by: Gabriela Gutierrez <[email protected]>
Change test from `createReturnValuesForGitHubActionsWorkflowPinned` function to `createReturnForIsGitHubActionsWorkflowPinned` wrapper function so we can test logs. We have adjusted the existing test cases and included new test cases. Signed-off-by: Gabriela Gutierrez <[email protected]>
Break "various warnings" tests into smaller tests for pinned and unpinned dependencies and how they react to warn and debug messages. Plus add tests for how the score is affected when all dependencies are pinned, when no dependencies are pinned, when there are no dependencies, and partial dependencies pinned. Also, how dependencies unpinned in 1 or multiple ecossystems affect the warn messages, add one unpinned case for each ecossystem to see if they are being detected and separate the download then run 2 possible cases, there are currently scoring and logging wrong due to a bug. Signed-off-by: Gabriela Gutierrez <[email protected]>
Signed-off-by: Gabriela Gutierrez <[email protected]>
When we changed the scoring method to ignore not applicable scores, we removed the normalization of inconclusive scores to 0. The normalization was done by `maxScore` function, that was deleted in the process. Signed-off-by: Gabriela Gutierrez <[email protected]>
Signed-off-by: Gabriela Gutierrez <[email protected]>
Signed-off-by: Gabriela Gutierrez <[email protected]>
Signed-off-by: Gabriela Gutierrez <[email protected]>
Signed-off-by: Gabriela Gutierrez <[email protected]>
Signed-off-by: Gabriela Gutierrez <[email protected]>
Signed-off-by: Gabriela Gutierrez <[email protected]>
If, for example, you have GitHub-owned actions and none Third-party actions, you should receive a "no Third-party actions found" log and don't receive a "all Third-party actions are pinned" log. At the same time, you deserve the score of pinning Third-party to complement the GitHub-owned score. Signed-off-by: Gabriela Gutierrez <[email protected]>
The repo being tested, `ossf-tests/scorecard-check-pinned-dependencies-e2e`, has no Third-party actions only GitHub-owned actions, that are unpinned, no npm installs, multiple go installs all pinned, and all other dependencies types are unpinned. This gives us 8 for actionScore, -1 for npm score, 10 for goScore, and 0 for all other scores. Previously the total score was 28/7 =~ 4, and now the total score is 18/6 =~ 3. The number of logs remain the same. The "all Third-party actions are pinned" will be replaced by "no Third-party actions found", which is a more realistic info and same thing for npm installs. Signed-off-by: Gabriela Gutierrez <[email protected]>
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #3436 +/- ##
==========================================
+ Coverage 64.40% 64.68% +0.27%
==========================================
Files 188 188
Lines 13425 13412 -13
==========================================
+ Hits 8647 8676 +29
+ Misses 4304 4273 -31
+ Partials 474 463 -11 |
spencerschrock
left a comment
There was a problem hiding this comment.
Left a partial review, first thoughts:
- Touching asPointer in unrelated files makes the change bigger than it needs to be
- The scoring approach is different than discussed.
Signed-off-by: Gabriela Gutierrez <[email protected]>
|
Ok, ready to take a look again! |
spencerschrock
left a comment
There was a problem hiding this comment.
Looks good, just some small optional things.
|
I plan on running a quick A/B of scoring changes to make sure everything looks good, and following up with my earlier request for feedback from the other maintainers. |
Signed-off-by: Gabriela Gutierrez <[email protected]>
Signed-off-by: Gabriela Gutierrez <[email protected]>
In general, I think this is working as intended. Across a random selection of 10 popular repos, we see 6 of them drop to a score of 0 as none of their dependencies are pinned: For the repos that still score non-zero, should there be any go run main.go --repo github.com/apache/commons-codec --checks Pinned-Dependencies --format json --show-details | jq
{
"date": "2023-09-18T13:41:25-07:00",
"repo": {
"name": "github.com/apache/commons-codec",
"commit": "585497f09b026f6602daf986723a554e051bdfe6"
},
"scorecard": {
"version": "",
"commit": "unknown"
},
"score": 10,
"checks": [
{
"details": null,
"score": 10,
"reason": "all dependencies are pinned",
"name": "Pinned-Dependencies",
"documentation": {
"url": "https://github.com/ossf/scorecard/blob/main/docs/checks.md#pinned-dependencies",
"short": "Determines if the project has declared and pinned the dependencies of its build process."
}
}
],
"metadata": null
}
There was no objection from other maintainers. |
Yes. I will add an Info log for "2 out of 10 go install dependencies pinned" for ecosystems we do find. So we have the information if all dependencies of an ecossystem are pinned, and how much work was that. I will address this tomorrow. |
Signed-off-by: Gabriela Gutierrez <[email protected]>
The number of info logs should be same number of identified ecossystems. GitHub-owned GitHubAction and third-party GitHubAction count as different ecossytems. Signed-off-by: Gabriela Gutierrez <[email protected]>
The repo being tested, `ossf-tests/scorecard-check-pinned-dependencies-e2e`, has GitHub-owned GitHubActions, containerImage, downloadThenRun, pipCommand and goCommand dependencies. Therefore it will have 5 Info logs, one for each ecossystem. Signed-off-by: Gabriela Gutierrez <[email protected]>
Signed-off-by: Gabriela Gutierrez <[email protected]>
spencerschrock
left a comment
There was a problem hiding this comment.
I think this looks good thanks.
go run main.go --repo ossf/scorecard --checks Pinned-Dependencies --format json --show-details | jq
{
"date": "2023-09-24T20:13:47-07:00",
"repo": {
"name": "github.com/ossf/scorecard",
"commit": "5a5a6561d65067678990e69dc3d16a711c98c048"
},
"scorecard": {
"version": "",
"commit": "unknown"
},
"score": 9,
"checks": [
{
"details": [
"Warn: third-party GitHubAction not pinned by hash: .github/workflows/goreleaser.yaml:89: update your workflow using https://app.stepsecurity.io/secureworkflow/ossf/scorecard/goreleaser.yaml/main?enable=pin",
"Warn: third-party GitHubAction not pinned by hash: .github/workflows/slsa-goreleaser.yml:47: update your workflow using https://app.stepsecurity.io/secureworkflow/ossf/scorecard/slsa-goreleaser.yml/main?enable=pin",
"Info: 51 out of 51 GitHub-owned GitHubAction dependencies pinned",
"Info: 46 out of 48 third-party GitHubAction dependencies pinned",
"Info: 25 out of 25 containerImage dependencies pinned",
"Info: 1 out of 1 goCommand dependencies pinned"
],|
By the way, I did the A/B comparison, and I think these info statements help. Repos in general had a score drop as expected. More repos had a |
|
Good to know, thanks for the feedback! |
What kind of change does this PR introduce?
This is a bug fix. Pinned-Dependencies score used to score a 10 for ecossystems not used by the project. Now, it scores -1 when the ecossystem is not used, and this score is not included in the final score for Pinned-Dependencies.
What is the current behavior?
For example, if project A uses only Python and has only Python or pip dependencies, the project would score 10 for Dockerfiles, npm, go and other ecossystems dependencies it does not use.
What is the new behavior (if this is a feature change)?**
In this PR, if no dependencies are found for an ecossystem, this ecossystem score is not included in the final score.
Which issue(s) this PR fixes
Fixes #3254
Fixes #2679
Special notes for your reviewer
This change will likely decrease the Pinned-Dependencies score for all projects.
Does this PR introduce a user-facing change?
For user-facing changes, please add a concise, human-readable release note to
the
release-note(In particular, describe what changes users might need to make in their
application as a result of this pull request.)