Discussion of permissions and security in README / sample workflows needs clarification?

[ferdnyc]

The documentation in README.md takes some pains to consider issues of security when integrating this Action that will have write access to your repo, which is great. But the details don't seem to match up with the text.

The dual-workflow setup is introduced with,

The following snippet is targeted for cases where you expect PRs from users that don't have write access to the repository. Posting the comment is done in 2 steps:

1. Checkout the repository and generate the comment to be posted. For security reasons, we don't want to give permissions to a workflow that checks out untrusted code
2. From a trusted workflow, publish the comment on the PR

...But since #122, the ci.yml ("untrusted" workflow) has the exact same permissions as the supposedly "trusted" coverage.yml workflow:

ci.yml:

python-coverage-comment-action/README.md

Lines 107 to 116 in d2ae3c8

	name: Run tests & display coverage
	runs-on: ubuntu-latest
	permissions:
	# Gives the action the necessary permissions for publishing new
	# comments in pull requests.
	pull-requests: write
	# Gives the action the necessary permissions for pushing data to the
	# python-coverage-comment-action branch, and for editing existing
	# comments (to avoid publishing multiple comments in the same PR)
	contents: write

coverage.yml:

python-coverage-comment-action/README.md

Lines 151 to 164 in d2ae3c8

	name: Run tests & display coverage
	runs-on: ubuntu-latest
	if: github.event.workflow_run.event == 'pull_request' && github.event.workflow_run.conclusion == 'success'
	permissions:
	# Gives the action the necessary permissions for publishing new
	# comments in pull requests.
	pull-requests: write
	# Gives the action the necessary permissions for editing existing
	# comments (to avoid publishing multiple comments in the same PR)
	contents: write
	# Gives the action the necessary permissions for looking up the
	# workflow that launched this workflow, and download the related
	# artifact that contains the comment to be published
	actions: read

also:

python-coverage-comment-action/README.md

Lines 165 to 167 in d2ae3c8

	steps:
	# DO NOT run actions/checkout here, for security reasons
	# For details, refer to https://securitylab.github.com/research/github-actions-preventing-pwn-requests/

So what makes the coverage.yml "trusted", and why is it not OK to run actions/checkout there, but it's OK to run it in ci.yml with the same permissions?

If granting permissions to ci.yml is no longer avoidable due to changes in the GitHub backend, then it is what it is, but that seems like it makes the whole "split workflow" thing sort of pointless and complicated for no reason, no? And it would probably be better to just drop the security advice that's no longer being followed from the docs. (Unless I've misunderstood?)

[ewjoachim]

Hey there, thanks for opening this.

The part that makes it secure is not exactly linked to permissions but to the fact the part that interacts with the PR code is in:

python-coverage-comment-action/README.md

Lines 99 to 100 in d2ae3c8

	on:
	pull_request:

While the part that can write to the repo is in:

python-coverage-comment-action/README.md

Lines 143 to 144 in d2ae3c8

	on:
	workflow_run:

pull_request has some specific limits in place that make it much much harder for forked repos to get write access. This is detailed here:

You can use the permissions key to add and remove read permissions for forked repositories, but typically you can't grant write access. The exception to this behavior is where an admin user has selected the Send write tokens to workflows from pull requests option in the GitHub Actions settings. For more information, see "Managing GitHub Actions settings for a repository."

And here:

With the exception of GITHUB_TOKEN, secrets are not passed to the runner when a workflow is triggered from a forked repository. The GITHUB_TOKEN has read-only permissions in pull requests from forked repositories. For more information, see "Automatic token authentication."

So the permissions are here to define what exactly the action needs to be able to do on the repo, and the permissions will not be enough to give forked repos write access (unless you check the checkbox that is mentionned in the doc exceropt above, which could as well be named please hack me). The permissions are mainly protecting the action user (you) from the action creator (me) in case I was the attacker (but given you give contents: write status, well, let's hope I don't turn to be evil :) Let's say that if you're afraid of this, your best course of action is to pin the action to a given commit hash and review the code when you update the hash, or fork the action altogether. Note that we have dependencies you may want to monitor too :) I fully understand that this is not what you had in mind, but I'm stating it for completeness).

