✨ Structured results for permissions

[laurentsimon]

This PR is the first of a series to implement the structured results. It starts with the permission check.

This PR is very large because it includes not just the permission check's changes, but also all the needed changes to incorporate the structured results. Fortunately, the majority of files changes are small fixes to change package names.

The main changes to review:

1. rule/ package. This takes a directory with a list of *.yml files. Each yml file contains the information for a rule.
2. A check is composed of multiple rules
3. finding is a finding. It uses the rule to create a finding, and additional information such as a location, snippet, etc. provided by a check.This is eventually displayed to users
4. pkg is updated to use the structured results
5. A new format extended-json for the --format option is used. The SCORECARD_EXPERIMENTAL=1 must be set to enable it.

An example result looks like the following:
SCORECARD_EXPERIMENTAL=1 go run . --repo=GoogleCloudPlatform/rad-lab --format extended-json --show-details --checks Token-Permissions | jq

{
  "date": "2023-01-06",
  "repo": {
    "name": "github.com/GoogleCloudPlatform/rad-lab",
    "commit": "4a94eb550a8fee9fd2e57a0ddaa70ded65903acb"
  },
  "scorecard": {
    "version": "(devel)",
    "commit": "unknown"
  },
  "score": 0,
  "checks": [
    {
      "risk": "High",
      "outcome": "Negative",
      "score": 0,
      "reason": "non read-only tokens detected in GitHub workflows",
      "name": "Token-Permissions",
      "documentation": {
        "url": "https://github.com/ossf/scorecard/blob/main/docs/checks.md#token-permissions",
        "short": "Determines if the project's workflows follow the principle of least privilege."
      }
      "findings": [
        {
          "rule": "GitHubWorkflowPermissionsTopNoWrite",
          "outcome": "Negative",
          "risk": "High",
          "message": "topLevel 'contents' permission set to 'write'",
          "location": {
            "type": 1,
            "value": ".github/workflows/build-module-readme.yml",
            "lineStart": 28,
            "snippet": "write"
          },
          "remediation": {
            "text": "Update your workflow using https://app.stepsecurity.io/secureworkflow/GoogleCloudPlatform/rad-lab/build-module-readme.yml/main?enable=permissions",
            "markdown": "Update your workflow using [https://app.stepsecurity.io/secureworkflow](https://app.stepsecurity.io/secureworkflow/GoogleCloudPlatform/rad-lab/build-module-readme.yml/main?enable=permissions).",
            "effort": "Low"
          }
        },
        {
          "rule": "GitHubWorkflowPermissionsTopNoWrite",
          "outcome": "Positive",
          "risk": "High",
          "message": "topLevel 'contents' permission set to 'read'",
          "location": {
            "type": 1,
            "value": ".github/workflows/check-tf-plan.yml",
            "lineStart": 30
          }
        },
...
    

There are several TODOS in the code which I will create a tracking issue for once the review is complete.

Structured results for permissions

[spencerschrock]

Still need to go through this, but WDUT about using the new extended-json format to implement #2577. Rules could also have numeric IDs?

A new results format would be a good time to add it since backwards compatibility isn't a concern.

[laurentsimon]

Still need to go through this, but WDUT about using the new extended-json format to implement #2577. Rules could also have numeric IDs?

A new results format would be a good time to add it since backwards compatibility isn't a concern.

The rule names are essentially ruleID that are more explicit than numbers. I can definitely add a number, but we have to be sure we don't get collision, ie we need some proper pre-submit to verify this over time. (at least in the current implementation because rule files are stored in a folder named after the check, here permissions/*.yml). If we want to stored all files i a single folder, it becomes easier. I've kept the rules in different folders to simplify reviewing which rules are used in which check. I certainly need some feedback about this decision...

[joycebrum]

Regarding the

...
"remediation": {
  "text": "Update your workflow using https://app.stepsecurity.io/secureworkflow/GoogleCloudPlatform/rad-lab/build-module-readme.yml/main?enable=permissions",
  "markdown": "Update your workflow using [https://app.stepsecurity.io/secureworkflow](https://app.stepsecurity.io/secureworkflow/GoogleCloudPlatform/rad-lab/build-module-readme.yml/main?enable=permissions).",
  "effort": "Low"
}
...

Is it possible to also suggest a direct remediation instruction? It has happened that a maintainer didn't like the tool and got confused about which options to enable to fix the check and though the remediation was also saying to enable Harden Runner which he didn't seem to want.

Maybe with a remediation like "Update your workflow by adding contents: read permission or using [...]", we would not rely fully on the step security tool and the maintainer can rather fix himself.

In this case, for example, the https://app.stepsecurity.io/secureworkflow/GoogleCloudPlatform/rad-lab/build-module-readme.yml/main?enable=permissions does not fix the contents: write permission to contents: read, so the maintainer could ended up confused on what the check would be expecting from him since the stepsecurity itself didn't change anything.

In the print above I've selected the pin dependencies just to show that the changes should be applied.

[laurentsimon]

Regarding the

...
"remediation": {
  "text": "Update your workflow using https://app.stepsecurity.io/secureworkflow/GoogleCloudPlatform/rad-lab/build-module-readme.yml/main?enable=permissions",
  "markdown": "Update your workflow using [https://app.stepsecurity.io/secureworkflow](https://app.stepsecurity.io/secureworkflow/GoogleCloudPlatform/rad-lab/build-module-readme.yml/main?enable=permissions).",
  "effort": "Low"
}
...

Is it possible to also suggest a direct remediation instruction? It has happened that a maintainer didn't like the tool and got confused about which options to enable to fix the check and though the remediation was also saying to enable Harden Runner which he didn't seem to want.

Maybe with a remediation like "Update your workflow by adding contents: read permission or using [...]", we would not rely fully on the step security tool and the maintainer can rather fix himself.

In this case, for example, the https://app.stepsecurity.io/secureworkflow/GoogleCloudPlatform/rad-lab/build-module-readme.yml/main?enable=permissions does not fix the contents: write permission to contents: read, so the maintainer could ended up confused on what the check would be expecting from him since the stepsecurity itself didn't change anything.

In the print above I've selected the pin dependencies just to show that the changes should be applied.

Thanks for the feedback. One issue of asking users to fix the top-level themselves is that some of the steps may require write permissions, so their workflow would break after an update. If I update the remediation to explicitly ask to users to untick harden runner and pinned deps, would it be OK?

[laurentsimon]

qq:

1. do we want to keep the score for this format?
2. do we want to "flatten" the results and remove the "checks": and instead only provide a list of findings: []finding? If so, we may still need to check field in the findings to indicate to users which check the result belongs to

[laurentsimon]

Regarding the

...
"remediation": {
  "text": "Update your workflow using https://app.stepsecurity.io/secureworkflow/GoogleCloudPlatform/rad-lab/build-module-readme.yml/main?enable=permissions",
  "markdown": "Update your workflow using [https://app.stepsecurity.io/secureworkflow](https://app.stepsecurity.io/secureworkflow/GoogleCloudPlatform/rad-lab/build-module-readme.yml/main?enable=permissions).",
  "effort": "Low"
}
...

Is it possible to also suggest a direct remediation instruction? It has happened that a maintainer didn't like the tool and got confused about which options to enable to fix the check and though the remediation was also saying to enable Harden Runner which he didn't seem to want.
Maybe with a remediation like "Update your workflow by adding contents: read permission or using [...]", we would not rely fully on the step security tool and the maintainer can rather fix himself.
In this case, for example, the https://app.stepsecurity.io/secureworkflow/GoogleCloudPlatform/rad-lab/build-module-readme.yml/main?enable=permissions does not fix the contents: write permission to contents: read, so the maintainer could ended up confused on what the check would be expecting from him since the stepsecurity itself didn't change anything.

In the print above I've selected the pin dependencies just to show that the changes should be applied.

Thanks for the feedback. One issue of asking users to fix the top-level themselves is that some of the steps may require write permissions, so their workflow would break after an update. If I update the remediation to explicitly ask to users to untick harden runner and pinned deps, would it be OK?

@varunsh-coder there seems to be a bug in the webpage: the contents: write is not removed? Is it because you don't know which permissions the Actions used in steps require?

[joycebrum]

Thanks for the feedback. One issue of asking users to fix the top-level themselves is that some of the steps may require write permissions, so their workflow would break after an update. If I update the remediation to explicitly ask to users to untick harden runner and pinned deps, would it be OK?

I think it would still be fully dependent of the tool, but it would be much better because it will be clear to the user what he was supposed to use in the tool to fix the check. Something like

Update your workflow using the "Restrict permissions for GITHUB_TOKEN" at <link>

Or maybe adding the "fix it yourself as a turn around" with something like:

Update your workflow using <link> or set the permission default to read (`contents: read`) giving write permissions only to jobs

Both options I think would be great.

[laurentsimon]

Thanks for the feedback. One issue of asking users to fix the top-level themselves is that some of the steps may require write permissions, so their workflow would break after an update. If I update the remediation to explicitly ask to users to untick harden runner and pinned deps, would it be OK?

I think it would still be fully dependent of the tool, but it would be much better because it will be clear to the user what he was supposed to use in the tool to fix the check. Something like

Update your workflow using the "Restrict permissions for GITHUB_TOKEN" at <link>

Or maybe adding the "fix it yourself as a turn around" with something like:

Update your workflow using <link> or set the permission default to read (`contents: read`) giving write permissions only to jobs

Both options I think would be great.

That's very helpful, thanks. I realize I did not explain well. Can you take a look at the changes I made? You can edit the PR yourself as well by using the branch on my repo, so feel free to make additional changes you think would be meaningful. Let me know once you've made the changes. Thanks!

[varunsh-coder]

Regarding the

...
"remediation": {
  "text": "Update your workflow using https://app.stepsecurity.io/secureworkflow/GoogleCloudPlatform/rad-lab/build-module-readme.yml/main?enable=permissions",
  "markdown": "Update your workflow using [https://app.stepsecurity.io/secureworkflow](https://app.stepsecurity.io/secureworkflow/GoogleCloudPlatform/rad-lab/build-module-readme.yml/main?enable=permissions).",
  "effort": "Low"
}
...

Is it possible to also suggest a direct remediation instruction? It has happened that a maintainer didn't like the tool and got confused about which options to enable to fix the check and though the remediation was also saying to enable Harden Runner which he didn't seem to want.
Maybe with a remediation like "Update your workflow by adding contents: read permission or using [...]", we would not rely fully on the step security tool and the maintainer can rather fix himself.
In this case, for example, the https://app.stepsecurity.io/secureworkflow/GoogleCloudPlatform/rad-lab/build-module-readme.yml/main?enable=permissions does not fix the contents: write permission to contents: read, so the maintainer could ended up confused on what the check would be expecting from him since the stepsecurity itself didn't change anything.

In the print above I've selected the pin dependencies just to show that the changes should be applied.

Thanks for the feedback. One issue of asking users to fix the top-level themselves is that some of the steps may require write permissions, so their workflow would break after an update. If I update the remediation to explicitly ask to users to untick harden runner and pinned deps, would it be OK?

@varunsh-coder there seems to be a bug in the webpage: the contents: write is not removed? Is it because you don't know which permissions the Actions used in steps require?

@laurentsimon as of now, if a workflow already has permissions defined at the top level, we do not change it. We just give a message that it already has permissions. I can take an action item to re-evaluate the permissions if they are present and suggest better permissions, if possible.

Also, regarding checkboxes, we can come up with a way to only show the option as per the query string, e.g. for token permissions, only show that and not the other boxes. That might be better than adding documentation to not select other options. We have also added more remediations via pull-request, e.g. to add dependabot config and CodeQL. Should we discuss this remediation experience in one of the upcoming Scorecard community meetings?

[joycebrum]

Some improvements/fixes to the steps write permission response.

[github-actions]

Integration tests success for
[3622896]
(https://github.com/ossf/scorecard/actions/runs/4047513898)

[github-actions]

Integration tests success for
[7fc0e6f]
(https://github.com/ossf/scorecard/actions/runs/4049926149)