On Sat, Apr 9, 2022 at 8:10 AM Chesnay Schepler <ches...@apache.org> wrote: > > i think we'd be alright with those caveats. > > 1) It depends on how far you wanna go. If the goal is to only protect > the workflow files themselves (and not any scripts that the workflow calls), > then you only need to check out a single version of the code, namely the > PR. The way I see it whether the workflow calls some PR-controlled > script or test largely results in the same threats (that is, running > weird stuff or making changes to the current shell).
If you have a simple workflow - that will work. The more complex the build logic, the more you need scripts called from the workflows (also because you do not want to base your build logic on a proprietary CI stuff too much (been there, done that). So scripts are inevitable as you add stuff IMHO And yes - this is the risk. We have dedicated folders where we run pull request scripts from and those are always used from "main" version in our case. But that's prone to error (see the PR in my last example) > 2) That's gonna be unintuitive and will definitely end up breaking CI a > few times, but for the most part changes to the CI workflow can also be > verified > from runs in the contributors fork in github-hosted runners. The > alternative is to "merge" the PR into a separate branch in the main repo for > testing purposes. That won't work. It's not the "merge" that is the problem is that if you run a "pull_request_target" - it's ALWAYS run from your "main" branch. no exceptions. So the only way to test it, is to merge it in your "main" branch. This can be the main branch in a fork though (but it cannot be "different" branch in your repo). And then it gets complicated as you want to test the workflow for a PR running from a fork. Because then you need to test PR from a fork of your fork to the "main" in your fork. See here for example: https://github.com/potiuk/airflow/pull/158 This is a test PR to "main" in my "potiuk" fork of airflow from a branch in the fork of my fork "higrys" where I use different user who is not a commiter in the airflow repo to test it. I had to first push my branch as "main" to my "potiuk" fork: git push -f potiuk my_branch:main Then make a bogus change and push it to my "higrys" fork. git push higrys my_bogus_change_based_on_mybranch And then make A PR from "higrys" fork to "main" of "potiuk" fork. This is the only way (other than testing-in-production) that worked for me. It's certainly doable but you might get lots between the forks pretty easily. Lots of forks. I feel hungry now > 3) The "bitcoin miner problem" is giving me headaches. On the one hand > I'd love to just say "hey let's just run manual builds for for all > non-committers", > on the other hand I know that will be tedious AF and result in stuff > being merged without CI being run at all. > My current plan is to add some form of self-contained pre-flight check > as another job to the pull_request_target > workflow that fails with an error if it is a first-time contributor. > That _may_ cover most cases; I think we'd be fine with re-evaluating > this later on > if necessary, accepting some risk (so long as that risk is limited to > reduced CI capacity/stability). Yep. This one has the big potential for headeache induction. > > you should [...] make sure that your `pull_request_target" workflow > is the only one that can use self-hosted runners and make sure no other > workflows can > If there are no 'pull_request' workflows this is covered, correct? If so > I think we'd just ban those entirely. Here I am not sure actually. I've been thinking about it and I see a potential scenario when this is not enough.. I have not tested it yet what happens when you have "pull_request_target.yml" approved to work on self-hosted runner and someone makes a change in their PR and changes "pull_request_target" to "pull_request". I am not sure if GitHub would flag this in any way: * this protection is based on workflow file name, not the workflow "trigger" * the workflow gets unchanged, only the trigger changes in such PR So theoretically I see this scenario: * you only have one Pull_request_target workflow (ci.yml). * you configure project so only 'ci.yml' can be run as self-hosted * You think you are safe because the workflow will only run from main and this is the only workflow that can run on self-hosted runners So far, so good. However: * someone (non-commiter) changes `pull_request_target` trigger to be `pull_request` in a 'ci.yml'. The ci.yml remain the same, and is allowed to run on 'self-hosted'. The workflow is the same - it's just triggered differently. So as a result the non-commiter runs code from the PR of their on your self-hosted machine Maybe GitHub protects from that - I have not tested it. For sure I know you cannot submit a new "workflow.yaml" in a PR - it will not run as a "new workflow" - it has to be approved by a committer. But in this case this is THE SAME workflow as before - just triggered diffferently, and I changed the triggers for my past workflows multiple times and I never remember anything blocking it. If you could test that scenario - that would be awesome :) Maybe a chance for bug-bounty together if we prove the scenario is leaky :D BTW. The whole discussion we are having is basically a warning. If we need to reason about some secuirty scenarios in such a complex way - then very likely there are holes in the process that we have not thought about - that's why I am very sceptical about having anything on the "service" level that infra could support without Github finally hardening the approach on their own and providing more robust solutions. J.
