trigger workflows to run for pull_request event

[seanpreston]

Purpose

This PR adds the pull_request_target event to our workflows, which should allow PRs on forked repos to trigger workflow events (ie. CI via Github Actions).

See: https://docs.github.com/en/actions/learn-github-actions/events-that-trigger-workflows#pull_request_target for full docs.

This event runs in the base context of a pull request, so I believe this change has to be made in the repo itself, before any workflows on forked PRs will run.

As @iamkelllly has mentioned here: #26 (comment), there's also something we'll need to do with the Github tokens, to stop them from being able to read secrets being stored in the underlying repository, so in lieu of completing that here I've changed the repository settings to force us to approve any workflow runs for outside collaborators, which we can choose to not do until we're sure the token issue is handled.

This should yield an approval process like: https://docs.github.com/en/actions/managing-workflow-runs/approving-workflow-runs-from-public-forks

Ticket

Closes #26

[NevilleS]

I'd love a few others to weigh in here on the security side (paging @ThomasLaPiana and @PSalant726) just so we get this right. I like the idea of allowing forked PRs to run our actions for CI, but want to ensure we are very careful about how that can expose the GitHub secrets.

A couple articles I found on this:

• https://securitylab.github.com/research/github-actions-preventing-pwn-requests/
• https://securitylab.github.com/research/github-actions-untrusted-input/
• https://securitylab.github.com/research/github-actions-building-blocks/

On my reading I think I'd prefer:

• using pull_request (instead of pull_request_target) as the trigger, which should be able to build & run unit tests without any secrets... I think?
• only ever using the docker secrets on main, never on any pull_request or pull_request_target workflows
• having a separate workflow for the integration tests. this can be manual for now (i.e. a core member checks out and runs the code), but we could also make it so you add a label like safe-to-test and that allows integration tests to run- see the example from here https://securitylab.github.com/research/github-actions-preventing-pwn-requests/

    on:
      pull_request_target:
        types: [labeled]

    jobs:
      build:
        name: Build and test
        runs-on: ubuntu-latest
        if: contains(github.event.pull_request.labels.*.name, 'safe to test')

[NevilleS]

TL;DR from my comment (#86 (comment))

If you set this to pull_request instead of pull_request_target for now, we'll get 90% of the value of CI but I'd expect tests that rely on secrets to fail. That's a secure place to start from

[PSalant726]

Building on this, I don't think it achieves what we want anyway. From the documentation linked by Sean:

This event runs in the context of the base of the pull request, rather than in the merge commit as the pull_request event does.

I could be misunderstanding, but I interpret this as "run our workflows against the commit from which the PR was branched", not the latest commit on the PR itself. It's intended to be used as more of a housekeeping trigger, to add labels etc. to PRs. Isn't the intent to run CI actions against the changeset on the PR?

Separately, these changes, as written, don't seem to be using any of the additional permissions afforded by the pull_request_target trigger except to allow for execution of the external resource integration tests. Effectively, this change only serves to expose the ${{ secrets.REDSHIFT_TEST_URI }} and ${{ secrets.SNOWFLAKE_TEST_URI }} values to PRs opened by potentially untrusted users (repo forks).

[seanpreston]

If you set this to pull_request instead of pull_request_target for now, we'll get 90% of the value of CI but I'd expect tests that rely on secrets to fail. That's a secure place to start from

Will update.

having a separate workflow for the integration tests. this can be manual for now (i.e. a core member checks out and runs the code), but we could also make it so you add a label like safe-to-test and that allows integration tests to run- see the example from here https://securitylab.github.com/research/github-actions-preventing-pwn-requests/

I'd like to understand more about how the tokens are issued too. If it's the case that the tokens are short lived and can be set to only be generated on manual approval of a job run, it might not be a huge issue either way.

[PSalant726]

@NevilleS 's suggestion about configuring the external integration tests to only run against PR's labeled safe-to-test (or similar) seems like a good potential route forward.

As another option, we could create an additional, similar automation that restricts the execution of the external integration tests only to PRs opened either 1) from a branch off of the repo itself (not a fork) or 2) by a member of the Ethyca GH org and/or the @ethycadev user group.

I don't see any way for us to safely automate the execution of any job that requires access to repository secrets when the PR being tested was opened by an untrusted user. For PRs opened by external contributors, I would recommend that an Ethyca employee first reviews the code changes here on GH, then, if everything seems safe, checkout the fork's branch locally and execute the external integration tests manually. Since all PRs require an approval from a maintainer before merging anyway, this extra step doesn't seem like that much to ask.

As a last step, we should find a way to mark the external integration tests as "not run", and prevent the pipeline from displaying as passed, so we don't miss the previous manual step.

[PSalant726]

Building on this, I don't think it achieves what we want anyway. From the documentation linked by Sean:

This event runs in the context of the base of the pull request, rather than in the merge commit as the pull_request event does.

I could be misunderstanding, but I interpret this as "run our workflows against the commit from which the PR was branched", not the latest commit on the PR itself. It's intended to be used as more of a housekeeping trigger, to add labels etc. to PRs. Isn't the intent to run CI actions against the changeset on the PR?

Separately, these changes, as written, don't seem to be using any of the additional permissions afforded by the pull_request_target trigger except to allow for execution of the external resource integration tests. Effectively, this change only serves to expose the ${{ secrets.REDSHIFT_TEST_URI }} and ${{ secrets.SNOWFLAKE_TEST_URI }} values to PRs opened by potentially untrusted users (repo forks).

[NevilleS]

From what I can see, the safe approach here for forks is to either do a manual checkout or use something that only we approve, ie labels or some more complex action like the `ok-to-test` action that is also linked in one of the GitHub security articles. But yeah running our regular CI tests without secrets is OK with me - with the collaborator approval that is, just to also defend against spurious PRs costing us a lot of action minutes 😁

