[mobile] ci: switch to pull_request_target

[Yannic]

This changes the PR trigger from pull_request to pull_request_target. The latter is executed in the context of the PR target (usually main) and has access to secrets stored in GitHub, while the former is executed in the context of the PR head, which is potentially in a fork and thus doesn get access to secrets.

This is part of the effort to reduce flakyness in the CI build because of authentication problems against the EngFlow Remote Execution cluster.

[Yannic]

@jpsim PTAL

[jpsim]

There's an iOS test flake and an RBE flake:

ERROR: Failed to query remote execution capabilities: null

Retrying.

[jpsim]

Does the fact that these jobs have both pull_request_target and pull_request set mean that jobs will be scheduled in duplicate? Or is GitHub smart enough to only pick one of these as a trigger?

[Yannic]

GitHub is not smart enough to see it's the same. The intent is to merge this PR, then ask a repo admin to change the required check (because they count as different checks), and then remove pull_request.

Alternatively, we can always remove the required check, land this with only pull_request_target, and then enable it again. Let me know what you prefer.

[jpsim]

ok I'll reassign a repo admin so they can coordinate this

[jpsim]

/assign @alyssawilk

A repo admin needs to coordinate with this change

[alyssawilk]

I'm in meetings until 2 PM EST but I can help after if @phlax doesn't beat me to it

[phlax]

im a bit concerned with this...

Workflows triggered via pull_request_target have write permission to the target repository. They also have access to target repository secrets.

https://securitylab.github.com/research/github-actions-preventing-pwn-requests/

is this something we are happy with?

there has been various discussions around this previously - specifically in the context of moving off azp - the fear was that this exposed too much and so we never went down this path

im not sure i entirely understand the implications or exactly how it works but reading the above docs doesnt give me any more confidence

cc @mattklein123 @lizan

[Yannic]

How does azp handle secrets?

[phlax]

How does azp handle secrets?

separate pipelines

[phlax]

Discussing with @Yannic offline i have a slightly better understanding of the implication of pull_request_target

iiuc we need to land #25770 (or something like it) first and then have a very strict policy around token exposure

if we are definitely not exposing write permissions to the repo then im happy for this to go forward (with the caveat that write permissions are not necessarily the only thing exposed - but i guess thats unavoidable)

[Yannic]

Pushing a commit to restrict permissions to package: read (which is temporarily needed until we switch auth)

[phlax]

as discussed offline this PR wont currently work as it would just test main on every PR push

fixing this is exactly the thing that potentially exposes repo creds/secrets

discussing with @Yannic , and looking over the gh docs, its not immediately clear if/how this pattern can be used for PRs - gh themselves pretty much say not to

what seems to be the core issue is that you if you run untrusted code in this context there is no way to prevent access to the github secret

if the secret has very restricted permissions, perhaps that is not such a problem, altho the docs are ambiguous and discouraging about this

probably we want to try snaffling a github token with permissions: {} and seeing what can be done with it - if it cant access secrets or write to the repo (as you might expect) then i guess we can do this (even if gh says not to), but we are going to have to be super vigilant about managing these workflows

[phlax]

we also have to consider very carefully allowing any untrusted code write access to the github token as that basically gives away everything else

[phlax]

@Yannic i now understand better what the problem is with this code, and more specifically why the npm call is a problem:

# 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 }}

    - uses: actions/setup-node@v1
    - run: |
        npm install
        npm build

    - uses: completely/fakeaction@v2
      with:
        arg1: ${{ secrets.supersecret }}

    - uses: fakerepo/comment-on-pr@v1
      with:
        message: |
          Thank you!

The problem is not that the secret would be directly exposed to whatever happens with npm (ie untrusted code) - the problem is that the untrusted code can change future actions - eg by overwriting the completely/fakeaction with some creds snaffling code

Essentially, i believe, this is why github (and others) advise against using pull_request_target for pull_request at all - its incredibly difficult to prevent these kind of attacks visually or programatically

[alyssawilk]

/wait on merge, comments

[github-actions]

This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

[github-actions]

This pull request has been automatically closed because it has not had activity in the last 37 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!