Set permissions for GitHub actions

[neilnaveen]

• Included permissions for the action. https://github.com/ossf/scorecard/blob/main/docs/checks.md#token-permissions

https://docs.github.com/en/actions/using-workflows/workflow-syntax-for-github-actions#permissions

https://docs.github.com/en/actions/using-jobs/assigning-permissions-to-jobs

Keeping your GitHub Actions and workflows secure Part 1: Preventing pwn requests

Restrict the GitHub token permissions only to the required ones; this way, even if the attackers will succeed in compromising your workflow, they won’t be able to do much.

Signed-off-by: neilnaveen [email protected]

[kitbellew]

@neilnaveen I'm not an expert on this, despite trying to read to documentation links you've provided.

could you explain

• why you've changed only these workflows, and not the remaining ones (like release)?
• what will happen if write permissions are removed in each case?

cc @tgodzik

[tgodzik]

@neilnaveen I'm not an expert on this, despite trying to read to documentation links you've provided.

could you explain

• why you've changed only these workflows, and not the remaining ones (like release)?
• what will happen if write permissions are removed in each case?

cc @tgodzik

Not sure if this is needed, since we do need to approve any workflow which runs from new contributors. And besides couldn't the attacker just remove the permission setting? CI workflow runs the changed yaml configuration.

[neilnaveen]

@neilnaveen I'm not an expert on this, despite trying to read to documentation links you've provided.

could you explain

• why you've changed only these workflows, and not the remaining ones (like release)?
  those were the simple ones to do.
• what will happen if write permissions are removed in each case?

then the workflow wouldn't be able to write, and it would fail.

cc @tgodzik

[neilnaveen]

Not sure if this is needed, since we do need to approve any workflow which runs from new contributors.

It may be a contributor who has done more than one PR, and the subsequent PRs could be the attack vector .

And besides couldn't the attacker just remove the permission setting? CI workflow runs the changed yaml configuration

Even if the attacker attempts that, the PR reviewer would catch the change in the PR review.

[tgodzik]

Not sure if this is needed, since we do need to approve any workflow which runs from new contributors.

It may be a contributor who has done more than one PR, and the subsequent PRs could be the attack vector .

And besides couldn't the attacker just remove the permission setting? CI workflow runs the changed yaml configuration

Even if the attacker attempts that, the PR reviewer would catch the change in the PR review.

Wouldn't it be the same with any other change? So they can catch it if there is something funky going on? I am ok with the change, but I don't think it will improve the security that much. If someone is able to run the code actions they can run whatever they want there together with other changes.

[kitbellew]

@neilnaveen i am trying to understand several things:

• is this a safe change? (does it introduce wider permissions than what we had before? what is the default permission, i.e. if we don't specify anything?)
• is this a sufficient change? will anything break?
• why doesn't it affect the remaining workflows? (such as release.yml)

[mdedetrich]

After doing similar changes in one of my projects (actually a company project after it got reviewed by our security department) I will just throw in my 2 cents

Wouldn't it be the same with any other change? So they can catch it if there is something funky going on? I am ok with the change, but I don't think it will improve the security that much. If someone is able to run the code actions they can run whatever they want there together with other changes.

So assuming everything runs as its designed to do and there are no breaches, the changes will not improve security much however that is kind of besides the point, i.e. you should have measures incase there is some sought of breach. In such a case these changes can help, i.e. if you set a permissions: contents: read on a workflow then the volume is mounted as read only which means that its actually not possible for any code that happens to run in the repo to modify anything. If you can imagine there is some malicious code that somehow gets into one of your workflows (intentional or not, i.e. lets suppose something gets into yarn) then it reduces the attack surface.

is this a safe change? (does it introduce wider permissions than what we had before? what is the default permission, i.e. if we don't specify anything?)

So the default permissions are actually much more open then what this PR does. By default you have both write and read permissions, and so if you just restrict it to read via permissions: contents: read then you have a smaller set of permissions. On the other hand adding the permissions: contents: write makes it explicit/clear that you do actually need to write to something (which is commented on this PR) but there isn't reading that needs to be done on the mounted volume.

is this a sufficient change? will anything break?

I don't think sufficiency should be a concern here, if its not enough you can always improve it in a later PR. In terms of breaking, if something does break then its going to be due to the permissions being too strict and its fairly easy to diagnose i.e. you would get an error saying "Insufficient permissions to write to file" or something along those lines.

why doesn't it affect the remaining workflows? (such as release.yml)

Permission changes are granular, i.e. they apply per workflow or even per job. If you put the permissions block at the top of a .yaml file then it will apply for all of the jobs however if you put it in a specific job then it will only apply for that specific job.

In my opinion these changes are quite safe, you can go further but as said previously this can be improved upon in further PR's.

[tgodzik]

I think this should be ok to merge then, what do you think @kitbellew ?

[kitbellew]

I think this should be ok to merge then, what do you think @kitbellew ?

I was just looking at it. Let's do it. If it breaks, we'll ask @mdedetrich for help :)