From 2c9a05c72115dae3b16908db9cea6a568acf9f10 Mon Sep 17 00:00:00 2001
From: laurentsimon <64505099+laurentsimon@users.noreply.github.com>
Date: Mon, 7 Jun 2021 11:01:18 -0700
Subject: [PATCH] =?UTF-8?q?=E2=9C=A8=20cleanup=20for=20token=20doc=20and?=
 =?UTF-8?q?=20code=20(#552)?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

* cleanup

* comment
---
 checks/checks.md      | 11 ++-----
 checks/checks.yaml    | 25 ++++-----------
 checks/permissions.go | 75 ++++++++++++++++++-------------------------
 3 files changed, 39 insertions(+), 72 deletions(-)

diff --git a/checks/checks.md b/checks/checks.md
index 6250e1f0..ac771e9c 100644
--- a/checks/checks.md
+++ b/checks/checks.md
@@ -128,15 +128,8 @@ This check looks for cryptographically signed tags in the last 5 tags. The check
 
 ## Token-Permissions 
 
-This check tries to determine if a project's GitHub workflows follow the principle of least privilege, i.e. if the GitHub tokens are set read-only by default. The check currently checks that the 'permission' keyword is used and set to read/none for the 'contents' permission for every workflow yaml file. If other permissions are set globally for the entire file, this check fails. Otherwise it succeeds. 
+This check tries to determine if a project's GitHub workflows follow the principle of least privilege, i.e. if the GitHub tokens are set read-only by default. For each workflow yaml file, the check looks for the permissions keyword. If it is set globally as read-only for the entire file,  this check succeeds. Otherwise it fails. The check cannot detect if the "read-only" GitHub permission settings is enabled, as there is no API available. 
 
 **Remediation steps**
-- Use: ``` permissions:
-  contents: read
-``` in all your .yaml files.
-- If you need more permissions, declare them in the job itself, e.g. ``` jobs: create_commit:
-  runs-on: ubuntu-latest 
-  permissions:
-    issues: write 
-```
+- Set permissions as `read-all` or `contents: read` as described in GitHub's [documentation](https://docs.github.com/en/actions/reference/workflow-syntax-for-github-actions#permissions).
 
diff --git a/checks/checks.yaml b/checks/checks.yaml
index 57137d0f..a364221b 100644
--- a/checks/checks.yaml
+++ b/checks/checks.yaml
@@ -19,27 +19,14 @@ checks:
     description: >-
       This check tries to determine if a project's GitHub workflows
       follow the principle of least privilege, i.e. if the GitHub tokens
-      are set read-only by default. The check currently checks that the 'permission'
-      keyword is used and set to read/none for the 'contents' permission for every workflow
-      yaml file. If other permissions are set globally for the entire file, this check fails.
-      Otherwise it succeeds.
+      are set read-only by default. For each workflow yaml file, the check looks
+      for the permissions keyword. If it is set globally as read-only for the entire file, 
+      this check succeeds. Otherwise it fails. The check cannot detect if the "read-only"
+      GitHub permission settings is enabled, as there is no API available.
     remediation:
       - >-
-        Use:
-        ```
-        permissions:
-          contents: read
-        ```
-        in all your .yaml files.
-      - >-
-        If you need more permissions, declare them in the job itself, e.g.
-        ```
-        jobs:
-        create_commit:
-          runs-on: ubuntu-latest 
-          permissions:
-            issues: write 
-        ```
+        Set permissions as `read-all` or `contents: read` as described in
+        GitHub's [documentation](https://docs.github.com/en/actions/reference/workflow-syntax-for-github-actions#permissions).
   Security-Policy:
     description: >-
       This check tries to determine if a project has published a security
diff --git a/checks/permissions.go b/checks/permissions.go
index 77a13427..838dfd0c 100644
--- a/checks/permissions.go
+++ b/checks/permissions.go
@@ -79,55 +79,42 @@ func validateMapPermissions(values map[interface{}]interface{}, path string,
 
 func validateReadPermissions(config map[interface{}]interface{}, path string,
 	logf func(s string, f ...interface{})) (bool, error) {
-	permissionFound := false
-	permissionRead := true
-	var err error
+	var permissions interface{}
 
-	// Iterate over the values.
-	for key, value := range config {
-		if key != "permissions" {
-			continue
-		}
-
-		// We have found the permissions keyword.
-		permissionFound = true
-
-		// Check the type of our values.
-		switch val := value.(type) {
-		// Empty string is nil type.
-		// It defaults to 'none'
-		case nil:
-
-		// String type.
-		case string:
-			if !strings.EqualFold(val, "read-all") && val != "" {
-				logf("!! token-permissions/github-token - permission set to '%v' in %v", val, path)
-				return false, nil
-			}
-
-		// Map type.
-		case map[interface{}]interface{}:
-			var res bool
-			if res, err = validateMapPermissions(val, path, logf); err != nil {
-				return false, err
-			}
-			if !res {
-				permissionRead = false
-			}
-
-		// Invalid type.
-		default:
-			return false, ErrInvalidGitHubWorkflowFile
-		}
-	}
-
-	// Did we find a permission at all?
-	if !permissionFound {
+	// Check if permissions are set explicitly.
+	permissions, ok := config["permissions"]
+	if !ok {
 		logf("!! token-permissions/github-token - no permission defined in %v", path)
 		return false, nil
 	}
 
-	return permissionRead, nil
+	// Check the type of our values.
+	switch val := permissions.(type) {
+	// Empty string is nil type.
+	// It defaults to 'none'
+	case nil:
+
+	// String type.
+	case string:
+		if !strings.EqualFold(val, "read-all") && val != "" {
+			logf("!! token-permissions/github-token - permission set to '%v' in %v", val, path)
+			return false, nil
+		}
+
+	// Map type.
+	case map[interface{}]interface{}:
+		if res, err := validateMapPermissions(val, path, logf); err != nil {
+			return false, err
+		} else if !res {
+			return false, nil
+		}
+
+	// Invalid type.
+	default:
+		return false, ErrInvalidGitHubWorkflowFile
+	}
+
+	return true, nil
 }
 
 // Check file content.