Token-Permissions check clarification

[varunsh-coder]

I am the founder of Step Security, a security startup to stop software supply chain attacks. Step security has built an App to automatically set the right token permissions for a given GitHub workflow.

During testing, I created PRs in several repos. Unfortunately, they are all getting a score of 0, even though they meet the least privilege requirement, since permissions are being set at the run (job) level.

Here are some examples:
https://github.com/flutter/flutter/blob/master/.github/workflows/lock.yaml
https://github.com/Azure/template-analyzer/blob/main/.github/workflows/codeql-analysis.yml
https://github.com/mgdm/htmlq/blob/master/.github/workflows/build.yml

The current requirement for the token permissions check in Scorecard is for the top (workflow) level permission to be set.

I would like the fixes done by the Step Security App to be inline with the Scorecard criteria.

Can you please clarify why the token permissions check needs the permission to be set at the top level?

Given that it is possible to write a workflow that follows the principle of least privilege by setting permissions at run (job) level, would you be willing to change the requirement to check if permissions are set at each run (job) level? Otherwise developers may need to add an extra line at the top level to meet the Scorecard criteria.

On a related note, I am planning to open source the Step Security App and create a web/ command line interface to make it easier to use. Would love to have the App be listed as a remediation option for the token permissions check. That way developers will find it easier to get a higher Scorecard score. Thanks!

[naveensrinivasan]

Given that it is possible to write a workflow that follows the principle of least privilege by setting permissions at run (job) level, would you be willing to change the requirement to check if permissions are set at each run (job) level? Otherwise developers may need to add an extra line at the top level to meet the Scorecard criteria.

I agree with this. @laurentsimon @azeemshaikh38 thoughts?

On a related note, I am planning to open source the Step Security App and create a web/ command line interface to make it easier to use. Would love to have the App be listed as a remediation option for the token permissions check. That way developers will find it easier to get a higher Scorecard score. Thanks!

That would be cool!

[laurentsimon]

Given that it is possible to write a workflow that follows the principle of least privilege by setting permissions at run (job) level, would you be willing to change the requirement to check if permissions are set at each run (job) level? Otherwise developers may need to add an extra line at the top level to meet the Scorecard criteria.

I agree with this. @laurentsimon @azeemshaikh38 thoughts?

we can definitely do that.

On a related note, I am planning to open source the Step Security App and create a web/ command line interface to make it easier to use. Would love to have the App be listed as a remediation option for the token permissions check. That way developers will find it easier to get a higher Scorecard score. Thanks!

That would be cool!

That would be awesome! Long-term, scorecard could provide a diff or new files that users can use to create PRs. A user, in this case, could even be a GitHub app, like https://github.com/ossf/allstar which could submit PRs automatically. For this, we'd need a library or a REST API. CLI/web is a great first step, though!

Do you have some idea around false negatives/positives for your app, i.e. permissions needed but not displayed by your app; and permissions not needed but that your app displays?

If your app has the right logic with no false positives/false negatives, then we could think of replacing scorecard's logic with yours, if it can work without privileged tokens like admin. Otherwise it's likely your implementation and scorecard's won't be in-sync. I expect your logic to be more advanced than ours.

Let's start with what you proposed: doc and update of our logic. Submit a PR for the doc update. I've created #1128 for tracking the relaxation of our check's logic.

Thanks so much for reaching out!

[varunsh-coder]

Given that it is possible to write a workflow that follows the principle of least privilege by setting permissions at run (job) level, would you be willing to change the requirement to check if permissions are set at each run (job) level? Otherwise developers may need to add an extra line at the top level to meet the Scorecard criteria.

I agree with this. @laurentsimon @azeemshaikh38 thoughts?

we can definitely do that.

On a related note, I am planning to open source the Step Security App and create a web/ command line interface to make it easier to use. Would love to have the App be listed as a remediation option for the token permissions check. That way developers will find it easier to get a higher Scorecard score. Thanks!

That would be cool!

That would be awesome! Long-term, scorecard could provide a diff or new files that users can use to create PRs. A user, in this case, could even be a GitHub app, like https://github.com/ossf/allstar which could submit PRs automatically. For this, we'd need a library or a REST API. CLI/web is a great first step, though!

Do you have some idea around false negatives/positives for your app, i.e. permissions needed but not displayed by your app; and permissions not needed but that your app displays?

The app cannot calculate the minimum permissions for all cases. It is essentially based on a knowledge base of permissions needed by each action. So, it is not perfect, but is fairly accurate, and as the knowledge base grows, it gets better at coverage. I will share preview of the website in few days. It is backed by an API...

If your app has the right logic with no false positives/false negatives, then we could think of replacing scorecard's logic with yours, if it can work without privileged tokens like admin. Otherwise it's likely your implementation and scorecard's won't be in-sync. I expect your logic to be more advanced than ours.

