Enable PRs from fork to build containers #107
Conversation
vivian-rook
requested review from
aborrero,
andrewbogott,
chicocvenancio,
david-caro,
supertassu and
yuvipanda
bug: T295754
blancadesal
left a comment
blancadesal
left a comment
There was a problem hiding this comment.
This looks fine to me as long as manual approval is required to trigger the workflows.
| - uses: actions/checkout@v2 | ||
| with: | ||
| token: ${{ secrets.PERSONAL_ACCESS_TOKEN }} | ||
| repository: ${{ github.event.pull_request.head.repo.full_name }} |
There was a problem hiding this comment.
With the caveat that my knowledge of this topic is somewhat limited, this seems to be the only potential vulnerability, as discussed here
How to not be vulnerable – Really, the answer to this is simple - if you're using the pull_request_target event in Github Actions, don't use actions/checkout to then checkout the pull request's code. If you do, then you are opening yourself up to the malicious pull request attack.
However, it doesn't look to me like the steps downstream can be tampered with to extract secrets/do other nefarious things.
There was a problem hiding this comment.
This is true that caution needs to be used in this situation. In this case I don't believe it can be exploited, as the code is never run, rather the code is pulled, then updated with sed and a commit is made. My understanding is there is no obvious way to exploit this.
On the other hand the job that actually builds the containers is a little closer to a secret (however a different secret). In its case the container is really built, with whatever they put in the Dockerfile. It is my understanding that the secret cannot be used in the Dockerfile, or anything that it calls, as the secret is never passed from github actions to the Dockerfile itself. When you want such a variable available to a script you have to pass it explicitly from github actions to the script, by first turning it into an env var, then calling the script with something like:
myscript.script $var_from_GHA
but we don't do that so we should be safe. Also this secret is, in my mind, somewhat less concerning if it were to be leaked, as the damage is limited to deleting/uploading of the paws containers to quay. Which are easily regenerated.
And finally, as you note, users who don't have write and submit a fork, will have to wait for someone with write to review and manually approve that the github action workflows be run. Giving us another layer of protection from weird stuff being run.
This change will enable a PR from a fork (rather than an internal branch) to run the workflows. In particular the PR will be able to build a container, and push it to quay (with the pr-) tag. So long as the PR submitter enabled the "Allow edits and access to secrets by maintainers" this will also go and update their values.yaml with the docker tag that the pr built.
This does open the door to some injection possibilities:
https://securitylab.github.com/research/github-actions-preventing-pwn-requests/
First the scope that we are talking about is two creds, one the robot account personal access token used by the commit, and the second a robot account token with write access to the five quay.io registries that the workflows manage.
According to the above the access token should be fine. As it doesn't compile or run the code that it pulls, just updates values and pushes it back out.
The quay.io robot does build the code that it is given. We could switch this to a "workflow_run" model, however I found that awkward, as the job could run, but wouldn't show up under the "Checks" section of a pr (it was visible in the overall "Actions" menu). This appears to be known:
https://github.community/t/workflow-run-runs-but-not-showing-in-the-pr-checks/183493
This makes it somewhat cumbersome to identify what stage the job is really in, and with the larger singleuser container result in the PR checks appearing done, but the container not being in quay.io yet, causing various confusion. As such I worked on it as a pull_request_target job instead. I don't believe there is a way to inject the token into the container and have it readable there. Though I could be wrong. If it were compromised we could end up with the quay.io containers being deleted, or replaced with some kind of junk.
At time of writing there is an additional protection, anyone without write access to the PAWS repo will need someone with write to run the workflow for them. So strange stuff shouldn't be able to run without someone looking at it.
#76 is something of a test for this. If we merge this PR, 76 should be able to run and build.
bug: T295754