From 1bc261a57ba96dfa55fae1fdd757ec874bc45b4e Mon Sep 17 00:00:00 2001 From: Raghav Kaul Date: Tue, 4 Apr 2023 19:29:17 +0000 Subject: [PATCH 1/8] Return inconclusive if there are no workflows Signed-off-by: Raghav Kaul --- checker/raw_result.go | 3 ++- checks/evaluation/dangerous_workflow.go | 4 ++++ checks/raw/dangerous_workflow.go | 2 ++ 3 files changed, 8 insertions(+), 1 deletion(-) diff --git a/checker/raw_result.go b/checker/raw_result.go index 5ec49e47998..bf8806ed8cb 100644 --- a/checker/raw_result.go +++ b/checker/raw_result.go @@ -313,7 +313,8 @@ const ( // DangerousWorkflowData contains raw results // for dangerous workflow check. type DangerousWorkflowData struct { - Workflows []DangerousWorkflow + Workflows []DangerousWorkflow + NumWorkflows int } // DangerousWorkflow represents a dangerous workflow. diff --git a/checks/evaluation/dangerous_workflow.go b/checks/evaluation/dangerous_workflow.go index be37c6cdee5..9aa76fe751c 100644 --- a/checks/evaluation/dangerous_workflow.go +++ b/checks/evaluation/dangerous_workflow.go @@ -30,6 +30,10 @@ func DangerousWorkflow(name string, dl checker.DetailLogger, return checker.CreateRuntimeErrorResult(name, e) } + if r.NumWorkflows == 0 { + return checker.CreateInconclusiveResult(name, "no workflows found") + } + for _, e := range r.Workflows { var text string switch e.Type { diff --git a/checks/raw/dangerous_workflow.go b/checks/raw/dangerous_workflow.go index 7db760e715c..1da80f0e2eb 100644 --- a/checks/raw/dangerous_workflow.go +++ b/checks/raw/dangerous_workflow.go @@ -102,6 +102,8 @@ var validateGitHubActionWorkflowPatterns fileparser.DoWhileTrueOnFileContent = f return true, nil } + pdata.NumWorkflows += 1 + workflow, errs := actionlint.Parse(content) if len(errs) > 0 && workflow == nil { return false, fileparser.FormatActionlintError(errs) From 88d1d2cfd46e2b308983a5b45f5e9417df0510fd Mon Sep 17 00:00:00 2001 From: Raghav Kaul Date: Tue, 4 Apr 2023 19:30:17 +0000 Subject: [PATCH 2/8] Return inconclusive if we don't have any workflows Signed-off-by: Raghav Kaul --- checker/raw_result.go | 1 + checks/evaluation/permissions/permissions.go | 4 ++++ checks/raw/permissions.go | 2 ++ 3 files changed, 7 insertions(+) diff --git a/checker/raw_result.go b/checker/raw_result.go index bf8806ed8cb..919071b4bf1 100644 --- a/checker/raw_result.go +++ b/checker/raw_result.go @@ -333,6 +333,7 @@ type WorkflowJob struct { // TokenPermissionsData represents data about a permission failure. type TokenPermissionsData struct { TokenPermissions []TokenPermission + NumTokens int } // PermissionLocation represents a declaration type. diff --git a/checks/evaluation/permissions/permissions.go b/checks/evaluation/permissions/permissions.go index fed2c092bb8..a661c372dae 100644 --- a/checks/evaluation/permissions/permissions.go +++ b/checks/evaluation/permissions/permissions.go @@ -40,6 +40,10 @@ func TokenPermissions(name string, c *checker.CheckRequest, r *checker.TokenPerm return checker.CreateRuntimeErrorResult(name, e) } + if r.NumTokens == 0 { + return checker.CreateInconclusiveResult(name, "no github tokens found") + } + score, err := applyScorePolicy(r, c) if err != nil { return checker.CreateRuntimeErrorResult(name, err) diff --git a/checks/raw/permissions.go b/checks/raw/permissions.go index e2415625a32..a35495af661 100644 --- a/checks/raw/permissions.go +++ b/checks/raw/permissions.go @@ -84,6 +84,8 @@ var validateGitHubActionTokenPermissions fileparser.DoWhileTrueOnFileContent = f return true, nil } + pdata.results.NumTokens += 1 + workflow, errs := actionlint.Parse(content) if len(errs) > 0 && workflow == nil { return false, fileparser.FormatActionlintError(errs) From dd90944634fb84c81a2fe57b5ba63b544b2e93ae Mon Sep 17 00:00:00 2001 From: Raghav Kaul Date: Tue, 4 Apr 2023 19:30:45 +0000 Subject: [PATCH 3/8] logging fixes Signed-off-by: Raghav Kaul --- checks/evaluation/contributors.go | 2 +- clients/gitlabrepo/tarball.go | 2 -- 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/checks/evaluation/contributors.go b/checks/evaluation/contributors.go index 1425f608d96..d419e6a1ea4 100644 --- a/checks/evaluation/contributors.go +++ b/checks/evaluation/contributors.go @@ -60,7 +60,7 @@ func Contributors(name string, dl checker.DetailLogger, sort.Strings(names) - if len(name) > 0 { + if len(names) > 0 { dl.Info(&checker.LogMessage{ Text: fmt.Sprintf("contributors work for %v", strings.Join(names, ",")), }) diff --git a/clients/gitlabrepo/tarball.go b/clients/gitlabrepo/tarball.go index 4646f192de1..02a40d46355 100644 --- a/clients/gitlabrepo/tarball.go +++ b/clients/gitlabrepo/tarball.go @@ -244,8 +244,6 @@ func (handler *tarballHandler) getFileContent(filename string) ([]byte, error) { fmt.Printf("err: %v\n", err) return nil, fmt.Errorf("error during tarballHandler.setup: %w", err) } - fmt.Printf("handler.tempDir: %v\n", handler.tempDir) - fmt.Printf("filename: %v\n", filename) content, err := os.ReadFile(filepath.Join(handler.tempDir, filename)) if err != nil { return content, fmt.Errorf("os.ReadFile: %w", err) From f89b817e77edcaf6b7c4ad9d263ba6417e5ec77c Mon Sep 17 00:00:00 2001 From: Raghav Kaul Date: Tue, 4 Apr 2023 19:38:48 +0000 Subject: [PATCH 4/8] fix panic Signed-off-by: Raghav Kaul --- clients/gitlabrepo/releases.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/clients/gitlabrepo/releases.go b/clients/gitlabrepo/releases.go index 6aca7adbc24..140aa701e38 100644 --- a/clients/gitlabrepo/releases.go +++ b/clients/gitlabrepo/releases.go @@ -70,9 +70,12 @@ func releasesFrom(data []*gitlab.Release) []clients.Release { for _, r := range data { release := clients.Release{ TagName: r.TagName, - URL: r.Assets.Links[0].DirectAssetURL, TargetCommitish: r.CommitPath, } + if len(r.Assets.Links) > 0 { + release.URL = r.Assets.Links[0].DirectAssetURL + } + for _, a := range r.Assets.Sources { release.Assets = append(release.Assets, clients.ReleaseAsset{ Name: a.Format, From a6b4e72f4e532ce197b84bae7cff15bd1fcd0b3b Mon Sep 17 00:00:00 2001 From: Raghav Kaul Date: Tue, 4 Apr 2023 20:08:20 +0000 Subject: [PATCH 5/8] Update README.md Signed-off-by: Raghav Kaul --- README.md | 42 +++++++++++++++++++++--------------------- 1 file changed, 21 insertions(+), 21 deletions(-) diff --git a/README.md b/README.md index ad0b17243d1..83304146402 100644 --- a/README.md +++ b/README.md @@ -433,27 +433,27 @@ These may be specified with the `--format` flag. For example, `--format=json`. The following checks are all run against the target project by default: -Name | Description | Risk Level | Token Required | Note ------------ | ----------------------------------------- | ---------- | --------------- | ----------------- -[Binary-Artifacts](docs/checks.md#binary-artifacts) | Is the project free of checked-in binaries? | High | PAT, GITHUB_TOKEN | -[Branch-Protection](docs/checks.md#branch-protection) | Does the project use [Branch Protection](https://docs.github.com/en/free-pro-team@latest/github/administering-a-repository/about-protected-branches) ? | High | PAT (`repo` or `repo> public_repo`), GITHUB_TOKEN | certain settings are only supported with a maintainer PAT -[CI-Tests](docs/checks.md#ci-tests) | Does the project run tests in CI, e.g. [GitHub Actions](https://docs.github.com/en/free-pro-team@latest/actions), [Prow](https://github.com/kubernetes/test-infra/tree/master/prow)? | Low | PAT, GITHUB_TOKEN | -[CII-Best-Practices](docs/checks.md#cii-best-practices) | Does the project have an [OpenSSF (formerly CII) Best Practices Badge](https://bestpractices.coreinfrastructure.org/en)? | Low | PAT, GITHUB_TOKEN | -[Code-Review](docs/checks.md#code-review) | Does the project practice code review before code is merged? | High | PAT, GITHUB_TOKEN | -[Contributors](docs/checks.md#contributors) | Does the project have contributors from at least two different organizations? | Low | PAT, GITHUB_TOKEN | -[Dangerous-Workflow](docs/checks.md#dangerous-workflow) | Does the project avoid dangerous coding patterns in GitHub Action workflows? | Critical | PAT, GITHUB_TOKEN | -[Dependency-Update-Tool](docs/checks.md#dependency-update-tool) | Does the project use tools to help update its dependencies? | High | PAT, GITHUB_TOKEN | -[Fuzzing](docs/checks.md#fuzzing) | Does the project use fuzzing tools, e.g. [OSS-Fuzz](https://github.com/google/oss-fuzz)? | Medium | PAT, GITHUB_TOKEN | -[License](docs/checks.md#license) | Does the project declare a license? | Low | PAT, GITHUB_TOKEN | -[Maintained](docs/checks.md#maintained) | Is the project at least 90 days old, and maintained? | High | PAT, GITHUB_TOKEN | -[Pinned-Dependencies](docs/checks.md#pinned-dependencies) | Does the project declare and pin [dependencies](https://docs.github.com/en/free-pro-team@latest/github/visualizing-repository-data-with-graphs/about-the-dependency-graph#supported-package-ecosystems)? | Medium | PAT, GITHUB_TOKEN | -[Packaging](docs/checks.md#packaging) | Does the project build and publish official packages from CI/CD, e.g. [GitHub Publishing](https://docs.github.com/en/free-pro-team@latest/actions/guides/about-packaging-with-github-actions#workflows-for-publishing-packages) ? | Medium | PAT, GITHUB_TOKEN | -[SAST](docs/checks.md#sast) | Does the project use static code analysis tools, e.g. [CodeQL](https://docs.github.com/en/free-pro-team@latest/github/finding-security-vulnerabilities-and-errors-in-your-code/enabling-code-scanning-for-a-repository#enabling-code-scanning-using-actions), [LGTM (deprecated)](https://lgtm.com), [SonarCloud](https://sonarcloud.io)? | Medium | PAT, GITHUB_TOKEN | -[Security-Policy](docs/checks.md#security-policy) | Does the project contain a [security policy](https://docs.github.com/en/free-pro-team@latest/github/managing-security-vulnerabilities/adding-a-security-policy-to-your-repository)? | Medium | PAT, GITHUB_TOKEN | -[Signed-Releases](docs/checks.md#signed-releases) | Does the project cryptographically [sign releases](https://wiki.debian.org/Creating%20signed%20GitHub%20releases)? | High | PAT, GITHUB_TOKEN | -[Token-Permissions](docs/checks.md#token-permissions) | Does the project declare GitHub workflow tokens as [read only](https://docs.github.com/en/actions/reference/authentication-in-a-workflow)? | High | PAT, GITHUB_TOKEN | -[Vulnerabilities](docs/checks.md#vulnerabilities) | Does the project have unfixed vulnerabilities? Uses the [OSV service](https://osv.dev). | High | PAT, GITHUB_TOKEN | -[Webhooks](docs/checks.md#webhooks) | Does the webhook defined in the repository have a token configured to authenticate the origins of requests? | High | maintainer PAT (`admin: repo_hook` or `admin> read:repo_hook` [doc](https://docs.github.com/en/rest/webhooks/repo-config#get-a-webhook-configuration-for-a-repository) | EXPERIMENTAL +Name | Description | Risk Level | Token Required | GitLab Support | Note +----------- | ----------------------------------------- | ---------- | --------------- | -------------- | --- | +[Binary-Artifacts](docs/checks.md#binary-artifacts) | Is the project free of checked-in binaries? | High | PAT, GITHUB_TOKEN | Fully Supported | +[Branch-Protection](docs/checks.md#branch-protection) | Does the project use [Branch Protection](https://docs.github.com/en/free-pro-team@latest/github/administering-a-repository/about-protected-branches) ? | High | PAT (`repo` or `repo> public_repo`), GITHUB_TOKEN | Fully Supported | certain settings are only supported with a maintainer PAT +[CI-Tests](docs/checks.md#ci-tests) | Does the project run tests in CI, e.g. [GitHub Actions](https://docs.github.com/en/free-pro-team@latest/actions), [Prow](https://github.com/kubernetes/test-infra/tree/master/prow)? | Low | PAT, GITHUB_TOKEN | Unsupported +[CII-Best-Practices](docs/checks.md#cii-best-practices) | Does the project have an [OpenSSF (formerly CII) Best Practices Badge](https://bestpractices.coreinfrastructure.org/en)? | Low | PAT, GITHUB_TOKEN | Fully Supported | +[Code-Review](docs/checks.md#code-review) | Does the project practice code review before code is merged? | High | PAT, GITHUB_TOKEN | Fully Supported | +[Contributors](docs/checks.md#contributors) | Does the project have contributors from at least two different organizations? | Low | PAT, GITHUB_TOKEN | Fully Supported | +[Dangerous-Workflow](docs/checks.md#dangerous-workflow) | Does the project avoid dangerous coding patterns in GitHub Action workflows? | Critical | PAT, GITHUB_TOKEN | Unsupported | +[Dependency-Update-Tool](docs/checks.md#dependency-update-tool) | Does the project use tools to help update its dependencies? | High | PAT, GITHUB_TOKEN | Unsupported | +[Fuzzing](docs/checks.md#fuzzing) | Does the project use fuzzing tools, e.g. [OSS-Fuzz](https://github.com/google/oss-fuzz)? | Medium | PAT, GITHUB_TOKEN | Fully Supported +[License](docs/checks.md#license) | Does the project declare a license? | Low | PAT, GITHUB_TOKEN | Fully Supported | +[Maintained](docs/checks.md#maintained) | Is the project at least 90 days old, and maintained? | High | PAT, GITHUB_TOKEN | Fully Supported | +[Pinned-Dependencies](docs/checks.md#pinned-dependencies) | Does the project declare and pin [dependencies](https://docs.github.com/en/free-pro-team@latest/github/visualizing-repository-data-with-graphs/about-the-dependency-graph#supported-package-ecosystems)? | Medium | PAT, GITHUB_TOKEN | Fully Supported | +[Packaging](docs/checks.md#packaging) | Does the project build and publish official packages from CI/CD, e.g. [GitHub Publishing](https://docs.github.com/en/free-pro-team@latest/actions/guides/about-packaging-with-github-actions#workflows-for-publishing-packages) ? | Medium | PAT, GITHUB_TOKEN | Fully Supported | +[SAST](docs/checks.md#sast) | Does the project use static code analysis tools, e.g. [CodeQL](https://docs.github.com/en/free-pro-team@latest/github/finding-security-vulnerabilities-and-errors-in-your-code/enabling-code-scanning-for-a-repository#enabling-code-scanning-using-actions), [LGTM (deprecated)](https://lgtm.com), [SonarCloud](https://sonarcloud.io)? | Medium | PAT, GITHUB_TOKEN | Unsupported | +[Security-Policy](docs/checks.md#security-policy) | Does the project contain a [security policy](https://docs.github.com/en/free-pro-team@latest/github/managing-security-vulnerabilities/adding-a-security-policy-to-your-repository)? | Medium | PAT, GITHUB_TOKEN | Fully Supported | +[Signed-Releases](docs/checks.md#signed-releases) | Does the project cryptographically [sign releases](https://wiki.debian.org/Creating%20signed%20GitHub%20releases)? | High | PAT, GITHUB_TOKEN | Fully Supported | +[Token-Permissions](docs/checks.md#token-permissions) | Does the project declare GitHub workflow tokens as [read only](https://docs.github.com/en/actions/reference/authentication-in-a-workflow)? | High | PAT, GITHUB_TOKEN | Unsupported | +[Vulnerabilities](docs/checks.md#vulnerabilities) | Does the project have unfixed vulnerabilities? Uses the [OSV service](https://osv.dev). | High | PAT, GITHUB_TOKEN | Fully Supported | +[Webhooks](docs/checks.md#webhooks) | Does the webhook defined in the repository have a token configured to authenticate the origins of requests? | High | maintainer PAT (`admin: repo_hook` or `admin> read:repo_hook` [doc](https://docs.github.com/en/rest/webhooks/repo-config#get-a-webhook-configuration-for-a-repository) | | EXPERIMENTAL ### Detailed Checks Documentation From 7e0af93b6f46533a3334e1d291d6b9a8aef0b0f9 Mon Sep 17 00:00:00 2001 From: Raghav Kaul Date: Tue, 4 Apr 2023 20:48:53 +0000 Subject: [PATCH 6/8] skip error when getting external status checks (requires full api access) Signed-off-by: Raghav Kaul --- clients/gitlabrepo/branches.go | 1 - 1 file changed, 1 deletion(-) diff --git a/clients/gitlabrepo/branches.go b/clients/gitlabrepo/branches.go index 79aeaafbb2a..b4ca8dc1c28 100644 --- a/clients/gitlabrepo/branches.go +++ b/clients/gitlabrepo/branches.go @@ -73,7 +73,6 @@ func (handler *branchesHandler) setup() error { handler.repourl.project, &gitlab.ListOptions{}) if err != nil && resp.StatusCode != 404 { handler.errSetup = fmt.Errorf("request for external status checks failed with error %w", err) - return } projectApprovalRule, resp, err := handler.glClient.Projects.GetApprovalConfiguration(handler.repourl.project) From bc7dd21dca6cf101801ffdaad67977d757c61b1f Mon Sep 17 00:00:00 2001 From: Raghav Kaul Date: Mon, 17 Apr 2023 14:56:22 +0000 Subject: [PATCH 7/8] update Signed-off-by: Raghav Kaul --- checks/permissions_test.go | 6 +++--- clients/gitlabrepo/branches.go | 4 +++- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/checks/permissions_test.go b/checks/permissions_test.go index 40affa16220..978c1e4291a 100644 --- a/checks/permissions_test.go +++ b/checks/permissions_test.go @@ -240,9 +240,9 @@ func TestGithubTokenPermissions(t *testing.T) { filenames: []string{"./testdata/script.sh"}, expected: scut.TestReturn{ Error: nil, - Score: checker.MaxResultScore, + Score: checker.InconclusiveResultScore, NumberOfWarn: 0, - NumberOfInfo: 2, + NumberOfInfo: 0, NumberOfDebug: 0, }, }, @@ -375,7 +375,7 @@ func TestGithubTokenPermissions(t *testing.T) { ctrl := gomock.NewController(t) mockRepo := mockrepo.NewMockRepoClient(ctrl) - mockRepo.EXPECT().GetDefaultBranchName().Return("main", nil) + mockRepo.EXPECT().GetDefaultBranchName().Return("main", nil).AnyTimes() main := "main" mockRepo.EXPECT().URI().Return("github.com/ossf/scorecard").AnyTimes() diff --git a/clients/gitlabrepo/branches.go b/clients/gitlabrepo/branches.go index b4ca8dc1c28..50949f14474 100644 --- a/clients/gitlabrepo/branches.go +++ b/clients/gitlabrepo/branches.go @@ -71,8 +71,10 @@ func (handler *branchesHandler) setup() error { projectStatusChecks, resp, err := handler.glClient.ExternalStatusChecks.ListProjectStatusChecks( handler.repourl.project, &gitlab.ListOptions{}) - if err != nil && resp.StatusCode != 404 { + if (err != nil || resp.StatusCode != 404) && + resp.StatusCode != 401 { // 401: permissions. pass token authorization issues silently handler.errSetup = fmt.Errorf("request for external status checks failed with error %w", err) + return } projectApprovalRule, resp, err := handler.glClient.Projects.GetApprovalConfiguration(handler.repourl.project) From cc89464288983383aeda4051b56ec331c46f0aef Mon Sep 17 00:00:00 2001 From: Raghav Kaul Date: Fri, 21 Apr 2023 18:44:09 +0000 Subject: [PATCH 8/8] fix dangerous workflow test Signed-off-by: Raghav Kaul --- checks/evaluation/dangerous_workflow_test.go | 21 +++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-) diff --git a/checks/evaluation/dangerous_workflow_test.go b/checks/evaluation/dangerous_workflow_test.go index 7726f33a64b..ae3eef25991 100644 --- a/checks/evaluation/dangerous_workflow_test.go +++ b/checks/evaluation/dangerous_workflow_test.go @@ -43,7 +43,23 @@ func TestDangerousWorkflow(t *testing.T) { r: &checker.DangerousWorkflowData{}, }, want: checker.CheckResult{ - Score: 10, + Score: checker.InconclusiveResultScore, + Reason: "no workflows found", + Version: 2, + Name: "DangerousWorkflow", + }, + }, + { + name: "DangerousWorkflow - found workflows, none dangerous", + args: args{ + name: "DangerousWorkflow", + dl: &scut.TestDetailLogger{}, + r: &checker.DangerousWorkflowData{ + NumWorkflows: 5, + }, + }, + want: checker.CheckResult{ + Score: checker.MaxResultScore, Reason: "no dangerous workflow patterns detected", Version: 2, Name: "DangerousWorkflow", @@ -55,6 +71,7 @@ func TestDangerousWorkflow(t *testing.T) { name: "DangerousWorkflow", dl: &scut.TestDetailLogger{}, r: &checker.DangerousWorkflowData{ + NumWorkflows: 1, Workflows: []checker.DangerousWorkflow{ { Type: checker.DangerousWorkflowUntrustedCheckout, @@ -82,6 +99,7 @@ func TestDangerousWorkflow(t *testing.T) { name: "DangerousWorkflow", dl: &scut.TestDetailLogger{}, r: &checker.DangerousWorkflowData{ + NumWorkflows: 1, Workflows: []checker.DangerousWorkflow{ { Type: checker.DangerousWorkflowScriptInjection, @@ -109,6 +127,7 @@ func TestDangerousWorkflow(t *testing.T) { name: "DangerousWorkflow", dl: &scut.TestDetailLogger{}, r: &checker.DangerousWorkflowData{ + NumWorkflows: 1, Workflows: []checker.DangerousWorkflow{ { Type: "foobar",