✨ Token-Permissions, Allow contents: write permission only for jobs…

… that are releasing (#1663) * Token-Permissions, distinguish contents/package Allowing `contents: write` permission only for jobs that are releasing jobs, not just packaging jobs.
ossf · Feb 23, 2022 · 808941a · 808941a
1 parent e41f859
commit 808941a
Show file tree

Hide file tree

Showing 6 changed files with 113 additions and 38 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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/...-workflow-permissions-release-writes.yaml → ...rmissions-contents-writes-no-release.yaml b/...-workflow-permissions-release-writes.yaml → ...rmissions-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
@@ -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 }}