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 👍 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...