Let's start with what you proposed: doc and update of our logic. Submit a PR for the doc update. I've created #1128 for tracking the relaxation of our check's logic.

I just submitted a PR for the doc/ requirement update. #1130. Do share your feedback.

Thanks so much for reaching out!

Thanks for the quick response on this. Looking forward to collaborating in the future!

[azeemshaikh38]

Thanks @varunsh-coder for reaching out. As @laurentsimon and @naveensrinivasan have already mentioned, we (Scorecard) needs to update the way Token-Permission checks for tokens. Also, very happy to collaborate with Step Security.

[laurentsimon]

I've thought a bit more about this. I agree that it is possible to write a workflow that follows the principle of least privilege by setting permissions at run (job) level. However, we would not be doing users a favor if we allowed them to not set permissions at the top level. By default, a workflow should have the least amount of privileges; and only if more are needed should the job add write permissions. It's too easy for a developer to copy/paste code from an example workflow and forget to set the permission in the job: this was the original intention behind having a permission defined at top level. The check currently supports the job-level permissions, but still requires an explicit permission at the top level for the reason above.
Based on this, I think the Step Security app should always set permissions: read-all at the top level.

[varunsh-coder]

Hi @laurentsimon I could try this out and get some user feedback. Based on the PRs I submitted on various repos, I found developers to be:

1. Either very security focussed (this PR), where they were reviewing each permission added to each job - in this case I did not observe the pattern of read-all at top level, OR
2. Not aware of the GitHub token (this PR and this PR), and closed the PR - in this case I expect developers might remove the read-all permission as soon as a job breaks due to insufficient permissions OR
3. Many who were aware of it and accepted the PR but did not ask for read-all to be added at the top.

Another thing to consider is how GitHub itself adds permissions to default workflows, e.g. I recently added a CodeQL workflow via the GitHub UI and it did not have read-all at the top, but had permissions at the job level.

I think this decision is not an urgent one, and as we observe more best practices for token permissions and get feedback, it can help shape the decision.

BTW, the Step Security web UI for adding permissions to workflows is available here. I will start to pilot it with few users. Feel free to give it a try. I am also working on an agent for GitHub hosted runner (being piloted here and here). Would love to pilot it on Scorecards repository so I can get feedback from you. Let me know if that is ok.

[laurentsimon]

Hi @laurentsimon I could try this out and get some user feedback. Based on the PRs I submitted on various repos, I found developers to be:

1. Either very security focussed (this PR), where they were reviewing each permission added to each job - in this case I did not observe the pattern of read-all at top level, OR
2. Not aware of the GitHub token (this PR and this PR), and closed the PR - in this case I expect developers might remove the read-all permission as soon as a job breaks due to insufficient permissions OR
3. Many who were aware of it and accepted the PR but did not ask for read-all to be added at the top.

Another thing to consider is how GitHub itself adds permissions to default workflows, e.g. I recently added a CodeQL workflow via the GitHub UI and it did not have read-all at the top, but had permissions at the job level.

