diogoteles08 opened a new issue, #27470: URL: https://github.com/apache/beam/issues/27470
### What needs to happen? # Motivation I'm creating this issue to bring your attention some dangerous workflow patterns currently present on your project involving the usage of `pull_request_target` along with a `actions/checkout`, which inevitably open doors to run untrusted code. There are 14 occurrences that seem to follow the very same algorithm, as the [beam_PreCommit_Go.yml](https://github.com/apache/beam/blob/master/.github/workflows/beam_PreCommit_Go.yml#L41) or [beam_PreCommit_Website.yml](https://github.com/apache/beam/blob/master/.github/workflows/beam_PreCommit_Website.yml#56) , and another file with different purpose: [build_runner_image.yml](https://github.com/apache/beam/blob/master/.github/workflows/beam_PreCommit_Go.yml#L41) . All of them were identified using the [Scorecard tool](https://github.com/ossf/scorecard). As a general rule, GitHub itself recommends that a [pull_request_target](https://docs.github.com/en/actions/using-workflows/events-that-trigger-workflows#pull_request_target) trigger shouldn't be used along with a checkout, as you can see on this warning displayed on the link to the trigger:  Although this is a very strong recommendation by itself, I'll try to give some concretes examples on how this pattern could be dangerous specifically to your code. 1. For file [beam_PreCommit_Go.yml](https://github.com/apache/beam/blob/master/.github/workflows/beam_PreCommit_Go.yml) and similars, I saw that you take the caution to set most of the permissions as read or None, but you still have the `actions: write` permission. This may seem safe, but as you can see on its documentation, it can be used to approve the run of some other malicious action through the endpoint `POST /repos/{owner}/{repo}/actions/runs/{run_id}/approve`. Also, at the line 46 you are running a local script after your checkout, which could be totally changed by a malicious PR and abuse any permission or secrets you may spoil. 2. At the file [build_runner_image.yml](https://github.com/apache/beam/blob/master/.github/workflows/build_runner_image.yml) you are not directly calling any script that could be changed, but you are using some critical secrets and pushing a docker image based on a image stored on the repository. I'm not sure exactly how the pushed image is being used, but an attacker could probably change the docker image to whatever they want. And given the image is being built at the same environment that the secrets were used, probably we can't have warranty that the image couldn't extract them. # Possible remediations There simply isn't a single nor a right solution to this, so we may want to evaluate the motivations behind the two jobs to say what's the best path to follow, but it follows some of the general ideas on how to proceed: ### Divide the workflow in two, so that the `pull_request_target` is done separately from the checkout We usually can think of ways to split the workflow to avoid this dangerous pattern. Looking at the [beam_PreCommit_Go.yml](https://github.com/apache/beam/blob/master/.github/workflows/beam_PreCommit_Go.yml) code, does the `rerun-job-action` really need to happen at the same workflow as the following steps of the workflow (e.g "Install Java", "Setup Gradle")? If not, the `rerun-job-action` can be called in a workflow with write permissions, and then use `workflow_run` to trigger another workflow to run the other steps without write permissions. Thinking even further on the pre_commit_go case, maybe you could have a single workflow with the job of calling the `rerun-job-action` and then use information about the trigger of this job to router a run of another workflow to use checkout and run the correspondent preCommit script. ### Use label verification We can use a `type: [labeled]` and a condition of `if: ${{ github.event.label.name == 'is ok to test' }}` to check for a label "is ok to test" for example, that you would manually add once you saw that nothing potentially dangerous would be running. This still isn't 100% save, because one can incautiously label a unsafe workflow as "ok to test", but it's safer. --- Finally, I'm available to help find a good solution for this, and even contribute with the PR if wanted. Thanks for the attention! ## Additional context I'm Diogo and I work on Google's Open Source Security Team([GOSST](https://github.com/diogoteles08#about-gosst-ghost)) in cooperation with the Open Source Security Foundation ([OpenSSF](https://openssf.org/)). My core job is to suggest and implement security changes on widely used open source projects 😊 ### Issue Priority Priority: 2 (default / most normal work should be filed as P2) ### Issue Components - [ ] Component: Python SDK - [ ] Component: Java SDK - [ ] Component: Go SDK - [ ] Component: Typescript SDK - [ ] Component: IO connector - [ ] Component: Beam examples - [ ] Component: Beam playground - [ ] Component: Beam katas - [ ] Component: Website - [ ] Component: Spark Runner - [ ] Component: Flink Runner - [ ] Component: Samza Runner - [ ] Component: Twister2 Runner - [ ] Component: Hazelcast Jet Runner - [ ] Component: Google Cloud Dataflow Runner -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
