plietar
commented
3 months ago I've also been playing with another idea, of merging the two workflows but keeping separate jobs with different permissions: https://github.com/plietar/malariasimulation/blob/7418cd51c1226523218f1a2f3e7899f3971594e5/.github/workflows/touchstone.yaml
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 pull_request_target, which acts with similar permissions as issue_comment.
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 pull_request workflow. To be honest I'm surprised to have not found any examples of this in GitHub's examples.
Let me know what you think and if this is something you think should be pursued.
assignUser
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.