valid point. GitHub does ask users to define global permissions for workflows, but currently only suggest the settings (https://github.blog/changelog/2021-04-20-github-actions-control-permissions-for-github_token/). So they agree that it should fail safe. I would love for them to tell users to use a config file instead of the setting.

I think this decision is not an urgent one, and as we observe more best practices for token permissions and get feedback, it can help shape the decision.

how about the following:

1. We accept workflows that have no top-level definitions but have job-level permissions: we give them a high score, say 8 or 9.
2. For those who go the extra mile and define a top level read-all or none, we give them a score of 10. This encourages developers to define a default fail-safe permission. I could imagine a situation where this backfires and users add a new job and update only the top-level definition instead of the job one. We could enforce that top-levels is only read-all or none, and that write permissions only occur in job levels.

BTW, the Step Security web UI for adding permissions to workflows is available here. I will start to pilot it with few users. Feel free to give it a try. I am also working on an agent for GitHub hosted runner (being piloted here and here). Would love to pilot it on Scorecards repository so I can get feedback from you. Let me know if that is ok.

This is great, thank you for sharing!

cc @josepalafox

[varunsh-coder]

Hi @laurentsimon

I believe your concern is around the scenario where a new job is added to an existing workflow with no top-level permissions, where all existing jobs had minimum job-level permissions, and that new job does not have permissions defined.

In this case if Scorecards had influenced the repo owner to add permissions for the rest of the jobs in the first place, it is likely the developer would run Scorecards again and get a lower score, and this will cause them to address permissions for the new job.

I think what is going to influence developers to fix their permissions is not the top-level fail-safe permission, because they can always remove it, and will likely do so if their new job fails due to lack of permissions. What is going to influence them is an external factor, which is to get a good score from Scorecards.

So, I think you don't have to worry about your concern. If each job has min permissions or top level has min permissions, the workflow should get a score of 10. If not, it should get a lower score. This lower score will cause the developers to fix the permissions either on top level or job level.

This approach also gives developers flexibility on how to address the issue - either at top level or job level - since both are technically valid options.

My thinking would have been different if the top-level permission were like a policy that is controlled by the admin, but that is not the case. It is just text in the workflow file.

[laurentsimon]

Thanks for sharing your thoughts, @varunsh-coder

I believe your concern is around the scenario where a new job is added to an existing workflow with no top-level permissions, where all existing jobs had minimum job-level permissions, and that new job does not have permissions defined.

correct.

In this case if Scorecards had influenced the repo owner to add permissions for the rest of the jobs in the first place, it is likely the developer would run Scorecards again and get a lower score, and this will cause them to address permissions for the new job.

correct. it's this risky time window that the top-level permission mitigates. We don't how often users will run scorecard. If they run it on all pull requests (we're hoping some will be able to do that with the GitHub action we're writing), then a missing permission would be caught right away. And I'd 100% agree with you that the top-level permission is redundant. But that may not be the case for everyone.

I think what is going to influence developers to fix their permissions is not the top-level fail-safe permission, because they can always remove it, and will likely do so if their new job fails due to lack of permissions. What is going to influence them is an external factor, which is to get a good score from Scorecards.

correct. That's why I was suggesting we can influence developers to add the top-level read-all/none permission by giving them a high score of 9, but not the 10. Most jobs only need read permissions, so there's a good chance developers won't have their workflow break. In the case where it breaks their workflow, the situation would be similar to the "job-only definition is enough" route.

[varunsh-coder]

Ok, that's fair. I will take an action item to add top-level permissions for the Step Security feature to auto-add permissions.

I can pilot the GitHub action for Scorecards, let me know if it is in a state to pilot it. It might also help to have a badge for Scorecard that developers can add to their repos - I don't know if this is already in the plans.

[laurentsimon]

Ok, that's fair. I will take an action item to add top-level permissions for the Step Security feature to auto-add permissions.

awesome. I really appreciate that you forced me to explain my thinking. If you have further thoughts, please don't hesitate to share them.

I can pilot the GitHub action for Scorecards, let me know if it is in a state to pilot it.

that's great. What exactly do you mean by piloting?
Now that we've reached consensus, we can start updating the code of the check.
cc @naveensrinivasan are you still interested in tackling this issue? (I recall from the meeting that you were interested)

It might also help to have a badge for Scorecard that developers can add to their repos - I don't know if this is already in the plans.

It's in the plans for next year, yes. We'll share a design via the ossf-scorecard-dev mailing list and in an issue once we have the design better thought of. If you have particular ideas about it, feel free to share them in the

[varunsh-coder]

By piloting, I meant I could try out the GitHub Action for Scorecards when it is ready. You had mentioned it as a way to run Scorecards on Pull request and I was looking for a way to do that. My comment on piloting was not related to this particular check/ issue...

[laurentsimon]

sounds good. We have a working prototype: follow the steps in #1074 (comment)

We will be changing the repo name of the GitHub action to comply with GitHub marketplace convention in 3 weeks or so from now. At this point it will become obvious that the change happens because the runs will fail. Ping me on this thread and I'll tell you the new name. The change should be reflected in this line https://github.com/ossf/scorecard/blob/main/.github/workflows/scorecard-analysis.yml#L25 anyway.

FYI, I'll be OOO from Nov 23rd to Dec 1 included.

[evverx]

FWIW top level permissions aren't redundant at all as it was seemingly implied in this thread. They protect forks where the "token permissions" setting has the default value. For example, the systemd repository set the global setting to "read-only" and ships GH Actions with "permissions: contents: read" at the top level to prevent its forks from running into any issues just because obviously nobody can change all the forks (apart from GitHub but it's unlikely they're going to do that mostly for compatibility reasons)

[varunsh-coder]

sounds good. We have a working prototype: follow the steps in #1074 (comment)

We will be changing the repo name of the GitHub action to comply with GitHub marketplace convention in 3 weeks or so from now. At this point it will become obvious that the change happens because the runs will fail. Ping me on this thread and I'll tell you the new name. The change should be reflected in this line https://github.com/ossf/scorecard/blob/main/.github/workflows/scorecard-analysis.yml#L25 anyway.

FYI, I'll be OOO from Nov 23rd to Dec 1 included.

I tried the Scorecards GitHub Action here. For some reason, GitHub does not render the SARIF results in the Code scanning alerts section. I was able to download and view the SARIF results from workflow artifacts. Results looked as expected for the checks it did show. Do have a look as I do not have knowledge of all checks that Scorecards does...

[varunsh-coder]

FWIW top level permissions aren't redundant at all as it was seemingly implied in this thread. They protect forks where the "token permissions" setting has the default value. For example, the systemd repository set the global setting to "read-only" and ships GH Actions with "permissions: contents: read" at the top level to prevent its forks from running into any issues just because obviously nobody can change all the forks (apart from GitHub but it's unlikely they're going to do that mostly for compatibility reasons)

Hi @evverx Sorry I could not understand your comment. Are you saying setting at top level is better relative to job level in case someone forks the repo? Did not understand the threat scenario...

[laurentsimon]

FWIW top level permissions aren't redundant at all as it was seemingly implied in this thread. They protect forks where the "token permissions" setting has the default value. For example, the systemd repository set the global setting to "read-only" and ships GH Actions with "permissions: contents: read" at the top level to prevent its forks from running into any issues just because obviously nobody can change all the forks (apart from GitHub but it's unlikely they're going to do that mostly for compatibility reasons)

Hi @evverx Sorry I could not understand your comment. Are you saying setting at top level is better relative to job level in case someone forks the repo? Did not understand the threat scenario...

I think what @evverx means is that if a repo has GitHub settings set to "workflows are by default using read-only"; then someone clones it but does not have the same settings set, the permissions will default to write for the clone repo. Is that correct?

[laurentsimon]

sounds good. We have a working prototype: follow the steps in #1074 (comment)
We will be changing the repo name of the GitHub action to comply with GitHub marketplace convention in 3 weeks or so from now. At this point it will become obvious that the change happens because the runs will fail. Ping me on this thread and I'll tell you the new name. The change should be reflected in this line https://github.com/ossf/scorecard/blob/main/.github/workflows/scorecard-analysis.yml#L25 anyway.
FYI, I'll be OOO from Nov 23rd to Dec 1 included.

I tried the Scorecards GitHub Action here. For some reason, GitHub does not render the SARIF results in the Code scanning alerts section. I was able to download and view the SARIF results from workflow artifacts. Results looked as expected for the checks it did show. Do have a look as I do not have knowledge of all checks that Scorecards does...

If I'm not mistaken, you ran scorecard only on PRs and not on push/cron events. GitHub will only add results to the scanning dashboard for the push to a branch. I think you may be able to see the results if you search for the PR branch in the dashboard, but they are not shown by default because it's not the default branch. Can you try run scorecard on a push?

[varunsh-coder]

Ran it on push and see the results in the code scanning dashboard now. Not sure if you will have access to view the results though...It will take me some time to review each result for accuracy. Will spend time later today on it.

[varunsh-coder]

I did a quick analysis and have posted results here. Feel free to comment on that issue to discuss the results.

W.r.t token permissions, I am curious what the code scanning alert/ SARIF output would be after the change is made to the logic as discussed in this thread. Let us say a workflow does not have top level permissions, but each job has permissions, will it create a code scanning alert? If so what severity? Or will it not create an alert, but give a score of 9.

[laurentsimon]

That's a really good question. Today, we use the risk level from the https://github.com/ossf/scorecard/blob/main/docs/checks/internal/checks.yaml#L589

In this case, it would be tagged as High. You're right that's not ideal. We don't have the ability to change the risk level based on score. For now, I think a developer would need to set the threshold score as 9 fo the Token-Permissions check in the policy file https://github.com/ossf/scorecard/blob/main/.github/scorecard-policy.yml#L18 to get rid of the result, or click "Won't Fix". I agree it's not ideal and we should be able to update the risk based on score. Could you create a tracking issue for it?

We originally did not implement this level of granularity to keep things simple, but you're right it's going to be needed.

I'm not sure we'll be able to implement this for the first release, but we can add it in the next iteration. Is that OK?

Thanks so much for flagging this!

[varunsh-coder]

That's a really good question. Today, we use the risk level from the https://github.com/ossf/scorecard/blob/main/docs/checks/internal/checks.yaml#L589

In this case, it would be tagged as High. You're right that's not ideal. We don't have the ability to change the risk level based on score. For now, I think a developer would need to set the threshold score as 9 fo the Token-Permissions check in the policy file https://github.com/ossf/scorecard/blob/main/.github/scorecard-policy.yml#L18 to get rid of the result, or click "Won't Fix". I agree it's not ideal and we should be able to update the risk based on score. Could you create a tracking issue for it?

We originally did not implement this level of granularity to keep things simple, but you're right it's going to be needed.

I'm not sure we'll be able to implement this for the first release, but we can add it in the next iteration. Is that OK?

Thanks so much for flagging this!

I have created a tracking issue for it. Feel free to update/ add comments in case my understanding/ terminology is not correct. Thanks!