Secrets to forked PRs

[disa6302]

Hello,

I am using this action to run some unit tests that require AWS creds on a public repo. I am using these by setting secrets in the repository settings. However, I notice that the secrets are not being read when a PR is opened by external contributors. I have "Approve and run" set up for any external contributions, but enabling that does not help either. Maybe this is not the right channel and not really specific to AWS credentials, but the question is specific to how secrets are dealt with in GHA. I have read multiple articles talking about this, but have not gotten to a concrete solution. Anybody successful in setting GHA for forked PRs?

Note, I have gone through pull-request-target which seems to run the CI on the verified branches and not the changes in the PR which to me seems pointless.

[peterwoodworth]

Hey @disa6302,

This setup you're describing sounds like it would be a massive security risk. I'm not sure what your exact use case is or if you have other security measures in place, but I would recommend against allowing forked PRs to run code in an environment with your AWS credentials unless other measures are in place.

That said, you're seeing the correct behavior when workflows ran on pull_request cannot access secrets. This is how it works, and it's done so that developers don't unintentionally expose their secret credentials.

pull_request_target was introduced to work around this, seemingly so that anyone who does want to run PR code in an environment with secrets can do so while minimizing the risk people accidentally setup their environment in this way. But you're right, at first it doesn't seem that pull_request_target would be that helpful for the reason you describe. In my opinion, this article does the best job at describing the solution with full examples, in a secure manner.

[disa6302]

@peterwoodworth ,

Thank you for your response and the article! Might be a stupid question, but I am unable to think of scenarios pull_request_target would actually be useful. Why would running a CI on a base branch be required with forked PR? Isnt the purpose of CI to test out new changes before merging to base branch?

For your knowledge, the CI does not actually generate or publish any artifacts, but the CI only builds the unit test targets and tests out different cmake build options and runs unit tests, hence, it is important that the CI runs on forked PRs because we need unit tests to pass before it can be merged. The repo does have the approve and run workflow and I do see that the CI run can be controlled with labels as per the article. So that is great!

[peterwoodworth]

No worries for asking questions @disa6302,

I didn't directly answer how you can use pull_request_target because I don't want to come across like I'm recommending this unless you know exactly what you're doing. The article explains how you can use pull_request_target to run the submitter code while having access to secrets in your repository. What you need to do is check out the pull request head, and then you can do whatever you'd like

# INSECURE. Provided as an example only.
on:
  pull_request_target

jobs:
  build:
    name: Build and test
    runs-on: ubuntu-latest
    steps:
    - uses: actions/checkout@v2
      with:
        ref: ${{ github.event.pull_request.head.sha }}

There are other reasons why pull_request_target would be useful outside of this insecure use-case. In general, if you don't need what the submitter has actually written in their PR and want to avoid doing that accidentally, and/or you need access to your repository secrets, you would want to use pull_request_target.

Why would running a CI on a base branch be required with forked PR?

It's not required to run CI on the base branch with a forked PR - It's just heavily recommended against to automatically run code from a forked PR in the context of your own repository when the environment has write permissions to your repository, or any other credentials you may have.

Isnt the purpose of CI to test out new changes before merging to base branch?

You can certainly run tests on submitter code in forked PRs to test changes before merge, we do that in this repository in tests-unit.yml. This action is triggered on pull_request, so it is within the context of the PR rather than the base. It is ok to run the unit tests automatically in this repo because even if the submitter code has something malicious, it won't have access to our secrets, or any write access to our repository, so there won't be any negative impact on our end.

On the other hand, tests-integ.yml by comparison does not run when PRs are submitted because it requires access to secrets that contain our AWS credentials that are necessary for integration tests. We have it set up so this is only ran manually. We run this before releasing; it also enables me to be able to easily manually run it during development since I will have access to the secrets as a maintainer working on a branch. There are a number of ways to make this more streamlined, but they all still require a manual look to ensure there's nothing malicious in the code before it is ran. I am fine with only running the integration tests manually in this case, since this is a lightweight action.

I also work over at AWS CDK, which is much larger of a project. However, it is similar over there (oversimplified warning) - unit tests run on PR submission, but integration tests that actually deploy resources does not occur. Instead, there's a check to see if the changes in the PR would cause any of the integration tests to have a different output, and if so then the PR submitter has to run those on their own with their own credentials (which will refresh the check to see if the PR will cause any additional changes to integration tests), or ask one of the maintainers to do so for them. This is in large part because allowing submitter code to run in an environment with credentials that can do CloudFormation deployments is a batant security hazard

[disa6302]

That makes sense! Thank you for the detailed clarification, this is super helpful. Given this reasoning, it does make sense to use the INSECURE method I saw in the article in my use case and have CI fail selectively based on the CI job.

[kellertk]

Just to add some more information onto Peter's answer:

There are some scenarios when pull_request_target makes sense. After all, the big difference between this and pull_request is the latter only has a read only GITHUB_TOKEN, but the former has more permissions. Any workflow that needs to make modifications to your repository - like posting a comment on the PR, closing the PR, creating a new issue, etc. - would need a read/write token. These are very valid uses of pull_request_target and don't present a security risk.

The trouble comes in when you both run in the context of the target of the pull request (that is, your repository, with your repository secrets and permissions) AND check out the PR code in a way that could be executed somehow. Imagine a workflow that runs on a PR that executes tests with jest. Running in pull_request_target could be a disaster. A malicious actor could write arbitrary JS code in your test files on their fork and do something like call the GitHub API to fetch your repository secrets and upload them a server they control. Then all they would have to do is open a PR on your repo, and your secrets are now compromised.

Basically, pull_request_target is unsafe when run from public forks unless your workflow does not in any way execute the code from the PR, even in a test, or you have other safeguards in place.

This blog post does a good job of summarizing the problem, as well as the article series from GitHub that Peter linked up above.

[disa6302]

Thanks @kellertk . Both the articles clarify the dos and donts with pull_request_target. This is what I needed.

[github-actions[bot]]

Hello! Reopening this discussion to make it searchable.