(note: it's neithertheless relevant to give the on: pull_request part write permissions even if they're going to be ignored for PR from forks because they're not going to be ignored for PRs from users with write access, which means the PR comment will be posted right away without having to wait for the 2nd part of the workflow)

On the other hand, regarding on: pull_request vs on: workflow_run

• The part that has on: pull_request is guaranteed to be read-only on forks, and cannot make changes to the repo (including posting the PR comment). It's safe that this part checks out the PR changeset. The action that runs is the action as defined by the PR (so in case of a malicious PR, the PR author has complete control over the action). When called on: pull_request, the action is configured to do everything in this part, apart from posting the comment to the repo for forked PRs (the thing it cannot do due to permissions), so in this case, it writes a file containing the coverage comment to post to the action artifacts.
• The part that has on: workflow_run may have write permissions. The action that runs is the action as defined on the default branch, so even if there's a malicious PR, they can change the definition for this action, but it won't run the way they've defined it (at least if we don't merge the PR, which is why it's good to review external PRs before merging :D ). In that part, if we were to checkout the PR code, we would need to be extremely careful not to run anything that may come from the malicious PR. And there may be very very sneaky ways the PR content could be crafted so that we end up executing code from the PR. The only thing the action does when called on: workflow_run is read the artifact file and post it as a comment. As the (potnetially malicious) PR is not checked out, the only thing controlled by the (potential) attacker at this point is the content of the artifact file.

This means there's still a risk: a malicious PR could control what file gets used as the basis for posting the PR comment, so they could craft a PR that makes GHA post "All your base are belong to us" instead of the coverage report, and it would only work on their own PR. That's not very disruptive.

If you're interested in what exactly we do with the file downloaded from the artifacts (and wonder if there could be a way an attacker would escape the normal code path), the corresponding code is here:
https://github.com/py-cov-action/python-coverage-comment-action/blob/main/coverage_comment/github.py#L67-L71
https://github.com/py-cov-action/python-coverage-comment-action/blob/main/coverage_comment/main.py#L321-L343
https://github.com/py-cov-action/python-coverage-comment-action/blob/main/coverage_comment/github.py#L168-L175
https://github.com/py-cov-action/python-coverage-comment-action/blob/main/coverage_comment/github_client.py#L76-L85

So as long as httpx doesn't make anything special on the payload, I don't have an idea what an attacker might attempt to do. Note that the attacker doesn't control how the zipfile is made (GitHub does) so I don't think they can even do a zip bomb.

But all in all, this has nothing to do with the permissions which are an additional layer that is not the main protection (though it always helps).

Hope it answered your question and also, happy to have the opportunity to explain this action's stance on security in (excruciating) detail. Feel free if you have follow-up questions :)

Also, these three articles are always relevant (they're a must-read if you do anything github-actions)

[ferdnyc]

@ewjoachim Thanks for such a detailed response!

(note: it's neithertheless relevant to give the on: pull_request part write permissions even if they're going to be ignored for PR from forks because they're not going to be ignored for PRs from users with write access, which means the PR comment will be posted right away without having to wait for the 2nd part of the workflow)

Ah, OK! I think that's the main thing I was missing, that the ci.yml code effectively acts as a "hybrid" workflow that'll operate differently on internal vs. external PRs.

(As a completely random aside, this line from that second article is hilarious:)

Nevertheless an email address like `echo${IFS}hello`@domain.com is perfectly valid and may be used for both shell injection and receiving emails at the same time.

There are probably some things that can be done to secure the workflows even more (just kind of thinking out loud here, it's not Python Coverage Comment's job to deal with this stuff):

1. Setting up CODEOWNERS mappings is an obvious one, so that any PR that attempts to change either of the coverage workflows is immediately flagged for review by someone who knows that any PR that touches ci.yml should be heavily scrutinized, and PRs shouldn't be touching coverage.yml at all.
2. I wonder if at least the trusted workflow could be made an organization workflow (template), instead, while still retaining all of its functionality? If the meat of coverage.yml doesn't live in the repo at all, that'd obviously be the most secure setup. (For those repos that are part of an organization.)

[ewjoachim]

Setting up CODEOWNERS

Well I think it's safe to assume people review PRs before merging them :D Whether they want to use CODEOWNERS or anything else, I feel it's their decision. This would be true for literally any workflow they have: if they merge it without reviewing, then they'll get hacked.

organization workflow

It's a bit of the same point. This is a step people may want to take to secure their workflows independently from this action. Also, if the workflow is public, it's public, whether that's in the same repo or in an org-specific repo, so I guess, there could be malicious PRs either way.

[ferdnyc]

Setting up CODEOWNERS

Well I think it's safe to assume people review PRs before merging them :D

Oh, I don't think it's safe to assume that AT ALL, unfortunately. And the more committers a project has, the less safe it is to assume that the RIGHT people review a PR before it's merged. (Hence, I guess, CODEOWNERS in general.)

organization workflow

It's a bit of the same point. This is a step people may want to take to secure their workflows independently from this action. Also, if the workflow is public, it's public, whether that's in the same repo or in an org-specific repo, so I guess, there could be malicious PRs either way.

Sure, but the repo an organization workflow lives in (<org>/.github) can have a tiny fraction of the committers that a project repo can. And more importantly, then there's no way a project PR can slip workflow modifications in with other, unrelated code changes.

[ferdnyc]

Anyway, closing this as answered. Thanks again!