🐛 ensure Signed-Releases only scores 5 releases (#3768)

* limit releasesHaveProvenance probe to 5 releases and check in evaluation code too Signed-off-by: Spencer Schrock <sschrock@google.com> * add tests Signed-off-by: Spencer Schrock <sschrock@google.com> --------- Signed-off-by: Spencer Schrock <sschrock@google.com>
ossf · Jan 3, 2024 · 658a77b · 658a77b
1 parent 2bad6e7
commit 658a77b
Show file tree

Hide file tree

Showing 4 changed files with 105 additions and 1 deletion.
diff --git a/checks/evaluation/signed_releases.go b/checks/evaluation/signed_releases.go
@@ -106,8 +106,11 @@ func SignedReleases(name string,
 
 	totalReleases := len(uniqueReleaseTags)
 
+	// TODO, the evaluation code should be the one limiting to 5, not assuming the probes have done it already
+	// however there are some ordering issues to consider, so going with the easy way for now
 	if totalReleases > 5 {
-		totalReleases = 5
+		err := sce.CreateInternal(sce.ErrScorecardInternal, "too many releases, please report this")
+		return checker.CreateRuntimeErrorResult(name, err)
 	}
 	if totalReleases == 0 {
 		// This should not happen in production, but it is useful to have

diff --git a/checks/evaluation/signed_releases_test.go b/checks/evaluation/signed_releases_test.go
@@ -19,6 +19,7 @@ import (
 	"testing"
 
 	"github.com/ossf/scorecard/v4/checker"
+	sce "github.com/ossf/scorecard/v4/errors"
 	"github.com/ossf/scorecard/v4/finding"
 	"github.com/ossf/scorecard/v4/probes/releasesAreSigned"
 	"github.com/ossf/scorecard/v4/probes/releasesHaveProvenance"
@@ -31,6 +32,7 @@ const (
 	release2 = 2
 	release3 = 3
 	release4 = 4
+	release5 = 5
 )
 
 const (
@@ -262,6 +264,37 @@ func TestSignedReleases(t *testing.T) {
 				NumberOfDebug: 5,
 			},
 		},
+		{
+			name: "too many releases (6 when lookback is 5)",
+			findings: []finding.Finding{
+				// Release 1:
+				// Release 1, Asset 1:
+				signedProbe(release0, asset0, finding.OutcomePositive),
+				provenanceProbe(release0, asset0, finding.OutcomePositive),
+				// Release 2:
+				// Release 2, Asset 1:
+				signedProbe(release1, asset0, finding.OutcomePositive),
+				provenanceProbe(release1, asset0, finding.OutcomePositive),
+				// Release 3, Asset 1:
+				signedProbe(release2, asset0, finding.OutcomePositive),
+				provenanceProbe(release2, asset0, finding.OutcomePositive),
+				// Release 4, Asset 1:
+				signedProbe(release3, asset0, finding.OutcomePositive),
+				provenanceProbe(release3, asset0, finding.OutcomePositive),
+				// Release 5, Asset 1:
+				signedProbe(release4, asset0, finding.OutcomePositive),
+				provenanceProbe(release4, asset0, finding.OutcomePositive),
+				// Release 6, Asset 1:
+				signedProbe(release5, asset0, finding.OutcomePositive),
+				provenanceProbe(release5, asset0, finding.OutcomePositive),
+			},
+			result: scut.TestReturn{
+				Score:         checker.InconclusiveResultScore,
+				Error:         sce.ErrScorecardInternal,
+				NumberOfInfo:  12, // 2 (signed + provenance) for each release
+				NumberOfDebug: 6,  // 1 for each release
+			},
+		},
 	}
 
 	for _, tt := range tests {

diff --git a/probes/releasesHaveProvenance/impl.go b/probes/releasesHaveProvenance/impl.go
@@ -45,6 +45,7 @@ const (
 
 var provenanceExtensions = []string{".intoto.jsonl"}
 
+//nolint:gocognit // bug hotfix
 func Run(raw *checker.RawResults) ([]finding.Finding, string, error) {
 	if raw == nil {
 		return nil, "", fmt.Errorf("%w: raw", uerror.ErrNil)
@@ -61,6 +62,9 @@ func Run(raw *checker.RawResults) ([]finding.Finding, string, error) {
 		if len(release.Assets) == 0 {
 			continue
 		}
+		if i == releaseLookBack {
+			break
+		}
 		totalReleases++
 		hasProvenance := false
 		for j := range release.Assets {

diff --git a/probes/releasesHaveProvenance/impl_test.go b/probes/releasesHaveProvenance/impl_test.go
@@ -152,6 +152,70 @@ func Test_Run(t *testing.T) {
 				finding.OutcomeNegative,
 			},
 		},
+		{
+			name: "enforece lookback limit of 5 releases",
+			raw: &checker.RawResults{
+				SignedReleasesResults: checker.SignedReleasesData{
+					Releases: []clients.Release{
+						{
+							TagName: "v6.0",
+							Assets: []clients.ReleaseAsset{
+								{Name: "binary.tar.gz"},
+								{Name: "binary.tar.gz.sig"},
+								{Name: "binary.tar.gz.intoto.jsonl"},
+							},
+						},
+						{
+							TagName: "v5.0",
+							Assets: []clients.ReleaseAsset{
+								{Name: "binary.tar.gz"},
+								{Name: "binary.tar.gz.sig"},
+								{Name: "binary.tar.gz.intoto.jsonl"},
+							},
+						},
+						{
+							TagName: "v4.0",
+							Assets: []clients.ReleaseAsset{
+								{Name: "binary.tar.gz"},
+								{Name: "binary.tar.gz.sig"},
+								{Name: "binary.tar.gz.intoto.jsonl"},
+							},
+						},
+						{
+							TagName: "v3.0",
+							Assets: []clients.ReleaseAsset{
+								{Name: "binary.tar.gz"},
+								{Name: "binary.tar.gz.sig"},
+								{Name: "binary.tar.gz.intoto.jsonl"},
+							},
+						},
+						{
+							TagName: "v2.0",
+							Assets: []clients.ReleaseAsset{
+								{Name: "binary.tar.gz"},
+								{Name: "binary.tar.gz.sig"},
+								{Name: "binary.tar.gz.intoto.jsonl"},
+							},
+						},
+						{
+							TagName: "v1.0",
+							Assets: []clients.ReleaseAsset{
+								{Name: "binary.tar.gz"},
+								{Name: "binary.tar.gz.sig"},
+								{Name: "binary.tar.gz.intoto.jsonl"},
+							},
+						},
+					},
+				},
+			},
+			outcomes: []finding.Outcome{
+				finding.OutcomePositive,
+				finding.OutcomePositive,
+				finding.OutcomePositive,
+				finding.OutcomePositive,
+				finding.OutcomePositive,
+			},
+		},
 	}
 	for _, tt := range tests {
 		tt := tt // Re-initializing variable so it is not changed while executing the closure below