…

On Thu, Nov 18, 2021, 12:45 PM Phil Salant ***@***.***> wrote: ***@***.**** requested changes on this pull request. @NevilleS <https://github.com/NevilleS> 's suggestion about configuring the external integration tests to only run against PR's labeled safe-to-test (or similar) seems like a good potential route forward. As another option, we could create an additional, similar automation that restricts the execution of the external integration tests only to PRs opened either 1) from a branch off of the repo itself (not a fork) or 2) by a member of the Ethyca GH org and/or the @ethycadev user group. I don't see any way for us to safely automate the execution of any job that requires access to repository secrets when the PR being tested was opened by an untrusted user. For PRs opened by external contributors, I would recommend that an Ethyca employee first reviews the code changes here on GH, then, if everything seems safe, checkout the fork's branch locally and execute the external integration tests manually. Since all PRs require an approval from a maintainer before merging anyway, this extra step doesn't seem like *that* much to ask. As a last step, we should find a way to mark the external integration tests as "not run", and prevent the pipeline from displaying as passed, so we don't miss the previous manual step. ------------------------------ In .github/workflows/pr_checks.yml <#86 (comment)>: > @@ -1,5 +1,8 @@ name: Run CI -on: [push] +on: [ + push, + pull_request_target Building on this, I don't think it achieves what we want anyway. From the documentation linked by Sean <https://docs.github.com/en/actions/learn-github-actions/events-that-trigger-workflows#pull_request_target> : This event runs in the context of the base of the pull request, rather than in the merge commit as the pull_request event does. I could be misunderstanding, but I interpret this as "run our workflows against the commit from which the PR was branched", not the latest commit on the PR itself. It's intended to be used as more of a housekeeping trigger, to add labels etc. to PRs. Isn't the intent to run CI actions against the changeset on the PR? Separately, these changes, as written, don't seem to be using any of the additional permissions afforded by the pull_request_target trigger *except* to allow for execution of the external resource integration tests. Effectively, this change *only* serves to expose the ${{ secrets.REDSHIFT_TEST_URI }} and ${{ secrets.SNOWFLAKE_TEST_URI }} values to PRs opened from by potentially untrusted users (repo forks). — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#86 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAN72N276IWZIILV3PTUNBTUMU3VJANCNFSM5IJL42OA> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.

[seanpreston]

To be thorough I have done some testing this afternoon by opening up a PR against this repo from a new account. I can confirm that the new account is unable to add labels:

[iamkelllly]

Given that we're referencing pull_request rather than pull_request_target then, can we update the PR title @seanpreston ?

[seanpreston]

As a last step, we should find a way to mark the external integration tests as "not run", and prevent the pipeline from displaying as passed, so we don't miss the previous manual step.

@PSalant726, it looks like this happens automatically:

UPDATE: and then once the label has been added:

We ought to stop every step from re-running each time a PR is labeled if we're also going to run them on push

[seanpreston]

I've updated this PR to factor in a couple of changes requested by both @NevilleS and @PSalant726:

• CI checks are now split into Safe and Unsafe categories
• Safe CI checks are run on every pull_request event. This allows forked PRs to run them (?), and is also triggered every time a commit is added to the PR (as before).
• Unsafe CI checks are triggered by the labeled type of pull_request events, and will only run if the label added contains the text run unsafe ci checks. NB. this category will still show a skipped label in the list of checks for push events, so the reviewer is aware that these checks have not run at the time of review.

[seanpreston]

This condition was a bit confusing to me on first pass, so to break down for anyone else:

github.event.pull_request.labels.*.name this statement is applying a filter on the github.event.pull_request.labels var, pull out a list of label names, e.g. ["safe to run", "dont merge", ...].

contains(list, "value") is then check if "value" is an element of list.

It seems simple when you put it that way, but if you don't know about the filter stage, it could look as though github.event.pull_request.labels.*.name evaluates to a string and we're checking if that string contains the PR name (which would be less robust).

https://docs.github.com/en/actions/learn-github-actions/expressions#object-filters

[NevilleS]

This looks good to me now.

However, with the number of actions we're running, I think we need to combine some of these jobs together to make the whole thing run faster because we're definitely burning a ton of CI minutes on every single PR

I'll open a new issue for that

[seanpreston]

However, with the number of actions we're running, I think we need to combine some of these jobs together to make the whole thing run faster because we're definitely burning a ton of CI minutes on every single PR

The number of minutes burned might actually go down with this change, since we're switching from running the workflows on every push to only events related to pull requests.

[PSalant726]

@NevilleS -

However, with the number of actions we're running, I think we need to combine some of these jobs together to make the whole thing run faster because we're definitely burning a ton of CI minutes on every single PR

Ideally the pipeline would:

• Build the container artifacts in a build stage, and make them available to the other stages that require them
• Execute static analysis tools and test harnesses in discreet pipeline lint and test stages, dependent on the build artifacts where needed (and, crucially, not dependent where not needed)

We have an issue in fidesctl to make a similar change.

@seanpreston -
It's not enough to block this PR for me, but I think a cleaner implementation would keep things in a single file with a generic name like Fidesops. The jobs are still discreet and can have if conditions applied independently.

[seanpreston]

It's not enough to block this PR for me, but I think a cleaner implementation would keep things in a single file with a generic name like Fidesops. The jobs are still discreet and can have if conditions applied independently.

I did have it like this initially. Because we're using both push and pull_request triggers, we'd end up needing custom if: ... conditionals on every job to be sure we're not running every job in both contexts. That was generating 15 job runs for every push which seemed excessive. There might be another way to configure it that I didn't discover, but this felt clean enough, and has the added benefit of anything unsafe being explicit and in its own file.