I think it could be solved by requiring maintainers to kick off the CI job for every PR coming from a first-time contributor manually. I would not be annoyed by this provided that regular contributors don’t have to go through this. I think GH can further reduce the impact of this by not applying the limitation to first-time contributors who have lots of followers, forks, PR reviews, commits in the last year.
Also, edits to the CI script could be made suspect. I never had a first-time contributor on my projects start by making changes to the CI pipeline.
I am pretty sure smart folks at GH thought of this and just deliberating whether to introduce such a breaking flow to maintainers.
I think that’d definitely help. The Kubernetes project requires a maintainer to reply with /ok-to-test for the CI bot to begin testing a PR. This also helps reduces load on their CI systems, as the project gets a lot of commits.
What's to stop attackers from making a one-off harmless edit ("forgot a comma in readme") and then, once they're whitelisted, deploying a malicious executable to the CI pipeline.
I think the root issue is that people without write access to your repo can queue arbitrary compute on your dime by simply creating a PR and changing the GitHub workflow files (the definition for GitHub actions). This is even a bigger issue for companies with self-hosted runners who can't use those for public repos as an attacker could file a PR with malicious code and compromise a machine (https://docs.github.com/en/actions/hosting-your-own-runners/...)
One possible solution here is that a maintainer needs to okay any change that changes a workflow file before it runs. Not sure if that introduces other problems...
Or put the action definitions outside of the repository for which they apply.
I see the advantages of having CI configuration right next to the code, but once you start deploying multiple branches or accepting outside contributors, the downsides start to outweigh the benefits.
Having the maintainer trigger the CI job moves the problem elsewhere. Attackers will create new repositories (or take over existing ones) and create the PRs, for which they will trigger the CI jobs.
The more apparent problem is that CI jobs can execute arbitrary code and are not limited wrt. their execution time. If limited, it would render them useless when used for cryptomining.
In this specific case, a simple rule change should suffice: CI code committed for the first time does not run until after the PR closes. These guys are exploiting a loophole where you can add CI code and it runs whenever you initially open the PR. Requiring the PR to be closed before the code runs would solve this.
> Requiring the PR to be closed before the code runs would solve this.
That's not going to work! You want to make sure all the tests that run as part of the CI pass before you merge. What you can do is to make a blanket ban on auto-running the CI pipeline if the CI config was changed till the maintainer clicks Run Actions.
This simple rule change would defeat the purpose of running CI CD on external contributions: I want to see if the tests run and everything is up to my quality standards. I don't want to manually trigger the pipeline, that adds around 5 minute to every PR I receive...
However, as an attacker, I can still execute anything I want. Sure, maybe it's not as convenient as replacing the yml file, but I could embed a script in the tests that will just mine as long as possible.
The point is that you didn't solve anything, you just ruined CI CD
> that adds around 5 minute to every PR I receive...
I am sure not every PR you get touches files under '.github/workflows'. Are you sure you were replying to me and not @_fat_santa? It's his approach that ruins CI/CD as far as tests are concerned.
> Sure, maybe it's not as convenient as replacing the yml file, but I could embed a script in the tests that will just mine as long as possible.
Yes, but that slows things down a lot. Now you need to write a fake unit test that spawns a process and that will require to clone a project, get a project to build, writing different code for different programming languages and unit test frameworks...
Also, edits to the CI script could be made suspect. I never had a first-time contributor on my projects start by making changes to the CI pipeline.
I am pretty sure smart folks at GH thought of this and just deliberating whether to introduce such a breaking flow to maintainers.