Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow access to secrets for external contributors #574

Merged
merged 3 commits into from
Jun 4, 2021

Conversation

0x2b3bfa0
Copy link
Member

@0x2b3bfa0 0x2b3bfa0 commented Jun 2, 2021

Update: see this blog post we wrote

I forgot to explicitly add access to secrets for external contributors with pull_request_target but now it should work as expected. Please keep in mind that approving the test-external environment on malicious pull requests may lead to token exfiltration among other niceties.

⚠️ Warning: don't use this without a protected environment that requires manual approval.

...after requiring manual approval through GitHub Environments
@0x2b3bfa0 0x2b3bfa0 added testing Unit tests & debugging security Sensitive flaws labels Jun 2, 2021
@0x2b3bfa0 0x2b3bfa0 requested a review from casperdcl June 2, 2021 16:43
@0x2b3bfa0 0x2b3bfa0 self-assigned this Jun 2, 2021
@0x2b3bfa0 0x2b3bfa0 marked this pull request as ready for review June 2, 2021 16:44
@0x2b3bfa0 0x2b3bfa0 linked an issue Jun 2, 2021 that may be closed by this pull request
@0x2b3bfa0 0x2b3bfa0 requested a review from casperdcl June 3, 2021 18:03
Copy link
Contributor

@casperdcl casperdcl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does defaults.environment work?

.github/workflows/test-deploy.yml Show resolved Hide resolved
.github/workflows/test-deploy.yml Show resolved Hide resolved
.github/workflows/test-deploy.yml Show resolved Hide resolved
.github/workflows/test-deploy.yml Show resolved Hide resolved
@0x2b3bfa0
Copy link
Member Author

0x2b3bfa0 commented Jun 4, 2021

does defaults.environment work?

No, it doesn't seem to work; the editor highlights it as erroneous. In case it did, it would require approval for each individual step. It's a pity, because it would be both elegant and failsafe.

Copy link
Contributor

@casperdcl casperdcl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GH business logic of actions/checkout is broken due to their (ab)use of on.pull_request_target for two different purposes, one of which requires checkout with.ref: pull_request.head.sha. No clean work-around until they implement something like on.pull_request_target_v2

😢

@casperdcl
Copy link
Contributor

worth writing a short blog post about @0x2b3bfa0? (/CC @shcheklein)

@0x2b3bfa0
Copy link
Member Author

worth writing a short blog post about @0x2b3bfa0?

(I hope you didn't omit a comma before “@0x2b3bfa0” on purpose) 🥶

@0x2b3bfa0
Copy link
Member Author

0x2b3bfa0 commented Jul 19, 2022

Warning
This is an unfinished draft, handle with care!

Run tests with secrets on external pull requests, securely

Some open source projects in the cloud-native space can only be tested by means of integration tests against actual third-party APIs which require authentication ... https://thenewstack.io/the-cloud-native-community-needs-to-talk-about-testing

Example

Configuration

  1. Create some encrypted secrets, e.g. EXAMPLE in the snippets below
  2. Create an environment named authorize with required reviewers enabled

Workflow

on: pull_request_target

jobs:
  authorize:
    environment: ${{
      github.event_name == 'pull_request_target' &&
      github.event.pull_request.head.repo.full_name != github.repository &&
      'external' ||
      'internal' }}
    runs-on: ubuntu-latest
    steps:
    - run: true
 
  test:
    needs: authorize
    runs-on: ubuntu-latest
    steps:
    - uses: actions/checkout@v3
      with:
        ref: ${{ github.event.pull_request.head.sha || github.ref }}
    - run: printenv EXAMPLE
      env:
        EXAMPLE: ${{ secrets.EXAMPLE }}

Explanation

on: pull_request_target

The pull_request_target event behaves similarly to the pull_request event but, unlike the latter, also passes secrets to workflows triggered from forks. Using this event without any additional cautionary measures could allow anybody to open a pwn request[^1] to exfiltrate secrets.

authorize:
  environment: ${{
    github.event_name == 'pull_request_target' &&
    github.event.pull_request.head.repo.full_name != github.repository &&
    'external' ||
    'internal' }}
  runs-on: ubuntu-latest
  steps:
  - run: true

The authorize job checks if the workflow was triggered with the pull_request_target event and if it was run from a fork. If that's the case, it will require human approval from the external environment created beforehand, making sure that no external contributor can run the next job without explicit authorization. Pull requests from internal contributors (those who don't use forks) will run automatically, without requiring manual approval.

test:
  needs: authorize
  runs-on: ubuntu-latest
  steps:
  - uses: actions/checkout@v3
    with:
      ref: ${{ github.event.pull_request.head.sha || github.ref }}
  - run: printenv EXAMPLE
    env:
      EXAMPLE: ${{ secrets.EXAMPLE }}

Finally, the test job where secrets are used needs the previous authorize job, so it can't run unless it's allowed to. All the security of this approach is based on the idea of a human reviewing the pull request code and approving runs after making sure that there is no malicious code on them, hence it also overrides the ref from actions/checkout to run on the pull request branch rather than on the main branch.

See Also

Internal

Failed Attempts

Send secrets to workflows from fork pull requests

Back in 2020, GitHub introduced an option to send secrets to workflows from fork pull requests, but only for private repositories.

Require approval for all the outside collaborators

Admittedly, the authorize job in the workflow isn't particularly clean, and it would have been better to require approval for all the outside collaborators, but it doesn’t have any effect for the pull_request_target event:

Workflows triggered by pull_request_target events are run in the context of the base branch. Since the base branch is considered trusted, workflows triggered by these events will always run, regardless of approval settings.

Anecdotes

@casperdcl
Copy link
Contributor

casperdcl commented Aug 16, 2022

hope you didn't omit a comma [...] on purpose

xkcd#191

@fxmarty
Copy link

fxmarty commented Aug 1, 2023

Hi @0x2b3bfa0, this approach seem to work decently well. Isn't it still prone to a race condition in which the attacker may push new changes after the workflow was approved? As explained in: https://securitylab.github.com/research/github-actions-preventing-pwn-requests/

Could the PR be updated with malicious code just after the approval? How different is it from the label case?

@0x2b3bfa0
Copy link
Member Author

Hello, @fxmarty! Unlike with the label approach, manual workflow approvals are tied to a specific commit, so, as far as I recall, there is no risk of race conditions. See this blog post for more information.

@fxmarty
Copy link

fxmarty commented Aug 1, 2023

Thank you @0x2b3bfa0! It works well indeed!

@fxmarty
Copy link

fxmarty commented Aug 2, 2023

The only issue I have now is that I have external collaborators with write access to my repo. I would like them to be able to run automatically the workflow - but it appears only member from the external environment can (and there can only be 6 members/teams there).

Edit: possible solution: have an environment variable for trusted contributors, and check on github.event.pull_request.user.login in the authorize job (not sure how though)

@fxmarty
Copy link

fxmarty commented Aug 2, 2023

@0x2b3bfa0
Copy link
Member Author

0x2b3bfa0 commented Aug 6, 2023

Maybe something like this?

    environment:
      ${{ github.event_name == 'pull_request_target' 
       && github.event.pull_request.head.repo.full_name != github.repository
       && github.event.pull_request.user.login != 'authorized_user_1'
       && github.event.pull_request.user.login != 'authorized_user_2'
       && github.event.pull_request.user.login != 'authorized_user_n'
       && 'external'
       || 'internal'
       }}

Beware of user name changes, though!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
security Sensitive flaws testing Unit tests & debugging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix GitHub environments for external contributors
3 participants