From 74d6973db94810345644a7c55093b552a5fe669b Mon Sep 17 00:00:00 2001 From: raghavkaul <8695110+raghavkaul@users.noreply.github.com> Date: Fri, 21 Apr 2023 14:58:42 -0400 Subject: [PATCH] =?UTF-8?q?=E2=9C=A8=20GitLab:=20Documentation=20and=20cle?= =?UTF-8?q?aner=20errors=20(#2821)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Return inconclusive if there are no workflows Signed-off-by: Raghav Kaul * Return inconclusive if we don't have any workflows Signed-off-by: Raghav Kaul * logging fixes Signed-off-by: Raghav Kaul * fix panic Signed-off-by: Raghav Kaul * Update README.md Signed-off-by: Raghav Kaul * skip error when getting external status checks (requires full api access) Signed-off-by: Raghav Kaul * update Signed-off-by: Raghav Kaul * fix dangerous workflow test Signed-off-by: Raghav Kaul --------- Signed-off-by: Raghav Kaul Signed-off-by: Avishay --- README.md | 42 ++++++++++---------- checker/raw_result.go | 4 +- checks/evaluation/contributors.go | 2 +- checks/evaluation/dangerous_workflow.go | 4 ++ checks/evaluation/dangerous_workflow_test.go | 21 +++++++++- checks/evaluation/permissions/permissions.go | 4 ++ checks/permissions_test.go | 6 +-- checks/raw/dangerous_workflow.go | 2 + checks/raw/permissions.go | 2 + clients/gitlabrepo/branches.go | 3 +- clients/gitlabrepo/releases.go | 5 ++- clients/gitlabrepo/tarball.go | 2 - 12 files changed, 66 insertions(+), 31 deletions(-) diff --git a/README.md b/README.md index ad0b17243d1b..83304146402e 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 diff --git a/checker/raw_result.go b/checker/raw_result.go index 318a039a997c..8b991d184071 100644 --- a/checker/raw_result.go +++ b/checker/raw_result.go @@ -315,7 +315,8 @@ const ( // DangerousWorkflowData contains raw results // for dangerous workflow check. type DangerousWorkflowData struct { - Workflows []DangerousWorkflow + Workflows []DangerousWorkflow + NumWorkflows int } // DangerousWorkflow represents a dangerous workflow. @@ -334,6 +335,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/contributors.go b/checks/evaluation/contributors.go index 1425f608d964..d419e6a1ea4b 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/checks/evaluation/dangerous_workflow.go b/checks/evaluation/dangerous_workflow.go index be37c6cdee5d..9aa76fe751cf 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/evaluation/dangerous_workflow_test.go b/checks/evaluation/dangerous_workflow_test.go index 7726f33a64bc..ae3eef259913 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", diff --git a/checks/evaluation/permissions/permissions.go b/checks/evaluation/permissions/permissions.go index fed2c092bb82..a661c372dae5 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/permissions_test.go b/checks/permissions_test.go index 23a4b83bbd5f..066ed36f8187 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, }, }, @@ -386,7 +386,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/checks/raw/dangerous_workflow.go b/checks/raw/dangerous_workflow.go index 7db760e715c4..1da80f0e2eb0 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) diff --git a/checks/raw/permissions.go b/checks/raw/permissions.go index c2004ec89cd6..8f791b2b9c6e 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) diff --git a/clients/gitlabrepo/branches.go b/clients/gitlabrepo/branches.go index 79aeaafbb2a8..50949f14474f 100644 --- a/clients/gitlabrepo/branches.go +++ b/clients/gitlabrepo/branches.go @@ -71,7 +71,8 @@ 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 } diff --git a/clients/gitlabrepo/releases.go b/clients/gitlabrepo/releases.go index 6aca7adbc249..140aa701e383 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, diff --git a/clients/gitlabrepo/tarball.go b/clients/gitlabrepo/tarball.go index 4646f192de11..02a40d46355f 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)