🐛 Pinned-Dependencies: only score detected ecosystems (ossf#3436)

* feat: Define if dependency is pinned or unpinned 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]> * refactor: Convert diff var types to pointer 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]> * fix: Pinned Dependency field type Field needs to be a pointer to work when accessing values on evaluation. Signed-off-by: Gabriela Gutierrez <[email protected]> * feat: Count pinned and unpinned deps 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]> * feat: Flag not applicable ecossystems 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]> * feat: Score only applicable ecossystems Signed-off-by: Gabriela Gutierrez <[email protected]> * feat: If no dependencies then create inconclusive score Signed-off-by: Gabriela Gutierrez <[email protected]> * test: GitHub Actions score and logs 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]> * test: Pinned dependencies score 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]> * test: Ecossystems score and logs Signed-off-by: Gabriela Gutierrez <[email protected]> * test: Remove deleted maxScore function test 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]> * test: Adding GitHub Actions dependencies to result Signed-off-by: Gabriela Gutierrez <[email protected]> * test: Update GitHub Actions result Signed-off-by: Gabriela Gutierrez <[email protected]> * test: Update pip installs result Signed-off-by: Gabriela Gutierrez <[email protected]> * fix: Handle if nuget dependency is pinned or unpinned Signed-off-by: Gabriela Gutierrez <[email protected]> * tests: Fix check warnings for unpinned dependencies Signed-off-by: Gabriela Gutierrez <[email protected]> * fix: Linter errors Signed-off-by: Gabriela Gutierrez <[email protected]> * fix: GitHub Actions pinned log 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]> * test: Fix "ossf-tests/scorecard-check-pinned-dependencies-e2e" 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]> * Revert rename `asPointer` to `asStringPointer` Signed-off-by: Gabriela Gutierrez <[email protected]> * fix: Handle deps with parsing error and undefined pinning When a dependency has a parsing error it ends up with a `Msg` field. In this case, the dependency should not count in the final score, so we should not `updatePinningResults` in this case. Also, to continue with the evaluation calculation, we need to make sure the dependencies have a `Pinned` state. Here we are adding this validation for it along with a debug log. Signed-off-by: Gabriela Gutierrez <[email protected]> * test: Delete unecessary test We already have separate test for if 1 unpinned dependency shows a warn message, and 2 cases for when dependencies have errors and show a debug message. Signed-off-by: Gabriela Gutierrez <[email protected]> * test: Add missing dep Location cases Signed-off-by: Gabriela Gutierrez <[email protected]> * fix: Simplify Dockerfile pinned as name logic Signed-off-by: Gabriela Gutierrez <[email protected]> * fix: If ecossystem is not found show debug log If ecossystem is not found show debug log, not info log. This affects the tests, all not found ecossystems will "move" from info logs to debug logs. We are also complementing the `all dependencies pinned` and `all dependencies unpinned` cases so we have the max score case and the min score case using all kinds of dependencies. Signed-off-by: Gabriela Gutierrez <[email protected]> * test: Fix e2e tests and more unit tests Signed-off-by: Gabriela Gutierrez <[email protected]> * feat: Iterate all dependency types for final score Now we iterate all existing dependency types in the final score. This will fix the problem of new ecossystems not being count in the final score because we needed to update the evaluation part. This also fixes the problem of download then run being counted twice for the score. Now, we only have debug logs when there are errors with the dependency metadata. That means we don't log anymore when dependencies of an ecossystem are not found. We changed the info log format when dependencies are all pinned. We simplified the calculation of the scores. We removed unused error returns. And now we only iterate existing ecossystems. If an ecossystem is not found we will not iterate it. Signed-off-by: Gabriela Gutierrez <[email protected]> * feat: Proportional score We count all pinned dependencies over the total found dependencies of all ecossystems for the final score. But, we still want to give low prioritity to GHA GitHub-owned dependencies over GHA third-party dependencies. That's why we are doing a weighted proportional score, all ecossystems have a normal weight of 10 but GHAs have a weight. If you only have GitHub-owned, it will count as 10, because GHA don't weight less then other ecossystems. Same for GHA third-party, if you only have GHA third-party, it will also count as 10, because GHAs don't weight less then other ecossystems. But if you have both GHA GitHub-owned and third-party, GitHub-owned count less then third-party. Trying to keep the same weight as before, GitHub-owned weights 8 and third-party weights 2. These weights will make the score be more penalized if you have unpinned third-party and less penalized if you have unpinned GitHub-owned. Signed-off-by: Gabriela Gutierrez <[email protected]> * fix: GHA weights in proportional score Signed-off-by: Gabriela Gutierrez <[email protected]> * test: Fix scores and logs checking Add new cases for GHA scores since it's weighted differently now. Remove `createReturnValues` test since the function was removed. Fix current tests to adjust number of logs since we don't log if all dependencies are pinned or not anymore. Fix partially pinned score. Signed-off-by: Gabriela Gutierrez <[email protected]> * test: Fix e2e test 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 GHA ecossytem, -1 for npm score, 10 for goScore, and 0 for all other scores. Previously the total score was 18/6 =~ 3. Now, we count 5/6 GitHub-owned GHA pinned, 23/36 containerImage pinned, 0/88 downloadThenRun pinned, 2/49 pipCommand pinned, 17/17 goCommand pinned. This results in 47/186 pinned dependencies which results in 2.5 score, that is rounded down to 2. Plus, the number of info was reduced since we don't log info for "all pinned dependencies in X ecossystem" anymore. Signed-off-by: Gabriela Gutierrez <[email protected]> * refactor: Rename to ProportionalScoreWeighted Signed-off-by: Gabriela Gutierrez <[email protected]> * refactor: Var declarations to create proportional score Signed-off-by: Gabriela Gutierrez <[email protected]> * fix: Remove unnecessary pointer Signed-off-by: Gabriela Gutierrez <[email protected]> * fix: Dependencies priority declaration Signed-off-by: Gabriela Gutierrez <[email protected]> * fix: Ecosystem spelling Signed-off-by: Gabriela Gutierrez <[email protected]> * fix: Handle 0 weight and 0 total when creating proportional weighted score Signed-off-by: Gabriela Gutierrez <[email protected]> * fix: Revert -d flag identification change Signed-off-by: Gabriela Gutierrez <[email protected]> * fix: npm ci command is npm download and is pinned Signed-off-by: Gabriela Gutierrez <[email protected]> * fix: Linter errors Signed-off-by: Gabriela Gutierrez <[email protected]> * fix: Unexport error variable to other packages Signed-off-by: Gabriela Gutierrez <[email protected]> * refactor: Simplify no score groups condition Signed-off-by: Gabriela Gutierrez <[email protected]> * feat: Log proportion of dependencies pinned Signed-off-by: Gabriela Gutierrez <[email protected]> * test: Fix unit tests to include info logs 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]> * test: Fix e2e tests to include info logs 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]> * fix: Linter error Signed-off-by: Gabriela Gutierrez <[email protected]> --------- Signed-off-by: Gabriela Gutierrez <[email protected]> Signed-off-by: Allen Shearin <[email protected]>
kgangerlm · Nov 13, 2023 · 09e7030 · 09e7030
1 parent 64c09c3
commit 09e7030
Show file tree

