Add runner-level config to enforce PR security

[ashb]

This closes #494 at the runner level to give "just enough protection" to allow using self-hosted runners with public repos.

This PR could do with a bit of a tidy up (some strings should likely become constants/enums, and a few things should possibly be added as methods on the GitHubContext class) but I wanted to get a :+1: from the approach first.

(Also C# is not my "native" language so there may be some oddities in here)

By default, the current behaviour is unchanged -- all jobs passed to the runner are executed.

If the .runner config file has this block added to it:

  "pullRequestSecurity": {}

Then by only PRs from "CONTRIBUTORS" (as defined by the field in https://docs.github.com/en/free-pro-team@latest/graphql/reference/objects#pullrequest -- nothing for us to have to work out ourselves.)

It is also possible to explicitly list users that are allowed to run jobs on this worker:

  "pullRequestSecurity": {
    "allowedAuthors": ["ashb"]
  }

Or to only allow the given users, but not all contributors:

  "pullRequestSecurity": {
    "allowContributors": false,
    "allowedAuthors": ["ashb"]
  }

Owners of the repo are always allowed to run jobs.

[ashb]

@chrispat Tagging you since you commented on #494.

I agree that solving this at the GitHub level would be a better solution, but on apache/airflow we are struggling without being able to use self-hosted runners (we share an actions queue with every other Apache Software Foundation project using Github, so leads to rather unpredictable and uncontrollable queue lengths.)

So here is a "maybe good enough approach" -- happy to redesign things/rework as needed, but I think this is a good stop-gap measure.

Thoughts?

[ashb]

(Oh and of course this will need tests adding.)

[ashb]

Question: should these security settings also apply to branch builds?

[pietroalbini]

This is similar to what we're doing to use self-hosted runners in the Rust project. Our current solution of using a fork with a patch similar to this is far from ideal, as we need to be quick to rebase our changes on top of the latest releases before the runner stops working due to the auto-update being prevented. Not having to rebase the fork every week or two by having a patch land in the runner would be awesome.

[ashb]

@pietroalbini Would love to collaborate on this. Does this PR do everything you need for Rust?

[pietroalbini]

We don't plan to execute any pull request build on self-hosted runners, so a toggle to just reject all PRs (even if those are sent by owners) would be best for Rust. For reference this is the patch on our fork we're currently running in prod.

[ashb]

Gotcha, yes that makes sense. I'll roll that in to my PR.

[hross]

This has been open a while as a draft (and original issue) so I thought I would add some commentary here so you know that we are talking/thinking about this. A couple of thoughts:

• Practically speaking, I like this approach because it is simple and accomplishes something nice for a class of customer/use case.
• I agree with @chrispat that we should address this on the service, but there is obviously a much higher barrier there (UI, experience, etc).
• If we ship a feature like this, with a config variable, in the runner we are locked into supporting the data contract... forever, for all intents and purposes (if this ships to an on premises customer, for instance, there is a long support timeline). So we have to be very deliberate about what contracts we introduce, even though introducing them is pretty easy.
• This might be lulling you into some level of false security, since the runner config itself could be modified by some malicious entity and then used to run PRs that shouldn't be run, unsuspectingly. If these settings are stored at the server level there is no local way to modify them and would close this vector of attack.

So TLDR for now is I don't think we would merge this, but this is a great concept that we will talk about internally and see how we can tackle a better experience.

I also want to mention we really appreciate your contribution and willingness to dig into the code so please don't take us not merging the PR as a reason to not continue to push on interesting ideas like this.

[pietroalbini]

Thanks for looking into this @hross!

This might be lulling you into some level of false security, since the runner config itself could be modified by some malicious entity and then used to run PRs that shouldn't be run, unsuspectingly. If these settings are stored at the server level there is no local way to modify them and would close this vector of attack.

I'm not sure that's actually a threat this PR is meant to solve. My understanding is, this PR would successfully prevent people without write access to the repository from ever submitting a build to the runner, so the only way the configuration change could happen is if the malicious entity already has write access to the repo (which allows them to just push a branch instead of opening PRs).

Also, if the runner is executed on a persistent VM then a malicious entity with the ability to change the configuration could also reconfigure the runner to point to a different repository without the server side limitation. If the runner is executed on an ephemeral VM instead, the configuration change wouldn't matter as the change wouldn't persist across executions.

[hross]

You are totally right about this, of course (and all of your other comments):

Also, if the runner is executed on a persistent VM then a malicious entity with the ability to change the configuration could also reconfigure the runner to point to a different repository without the server side limitation. If the runner is executed on an ephemeral VM instead, the configuration change wouldn't matter as the change wouldn't persist across executions.

It's more about what does an inexperienced user think about a "security setting" and do they understand the implications of what you're explaining? Do we design for that (ideally yes). In any case it's something we have to think about before we'd release a setting like this as "official".

[pietroalbini]

It's more about what does an inexperienced user think about a "security setting" and do they understand the implications of what you're explaining? Do we design for that (ideally yes). In any case it's something we have to think about before we'd release a setting like this as "official".

I see your point!

A possible solution would be to call this a "filter" instead of a security feature, so that people who stumble into it without any extra context won't be misled by its name. Then, when the feature is documented on docs.github.com some extra context can be added there to explain what this feature does and doesn't protects against.

I thought about possible designs for the configuration with the new "filter" naming:

• A simpler one which only allows choosing between everyone, collaborators or nobody:

  {
    "pullRequestsFilter": "allowFromEveryone|allowFromCollaborators|reject"
  }
  

• A more complete but potentially more confusing one, which allows to configure more complex filters:

  {
    "pullRequests": {
      "action": "allow|filter|reject",
      "filters": {
        "collaborators": true,
        "authors": ["pietroalbini", "ashb"]
      }
    }
  }
  

I'm also really happy to collaborate with y'all to brainstorm possible documentation

[ashb]

Having now started using this "in anger" the current config scheme I have in this PR is clearly not right.

[ashb]

I've just run in to a less-than ideal behaviour (which is likely only a problem for the apache org and any other umbrella orgs): the MEMBER author_association wins out over the contributor (or more accurately, my user is not a a contributor but a member)

This means that for us, it would allow every apache author to be able to run on our self-hosted runners, which isn't what we want -- just anyone able to commit to the repo is what we are aiming for.

[jens-maus]

@ashb I have followed this PR here and find your enhancement to the actions/runner very useful. It is really a pity that GitHub seems to ignore it so far because I think it would be great if they could add some more security for using self-hosted runners also for public repos. And your PR seem to provide some quite straight forward way of securing the use of self-hosted runners in public repos and I still hope GitHub will once pickup your idea and implement it because we really need some additional level of security here for using self-hosted runners also in public repos!

Nevertheless, do you have any resource where you publish own/separate builds of actions/runner with your PR modification? This would allow to use these instead of the GitHub version of actions/runner which is know to be prone to the pull-request security issue we all know.

[ashb]

Nevertheless, do you have any resource where you publish own/separate builds of actions/runner with your PR modification? This would allow to use these instead of the GitHub version of actions/runner which is know to be prone to the pull-request security issue we all know.

My fork is set up with automation (GitHub actions of course) to automatically build a release with my changes incorporated when ever a release is published in the upstream repo https://github.com/ashb/runner/releases

[jens-maus]

That's great! And if using your runner instead of the standard github one it will also automatically update itself then from your fork and not switch back to the official one?

[ashb]

That's great! And if using your runner instead of the standard github one it will also automatically update itself then from your fork and not switch back to the official one?

No, not yet sadly. I haven't worked out the message/mechanism that GitHub use to out that message (or the contents of that message) so I cant "redirect" the update.

[jens-maus]

No, not yet sadly. I haven't worked out the message/mechanism that GitHub use to out that message (or the contents of that message) so I cant "redirect" the update.

That's indeed sad. I also couldn't quickly grep for the update url or similar in the runner source code. Too bad that GitHub isn't providing more documentation on the actual runner implementation. Do you know at least of a way to disable the automatic update? Because otherwise changes are high that if I run your PR secured runner variant it will be automatically updated to the official runner once github released a new one. And this would remove the additional PR security of your work...

[ashb]

Do you know at least of a way to disable the automatic update? Because otherwise changes are high that if I run your PR secured runner variant it will be automatically updated to the official runner once github released a new one. And this would remove the additional PR security of your work...

Yeah, something like this would do it https://github.com/rust-lang/gha-runner/commit/c9740d87fe593ad86f31a7a900b4a7216a42d7a7

[ashb]

Aand it turns out that this author_association is not always reliable either: https://github.community/t/github-actions-have-me-as-contributor-role-when-im-owner/138933/3

[ashb]

For anyone following along and using my PR/releases I have just made a change to make the decision based on the actor, not the PR author. From my testing this seems to be the authenticated user who performed the git push (so is not spoofable by changing git commit author /committer).

In practice this means that if a repo own force pushes a PR opened by someone else, that job will be eligible for running on self-hosted runners (depending on what runs-on is set to in the Workflow yaml of course)

[jens-maus]

@ashb Thanks for the update! Do you have any ready-to-use howto on using your modified runner instead of the default github runner with the PR security issue? And potentially also explaining how to disable the self-updates or how to change it to actually perform the self-update based on your modifications?

[ashb]

@jens-maus Not yet, no, I've not gotten around to writing it up. I've been focused on ironing out the bugs in having my own self-hosted runners (nothing to do with this PR, mostly all around the auto-scaling etc.)

[jens-maus]

@ashb Ok, would be great to have such a short howto at leaset attached to the PR in the top entry here so that anyone can grab it and use your runners with improved PR security rather than waiting for GitHub to implement something similar or merge your PR.

BTW: @hross, @chrispat, any chance on getting some improvements here? It's really a pity that self-hosted runners cannot be safely used in public repositories because of this known security issue. Shouldn't really take so long to get such an additional security thing implemented so that public OSS projects can use self-hosted runners here on GitHub, isn't it?

[codersasha]

We need this for our use case as well. We have a private repo, but use GitHub Actions runners to deploy to our various environments (we have self-hosted runners on those environments). We don't want our employees to be able to run these 'sensitive' runners without following our formal review process, which includes merging the PR.

We were recently able to execute arbitrary code on our repo (and hence on our self-hosted runners) before the code was able to be reviewed, since GitHub actions runs the GitHub actions as they are in the PR, not on main.

[mrexodia]

How do you deal with updates @ashb ? Your fork looks like something I definitely want to use, but it would automatically replace itself whenever GitHub feels like it with the official release and renders my runner vulnerable right?

https://github.com/rust-lang/gha-runner/commit/c9740d87fe593ad86f31a7a900b4a7216a42d7a7 seems to be fine, but that's not in your patch right?

[ChristopherHX]

Yes this fork would update itself once the github actions service deploys an update, while my fork doesn't link to an very old runner, which claims to have version 3.x.x

Alternativly change the runner version number file of this fork to 3.0.0, build it and it won't update anytime soon. See my other comment https://github.com/actions/runner/issues/246#issuecomment-917376652 It is better than the plain rust-lang workaround, the runner won't stop receiving jobs if it is outdated.

To preserve the actual runner version in job logs see https://github.com/NixOS/nixpkgs/pull/137673