From 808941a4c249a75de4c5911174795d5a7ad74a3a Mon Sep 17 00:00:00 2001 From: Chris McGehee Date: Tue, 22 Feb 2022 16:23:07 -0800 Subject: [PATCH] =?UTF-8?q?=E2=9C=A8=20Token-Permissions,=20Allow=20`conte?= =?UTF-8?q?nts:=20write`=20permission=20only=20for=20jobs=20that=20are=20r?= =?UTF-8?q?eleasing=20(#1663)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Token-Permissions, distinguish contents/package Allowing `contents: write` permission only for jobs that are releasing jobs, not just packaging jobs. --- checks/fileparser/github_workflow.go | 32 +++++++++++++++- checks/packaging.go | 26 +------------ checks/permissions.go | 37 ++++++++++++++++--- checks/permissions_test.go | 23 +++++++++--- ...rmissions-contents-writes-no-release.yaml} | 2 +- ...w-permissions-contents-writes-release.yaml | 31 ++++++++++++++++ 6 files changed, 113 insertions(+), 38 deletions(-) rename checks/testdata/.github/workflows/{github-workflow-permissions-release-writes.yaml => github-workflow-permissions-contents-writes-no-release.yaml} (97%) create mode 100644 checks/testdata/.github/workflows/github-workflow-permissions-contents-writes-release.yaml diff --git a/checks/fileparser/github_workflow.go b/checks/fileparser/github_workflow.go index 915f3ac0..9c4a42b5 100644 --- a/checks/fileparser/github_workflow.go +++ b/checks/fileparser/github_workflow.go @@ -330,8 +330,36 @@ type JobMatcherStep struct { Run string } -// Matches returns true if the job matches the job matcher. -func (m *JobMatcher) Matches(job *actionlint.Job) bool { +// AnyJobsMatch returns true if any of the jobs have a match in the given workflow. +func AnyJobsMatch(workflow *actionlint.Workflow, jobMatchers []JobMatcher, fp string, dl checker.DetailLogger, + logMsgNoMatch string) bool { + for _, job := range workflow.Jobs { + for _, matcher := range jobMatchers { + if !matcher.matches(job) { + continue + } + + dl.Info(&checker.LogMessage{ + Path: fp, + Type: checker.FileTypeSource, + Offset: GetLineNumber(job.Pos), + Text: matcher.LogText, + }) + return true + } + } + + dl.Debug(&checker.LogMessage{ + Path: fp, + Type: checker.FileTypeSource, + Offset: checker.OffsetDefault, + Text: logMsgNoMatch, + }) + return false +} + +// matches returns true if the job matches the job matcher. +func (m *JobMatcher) matches(job *actionlint.Job) bool { for _, stepToMatch := range m.Steps { hasMatch := false for _, step := range job.Steps { diff --git a/checks/packaging.go b/checks/packaging.go index 8b5cdda3..aa0f752e 100644 --- a/checks/packaging.go +++ b/checks/packaging.go @@ -187,7 +187,7 @@ func isPackagingWorkflow(workflow *actionlint.Workflow, fp string, dl checker.De Uses: "relekang/python-semantic-release", }, }, - LogText: "candidate python publishing workflow using pypi", + LogText: "candidate python publishing workflow using python-semantic-release", }, { // Go packages. @@ -212,27 +212,5 @@ func isPackagingWorkflow(workflow *actionlint.Workflow, fp string, dl checker.De }, } - for _, job := range workflow.Jobs { - for _, matcher := range jobMatchers { - if !matcher.Matches(job) { - continue - } - - dl.Info(&checker.LogMessage{ - Path: fp, - Type: checker.FileTypeSource, - Offset: fileparser.GetLineNumber(job.Pos), - Text: matcher.LogText, - }) - return true - } - } - - dl.Debug(&checker.LogMessage{ - Path: fp, - Type: checker.FileTypeSource, - Offset: checker.OffsetDefault, - Text: "not a publishing workflow", - }) - return false + return fileparser.AnyJobsMatch(workflow, jobMatchers, fp, dl, "not a publishing workflow") } diff --git a/checks/permissions.go b/checks/permissions.go index cce18019..cc9a033f 100644 --- a/checks/permissions.go +++ b/checks/permissions.go @@ -547,10 +547,37 @@ func requiresPackagesPermissions(workflow *actionlint.Workflow, fp string, dl ch return isPackagingWorkflow(workflow, fp, dl) } -// Note: this needs to be improved. -// Currently we don't differentiate between publishing on GitHub vs -// pubishing on registries. In terms of risk, both are similar, as -// an attacker would gain the ability to push a package. +// requiresContentsPermissions returns true if the workflow requires the `contents: write` permission. func requiresContentsPermissions(workflow *actionlint.Workflow, fp string, dl checker.DetailLogger) bool { - return requiresPackagesPermissions(workflow, fp, dl) + return isReleasingWorkflow(workflow, fp, dl) +} + +// isReleasingWorkflow returns true if the workflow involves creating a release on GitHub. +func isReleasingWorkflow(workflow *actionlint.Workflow, fp string, dl checker.DetailLogger) bool { + jobMatchers := []fileparser.JobMatcher{ + { + // Python packages. + // This is a custom Python packaging/releasing workflow based on semantic versioning. + Steps: []*fileparser.JobMatcherStep{ + { + Uses: "relekang/python-semantic-release", + }, + }, + LogText: "candidate python publishing workflow using python-semantic-release", + }, + { + // Go packages. + Steps: []*fileparser.JobMatcherStep{ + { + Uses: "actions/setup-go", + }, + { + Uses: "goreleaser/goreleaser-action", + }, + }, + LogText: "candidate golang publishing workflow", + }, + } + + return fileparser.AnyJobsMatch(workflow, jobMatchers, fp, dl, "not a releasing workflow") } diff --git a/checks/permissions_test.go b/checks/permissions_test.go index 1b0342c2..9fb57fc4 100644 --- a/checks/permissions_test.go +++ b/checks/permissions_test.go @@ -94,8 +94,8 @@ func TestGithubTokenPermissions(t *testing.T) { Error: nil, Score: checker.MaxResultScore, NumberOfWarn: 0, - NumberOfInfo: 3, - NumberOfDebug: 3, + NumberOfInfo: 2, + NumberOfDebug: 4, }, }, { @@ -264,8 +264,19 @@ func TestGithubTokenPermissions(t *testing.T) { }, }, { - name: "release workflow write", - filenames: []string{"./testdata/.github/workflows/github-workflow-permissions-release-writes.yaml"}, + name: "package workflow contents write", + filenames: []string{"./testdata/.github/workflows/github-workflow-permissions-contents-writes-no-release.yaml"}, + expected: scut.TestReturn{ + Error: nil, + Score: checker.MinResultScore, + NumberOfWarn: 1, + NumberOfInfo: 2, + NumberOfDebug: 3, + }, + }, + { + name: "release workflow contents write", + filenames: []string{"./testdata/.github/workflows/github-workflow-permissions-contents-writes-release.yaml"}, expected: scut.TestReturn{ Error: nil, Score: checker.MaxResultScore, @@ -281,8 +292,8 @@ func TestGithubTokenPermissions(t *testing.T) { Error: nil, Score: checker.MaxResultScore, NumberOfWarn: 0, - NumberOfInfo: 3, - NumberOfDebug: 3, + NumberOfInfo: 2, + NumberOfDebug: 4, }, }, { diff --git a/checks/testdata/.github/workflows/github-workflow-permissions-release-writes.yaml b/checks/testdata/.github/workflows/github-workflow-permissions-contents-writes-no-release.yaml similarity index 97% rename from checks/testdata/.github/workflows/github-workflow-permissions-release-writes.yaml rename to checks/testdata/.github/workflows/github-workflow-permissions-contents-writes-no-release.yaml index 09304dcb..3d6c2845 100644 --- a/checks/testdata/.github/workflows/github-workflow-permissions-release-writes.yaml +++ b/checks/testdata/.github/workflows/github-workflow-permissions-contents-writes-no-release.yaml @@ -11,7 +11,7 @@ # 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. -name: release workflow +name: publish workflow on: [push] permissions: diff --git a/checks/testdata/.github/workflows/github-workflow-permissions-contents-writes-release.yaml b/checks/testdata/.github/workflows/github-workflow-permissions-contents-writes-release.yaml new file mode 100644 index 00000000..cc1c7c55 --- /dev/null +++ b/checks/testdata/.github/workflows/github-workflow-permissions-contents-writes-release.yaml @@ -0,0 +1,31 @@ +# Copyright 2022 Security 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. +name: release workflow +on: [push] +permissions: + +jobs: + Explore-GitHub-Actions: + runs-on: ubuntu-latest + permissions: + contents: write + steps: + - name: setup + uses: actions/setup-go@ + - name: release + uses: goreleaser/goreleaser-action@ + with: + version: latest + env: + GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}