Restrict the permissions granted to the workflows. #136
Conversation
This ensures untrusted code by external contributors cannot access highly-priviledged GitHub tokens, even when triggered by a PR comment. The touchstone actions are split into two parts, one "receive" action which does the actual benchmarking and one "comment" action that publishes the results. This is designed so that the receive may safely run untrusted code by external contributors, while running with weaker permission. Only the comment actions needs write permissions to the repository. The permission drop is normally performed automatically by GitHub when a pull_request event is triggered from a fork repository. However, when the workflow is triggered by the issue_comment, by default it runs with a permissive token, with full access to the repository, even if PR comes from a fork. Because the library encourages one to set a condition on who can trigger the workflow, the impact of this was limited. An owner/collaborator would have had to issue the benchmark command before the untrusted code can run. Nevertheless, reducing the permission set is safer as it protects the repository even if the owner issues initiates the benchmark before fully reviewing the PR code. [GitHub has actually changed the default token permission][0] for the issue_comment trigger recently, from write-all to contents read only, similar to what this change tries to accomplish. However that is a global per-repository setting that users may want to change, and it only applies to repositories that were created after Februrary 2023. Conversly, the comment workflow needs permission to set statuses and comment on issues. Whether or not the workflow has the permission by default depends on the aforementioned repo-wide setting, and thus on the date the repo was created. By being explicit about it in the workflow definition we can make sure it is reliably granted. This also reduces the extent of the write permissions the workflow could have gotten by default, which is always a good thing even if the workflow is relatively simple. [0]: https://github.blog/changelog/2023-02-02-github-actions-updating-the-default-github_token-permissions-to-read-only/
|
I've also been playing with another idea, of merging the two workflows but keeping separate jobs with different permissions: This simplifies the process a bit and makes reporting of the workflow status a bit more reliable. The same could be achieve even if one wants to run benchmarks automatically by using This veers off a bit from Github's "Preventing pwn requests" blog post, but it should be equivalent in terms of security since the permissions of the job that runs the untrusted code is locked down, as much or even more than a Let me know what you think and if this is something you think should be pursued. |
There was a problem hiding this comment.
Thanks again for the detailed description, appreciate it! IIRC at the time we wrote the workflow templates the permission node didn't exists yet. Regardless it's a needed addition!
Let me know what you think and if this is something you think should be pursued.
I'll have to think about it in detail but I don't think it's a great idea as it's a bad pattern that people might use in another context without being aware of the security implications (which is probably why there are no examples). And while you can lock down permissions you can not prevent secret access, which need to be explicitly made available but still.I have seen exploits where the runner process memory was dumped and secrets from another step extracted...
I think that should also be a setting but 🤷 I am also so annoyed that a normal pull_request trigger can't comment on PRs (any public account can do it, why do I need elevated permissions?!).
|
(sorry, forgot to actually merge this ^^) |
This ensures untrusted code by external contributors cannot access highly-priviledged GitHub tokens, even when triggered by a PR comment.
The touchstone actions are split into two parts, one "receive" action which does the actual benchmarking and one "comment" action that publishes the results. This is designed so that the receive may safely run untrusted code by external contributors, while running with weaker permission. Only the comment actions needs write permissions to the repository.
The permission drop is normally performed automatically by GitHub when a pull_request event is triggered from a fork repository. However, when the workflow is triggered by the issue_comment, by default it runs with a permissive token, with full access to the repository, even if PR comes from a fork.
Because the library encourages one to set a condition on who can trigger the workflow, the impact of this was limited. An owner/collaborator would have had to issue the benchmark command before the untrusted code can run. Nevertheless, reducing the permission set is safer as it protects the repository even if the owner issues initiates the benchmark before fully reviewing the PR code.
GitHub has actually changed the default token permission for the issue_comment trigger recently, from write-all to contents read only, similar to what this change tries to accomplish. However that is a global per-repository setting that users may want to change, and it only applies to repositories that were created after February 2023.
Conversly, the comment workflow needs permission to set statuses and comment on issues. Whether or not the workflow has the permission by default depends on the aforementioned repo-wide setting, and thus on the date the repo was created. By being explicit about it in the workflow definition we can make sure it is reliably granted. This also reduces the extent of the write permissions the workflow could have gotten by default, which is always a good thing even if the workflow is relatively simple.