Hide file tree

Showing 11 changed files with 1,523 additions and 600 deletions.
diff --git a/checker/check_result.go b/checker/check_result.go
@@ -16,6 +16,7 @@
 package checker
 
 import (
+	"errors"
 	"fmt"
 	"math"
 
@@ -50,6 +51,10 @@ const (
 	DetailDebug
 )
 
+// errSuccessTotal indicates a runtime error because number of success cases should
+// be smaller than the total cases to create a proportional score.
+var errSuccessTotal = errors.New("unexpected number of success is higher than total")
+
 // CheckResult captures result from a check run.
 //
 //nolint:govet
@@ -88,6 +93,14 @@ type LogMessage struct {
 	Remediation *rule.Remediation // Remediation information, if any.
 }
 
+// ProportionalScoreWeighted is a structure that contains
+// the fields to calculate weighted proportional scores.
+type ProportionalScoreWeighted struct {
+	Success int
+	Total   int
+	Weight  int
+}
+
 // CreateProportionalScore creates a proportional score.
 func CreateProportionalScore(success, total int) int {
 	if total == 0 {
@@ -97,6 +110,40 @@ func CreateProportionalScore(success, total int) int {
 	return int(math.Min(float64(MaxResultScore*success/total), float64(MaxResultScore)))
 }
 
+// CreateProportionalScoreWeighted creates the proportional score
+// between multiple successes over the total, but some proportions
+// are worth more.
+func CreateProportionalScoreWeighted(scores ...ProportionalScoreWeighted) (int, error) {
+	var ws, wt int
+	allWeightsZero := true
+	noScoreGroups := true
+	for _, score := range scores {
+		if score.Success > score.Total {
+			return InconclusiveResultScore, fmt.Errorf("%w: %d, %d", errSuccessTotal, score.Success, score.Total)
+		}
+		if score.Total == 0 {
+			continue // Group with 0 total, does not count for score
+		}
+		noScoreGroups = false
+		if score.Weight != 0 {
+			allWeightsZero = false
+		}
+		// Group with zero weight, adds nothing to the score
+
+		ws += score.Success * score.Weight
+		wt += score.Total * score.Weight
+	}
+	if noScoreGroups {
+		return InconclusiveResultScore, nil
+	}
+	// If has score groups but no groups matter to the score, result in max score
+	if allWeightsZero {
+		return MaxResultScore, nil
+	}
+
+	return int(math.Min(float64(MaxResultScore*ws/wt), float64(MaxResultScore))), nil
+}
+
 // AggregateScores adds up all scores
 // and normalizes the result.
 // Each score contributes equally.

diff --git a/checker/check_result_test.go b/checker/check_result_test.go
@@ -139,6 +139,273 @@ func TestCreateProportionalScore(t *testing.T) {
 	}
 }
 
+func TestCreateProportionalScoreWeighted(t *testing.T) {
+	t.Parallel()
+	type want struct {
+		score int
+		err   bool
+	}
+	tests := []struct {
+		name   string
+		scores []ProportionalScoreWeighted
+		want   want
+	}{
+		{
+			name: "max result with 1 group and normal weight",
+			scores: []ProportionalScoreWeighted{
+				{
+					Success: 1,
+					Total:   1,
+					Weight:  10,
+				},
+			},
+			want: want{
+				score: 10,
+			},
+		},
+		{
+			name: "min result with 1 group and normal weight",
+			scores: []ProportionalScoreWeighted{
+				{
+					Success: 0,
+					Total:   1,
+					Weight:  10,
+				},
+			},
+			want: want{
+				score: 0,
+			},
+		},
+		{
+			name: "partial result with 1 group and normal weight",
+			scores: []ProportionalScoreWeighted{
+				{
+					Success: 2,
+					Total:   10,
+					Weight:  10,
+				},
+			},
+			want: want{
+				score: 2,
+			},
+		},
+		{
+			name: "partial result with 2 groups and normal weights",
+			scores: []ProportionalScoreWeighted{
+				{
+					Success: 2,
+					Total:   10,
+					Weight:  10,
+				},
+				{
+					Success: 8,
+					Total:   10,
+					Weight:  10,
+				},
+			},
+			want: want{
+				score: 5,
+			},
+		},
+		{
+			name: "partial result with 2 groups and odd weights",
+			scores: []ProportionalScoreWeighted{
+				{
+					Success: 2,
+					Total:   10,
+					Weight:  8,
+				},
+				{
+					Success: 8,
+					Total:   10,
+					Weight:  2,
+				},
+			},
+			want: want{
+				score: 3,
+			},
+		},
+		{
+			name: "all groups with 0 weight, no groups matter for the score, results in max score",
+			scores: []ProportionalScoreWeighted{
+				{
+					Success: 1,
+					Total:   1,
+					Weight:  0,
+				},
+				{
+					Success: 1,
+					Total:   1,
+					Weight:  0,
+				},
+			},
+			want: want{
+				score: 10,
+			},
+		},
+		{
+			name: "not all groups with 0 weight, only groups with weight matter to the score",
+			scores: []ProportionalScoreWeighted{
+				{
+					Success: 1,
+					Total:   1,
+					Weight:  0,
+				},
+				{
+					Success: 2,
+					Total:   10,
+					Weight:  8,
+				},
+				{
+					Success: 8,
+					Total:   10,
+					Weight:  2,
+				},
+			},
+			want: want{
+				score: 3,
+			},
+		},
+		{
+			name: "no total, results in inconclusive score",
+			scores: []ProportionalScoreWeighted{
+				{
+					Success: 0,
+					Total:   0,
+					Weight:  10,
+				},
+			},
+			want: want{
+				score: -1,
+			},
+		},
+		{
+			name: "some groups with 0 total, only groups with total matter to the score",
+			scores: []ProportionalScoreWeighted{
+				{
+					Success: 0,
+					Total:   0,
+					Weight:  10,
+				},
+				{
+					Success: 2,
+					Total:   10,
+					Weight:  10,
+				},
+			},
+			want: want{
+				score: 2,
+			},
+		},
+		{
+			name: "any group with number of successes higher than total, results in inconclusive score and error",
+			scores: []ProportionalScoreWeighted{
+				{
+					Success: 1,
+					Total:   0,
+					Weight:  10,
+				},
+			},
+			want: want{
+				score: -1,
+				err:   true,
+			},
+		},
+		{
+			name: "only groups with weight and total matter to the score",
+			scores: []ProportionalScoreWeighted{
+				{
+					Success: 1,
+					Total:   1,
+					Weight:  0,
+				},
+				{
+					Success: 0,
+					Total:   0,
+					Weight:  10,
+				},
+				{
+					Success: 2,
+					Total:   10,
+					Weight:  8,
+				},
+				{
+					Success: 8,
+					Total:   10,
+					Weight:  2,
+				},
+			},
+			want: want{
+				score: 3,
+			},
+		},
+		{
+			name: "only groups with weight and total matter to the score but no groups have success, results in min score",
+			scores: []ProportionalScoreWeighted{
+				{
+					Success: 1,
+					Total:   1,
+					Weight:  0,
+				},
+				{
+					Success: 0,
+					Total:   0,
+					Weight:  10,
+				},
+				{
+					Success: 0,
+					Total:   10,
+					Weight:  8,
+				},
+				{
+					Success: 0,
+					Total:   10,
+					Weight:  2,
+				},
+			},
+			want: want{
+				score: 0,
+			},
+		},
+		{
+			name: "group with 0 weight counts as max score and group with 0 total does not count",
+			scores: []ProportionalScoreWeighted{
+				{
+					Success: 2,
+					Total:   8,
+					Weight:  0,
+				},
+				{
+					Success: 0,
+					Total:   0,
+					Weight:  10,
+				},
+			},
+			want: want{
+				score: 10,
+			},
+		},
+	}
+	for _, tt := range tests {
+		tt := tt
+		t.Run(tt.name, func(t *testing.T) {
+			t.Parallel()
+			got, err := CreateProportionalScoreWeighted(tt.scores...)
+			if err != nil && !tt.want.err {
+				t.Errorf("CreateProportionalScoreWeighted unexpected error '%v'", err)
+				t.Fail()
+			}
+			if err == nil && tt.want.err {
+				t.Errorf("CreateProportionalScoreWeighted expected error and got none")
+				t.Fail()
+			}
+			if got != tt.want.score {
+				t.Errorf("CreateProportionalScoreWeighted() = %v, want %v", got, tt.want.score)
+			}
+		})
+	}
+}
+
 func TestNormalizeReason(t *testing.T) {
 	t.Parallel()
 	type args struct {

diff --git a/checker/raw_result.go b/checker/raw_result.go
@@ -121,6 +121,7 @@ type Dependency struct {
 	PinnedAt *string
 	Location *File
 	Msg      *string // Only for debug messages.
+	Pinned   *bool
 	Type     DependencyUseType
 }