diff --git a/.github/workflows/stale.yml b/.github/workflows/stale.yml index 974bfed6c9fd..7d81d39ed126 100644 --- a/.github/workflows/stale.yml +++ b/.github/workflows/stale.yml @@ -36,7 +36,7 @@ jobs: repo-token: ${{ secrets.GITHUB_TOKEN }} stale-issue-message: 'This issue is stale because it has been open for 60 days with no activity.' stale-pr-message: 'This pull request is stale because it has been open for 10 days with no activity' - exempt-issue-labels: 'priority,bug,good first issue' + exempt-issue-labels: 'priority,bug,good first issue,backlog,help wanted' exempt-issue-milestones: 'Structured results' exempt-pr-labels: 'awaiting-approval,work-in-progress' days-before-pr-stale: '10' diff --git a/checks/branch_protection_test.go b/checks/branch_protection_test.go index 69c0ddd35ffc..9b5810399ae1 100644 --- a/checks/branch_protection_test.go +++ b/checks/branch_protection_test.go @@ -93,7 +93,8 @@ func TestReleaseAndDevBranchProtected(t *testing.T) { UpToDateBeforeMerge: &trueVal, Contexts: []string{"foo"}, }, - RequiredPullRequestReviews: &clients.PullRequestReviewRule{ + RequiredPullRequestReviews: clients.PullRequestReviewRule{ + Required: &trueVal, DismissStaleReviews: &trueVal, RequireCodeOwnerReviews: &trueVal, RequiredApprovingReviewCount: &oneVal, @@ -112,7 +113,8 @@ func TestReleaseAndDevBranchProtected(t *testing.T) { UpToDateBeforeMerge: &falseVal, Contexts: nil, }, - RequiredPullRequestReviews: &clients.PullRequestReviewRule{ + RequiredPullRequestReviews: clients.PullRequestReviewRule{ + Required: &trueVal, DismissStaleReviews: &falseVal, RequireCodeOwnerReviews: &falseVal, RequiredApprovingReviewCount: &zeroVal, @@ -151,7 +153,8 @@ func TestReleaseAndDevBranchProtected(t *testing.T) { UpToDateBeforeMerge: &falseVal, Contexts: nil, }, - RequiredPullRequestReviews: &clients.PullRequestReviewRule{ + RequiredPullRequestReviews: clients.PullRequestReviewRule{ + Required: &trueVal, DismissStaleReviews: &falseVal, RequireCodeOwnerReviews: &falseVal, RequiredApprovingReviewCount: &zeroVal, @@ -186,7 +189,8 @@ func TestReleaseAndDevBranchProtected(t *testing.T) { UpToDateBeforeMerge: &trueVal, Contexts: []string{"foo"}, }, - RequiredPullRequestReviews: &clients.PullRequestReviewRule{ + RequiredPullRequestReviews: clients.PullRequestReviewRule{ + Required: &trueVal, DismissStaleReviews: &trueVal, RequireCodeOwnerReviews: &trueVal, RequiredApprovingReviewCount: &oneVal, @@ -207,7 +211,8 @@ func TestReleaseAndDevBranchProtected(t *testing.T) { UpToDateBeforeMerge: &falseVal, Contexts: nil, }, - RequiredPullRequestReviews: &clients.PullRequestReviewRule{ + RequiredPullRequestReviews: clients.PullRequestReviewRule{ + Required: &trueVal, DismissStaleReviews: &falseVal, RequireCodeOwnerReviews: &falseVal, RequiredApprovingReviewCount: &zeroVal, @@ -242,7 +247,8 @@ func TestReleaseAndDevBranchProtected(t *testing.T) { UpToDateBeforeMerge: &trueVal, Contexts: []string{"foo"}, }, - RequiredPullRequestReviews: &clients.PullRequestReviewRule{ + RequiredPullRequestReviews: clients.PullRequestReviewRule{ + Required: &trueVal, DismissStaleReviews: &trueVal, RequireCodeOwnerReviews: &trueVal, RequiredApprovingReviewCount: &oneVal, @@ -263,7 +269,8 @@ func TestReleaseAndDevBranchProtected(t *testing.T) { UpToDateBeforeMerge: &trueVal, Contexts: []string{"foo"}, }, - RequiredPullRequestReviews: &clients.PullRequestReviewRule{ + RequiredPullRequestReviews: clients.PullRequestReviewRule{ + Required: &trueVal, DismissStaleReviews: &trueVal, RequireCodeOwnerReviews: &trueVal, RequiredApprovingReviewCount: &oneVal, @@ -299,7 +306,8 @@ func TestReleaseAndDevBranchProtected(t *testing.T) { UpToDateBeforeMerge: &falseVal, Contexts: nil, }, - RequiredPullRequestReviews: &clients.PullRequestReviewRule{ + RequiredPullRequestReviews: clients.PullRequestReviewRule{ + Required: &trueVal, DismissStaleReviews: &falseVal, RequireCodeOwnerReviews: &falseVal, RequiredApprovingReviewCount: &zeroVal, @@ -337,7 +345,8 @@ func TestReleaseAndDevBranchProtected(t *testing.T) { UpToDateBeforeMerge: &falseVal, Contexts: nil, }, - RequiredPullRequestReviews: &clients.PullRequestReviewRule{ + RequiredPullRequestReviews: clients.PullRequestReviewRule{ + Required: &trueVal, DismissStaleReviews: &falseVal, RequireCodeOwnerReviews: &falseVal, RequiredApprovingReviewCount: &zeroVal, diff --git a/checks/evaluation/branch_protection.go b/checks/evaluation/branch_protection.go index 21352fb789af..cccf2e289778 100644 --- a/checks/evaluation/branch_protection.go +++ b/checks/evaluation/branch_protection.go @@ -285,9 +285,7 @@ func nonAdminReviewProtection(branch *clients.BranchRef) (int, int) { // Having at least 1 reviewer is twice as important as the other Tier 2 requirements. const reviewerWeight = 2 max += reviewerWeight - if branch.BranchProtectionRule.RequiredPullRequestReviews != nil && - branch.BranchProtectionRule.RequiredPullRequestReviews.RequiredApprovingReviewCount != nil && - *branch.BranchProtectionRule.RequiredPullRequestReviews.RequiredApprovingReviewCount > 0 { + if valueOrZero(branch.BranchProtectionRule.RequiredPullRequestReviews.RequiredApprovingReviewCount) > 0 { // We do not display anything here, it's done in nonAdminThoroughReviewProtection() score += reviewerWeight } @@ -329,15 +327,13 @@ func adminReviewProtection(branch *clients.BranchRef, dl checker.DetailLogger) ( } max++ - if branch.BranchProtectionRule.RequiredPullRequestReviews != nil { + if valueOrZero(branch.BranchProtectionRule.RequiredPullRequestReviews.Required) { score++ info(dl, log, "PRs are required in order to make changes on branch '%s'", *branch.Name) } else { warn(dl, log, "PRs are not required to make changes on branch '%s'; or we don't have data to detect it."+ "If you think it might be the latter, make sure to run Scorecard with a PAT or use Repo "+ "Rules (that are always public) instead of Branch Protection settings", *branch.Name) - // Warning it here because since `RequiredPullRequestReviews` is nil, we won't check reviewers count afterwards - warn(dl, log, "No reviewers required to merge changes on branch '%s'", *branch.Name) } return score, max @@ -349,8 +345,7 @@ func adminThoroughReviewProtection(branch *clients.BranchRef, dl checker.DetailL // Only log information if the branch is protected. log := branch.Protected != nil && *branch.Protected - if branch.BranchProtectionRule.RequiredPullRequestReviews != nil && - branch.BranchProtectionRule.RequiredPullRequestReviews.DismissStaleReviews != nil { + if branch.BranchProtectionRule.RequiredPullRequestReviews.DismissStaleReviews != nil { // Note: we don't increase max possible score for non-admin viewers. max++ switch *branch.BranchProtectionRule.RequiredPullRequestReviews.DismissStaleReviews { @@ -391,18 +386,13 @@ func nonAdminThoroughReviewProtection(branch *clients.BranchRef, dl checker.Deta max++ - // On this first check we exclude the case where PRs are not required to make changes, - // because it's already covered on adminReviewProtection function. - if branch.BranchProtectionRule.RequiredPullRequestReviews != nil && - branch.BranchProtectionRule.RequiredPullRequestReviews.RequiredApprovingReviewCount != nil { - if *branch.BranchProtectionRule.RequiredPullRequestReviews.RequiredApprovingReviewCount >= minReviews { - info(dl, log, "number of required reviewers is %d on branch '%s'", - *branch.BranchProtectionRule.RequiredPullRequestReviews.RequiredApprovingReviewCount, *branch.Name) - score++ - } else { - warn(dl, log, "number of required reviewers is %d on branch '%s', while the ideal suggested is %d", - *branch.BranchProtectionRule.RequiredPullRequestReviews.RequiredApprovingReviewCount, *branch.Name, minReviews) - } + reviewers := valueOrZero(branch.BranchProtectionRule.RequiredPullRequestReviews.RequiredApprovingReviewCount) + if reviewers >= minReviews { + info(dl, log, "number of required reviewers is %d on branch '%s'", reviewers, *branch.Name) + score++ + } else { + warn(dl, log, "number of required reviewers is %d on branch '%s', while the ideal suggested is %d", + reviewers, *branch.Name, minReviews) } return score, max @@ -416,8 +406,7 @@ func codeownerBranchProtection( log := branch.Protected != nil && *branch.Protected - if branch.BranchProtectionRule.RequiredPullRequestReviews != nil && - branch.BranchProtectionRule.RequiredPullRequestReviews.RequireCodeOwnerReviews != nil { + if branch.BranchProtectionRule.RequiredPullRequestReviews.RequireCodeOwnerReviews != nil { switch *branch.BranchProtectionRule.RequiredPullRequestReviews.RequireCodeOwnerReviews { case true: info(dl, log, "codeowner review is required on branch '%s'", *branch.Name) @@ -433,3 +422,12 @@ func codeownerBranchProtection( return score, max } + +// returns the pointer's value if it exists, the type's zero-value otherwise. +func valueOrZero[T any](ptr *T) T { + if ptr == nil { + var zero T + return zero + } + return *ptr +} diff --git a/checks/evaluation/branch_protection_test.go b/checks/evaluation/branch_protection_test.go index db25faf236b6..86c34c3e5e60 100644 --- a/checks/evaluation/branch_protection_test.go +++ b/checks/evaluation/branch_protection_test.go @@ -50,7 +50,7 @@ func TestIsBranchProtected(t *testing.T) { expected scut.TestReturn }{ { - name: "Configs as they are right after creating new Branch Protection setting", + name: "GitHub default settings", expected: scut.TestReturn{ Error: nil, Score: 3, @@ -62,12 +62,14 @@ func TestIsBranchProtected(t *testing.T) { Name: &branchVal, Protected: &trueVal, BranchProtectionRule: clients.BranchProtectionRule{ - AllowDeletions: &falseVal, - AllowForcePushes: &falseVal, - RequireLinearHistory: &falseVal, - EnforceAdmins: &falseVal, - RequireLastPushApproval: &falseVal, - RequiredPullRequestReviews: nil, + AllowDeletions: &falseVal, + AllowForcePushes: &falseVal, + RequireLinearHistory: &falseVal, + EnforceAdmins: &falseVal, + RequireLastPushApproval: &falseVal, + RequiredPullRequestReviews: clients.PullRequestReviewRule{ + Required: &falseVal, + }, CheckRules: clients.StatusChecksRule{ RequiresStatusChecks: &trueVal, Contexts: nil, @@ -103,7 +105,8 @@ func TestIsBranchProtected(t *testing.T) { Name: &branchVal, Protected: &trueVal, BranchProtectionRule: clients.BranchProtectionRule{ - RequiredPullRequestReviews: &clients.PullRequestReviewRule{ + RequiredPullRequestReviews: clients.PullRequestReviewRule{ + Required: &trueVal, DismissStaleReviews: &falseVal, RequireCodeOwnerReviews: &falseVal, RequiredApprovingReviewCount: &zeroVal, @@ -139,7 +142,8 @@ func TestIsBranchProtected(t *testing.T) { RequireLinearHistory: &falseVal, AllowForcePushes: &falseVal, AllowDeletions: &falseVal, - RequiredPullRequestReviews: &clients.PullRequestReviewRule{ + RequiredPullRequestReviews: clients.PullRequestReviewRule{ + Required: &trueVal, DismissStaleReviews: &falseVal, RequireCodeOwnerReviews: &falseVal, RequiredApprovingReviewCount: &zeroVal, @@ -175,7 +179,9 @@ func TestIsBranchProtected(t *testing.T) { UpToDateBeforeMerge: &falseVal, Contexts: nil, }, - RequiredPullRequestReviews: nil, + RequiredPullRequestReviews: clients.PullRequestReviewRule{ + Required: &falseVal, + }, }, }, }, @@ -202,7 +208,9 @@ func TestIsBranchProtected(t *testing.T) { UpToDateBeforeMerge: &trueVal, Contexts: []string{"foo"}, }, - RequiredPullRequestReviews: nil, + RequiredPullRequestReviews: clients.PullRequestReviewRule{ + Required: &falseVal, + }, }, }, }, @@ -229,7 +237,8 @@ func TestIsBranchProtected(t *testing.T) { UpToDateBeforeMerge: &trueVal, Contexts: []string{"foo"}, }, - RequiredPullRequestReviews: &clients.PullRequestReviewRule{ + RequiredPullRequestReviews: clients.PullRequestReviewRule{ + Required: &trueVal, DismissStaleReviews: &trueVal, RequireCodeOwnerReviews: &trueVal, RequiredApprovingReviewCount: &zeroVal, @@ -260,7 +269,8 @@ func TestIsBranchProtected(t *testing.T) { UpToDateBeforeMerge: &trueVal, Contexts: nil, }, - RequiredPullRequestReviews: &clients.PullRequestReviewRule{ + RequiredPullRequestReviews: clients.PullRequestReviewRule{ + Required: &trueVal, DismissStaleReviews: &falseVal, RequireCodeOwnerReviews: &falseVal, RequiredApprovingReviewCount: &oneVal, @@ -291,7 +301,6 @@ func TestIsBranchProtected(t *testing.T) { UpToDateBeforeMerge: nil, Contexts: nil, }, - RequiredPullRequestReviews: nil, }, }, }, @@ -318,7 +327,8 @@ func TestIsBranchProtected(t *testing.T) { UpToDateBeforeMerge: nil, Contexts: nil, }, - RequiredPullRequestReviews: &clients.PullRequestReviewRule{ + RequiredPullRequestReviews: clients.PullRequestReviewRule{ + Required: &trueVal, DismissStaleReviews: nil, RequireCodeOwnerReviews: &falseVal, RequiredApprovingReviewCount: &oneVal, @@ -349,7 +359,8 @@ func TestIsBranchProtected(t *testing.T) { UpToDateBeforeMerge: &falseVal, Contexts: []string{"foo"}, }, - RequiredPullRequestReviews: &clients.PullRequestReviewRule{ + RequiredPullRequestReviews: clients.PullRequestReviewRule{ + Required: &trueVal, DismissStaleReviews: &falseVal, RequireCodeOwnerReviews: &falseVal, RequiredApprovingReviewCount: &zeroVal, @@ -380,7 +391,8 @@ func TestIsBranchProtected(t *testing.T) { UpToDateBeforeMerge: &falseVal, Contexts: []string{"foo"}, }, - RequiredPullRequestReviews: &clients.PullRequestReviewRule{ + RequiredPullRequestReviews: clients.PullRequestReviewRule{ + Required: &trueVal, DismissStaleReviews: &falseVal, RequireCodeOwnerReviews: &falseVal, RequiredApprovingReviewCount: &zeroVal, @@ -412,7 +424,8 @@ func TestIsBranchProtected(t *testing.T) { UpToDateBeforeMerge: &falseVal, Contexts: []string{"foo"}, }, - RequiredPullRequestReviews: &clients.PullRequestReviewRule{ + RequiredPullRequestReviews: clients.PullRequestReviewRule{ + Required: &trueVal, DismissStaleReviews: &falseVal, RequireCodeOwnerReviews: &falseVal, RequiredApprovingReviewCount: &zeroVal, @@ -443,7 +456,8 @@ func TestIsBranchProtected(t *testing.T) { UpToDateBeforeMerge: &falseVal, Contexts: []string{"foo"}, }, - RequiredPullRequestReviews: &clients.PullRequestReviewRule{ + RequiredPullRequestReviews: clients.PullRequestReviewRule{ + Required: &trueVal, DismissStaleReviews: &falseVal, RequireCodeOwnerReviews: &falseVal, RequiredApprovingReviewCount: &zeroVal, @@ -474,7 +488,8 @@ func TestIsBranchProtected(t *testing.T) { UpToDateBeforeMerge: &trueVal, Contexts: []string{"foo"}, }, - RequiredPullRequestReviews: &clients.PullRequestReviewRule{ + RequiredPullRequestReviews: clients.PullRequestReviewRule{ + Required: &trueVal, DismissStaleReviews: &trueVal, RequireCodeOwnerReviews: &trueVal, RequiredApprovingReviewCount: &oneVal, @@ -505,7 +520,8 @@ func TestIsBranchProtected(t *testing.T) { UpToDateBeforeMerge: &trueVal, Contexts: []string{"foo"}, }, - RequiredPullRequestReviews: &clients.PullRequestReviewRule{ + RequiredPullRequestReviews: clients.PullRequestReviewRule{ + Required: &trueVal, DismissStaleReviews: &trueVal, RequireCodeOwnerReviews: &trueVal, RequiredApprovingReviewCount: &oneVal, @@ -537,7 +553,8 @@ func TestIsBranchProtected(t *testing.T) { UpToDateBeforeMerge: &trueVal, Contexts: []string{"foo"}, }, - RequiredPullRequestReviews: &clients.PullRequestReviewRule{ + RequiredPullRequestReviews: clients.PullRequestReviewRule{ + Required: &trueVal, DismissStaleReviews: &trueVal, RequireCodeOwnerReviews: &trueVal, RequiredApprovingReviewCount: &oneVal, diff --git a/checks/evaluation/signed_releases.go b/checks/evaluation/signed_releases.go index 5feb11b680f5..9f3673ffe21e 100644 --- a/checks/evaluation/signed_releases.go +++ b/checks/evaluation/signed_releases.go @@ -17,118 +17,137 @@ package evaluation import ( "fmt" "math" - "strings" "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" ) -var ( - signatureExtensions = []string{".asc", ".minisig", ".sig", ".sign"} - provenanceExtensions = []string{".intoto.jsonl"} -) - -const releaseLookBack = 5 - // SignedReleases applies the score policy for the Signed-Releases check. -// -//nolint:gocognit -func SignedReleases(name string, dl checker.DetailLogger, r *checker.SignedReleasesData) checker.CheckResult { - if r == nil { - e := sce.WithMessage(sce.ErrScorecardInternal, "empty raw data") +func SignedReleases(name string, + findings []finding.Finding, dl checker.DetailLogger, +) checker.CheckResult { + expectedProbes := []string{ + releasesAreSigned.Probe, + releasesHaveProvenance.Probe, + } + + if !finding.UniqueProbesEqual(findings, expectedProbes) { + e := sce.WithMessage(sce.ErrScorecardInternal, "invalid probe results") return checker.CreateRuntimeErrorResult(name, e) } - totalReleases := 0 - total := 0 - score := 0 - for _, release := range r.Releases { - if len(release.Assets) == 0 { - continue + // Debug all releases and check for OutcomeNotApplicable + // All probes have OutcomeNotApplicable in case the project has no + // releases. Therefore, check for any finding with OutcomeNotApplicable. + loggedReleases := make([]string, 0) + for i := range findings { + f := &findings[i] + + // Debug release name + releaseName, err := getReleaseName(f) + if err != nil { + e := sce.WithMessage(sce.ErrScorecardInternal, "could not get release name") + return checker.CreateRuntimeErrorResult(name, e) + } + if !contains(loggedReleases, releaseName) { + dl.Debug(&checker.LogMessage{ + Text: fmt.Sprintf("GitHub release found: %s", releaseName), + }) + loggedReleases = append(loggedReleases, releaseName) } - dl.Debug(&checker.LogMessage{ - Text: fmt.Sprintf("GitHub release found: %s", release.TagName), - }) - - totalReleases++ - signed := false - hasProvenance := false - - // Check for provenance. - for _, asset := range release.Assets { - for _, suffix := range provenanceExtensions { - if strings.HasSuffix(asset.Name, suffix) { - dl.Info(&checker.LogMessage{ - Path: asset.URL, - Type: finding.FileTypeURL, - Text: fmt.Sprintf("provenance for release artifact: %s", asset.Name), - }) - hasProvenance = true - total++ - break - } - } - if hasProvenance { - // Assign maximum points. - score += 10 - break - } + // Check if outcome is NotApplicable + if f.Outcome == finding.OutcomeNotApplicable { + dl.Warn(&checker.LogMessage{ + Text: "no GitHub releases found", + }) + // Generic summary. + return checker.CreateInconclusiveResult(name, "no releases found") + } + } + + totalPositive := 0 + releaseMap := make(map[string]int) + uniqueReleaseTags := make([]string, 0) + checker.LogFindings(findings, dl) + + for i := range findings { + f := &findings[i] + + releaseName, err := getReleaseName(f) + if err != nil { + return checker.CreateRuntimeErrorResult(name, err) } - if hasProvenance { - continue + if !contains(uniqueReleaseTags, releaseName) { + uniqueReleaseTags = append(uniqueReleaseTags, releaseName) } - dl.Warn(&checker.LogMessage{ - Path: release.URL, - Type: finding.FileTypeURL, - Text: fmt.Sprintf("release artifact %s does not have provenance", release.TagName), - }) - - // No provenance. Try signatures. - for _, asset := range release.Assets { - for _, suffix := range signatureExtensions { - if strings.HasSuffix(asset.Name, suffix) { - dl.Info(&checker.LogMessage{ - Path: asset.URL, - Type: finding.FileTypeURL, - Text: fmt.Sprintf("signed release artifact: %s", asset.Name), - }) - signed = true - total++ - break + if f.Outcome == finding.OutcomePositive { + totalPositive++ + + switch f.Probe { + case releasesAreSigned.Probe: + if _, ok := releaseMap[releaseName]; !ok { + releaseMap[releaseName] = 8 } - } - if signed { - // Assign 8 points. - score += 8 - break + case releasesHaveProvenance.Probe: + releaseMap[releaseName] = 10 } } + } - if !signed { - dl.Warn(&checker.LogMessage{ - Path: release.URL, - Type: finding.FileTypeURL, - Text: fmt.Sprintf("release artifact %s not signed", release.TagName), - }) - } - if totalReleases >= releaseLookBack { - break - } + if totalPositive == 0 { + return checker.CreateMinScoreResult(name, "Project has not signed or included provenance with any releases.") } + totalReleases := len(uniqueReleaseTags) + + if totalReleases > 5 { + totalReleases = 5 + } if totalReleases == 0 { - dl.Warn(&checker.LogMessage{ - Text: "no GitHub releases found", - }) - // Generic summary. + // This should not happen in production, but it is useful to have + // for testing. return checker.CreateInconclusiveResult(name, "no releases found") } + score := 0 + for _, s := range releaseMap { + score += s + } + score = int(math.Floor(float64(score) / float64(totalReleases))) - reason := fmt.Sprintf("%d out of %d artifacts are signed or have provenance", total, totalReleases) + reason := fmt.Sprintf("%d out of the last %d releases have a total of %d signed artifacts.", + len(releaseMap), totalReleases, totalPositive) return checker.CreateResultWithScore(name, reason, score) } + +func getReleaseName(f *finding.Finding) (string, error) { + m := f.Values + for k, v := range m { + var value int + switch f.Probe { + case releasesAreSigned.Probe: + value = int(releasesAreSigned.ValueTypeRelease) + case releasesHaveProvenance.Probe: + value = int(releasesHaveProvenance.ValueTypeRelease) + } + if v == value { + return k, nil + } + } + return "", sce.WithMessage(sce.ErrScorecardInternal, "could not get release tag") +} + +func contains(releases []string, release string) bool { + for _, r := range releases { + if r == release { + return true + } + } + return false +} diff --git a/checks/evaluation/signed_releases_test.go b/checks/evaluation/signed_releases_test.go index 53a8ee362e29..8aa8c8fe5f40 100644 --- a/checks/evaluation/signed_releases_test.go +++ b/checks/evaluation/signed_releases_test.go @@ -15,96 +15,269 @@ package evaluation import ( + "fmt" "testing" - "github.com/google/go-cmp/cmp" - "github.com/google/go-cmp/cmp/cmpopts" - "github.com/ossf/scorecard/v4/checker" - "github.com/ossf/scorecard/v4/clients" + "github.com/ossf/scorecard/v4/finding" + "github.com/ossf/scorecard/v4/probes/releasesAreSigned" + "github.com/ossf/scorecard/v4/probes/releasesHaveProvenance" scut "github.com/ossf/scorecard/v4/utests" ) +const ( + release0 = 0 + release1 = 1 + release2 = 2 + release3 = 3 + release4 = 4 +) + +const ( + asset0 = 0 + asset1 = 1 + asset2 = 2 + asset3 = 3 + asset4 = 4 + asset5 = 5 + asset6 = 6 + asset7 = 7 + asset8 = 8 + asset9 = 9 +) + +func signedProbe(release, asset int, outcome finding.Outcome) finding.Finding { + return finding.Finding{ + Probe: releasesAreSigned.Probe, + Outcome: outcome, + Values: map[string]int{ + fmt.Sprintf("v%d", release): int(releasesAreSigned.ValueTypeRelease), + fmt.Sprintf("artifact-%d", asset): int(releasesAreSigned.ValueTypeReleaseAsset), + }, + } +} + +func provenanceProbe(release, asset int, outcome finding.Outcome) finding.Finding { + return finding.Finding{ + Probe: releasesHaveProvenance.Probe, + Outcome: outcome, + Values: map[string]int{ + fmt.Sprintf("v%d", release): int(releasesHaveProvenance.ValueTypeRelease), + fmt.Sprintf("artifact-%d", asset): int(releasesHaveProvenance.ValueTypeReleaseAsset), + }, + } +} + func TestSignedReleases(t *testing.T) { t.Parallel() tests := []struct { - name string - releases []clients.Release - expectedResult checker.CheckResult + name string + findings []finding.Finding + result scut.TestReturn }{ { - name: "Full score", - releases: []clients.Release{ - { - TagName: "v1.0", - Assets: []clients.ReleaseAsset{ - {Name: "binary.tar.gz"}, - {Name: "binary.tar.gz.sig"}, - {Name: "binary.tar.gz.intoto.jsonl"}, - }, - }, + name: "Has one release that is signed but no provenance", + findings: []finding.Finding{ + signedProbe(0, 0, finding.OutcomePositive), + provenanceProbe(0, 0, finding.OutcomeNegative), }, - expectedResult: checker.CheckResult{ - Name: "Signed-Releases", - Version: 2, - Score: 10, - Reason: "1 out of 1 artifacts are signed or have provenance", + result: scut.TestReturn{ + Score: 8, + NumberOfInfo: 1, + NumberOfWarn: 1, + NumberOfDebug: 1, }, }, { - name: "Partial score", - releases: []clients.Release{ - { - TagName: "v1.0", - Assets: []clients.ReleaseAsset{ - {Name: "binary.tar.gz"}, - {Name: "binary.tar.gz.sig"}, - }, - }, + name: "Has one release that is signed and has provenance", + findings: []finding.Finding{ + signedProbe(0, 0, finding.OutcomePositive), + provenanceProbe(0, 0, finding.OutcomePositive), }, - expectedResult: checker.CheckResult{ - Name: "Signed-Releases", - Version: 2, - Score: 8, - Reason: "1 out of 1 artifacts are signed or have provenance", + result: scut.TestReturn{ + Score: 10, + NumberOfInfo: 2, + NumberOfDebug: 1, }, }, { - name: "No score", - releases: []clients.Release{ - { - TagName: "v1.0", - Assets: []clients.ReleaseAsset{ - {Name: "binary.tar.gz"}, - }, - }, + name: "Has one release that is not signed but has provenance", + findings: []finding.Finding{ + signedProbe(0, 0, finding.OutcomeNegative), + provenanceProbe(0, 0, finding.OutcomePositive), }, - expectedResult: checker.CheckResult{ - Name: "Signed-Releases", - Version: 2, - Score: 0, - Reason: "0 out of 1 artifacts are signed or have provenance", + result: scut.TestReturn{ + Score: checker.MaxResultScore, + NumberOfInfo: 1, + NumberOfWarn: 1, + NumberOfDebug: 1, + }, + }, + + { + name: "3 releases. One release has one signed, and one release has two provenance.", + findings: []finding.Finding{ + // Release 1: + // Asset 1: + signedProbe(release0, asset0, finding.OutcomeNegative), + provenanceProbe(release0, asset0, finding.OutcomeNegative), + // Asset 2: + signedProbe(release0, asset1, finding.OutcomePositive), + provenanceProbe(release0, asset1, finding.OutcomeNegative), + // Release 2 + // Asset 1: + signedProbe(release1, asset0, finding.OutcomeNegative), + provenanceProbe(release1, asset0, finding.OutcomeNegative), + // Release 2 + // Asset 2: + signedProbe(release1, asset1, finding.OutcomeNegative), + provenanceProbe(release1, asset1, finding.OutcomeNegative), + // Release 2 + // Asset 3: + signedProbe(release1, asset2, finding.OutcomeNegative), + provenanceProbe(release1, asset2, finding.OutcomeNegative), + // Release 3 + // Asset 1: + signedProbe(release2, asset0, finding.OutcomeNegative), + provenanceProbe(release2, asset0, finding.OutcomePositive), + // Asset 2: + signedProbe(release2, asset1, finding.OutcomeNegative), + provenanceProbe(release2, asset1, finding.OutcomePositive), + // Asset 3: + signedProbe(release2, asset2, finding.OutcomeNegative), + provenanceProbe(release2, asset2, finding.OutcomeNegative), + }, + result: scut.TestReturn{ + Score: 6, + NumberOfInfo: 3, + NumberOfWarn: 13, + NumberOfDebug: 3, + }, + }, + { + name: "5 releases. Two releases have one signed each, and two releases have one provenance each.", + findings: []finding.Finding{ + // Release 1: + // Release 1, Asset 1: + signedProbe(release0, asset0, finding.OutcomeNegative), + provenanceProbe(release0, asset0, finding.OutcomeNegative), + signedProbe(release0, asset1, finding.OutcomePositive), + provenanceProbe(release0, asset1, finding.OutcomeNegative), + // Release 2: + // Release 2, Asset 1: + signedProbe(release1, asset1, finding.OutcomePositive), + provenanceProbe(release1, asset0, finding.OutcomeNegative), + // Release 2, Asset 2: + signedProbe(release1, asset1, finding.OutcomeNegative), + provenanceProbe(release1, asset1, finding.OutcomeNegative), + // Release 2, Asset 3: + signedProbe(release1, asset2, finding.OutcomeNegative), + provenanceProbe(release1, asset2, finding.OutcomeNegative), + // Release 3, Asset 1: + signedProbe(release2, asset0, finding.OutcomeNegative), + provenanceProbe(release2, asset0, finding.OutcomePositive), + // Release 3, Asset 2: + signedProbe(release2, asset1, finding.OutcomeNegative), + provenanceProbe(release2, asset1, finding.OutcomeNegative), + // Release 3, Asset 3: + signedProbe(release2, asset2, finding.OutcomeNegative), + provenanceProbe(release2, asset2, finding.OutcomeNegative), + // Release 4, Asset 1: + signedProbe(release3, asset0, finding.OutcomeNegative), + provenanceProbe(release3, asset0, finding.OutcomePositive), + // Release 4, Asset 2: + signedProbe(release3, asset1, finding.OutcomeNegative), + provenanceProbe(release3, asset1, finding.OutcomeNegative), + // Release 4, Asset 3: + signedProbe(release3, asset2, finding.OutcomeNegative), + provenanceProbe(release3, asset2, finding.OutcomeNegative), + // Release 5, Asset 1: + signedProbe(release4, asset0, finding.OutcomeNegative), + provenanceProbe(release4, asset0, finding.OutcomeNegative), + // Release 5, Asset 2: + signedProbe(release4, asset1, finding.OutcomeNegative), + provenanceProbe(release4, asset1, finding.OutcomeNegative), + // Release 5, Asset 3: + signedProbe(release4, asset2, finding.OutcomeNegative), + provenanceProbe(release4, asset2, finding.OutcomeNegative), + // Release 5, Asset 4: + signedProbe(release4, asset3, finding.OutcomeNegative), + provenanceProbe(release4, asset3, finding.OutcomeNegative), + }, + result: scut.TestReturn{ + Score: 7, + NumberOfInfo: 4, + NumberOfWarn: 26, + NumberOfDebug: 5, }, }, { - name: "No releases", - releases: []clients.Release{}, - expectedResult: checker.CreateInconclusiveResult("Signed-Releases", "no releases found"), + name: "5 releases. All have one signed artifact.", + findings: []finding.Finding{ + // Release 1: + // Release 1, Asset 1: + signedProbe(release0, asset0, finding.OutcomeNegative), + provenanceProbe(release0, asset0, finding.OutcomeNegative), + signedProbe(release0, asset1, finding.OutcomePositive), + provenanceProbe(release0, asset1, finding.OutcomeNegative), + // Release 2: + // Release 2, Asset 1: + signedProbe(release1, asset0, finding.OutcomePositive), + provenanceProbe(release1, asset0, finding.OutcomeNegative), + // Release 2, Asset 2: + signedProbe(release1, asset1, finding.OutcomeNegative), + provenanceProbe(release1, asset1, finding.OutcomeNegative), + // Release 2, Asset 3: + signedProbe(release1, asset2, finding.OutcomeNegative), + provenanceProbe(release1, asset2, finding.OutcomeNegative), + // Release 3, Asset 1: + signedProbe(release2, asset0, finding.OutcomePositive), + provenanceProbe(release2, asset0, finding.OutcomePositive), + // Release 3, Asset 2: + signedProbe(release2, asset1, finding.OutcomeNegative), + provenanceProbe(release2, asset1, finding.OutcomeNegative), + // Release 3, Asset 3: + signedProbe(release2, asset2, finding.OutcomeNegative), + provenanceProbe(release2, asset2, finding.OutcomeNegative), + // Release 4, Asset 1: + signedProbe(release3, asset0, finding.OutcomePositive), + provenanceProbe(release3, asset0, finding.OutcomePositive), + // Release 4, Asset 2: + signedProbe(release3, asset1, finding.OutcomeNegative), + provenanceProbe(release3, asset1, finding.OutcomeNegative), + // Release 4, Asset 3: + signedProbe(release3, asset2, finding.OutcomeNegative), + provenanceProbe(release3, asset2, finding.OutcomeNegative), + // Release 5, Asset 1: + signedProbe(release4, asset0, finding.OutcomePositive), + provenanceProbe(release4, asset0, finding.OutcomeNegative), + // Release 5, Asset 2: + signedProbe(release4, asset1, finding.OutcomeNegative), + provenanceProbe(release4, asset1, finding.OutcomeNegative), + // Release 5, Asset 3: + signedProbe(release4, asset2, finding.OutcomeNegative), + provenanceProbe(release4, asset2, finding.OutcomeNegative), + // Release 5, Asset 4: + signedProbe(release4, asset3, finding.OutcomeNegative), + provenanceProbe(release4, asset3, finding.OutcomeNegative), + }, + result: scut.TestReturn{ + Score: 8, + NumberOfInfo: 7, + NumberOfWarn: 23, + NumberOfDebug: 5, + }, }, } - for _, tc := range tests { - tc := tc - t.Run(tc.name, func(t *testing.T) { + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { t.Parallel() - dl := &scut.TestDetailLogger{} - data := &checker.SignedReleasesData{Releases: tc.releases} - actualResult := SignedReleases("Signed-Releases", dl, data) - - if !cmp.Equal(tc.expectedResult, actualResult, - cmpopts.IgnoreFields(checker.CheckResult{}, "Error")) { - t.Errorf("SignedReleases() mismatch (-want +got):\n%s", cmp.Diff(tc.expectedResult, actualResult, - cmpopts.IgnoreFields(checker.CheckResult{}, "Error"))) + dl := scut.TestDetailLogger{} + got := SignedReleases(tt.name, tt.findings, &dl) + if !scut.ValidateTestReturn(t, tt.name, &tt.result, &got, &dl) { + t.Errorf("got %v, expected %v", got, tt.result) } }) } diff --git a/checks/signed_releases.go b/checks/signed_releases.go index be2dfe98a61b..522672431f76 100644 --- a/checks/signed_releases.go +++ b/checks/signed_releases.go @@ -19,6 +19,8 @@ import ( "github.com/ossf/scorecard/v4/checks/evaluation" "github.com/ossf/scorecard/v4/checks/raw" sce "github.com/ossf/scorecard/v4/errors" + "github.com/ossf/scorecard/v4/probes" + "github.com/ossf/scorecard/v4/probes/zrunner" ) // CheckSignedReleases is the registered name for SignedReleases. @@ -40,11 +42,17 @@ func SignedReleases(c *checker.CheckRequest) checker.CheckResult { return checker.CreateRuntimeErrorResult(CheckSignedReleases, e) } - // Return raw results. - if c.RawResults != nil { - c.RawResults.SignedReleasesResults = rawData + // Set the raw results. + pRawResults := getRawResults(c) + pRawResults.SignedReleasesResults = rawData + + // Evaluate the probes. + findings, err := zrunner.Run(pRawResults, probes.SignedReleases) + if err != nil { + e := sce.WithMessage(sce.ErrScorecardInternal, err.Error()) + return checker.CreateRuntimeErrorResult(CheckFuzzing, e) } // Return the score evaluation. - return evaluation.SignedReleases(CheckSignedReleases, c.Dlogger, &rawData) + return evaluation.SignedReleases(CheckSignedReleases, findings, c.Dlogger) } diff --git a/checks/signed_releases_test.go b/checks/signed_releases_test.go index 39bdc017fb76..b12268f3155a 100644 --- a/checks/signed_releases_test.go +++ b/checks/signed_releases_test.go @@ -16,6 +16,7 @@ package checks import ( "errors" + "fmt" "testing" "github.com/golang/mock/gomock" @@ -362,6 +363,42 @@ func TestSignedRelease(t *testing.T) { }, }, + { + name: "Error getting releases", + err: errors.New("Error getting releases"), + expected: checker.CheckResult{ + Score: -1, + Error: errors.New("Error getting releases"), + }, + }, + { + name: "9 Releases with assests with signed artifacts", + releases: []clients.Release{ + release("v0.8.5"), + release("v0.8.4"), + release("v0.8.3"), + release("v0.8.2"), + release("v0.8.1"), + release("v0.8.0"), + release("v0.7.0"), + release("v0.6.0"), + release("v0.5.0"), + release("v0.4.0"), + release("v0.3.0"), + release("v0.2.0"), + release("v0.1.0"), + release("v0.0.6"), + release("v0.0.5"), + release("v0.0.4"), + release("v0.0.3"), + release("v0.0.2"), + release("v0.0.1"), + }, + expected: checker.CheckResult{ + Score: 8, + }, + }, + { name: "Error getting releases", err: errors.New("Error getting releases"), @@ -410,3 +447,45 @@ func TestSignedRelease(t *testing.T) { }) } } + +func release(version string) clients.Release { + return clients.Release{ + TagName: version, + URL: fmt.Sprintf("https://github.com/test/test_artifact/releases/tag/%s", version), + TargetCommitish: "master", + Assets: []clients.ReleaseAsset{ + { + Name: fmt.Sprintf("%s_checksums.txt", version), + URL: fmt.Sprintf("https://github.com/test/repo/releases/%s/%s_checksums.txt", version, version), + }, + { + Name: fmt.Sprintf("%s_checksums.txt.sig", version), + URL: fmt.Sprintf("https://github.com/test/repo/releases/%s/%s_checksums.txt.sig", version, version), + }, + { + Name: fmt.Sprintf("%s_darwin_x86_64.tar.gz", version), + URL: fmt.Sprintf("https://github.com/test/repo/releases/%s/%s_darwin_x86_64.tar.gz", version, version), + }, + { + Name: fmt.Sprintf("%s_Linux_arm64.tar.gz", version), + URL: fmt.Sprintf("https://github.com/test/repo/releases/%s/%s_Linux_arm64.tar.gz", version, version), + }, + { + Name: fmt.Sprintf("%s_Linux_i386.tar.gz", version), + URL: fmt.Sprintf("https://github.com/test/repo/releases/%s/%s_Linux_i386.tar.gz", version, version), + }, + { + Name: fmt.Sprintf("%s_Linux_x86_64.tar.gz", version), + URL: fmt.Sprintf("https://github.com/test/repo/releases/%s/%s_Linux_x86_64.tar.gz", version, version), + }, + { + Name: fmt.Sprintf("%s_windows_i386.tar.gz", version), + URL: fmt.Sprintf("https://github.com/test/repo/releases/%s/%s_windows_i386.tar.gz", version, version), + }, + { + Name: fmt.Sprintf("%s_windows_x86_64.tar.gz", version), + URL: fmt.Sprintf("https://github.com/test/repo/releases/%s/%s_windows_x86_64.tar.gz", version, version), + }, + }, + } +} diff --git a/clients/branch.go b/clients/branch.go index 4cd6b3d42171..9cabdbc31419 100644 --- a/clients/branch.go +++ b/clients/branch.go @@ -23,11 +23,7 @@ type BranchRef struct { // BranchProtectionRule captures the settings enabled on a branch for security. type BranchProtectionRule struct { - // The nil value of this struct can mean either: - // 1. we can't tell if PRs are required to make changes; or - // 2. we know PRs are not required. The first case happens when Scorecard is run without admin token on - // a repo that uses the old Branch Protection setting (not the new Repo Rules, that are always public). - RequiredPullRequestReviews *PullRequestReviewRule + RequiredPullRequestReviews PullRequestReviewRule AllowDeletions *bool AllowForcePushes *bool RequireLinearHistory *bool @@ -45,6 +41,7 @@ type StatusChecksRule struct { // PullRequestReviewRule captures settings on a PullRequest. type PullRequestReviewRule struct { + Required *bool // are PRs required RequiredApprovingReviewCount *int32 DismissStaleReviews *bool RequireCodeOwnerReviews *bool diff --git a/clients/githubrepo/branches.go b/clients/githubrepo/branches.go index b8ba122ef8ee..4c42c07e1fc5 100644 --- a/clients/githubrepo/branches.go +++ b/clients/githubrepo/branches.go @@ -327,6 +327,7 @@ func (handler *branchesHandler) getBranch(branch string) (*clients.BranchRef, er func copyAdminSettings(src *branchProtectionRule, dst *clients.BranchProtectionRule) { copyBoolPtr(src.IsAdminEnforced, &dst.EnforceAdmins) copyBoolPtr(src.RequireLastPushApproval, &dst.RequireLastPushApproval) + copyBoolPtr(src.DismissesStaleReviews, &dst.RequiredPullRequestReviews.DismissStaleReviews) if src.RequiresStatusChecks != nil { copyBoolPtr(src.RequiresStatusChecks, &dst.CheckRules.RequiresStatusChecks) // TODO(#3255): Update when GitHub GraphQL bug is fixed @@ -340,14 +341,13 @@ func copyAdminSettings(src *branchProtectionRule, dst *clients.BranchProtectionR copyBoolPtr(&upToDateBeforeMerge, &dst.CheckRules.UpToDateBeforeMerge) } } - - // We're also checking if branch requires PRs because if it doesn't, - // the RequiredPullRequestReviews struct should be nil. - if src.DismissesStaleReviews != nil && branchRequiresPrs(src) { - if dst.RequiredPullRequestReviews == nil { - dst.RequiredPullRequestReviews = new(clients.PullRequestReviewRule) - } - copyBoolPtr(src.DismissesStaleReviews, &dst.RequiredPullRequestReviews.DismissStaleReviews) + // we always have the data to know if PRs are required + if dst.RequiredPullRequestReviews.Required == nil { + dst.RequiredPullRequestReviews.Required = asPtr(false) + } + // these values report as &false when PRs aren't required, so if they're true then PRs are required + if valueOrZero(src.RequireLastPushApproval) || valueOrZero(src.DismissesStaleReviews) { + dst.RequiredPullRequestReviews.Required = asPtr(true) } } @@ -358,39 +358,36 @@ func copyNonAdminSettings(src interface{}, dst *clients.BranchProtectionRule) { copyBoolPtr(v.AllowsDeletions, &dst.AllowDeletions) copyBoolPtr(v.AllowsForcePushes, &dst.AllowForcePushes) copyBoolPtr(v.RequiresLinearHistory, &dst.RequireLinearHistory) + copyInt32Ptr(v.RequiredApprovingReviewCount, &dst.RequiredPullRequestReviews.RequiredApprovingReviewCount) + copyBoolPtr(v.RequiresCodeOwnerReviews, &dst.RequiredPullRequestReviews.RequireCodeOwnerReviews) copyStringSlice(v.RequiredStatusCheckContexts, &dst.CheckRules.Contexts) - // If branch doesn't require PRs to make changes, we let the struct RequiredPullRequestReviews point to nil - if branchRequiresPrs(v) { - if dst.RequiredPullRequestReviews == nil { - dst.RequiredPullRequestReviews = new(clients.PullRequestReviewRule) - } - copyInt32Ptr(v.RequiredApprovingReviewCount, &dst.RequiredPullRequestReviews.RequiredApprovingReviewCount) - copyBoolPtr(v.RequiresCodeOwnerReviews, &dst.RequiredPullRequestReviews.RequireCodeOwnerReviews) + // we always have the data to know if PRs are required + if dst.RequiredPullRequestReviews.Required == nil { + dst.RequiredPullRequestReviews.Required = asPtr(false) + } + // GitHub returns nil for RequiredApprovingReviewCount when PRs aren't required and non-nil when they are + // RequiresCodeOwnerReviews is &false even if PRs aren't required, so we need it to be true + if v.RequiredApprovingReviewCount != nil || valueOrZero(v.RequiresCodeOwnerReviews) { + dst.RequiredPullRequestReviews.Required = asPtr(true) } case *refUpdateRule: copyBoolPtr(v.AllowsDeletions, &dst.AllowDeletions) copyBoolPtr(v.AllowsForcePushes, &dst.AllowForcePushes) copyBoolPtr(v.RequiresLinearHistory, &dst.RequireLinearHistory) + copyInt32Ptr(v.RequiredApprovingReviewCount, &dst.RequiredPullRequestReviews.RequiredApprovingReviewCount) + copyBoolPtr(v.RequiresCodeOwnerReviews, &dst.RequiredPullRequestReviews.RequireCodeOwnerReviews) copyStringSlice(v.RequiredStatusCheckContexts, &dst.CheckRules.Contexts) // Evaluate if we have data to infer that the project requires PRs to make changes. If we don't have data, we let - // the struct RequiredPullRequestReviews as nil + // Required stay nil if valueOrZero(v.RequiredApprovingReviewCount) > 0 || valueOrZero(v.RequiresCodeOwnerReviews) { - if dst.RequiredPullRequestReviews == nil { - dst.RequiredPullRequestReviews = new(clients.PullRequestReviewRule) - } - copyInt32Ptr(v.RequiredApprovingReviewCount, &dst.RequiredPullRequestReviews.RequiredApprovingReviewCount) - copyBoolPtr(v.RequiresCodeOwnerReviews, &dst.RequiredPullRequestReviews.RequireCodeOwnerReviews) + dst.RequiredPullRequestReviews.Required = asPtr(true) } } } -func branchRequiresPrs(data *branchProtectionRule) bool { - return data.RequiredApprovingReviewCount != nil -} - func getDefaultBranchNameFrom(data *ruleSetData) string { if data == nil || data.Repository.DefaultBranchRef.Name == nil { return "" @@ -511,6 +508,9 @@ func applyRepoRules(branchRef *clients.BranchRef, rules []*repoRuleSet) { AllowDeletions: asPtr(true), AllowForcePushes: asPtr(true), RequireLinearHistory: asPtr(false), + RequiredPullRequestReviews: clients.PullRequestReviewRule{ + Required: asPtr(false), + }, } translated.EnforceAdmins = asPtr(len(r.BypassActors.Nodes) == 0) @@ -534,8 +534,7 @@ func applyRepoRules(branchRef *clients.BranchRef, rules []*repoRuleSet) { } func translatePullRequestRepoRule(base *clients.BranchProtectionRule, rule *repoRule) { - base.RequiredPullRequestReviews = new(clients.PullRequestReviewRule) - + base.RequiredPullRequestReviews.Required = asPtr(true) base.RequiredPullRequestReviews.DismissStaleReviews = rule.Parameters.PullRequestParameters.DismissStaleReviewsOnPush base.RequiredPullRequestReviews.RequireCodeOwnerReviews = rule.Parameters.PullRequestParameters.RequireCodeOwnerReview base.RequireLastPushApproval = rule.Parameters.PullRequestParameters.RequireLastPushApproval @@ -583,7 +582,7 @@ func mergeBranchProtectionRules(base, translated *clients.BranchProtectionRule) base.RequireLinearHistory = translated.RequireLinearHistory } mergeCheckRules(&base.CheckRules, &translated.CheckRules) - mergePullRequestReviews(&base.RequiredPullRequestReviews, translated.RequiredPullRequestReviews) + mergePullRequestReviews(&base.RequiredPullRequestReviews, &translated.RequiredPullRequestReviews) } func mergeCheckRules(base, translated *clients.StatusChecksRule) { @@ -601,26 +600,19 @@ func mergeCheckRules(base, translated *clients.StatusChecksRule) { } } -func mergePullRequestReviews(base **clients.PullRequestReviewRule, translated *clients.PullRequestReviewRule) { - switch { - case translated == nil: - // "translated" have nothing to be merged in base - return - case *base == nil: - // result would be "translated" itself - *base = translated - return +func mergePullRequestReviews(base, translated *clients.PullRequestReviewRule) { + if base.Required == nil || valueOrZero(translated.Required) { + base.Required = translated.Required } - - if (*base).RequiredApprovingReviewCount == nil || - valueOrZero((*base).RequiredApprovingReviewCount) < valueOrZero(translated.RequiredApprovingReviewCount) { - (*base).RequiredApprovingReviewCount = translated.RequiredApprovingReviewCount + if base.RequiredApprovingReviewCount == nil || + valueOrZero(base.RequiredApprovingReviewCount) < valueOrZero(translated.RequiredApprovingReviewCount) { + base.RequiredApprovingReviewCount = translated.RequiredApprovingReviewCount } - if (*base).DismissStaleReviews == nil || valueOrZero(translated.DismissStaleReviews) { - (*base).DismissStaleReviews = translated.DismissStaleReviews + if base.DismissStaleReviews == nil || valueOrZero(translated.DismissStaleReviews) { + base.DismissStaleReviews = translated.DismissStaleReviews } - if (*base).RequireCodeOwnerReviews == nil || valueOrZero(translated.RequireCodeOwnerReviews) { - (*base).RequireCodeOwnerReviews = translated.RequireCodeOwnerReviews + if base.RequireCodeOwnerReviews == nil || valueOrZero(translated.RequireCodeOwnerReviews) { + base.RequireCodeOwnerReviews = translated.RequireCodeOwnerReviews } } diff --git a/clients/githubrepo/branches_test.go b/clients/githubrepo/branches_test.go index d54b79ce7500..e84674224496 100644 --- a/clients/githubrepo/branches_test.go +++ b/clients/githubrepo/branches_test.go @@ -206,10 +206,12 @@ func Test_applyRepoRules(t *testing.T) { RequiresStatusChecks: nil, Contexts: nil, }, - EnforceAdmins: &trueVal, - RequireLastPushApproval: nil, // this checkbox is enabled only if require status checks - RequireLinearHistory: &falseVal, - RequiredPullRequestReviews: nil, + EnforceAdmins: &trueVal, + RequireLastPushApproval: nil, // this checkbox is enabled only if require status checks + RequireLinearHistory: &falseVal, + RequiredPullRequestReviews: clients.PullRequestReviewRule{ + Required: &falseVal, + }, }, }, }, @@ -221,11 +223,13 @@ func Test_applyRepoRules(t *testing.T) { }, expected: &clients.BranchRef{ BranchProtectionRule: clients.BranchProtectionRule{ - AllowDeletions: &falseVal, - AllowForcePushes: &trueVal, - RequireLinearHistory: &falseVal, - EnforceAdmins: &trueVal, - RequiredPullRequestReviews: nil, + AllowDeletions: &falseVal, + AllowForcePushes: &trueVal, + RequireLinearHistory: &falseVal, + EnforceAdmins: &trueVal, + RequiredPullRequestReviews: clients.PullRequestReviewRule{ + Required: &falseVal, + }, }, }, }, @@ -237,11 +241,13 @@ func Test_applyRepoRules(t *testing.T) { }, expected: &clients.BranchRef{ BranchProtectionRule: clients.BranchProtectionRule{ - AllowDeletions: &falseVal, - AllowForcePushes: &trueVal, - RequireLinearHistory: &falseVal, - EnforceAdmins: &falseVal, - RequiredPullRequestReviews: nil, + AllowDeletions: &falseVal, + AllowForcePushes: &trueVal, + RequireLinearHistory: &falseVal, + EnforceAdmins: &falseVal, + RequiredPullRequestReviews: clients.PullRequestReviewRule{ + Required: &falseVal, + }, }, }, }, @@ -258,11 +264,13 @@ func Test_applyRepoRules(t *testing.T) { }, expected: &clients.BranchRef{ BranchProtectionRule: clients.BranchProtectionRule{ - AllowDeletions: &falseVal, - AllowForcePushes: &falseVal, - EnforceAdmins: &falseVal, // Downgrade: deletion does not enforce admins - RequireLinearHistory: &falseVal, - RequiredPullRequestReviews: nil, + AllowDeletions: &falseVal, + AllowForcePushes: &falseVal, + EnforceAdmins: &falseVal, // Downgrade: deletion does not enforce admins + RequireLinearHistory: &falseVal, + RequiredPullRequestReviews: clients.PullRequestReviewRule{ + Required: &falseVal, + }, }, }, }, @@ -280,11 +288,13 @@ func Test_applyRepoRules(t *testing.T) { }, expected: &clients.BranchRef{ BranchProtectionRule: clients.BranchProtectionRule{ - AllowDeletions: &falseVal, - AllowForcePushes: &falseVal, - EnforceAdmins: &falseVal, // Maintain: deletion enforces but forcepush does not - RequireLinearHistory: &falseVal, - RequiredPullRequestReviews: nil, + AllowDeletions: &falseVal, + AllowForcePushes: &falseVal, + EnforceAdmins: &falseVal, // Maintain: deletion enforces but forcepush does not + RequireLinearHistory: &falseVal, + RequiredPullRequestReviews: clients.PullRequestReviewRule{ + Required: &falseVal, + }, }, }, }, @@ -301,11 +311,13 @@ func Test_applyRepoRules(t *testing.T) { }, expected: &clients.BranchRef{ BranchProtectionRule: clients.BranchProtectionRule{ - AllowDeletions: &falseVal, - AllowForcePushes: &falseVal, - EnforceAdmins: &trueVal, // Maintain: base and rule are equal strictness - RequireLinearHistory: &falseVal, - RequiredPullRequestReviews: nil, + AllowDeletions: &falseVal, + AllowForcePushes: &falseVal, + EnforceAdmins: &trueVal, // Maintain: base and rule are equal strictness + RequireLinearHistory: &falseVal, + RequiredPullRequestReviews: clients.PullRequestReviewRule{ + Required: &falseVal, + }, }, }, }, @@ -317,11 +329,13 @@ func Test_applyRepoRules(t *testing.T) { }, expected: &clients.BranchRef{ BranchProtectionRule: clients.BranchProtectionRule{ - AllowDeletions: &trueVal, - AllowForcePushes: &falseVal, - EnforceAdmins: &trueVal, - RequireLinearHistory: &falseVal, - RequiredPullRequestReviews: nil, + AllowDeletions: &trueVal, + AllowForcePushes: &falseVal, + EnforceAdmins: &trueVal, + RequireLinearHistory: &falseVal, + RequiredPullRequestReviews: clients.PullRequestReviewRule{ + Required: &falseVal, + }, }, }, }, @@ -333,11 +347,13 @@ func Test_applyRepoRules(t *testing.T) { }, expected: &clients.BranchRef{ BranchProtectionRule: clients.BranchProtectionRule{ - AllowDeletions: &trueVal, - AllowForcePushes: &trueVal, - RequireLinearHistory: &trueVal, - EnforceAdmins: &trueVal, - RequiredPullRequestReviews: nil, + AllowDeletions: &trueVal, + AllowForcePushes: &trueVal, + RequireLinearHistory: &trueVal, + EnforceAdmins: &trueVal, + RequiredPullRequestReviews: clients.PullRequestReviewRule{ + Required: &falseVal, + }, }, }, }, @@ -362,7 +378,8 @@ func Test_applyRepoRules(t *testing.T) { EnforceAdmins: &trueVal, RequireLastPushApproval: &trueVal, RequireLinearHistory: &falseVal, - RequiredPullRequestReviews: &clients.PullRequestReviewRule{ + RequiredPullRequestReviews: clients.PullRequestReviewRule{ + Required: &trueVal, RequiredApprovingReviewCount: &zeroVal, }, }, @@ -392,7 +409,8 @@ func Test_applyRepoRules(t *testing.T) { EnforceAdmins: &trueVal, RequireLinearHistory: &falseVal, RequireLastPushApproval: &trueVal, - RequiredPullRequestReviews: &clients.PullRequestReviewRule{ + RequiredPullRequestReviews: clients.PullRequestReviewRule{ + Required: &trueVal, DismissStaleReviews: &trueVal, RequireCodeOwnerReviews: &trueVal, RequiredApprovingReviewCount: &twoVal, @@ -429,7 +447,9 @@ func Test_applyRepoRules(t *testing.T) { RequiresStatusChecks: &trueVal, Contexts: []string{"foo"}, }, - RequiredPullRequestReviews: nil, + RequiredPullRequestReviews: clients.PullRequestReviewRule{ + Required: &falseVal, + }, }, }, }, @@ -495,7 +515,8 @@ func Test_applyRepoRules(t *testing.T) { RequiresStatusChecks: &trueVal, Contexts: []string{"foo"}, }, - RequiredPullRequestReviews: &clients.PullRequestReviewRule{ + RequiredPullRequestReviews: clients.PullRequestReviewRule{ + Required: &trueVal, RequiredApprovingReviewCount: &twoVal, DismissStaleReviews: &trueVal, RequireCodeOwnerReviews: &trueVal, @@ -556,7 +577,10 @@ func Test_translationFromGithubAPIBranchProtectionData(t *testing.T) { RequiresStatusChecks: nil, Contexts: []string{}, }, - RequiredPullRequestReviews: nil, + RequiredPullRequestReviews: clients.PullRequestReviewRule{ + RequiredApprovingReviewCount: asPtr[int32](0), + RequireCodeOwnerReviews: &falseVal, + }, }, }, }, @@ -591,7 +615,12 @@ func Test_translationFromGithubAPIBranchProtectionData(t *testing.T) { RequiresStatusChecks: &falseVal, Contexts: []string{}, }, - RequiredPullRequestReviews: nil, + RequiredPullRequestReviews: clients.PullRequestReviewRule{ + Required: &falseVal, + RequireCodeOwnerReviews: &falseVal, + DismissStaleReviews: &falseVal, + RequiredApprovingReviewCount: nil, + }, }, }, }, diff --git a/clients/gitlabrepo/branches.go b/clients/gitlabrepo/branches.go index bd69d7e613e0..f50f5e3cdb07 100644 --- a/clients/gitlabrepo/branches.go +++ b/clients/gitlabrepo/branches.go @@ -193,7 +193,8 @@ func makeBranchRefFrom(branch *gitlab.Branch, protectedBranch *gitlab.ProtectedB Contexts: makeContextsFromResp(projectStatusChecks), } - pullRequestReviewRule := &clients.PullRequestReviewRule{ + pullRequestReviewRule := clients.PullRequestReviewRule{ + // TODO how do we know if they're required? DismissStaleReviews: newTrue(), RequireCodeOwnerReviews: &protectedBranch.CodeOwnerApprovalRequired, } diff --git a/cron/internal/format/json_raw_results.go b/cron/internal/format/json_raw_results.go index 9c19227c31e6..1a2d6e643311 100644 --- a/cron/internal/format/json_raw_results.go +++ b/cron/internal/format/json_raw_results.go @@ -210,17 +210,15 @@ func addBranchProtectionRawResults(r *jsonScorecardRawResult, bp *checker.Branch bp = &jsonBranchProtectionSettings{ AllowsDeletions: v.BranchProtectionRule.AllowDeletions, AllowsForcePushes: v.BranchProtectionRule.AllowForcePushes, + RequiresCodeOwnerReviews: v.BranchProtectionRule.RequiredPullRequestReviews.RequireCodeOwnerReviews, RequiresLinearHistory: v.BranchProtectionRule.RequireLinearHistory, + DismissesStaleReviews: v.BranchProtectionRule.RequiredPullRequestReviews.DismissStaleReviews, EnforcesAdmins: v.BranchProtectionRule.EnforceAdmins, RequiresStatusChecks: v.BranchProtectionRule.CheckRules.RequiresStatusChecks, RequiresUpToDateBranchBeforeMerging: v.BranchProtectionRule.CheckRules.UpToDateBeforeMerge, + RequiredApprovingReviewCount: v.BranchProtectionRule.RequiredPullRequestReviews.RequiredApprovingReviewCount, StatusCheckContexts: v.BranchProtectionRule.CheckRules.Contexts, } - if v.BranchProtectionRule.RequiredPullRequestReviews != nil { - bp.DismissesStaleReviews = v.BranchProtectionRule.RequiredPullRequestReviews.DismissStaleReviews - bp.RequiredApprovingReviewCount = v.BranchProtectionRule.RequiredPullRequestReviews.RequiredApprovingReviewCount - bp.RequiresCodeOwnerReviews = v.BranchProtectionRule.RequiredPullRequestReviews.RequireCodeOwnerReviews - } } r.Results.BranchProtections = append(r.Results.BranchProtections, jsonBranchProtection{ Name: *v.Name, diff --git a/pkg/json_raw_results.go b/pkg/json_raw_results.go index 7f2ef29d1358..5d63cacfec8b 100644 --- a/pkg/json_raw_results.go +++ b/pkg/json_raw_results.go @@ -712,17 +712,15 @@ func (r *jsonScorecardRawResult) addBranchProtectionRawResults(bp *checker.Branc bp = &jsonBranchProtectionSettings{ AllowsDeletions: v.BranchProtectionRule.AllowDeletions, AllowsForcePushes: v.BranchProtectionRule.AllowForcePushes, + RequiresCodeOwnerReviews: v.BranchProtectionRule.RequiredPullRequestReviews.RequireCodeOwnerReviews, RequiresLinearHistory: v.BranchProtectionRule.RequireLinearHistory, + DismissesStaleReviews: v.BranchProtectionRule.RequiredPullRequestReviews.DismissStaleReviews, EnforcesAdmins: v.BranchProtectionRule.EnforceAdmins, RequiresStatusChecks: v.BranchProtectionRule.CheckRules.RequiresStatusChecks, RequiresUpToDateBranchBeforeMerging: v.BranchProtectionRule.CheckRules.UpToDateBeforeMerge, + RequiredApprovingReviewCount: v.BranchProtectionRule.RequiredPullRequestReviews.RequiredApprovingReviewCount, StatusCheckContexts: v.BranchProtectionRule.CheckRules.Contexts, } - if v.BranchProtectionRule.RequiredPullRequestReviews != nil { - bp.DismissesStaleReviews = v.BranchProtectionRule.RequiredPullRequestReviews.DismissStaleReviews - bp.RequiredApprovingReviewCount = v.BranchProtectionRule.RequiredPullRequestReviews.RequiredApprovingReviewCount - bp.RequiresCodeOwnerReviews = v.BranchProtectionRule.RequiredPullRequestReviews.RequireCodeOwnerReviews - } } branches = append(branches, jsonBranchProtection{ Name: *v.Name, diff --git a/pkg/json_raw_results_test.go b/pkg/json_raw_results_test.go index 098d6dfc9745..261e6f81cee0 100644 --- a/pkg/json_raw_results_test.go +++ b/pkg/json_raw_results_test.go @@ -1114,7 +1114,8 @@ func TestJsonScorecardRawResult(t *testing.T) { BranchProtectionRule: clients.BranchProtectionRule{ AllowDeletions: boolPtr(true), AllowForcePushes: boolPtr(false), - RequiredPullRequestReviews: &clients.PullRequestReviewRule{ + RequiredPullRequestReviews: clients.PullRequestReviewRule{ + Required: boolPtr(true), RequireCodeOwnerReviews: boolPtr(true), DismissStaleReviews: boolPtr(true), RequiredApprovingReviewCount: intPtr(2), diff --git a/probes/entries.go b/probes/entries.go index 6414d224e215..3ae6dcae2ad5 100644 --- a/probes/entries.go +++ b/probes/entries.go @@ -45,6 +45,8 @@ import ( "github.com/ossf/scorecard/v4/probes/notArchived" "github.com/ossf/scorecard/v4/probes/notCreatedRecently" "github.com/ossf/scorecard/v4/probes/packagedWithAutomatedWorkflow" + "github.com/ossf/scorecard/v4/probes/releasesAreSigned" + "github.com/ossf/scorecard/v4/probes/releasesHaveProvenance" "github.com/ossf/scorecard/v4/probes/sastToolCodeQLInstalled" "github.com/ossf/scorecard/v4/probes/sastToolRunsOnAllCommits" "github.com/ossf/scorecard/v4/probes/sastToolSonarInstalled" @@ -117,6 +119,7 @@ var ( hasDangerousWorkflowScriptInjection.Run, hasDangerousWorkflowUntrustedCheckout.Run, } + Maintained = []ProbeImpl{ notArchived.Run, hasRecentCommits.Run, @@ -135,6 +138,10 @@ var ( CITests = []ProbeImpl{ testsRunInCI.Run, } + SignedReleases = []ProbeImpl{ + releasesAreSigned.Run, + releasesHaveProvenance.Run, + } probeRunners = map[string]func(*checker.RawResults) ([]finding.Finding, string, error){ securityPolicyPresent.Probe: securityPolicyPresent.Run, diff --git a/probes/releasesAreSigned/def.yml b/probes/releasesAreSigned/def.yml new file mode 100644 index 000000000000..5c77599a0678 --- /dev/null +++ b/probes/releasesAreSigned/def.yml @@ -0,0 +1,30 @@ +# Copyright 2023 OpenSSF Scorecard Authors +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +id: releasesAreSigned +short: Check that the projects Github and Gitlab releases are signed. +motivation: > + Signed releases allow consumers to verify their artifacts before consuming them. +implementation: > + The implementation checks whether a signature file is present in release assets. The probe checks the last 5 releases on Github and Gitlab. +outcome: + - For each of the last 5 releases, the probe returns OutcomePositive, if the release has a signature file in the release assets. + - For each of the last 5 releases, the probe returns OutcomeNegative, if the the release does not have a signature file in the release assets. + - If the project has no releases, the probe returns OutcomeNotApplicable. +remediation: + effort: Medium + text: + - Install Cosign by following https://docs.sigstore.dev/system_config/installation + - Sign your release artifacts using `cosign sign $YOUR_ARTIFACT`. See more at https://docs.sigstore.dev/signing/quickstart + - Publish your release and add the certificate and signature produced by Cosign. diff --git a/probes/releasesAreSigned/impl.go b/probes/releasesAreSigned/impl.go new file mode 100644 index 000000000000..735cc141df34 --- /dev/null +++ b/probes/releasesAreSigned/impl.go @@ -0,0 +1,133 @@ +// Copyright 2023 OpenSSF Scorecard Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +//nolint:stylecheck +package releasesAreSigned + +import ( + "embed" + "fmt" + "strings" + + "github.com/ossf/scorecard/v4/checker" + "github.com/ossf/scorecard/v4/finding" + "github.com/ossf/scorecard/v4/probes/internal/utils/uerror" +) + +//go:embed *.yml +var fs embed.FS + +const ( + Probe = "releasesAreSigned" + releaseLookBack = 5 +) + +// ValueType is the type of a value in the finding values. +type ValueType int + +const ( + // ValueTypeRelease represents a release name. + ValueTypeRelease ValueType = iota + // ValueTypeReleaseAsset represents a release asset name. + ValueTypeReleaseAsset +) + +var signatureExtensions = []string{".asc", ".minisig", ".sig", ".sign"} + +func Run(raw *checker.RawResults) ([]finding.Finding, string, error) { + if raw == nil { + return nil, "", fmt.Errorf("%w: raw", uerror.ErrNil) + } + + var findings []finding.Finding + + releases := raw.SignedReleasesResults.Releases + + totalReleases := 0 + for releaseIndex, release := range releases { + if len(release.Assets) == 0 { + continue + } + + if releaseIndex == releaseLookBack { + break + } + + totalReleases++ + signed := false + for j := range release.Assets { + asset := release.Assets[j] + for _, suffix := range signatureExtensions { + if !strings.HasSuffix(asset.Name, suffix) { + continue + } + // Create Positive Finding + // with file info + loc := &finding.Location{ + Type: finding.FileTypeURL, + Path: asset.URL, + } + f, err := finding.NewWith(fs, Probe, + fmt.Sprintf("signed release artifact: %s", asset.Name), + loc, + finding.OutcomePositive) + if err != nil { + return nil, Probe, fmt.Errorf("create finding: %w", err) + } + f.Values = map[string]int{ + release.TagName: int(ValueTypeRelease), + asset.Name: int(ValueTypeReleaseAsset), + } + findings = append(findings, *f) + signed = true + break + } + if signed { + break + } + } + if signed { + continue + } + + // Release is not signed + loc := &finding.Location{ + Type: finding.FileTypeURL, + Path: release.URL, + } + f, err := finding.NewWith(fs, Probe, + fmt.Sprintf("release artifact %s not signed", release.TagName), + loc, + finding.OutcomeNegative) + if err != nil { + return nil, Probe, fmt.Errorf("create finding: %w", err) + } + f.Values = map[string]int{ + release.TagName: int(ValueTypeRelease), + } + findings = append(findings, *f) + } + + if len(findings) == 0 { + f, err := finding.NewWith(fs, Probe, + "no GitHub/GitLab releases found", + nil, + finding.OutcomeNotApplicable) + if err != nil { + return nil, Probe, fmt.Errorf("create finding: %w", err) + } + findings = append(findings, *f) + } + return findings, Probe, nil +} diff --git a/probes/releasesAreSigned/impl_test.go b/probes/releasesAreSigned/impl_test.go new file mode 100644 index 000000000000..71b8e4444a02 --- /dev/null +++ b/probes/releasesAreSigned/impl_test.go @@ -0,0 +1,261 @@ +// Copyright 2023 OpenSSF Scorecard Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +//nolint:stylecheck +package releasesAreSigned + +import ( + "fmt" + "testing" + + "github.com/google/go-cmp/cmp" + "github.com/google/go-cmp/cmp/cmpopts" + + "github.com/ossf/scorecard/v4/checker" + "github.com/ossf/scorecard/v4/clients" + "github.com/ossf/scorecard/v4/finding" +) + +func Test_Run(t *testing.T) { + t.Parallel() + //nolint:govet + tests := []struct { + name string + raw *checker.RawResults + outcomes []finding.Outcome + err error + }{ + { + name: "Has one signed release.", + raw: &checker.RawResults{ + SignedReleasesResults: checker.SignedReleasesData{ + Releases: []clients.Release{ + { + 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, + }, + }, + { + name: "Has two signed releases.", + raw: &checker.RawResults{ + SignedReleasesResults: checker.SignedReleasesData{ + Releases: []clients.Release{ + { + TagName: "v1.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"}, + }, + }, + }, + }, + }, + outcomes: []finding.Outcome{ + finding.OutcomePositive, + finding.OutcomePositive, + }, + }, + { + name: "Has two unsigned releases.", + raw: &checker.RawResults{ + SignedReleasesResults: checker.SignedReleasesData{ + Releases: []clients.Release{ + { + TagName: "v1.0", + Assets: []clients.ReleaseAsset{ + {Name: "binary.tar.gz"}, + {Name: "binary.tar.gz.notSig"}, + {Name: "binary.tar.gz.intoto.jsonl"}, + }, + }, + { + TagName: "v2.0", + Assets: []clients.ReleaseAsset{ + {Name: "binary.tar.gz"}, + {Name: "binary.tar.gz.notSig"}, + {Name: "binary.tar.gz.intoto.jsonl"}, + }, + }, + }, + }, + }, + outcomes: []finding.Outcome{ + finding.OutcomeNegative, + finding.OutcomeNegative, + }, + }, + { + name: "Has two unsigned releases and one signed release.", + raw: &checker.RawResults{ + SignedReleasesResults: checker.SignedReleasesData{ + Releases: []clients.Release{ + { + TagName: "v1.0", + Assets: []clients.ReleaseAsset{ + {Name: "binary.tar.gz"}, + {Name: "binary.tar.gz.notSig"}, + {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: "v3.0", + Assets: []clients.ReleaseAsset{ + {Name: "binary.tar.gz"}, + {Name: "binary.tar.gz.notSig"}, + {Name: "binary.tar.gz.intoto.jsonl"}, + }, + }, + }, + }, + }, + outcomes: []finding.Outcome{ + finding.OutcomeNegative, + finding.OutcomePositive, + finding.OutcomeNegative, + }, + }, + { + name: "Many releases.", + raw: &checker.RawResults{ + SignedReleasesResults: checker.SignedReleasesData{ + Releases: []clients.Release{ + release("v0.8.5"), + release("v0.8.4"), + release("v0.8.3"), + release("v0.8.2"), + release("v0.8.1"), + release("v0.8.0"), + release("v0.7.0"), + release("v0.6.0"), + release("v0.5.0"), + release("v0.4.0"), + release("v0.3.0"), + release("v0.2.0"), + release("v0.1.0"), + release("v0.0.6"), + release("v0.0.5"), + release("v0.0.4"), + release("v0.0.3"), + release("v0.0.2"), + release("v0.0.1"), + }, + }, + }, + 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 + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + findings, s, err := Run(tt.raw) + if !cmp.Equal(tt.err, err, cmpopts.EquateErrors()) { + t.Errorf("mismatch (-want +got):\n%s", cmp.Diff(tt.err, err, cmpopts.EquateErrors())) + } + if err != nil { + return + } + if diff := cmp.Diff(Probe, s); diff != "" { + t.Errorf("mismatch (-want +got):\n%s", diff) + } + if diff := cmp.Diff(len(tt.outcomes), len(findings)); diff != "" { + t.Errorf("mismatch (-want +got):\n%s", diff) + } + for i := range tt.outcomes { + outcome := &tt.outcomes[i] + f := &findings[i] + if diff := cmp.Diff(*outcome, f.Outcome); diff != "" { + t.Errorf("mismatch (-want +got):\n%s", diff) + } + } + }) + } +} + +func release(version string) clients.Release { + return clients.Release{ + TagName: version, + URL: fmt.Sprintf("https://github.com/test/test_artifact/releases/tag/%s", version), + TargetCommitish: "master", + Assets: []clients.ReleaseAsset{ + { + Name: fmt.Sprintf("%s_checksums.txt", version), + URL: fmt.Sprintf("https://github.com/test/repo/releases/%s/%s_checksums.txt", version, version), + }, + { + Name: fmt.Sprintf("%s_checksums.txt.sig", version), + URL: fmt.Sprintf("https://github.com/test/repo/releases/%s/%s_checksums.txt.sig", version, version), + }, + { + Name: fmt.Sprintf("%s_darwin_x86_64.tar.gz", version), + URL: fmt.Sprintf("https://github.com/test/repo/releases/%s/%s_darwin_x86_64.tar.gz", version, version), + }, + { + Name: fmt.Sprintf("%s_Linux_arm64.tar.gz", version), + URL: fmt.Sprintf("https://github.com/test/repo/releases/%s/%s_Linux_arm64.tar.gz", version, version), + }, + { + Name: fmt.Sprintf("%s_Linux_i386.tar.gz", version), + URL: fmt.Sprintf("https://github.com/test/repo/releases/%s/%s_Linux_i386.tar.gz", version, version), + }, + { + Name: fmt.Sprintf("%s_Linux_x86_64.tar.gz", version), + URL: fmt.Sprintf("https://github.com/test/repo/releases/%s/%s_Linux_x86_64.tar.gz", version, version), + }, + { + Name: fmt.Sprintf("%s_windows_i386.tar.gz", version), + URL: fmt.Sprintf("https://github.com/test/repo/releases/%s/%s_windows_i386.tar.gz", version, version), + }, + { + Name: fmt.Sprintf("%s_windows_x86_64.tar.gz", version), + URL: fmt.Sprintf("https://github.com/test/repo/releases/%s/%s_windows_x86_64.tar.gz", version, version), + }, + }, + } +} diff --git a/probes/releasesHaveProvenance/def.yml b/probes/releasesHaveProvenance/def.yml new file mode 100644 index 000000000000..4cb0dc70b747 --- /dev/null +++ b/probes/releasesHaveProvenance/def.yml @@ -0,0 +1,28 @@ +# Copyright 2023 OpenSSF Scorecard Authors +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +id: releasesHaveProvenance +short: Check that the projects releases on Github and Gitlab have provenance. +motivation: > + Provenance give users security-critical, verifiable information so that consumers can verify their artifacts before consuming them. +implementation: > + The probe checks whether any of the assets in any of the last five releases on Github or Gitlab have a provenance file. +outcome: + - For each of the last 5 releases, the probe returns OutcomePositive, if the release has a provenance file in the release assets. + - For each of the last 5 releases, the probe returns OutcomeNegative, if the the release does not have a provenance file in the release assets. + - If the project has no releases, the probe returns OutcomeNotApplicable. +remediation: + effort: Medium + text: + - Generate provenance and add it alongside the release assets. Use the [slsa-github-generator](https://github.com/slsa-framework/slsa-github-generator) to add SLSA level 3 provenance to releases. diff --git a/probes/releasesHaveProvenance/impl.go b/probes/releasesHaveProvenance/impl.go new file mode 100644 index 000000000000..02ba874e0913 --- /dev/null +++ b/probes/releasesHaveProvenance/impl.go @@ -0,0 +1,134 @@ +// Copyright 2023 OpenSSF Scorecard Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +//nolint:stylecheck +package releasesHaveProvenance + +import ( + "embed" + "fmt" + "strings" + + "github.com/ossf/scorecard/v4/checker" + "github.com/ossf/scorecard/v4/finding" + "github.com/ossf/scorecard/v4/probes/internal/utils/uerror" +) + +//go:embed *.yml +var fs embed.FS + +const ( + Probe = "releasesHaveProvenance" + releaseLookBack = 5 +) + +// ValueType is the type of a value in the finding values. +type ValueType int + +const ( + // ValueTypeRelease represents a release name. + ValueTypeRelease ValueType = iota + // ValueTypeReleaseAsset represents a release asset name. + ValueTypeReleaseAsset +) + +var provenanceExtensions = []string{".intoto.jsonl"} + +func Run(raw *checker.RawResults) ([]finding.Finding, string, error) { + if raw == nil { + return nil, "", fmt.Errorf("%w: raw", uerror.ErrNil) + } + + var findings []finding.Finding + + releases := raw.SignedReleasesResults.Releases + + totalReleases := 0 + + for i := range releases { + release := releases[i] + if len(release.Assets) == 0 { + continue + } + totalReleases++ + hasProvenance := false + for j := range release.Assets { + asset := release.Assets[j] + for _, suffix := range provenanceExtensions { + if !strings.HasSuffix(asset.Name, suffix) { + continue + } + // Create Positive Finding + // with file info + loc := &finding.Location{ + Type: finding.FileTypeURL, + Path: asset.URL, + } + f, err := finding.NewWith(fs, Probe, + fmt.Sprintf("provenance for release artifact: %s", asset.Name), + loc, + finding.OutcomePositive) + if err != nil { + return nil, Probe, fmt.Errorf("create finding: %w", err) + } + f.Values = map[string]int{ + release.TagName: int(ValueTypeRelease), + asset.Name: int(ValueTypeReleaseAsset), + } + findings = append(findings, *f) + hasProvenance = true + break + } + if hasProvenance { + break + } + } + if hasProvenance { + continue + } + + // Release does not have provenance + loc := &finding.Location{ + Type: finding.FileTypeURL, + Path: release.URL, + } + f, err := finding.NewWith(fs, Probe, + fmt.Sprintf("release artifact %s does not have provenance", release.TagName), + loc, + finding.OutcomeNegative) + if err != nil { + return nil, Probe, fmt.Errorf("create finding: %w", err) + } + f.Values = map[string]int{ + release.TagName: int(ValueTypeRelease), + } + findings = append(findings, *f) + + if totalReleases >= releaseLookBack { + break + } + } + + if len(findings) == 0 { + f, err := finding.NewWith(fs, Probe, + "no GitHub releases found", + nil, + finding.OutcomeNotApplicable) + if err != nil { + return nil, Probe, fmt.Errorf("create finding: %w", err) + } + findings = append(findings, *f) + } + return findings, Probe, nil +} diff --git a/probes/releasesHaveProvenance/impl_test.go b/probes/releasesHaveProvenance/impl_test.go new file mode 100644 index 000000000000..bc8e148c5902 --- /dev/null +++ b/probes/releasesHaveProvenance/impl_test.go @@ -0,0 +1,183 @@ +// Copyright 2023 OpenSSF Scorecard Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +//nolint:stylecheck +package releasesHaveProvenance + +import ( + "testing" + + "github.com/google/go-cmp/cmp" + "github.com/google/go-cmp/cmp/cmpopts" + + "github.com/ossf/scorecard/v4/checker" + "github.com/ossf/scorecard/v4/clients" + "github.com/ossf/scorecard/v4/finding" +) + +func Test_Run(t *testing.T) { + t.Parallel() + //nolint:govet + tests := []struct { + name string + raw *checker.RawResults + outcomes []finding.Outcome + err error + }{ + { + name: "Has one release with provenance.", + raw: &checker.RawResults{ + SignedReleasesResults: checker.SignedReleasesData{ + Releases: []clients.Release{ + { + 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, + }, + }, + { + name: "Has two releases with provenance.", + raw: &checker.RawResults{ + SignedReleasesResults: checker.SignedReleasesData{ + Releases: []clients.Release{ + { + TagName: "v1.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"}, + }, + }, + }, + }, + }, + outcomes: []finding.Outcome{ + finding.OutcomePositive, + finding.OutcomePositive, + }, + }, + { + name: "Has two releases without provenance.", + raw: &checker.RawResults{ + SignedReleasesResults: checker.SignedReleasesData{ + Releases: []clients.Release{ + { + TagName: "v1.0", + Assets: []clients.ReleaseAsset{ + {Name: "binary.tar.gz"}, + {Name: "binary.tar.gz.sig"}, + {Name: "binary.tar.gz.intoto.notJsonl"}, + }, + }, + { + TagName: "v2.0", + Assets: []clients.ReleaseAsset{ + {Name: "binary.tar.gz"}, + {Name: "binary.tar.gz.sig"}, + {Name: "binary.tar.gz.intoto.notJsonl"}, + }, + }, + }, + }, + }, + outcomes: []finding.Outcome{ + finding.OutcomeNegative, + finding.OutcomeNegative, + }, + }, + { + name: "Has two releases without provenace and one with.", + raw: &checker.RawResults{ + SignedReleasesResults: checker.SignedReleasesData{ + Releases: []clients.Release{ + { + TagName: "v1.0", + Assets: []clients.ReleaseAsset{ + {Name: "binary.tar.gz"}, + {Name: "binary.tar.gz.notSig"}, + {Name: "binary.tar.gz.intoto.notJsonl"}, + }, + }, + { + TagName: "v2.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.notJsonl"}, + }, + }, + }, + }, + }, + outcomes: []finding.Outcome{ + finding.OutcomeNegative, + finding.OutcomePositive, + finding.OutcomeNegative, + }, + }, + } + for _, tt := range tests { + tt := tt // Re-initializing variable so it is not changed while executing the closure below + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + findings, s, err := Run(tt.raw) + if !cmp.Equal(tt.err, err, cmpopts.EquateErrors()) { + t.Errorf("mismatch (-want +got):\n%s", cmp.Diff(tt.err, err, cmpopts.EquateErrors())) + } + if err != nil { + return + } + if diff := cmp.Diff(Probe, s); diff != "" { + t.Errorf("mismatch (-want +got):\n%s", diff) + } + if diff := cmp.Diff(len(tt.outcomes), len(findings)); diff != "" { + t.Errorf("mismatch (-want +got):\n%s", diff) + } + for i := range tt.outcomes { + outcome := &tt.outcomes[i] + f := &findings[i] + if diff := cmp.Diff(*outcome, f.Outcome); diff != "" { + t.Errorf("mismatch (-want +got):\n%s", diff) + } + } + }) + } +}