Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add functionality for allow self-hosted runner to protect workflow file and allow only execution only for collaborators for PRS #494

Closed
MalloZup opened this issue May 20, 2020 · 69 comments · May be fixed by #783
Labels
extra-large Service Feature Feature scope to the pipelines service and launch app

Comments

@MalloZup
Copy link

MalloZup commented May 20, 2020

Description:

Hi all,
first of all I really like the github action runner and self-hosted runner.

It has however an issue with working for pull-request workflows.

I think the runner should have a way to be configurable to run only on Pull-requests from collaborator of X org or Repo.
The problem to be solved is to protect the github-workflow file and don't be changed by any arbitrary PR ( so there is no output redirection or other code executed)
it is a recurring topic in some forums, but there is no solution or any issue about this afaik. let me know, and ping me for any info .

best

@MalloZup MalloZup added the enhancement New feature or request label May 20, 2020
@MalloZup MalloZup changed the title Add functionality for allow self-hosted runner to protect workflow file and allow it execution only for collaborators. Add functionality for allow self-hosted runner to protect workflow file and allow only execution only for collaborators for PRS May 20, 2020
@TingluoHuang
Copy link
Member

@MalloZup sounds like some policy feature we can add on the service side even before sending the job to the runner.

@chrispat for feedback.

@MalloZup
Copy link
Author

MalloZup commented Jun 8, 2020

@TingluoHuang that could be awesome thx!

@plfiorini
Copy link

This would likely solve the security concerns of self-hosted runners

@MalloZup
Copy link
Author

MalloZup commented Jun 8, 2020

yes this is the goal 🌞

@chrispat
Copy link
Member

chrispat commented Jun 8, 2020

@MalloZup I don't believe this makes sense as a runner feature but rather something that is enforced on the GitHub.com side. There are scenarios where customers want this type of feature for hosted runners and not just self-hosted.

@TingluoHuang TingluoHuang added Service Feature Feature scope to the pipelines service and launch app and removed enhancement New feature or request service labels Jun 8, 2020
@MalloZup
Copy link
Author

MalloZup commented Jun 9, 2020

@chrispat yes I agree with you partially.
I believe globally it make sense to have it on GIthub.com side.
However I think that also the runner can still have it own way to setup this.

Is like when developing a CLI command line.
Let assume the Github.com is our configuration file, from which the user setup option for our CLI. ( the CLI is the runner)

If an user use the CLI command line arguments, this overwrite the configuration file input (github.com).

In any ways, even if it you will implement globally you will need to instruct and implement the mechanism on the runner, afaik and correct me if I'm wrong.

Do you have a link where there is the Github.com UI source code where I can open such issue for the global part?

I think we could continue with this issue implementation on the runner side, since lot of user are depending on this to have a pragmatic/secure CI.

Personally the UI github.com implementation, from my perspective is the last part and imho it will take much more to implementation time then implementing such feature on the runner ( which is just a partial feature).
Having such feature on the runner side, is just a step imho that it will need to be done also if the options are passed from github.

Thanks for your answer and time ping me for any info or I can help.
🌻

@chrispat
Copy link
Member

chrispat commented Jun 9, 2020

@MalloZup Given the architecture of Actions the runner is not the best place to implement this feature. The runner is a relatively small part of the overall Action system and is designed to be a remote execution daemon for the Actions workflow.

In the case of GitHub hosted runners, self-hosted runners or runners that spin up on demand you waste a lot of time and money by spinning up a runner on a VM or in a container to make a simply policy check that should be done by the GitHub Actions workflow engine.

@MalloZup
Copy link
Author

@chrispat do you know where I could open this issue for the github workflow engine?

Thank you in advance

@iagomelanias
Copy link

iagomelanias commented Jun 11, 2020

That feature would be extremely valuable to our team. 👍

We use Github Actions Runner labels to decide which infrastructure will be responsible for processing the job, some servers have more permissions than others. Here are some examples:

  • There are runners which are responsible for provisioning resources using Terraform. It's possible to do literally anything inside of them.
  • Some runners are responsible for deploying code to our stage environment.
  • Some runners are responsible for deploying code to our prod environment.
  • Other runners are responsible for only running tests using some cloud resources (storage, messaging, etc).

If a developer can change the labels and the jobs that are executed, all these security policies are thrown away. It would be possible to simply change the label of a test workflow to use a terraform runner and do anything there - delete a database, get access to any server or obtain any kind of sensitive data, like database passwords or private API keys. 💀

@MalloZup
Copy link
Author

@chrispat or @TingluoHuang I could not find a place where I can move the issue in more appropriate place. If you have any feedback regarding this , it would be extremely welcome. thx!

@chrispat
Copy link
Member

We don’t have a great place to track enhancements to the orchestration side of things as that is not open source.

We hope to have a public roadmap soon but the granularity of that may or may not account for these types of features.

@Penagwin
Copy link

We've been waiting for this feature since December. Our devs work out of forked repos, so we're unable to run tests on pull request because they're separate forks, even though everyone is apart of the organization that's private.

This means we have no feedback before merging stuff in.

@chrispat
Copy link
Member

@Penagwin that is a different issue and it is something we are working on enabling. There will be some additional settings in your org and repos that you will be able to enable that will allow runs from forks of private repos.

@Penagwin
Copy link

Penagwin commented Jun 22, 2020

@chrispat Is there an issue number I can track or an eta? I know covid threw a wrench in everything, but we've been waiting 8 months now...

@qinsoon
Copy link

qinsoon commented Jun 24, 2020

We are eager to see this feature as well. I am also wondering if there is a workaround/hack for now to protect workflow files from unprivileged contributors. I found none... If anyone has a suggestion, I am happy to dig it deeper.

@MalloZup
Copy link
Author

MalloZup commented Jun 24, 2020

@qinsoon this feature could be implemented on the runner. SO yes it is possible to protect the workflow file without having to wait of unknown Feature coming from the github gui, which imho will not come really soon.

for reference take this comment:
#494 (comment)

This is why I was also thinking that implementing it on the backend, is a more low-hanging fruit which will help the github action to become useful for people in short-term, without compromising the design of longterm ( taking this from gui).

So even if forking is always sub-optimal, I guess it would be ok to try it.

if anyone is willing to do this, create a LINK here so people can try it at least. 👍

@MalloZup
Copy link
Author

MalloZup commented Jun 24, 2020

as I imagine the feature , it could be like this in the minimal way:

  1. add a flag called like github_users="GINO;BAMBY;SUPERMAN" this flag is added to the action runner when started;
    ( if an user removal or addition, the admin of the worker need to restart the worker with new flag, but I guess this is not really time consuming)

only the user of github users can modify the workflow file.

The logic would be to have something #494 (comment) like stated on that comment.

I am not familiar to C# code and also maybe the runner itself would be need some developer documentation in order to explain its design and make it more accessible for the community to contribute, imho.

@chrispat
Copy link
Member

These features can't be implemented in the runner in an efficient and secure way. We need to check these sorts of permissions before we allocate a VM. Also the runner is a hostile environment that can execute arbitrary code from an end user and really should not be considered for any sort of security boundary.

Unfortunately, there is no way for me to give you an issue to track for new features in the GitHub.com code base.

@MalloZup
Copy link
Author

thx @chrispat fair enough. lets keep this issue open and maybe give update it here. enjoy.

@michhyun1
Copy link

We've been waiting for this feature since December. Our devs work out of forked repos, so we're unable to run tests on pull request because they're separate forks, even though everyone is apart of the organization that's private.

This means we have no feedback before merging stuff in.

I think this in itself needs to be its own issue. What's the point of a rep owner creating CI workflow github actions if that workflow doesn't run when an external customer submits a PR from a forked repo branch? I noticed the docs say 'private' base repo, but does it work for a public base repo?

Is there a GH issue for this that is currently being tracked?

@derekbelrose
Copy link

I was thinking the best way to handle this is to have a check box or something that requires CODEOWNER approval before running any webhooks in branch protections. This is a problem that exists not with just Github Actions but with Travis and CircleCI. A change to the respective workflows in a PR can trigger the workflow and they all use the PR as the basis for the workflow tasks.

So, I think being able to defer the triggering of webhooks until approval is a mitigation step. Make it off by default so not to break existing workflows.

@eladchen
Copy link

eladchen commented Jul 25, 2020

How about this:

image

An example:

Let's say .github/workflows/pull-request.yml exists, and the suggested checkbox above is checked, and
user John / DEV team is configured.

Workflow ".github/workflows/pull-request.yml" will not be executed if:

  • A pull request is created based of a forked repository
  • The pull request changed files include .github/workflows/pull-request.yml
  • The pull request is not authored (triggered?) by user John / a member of DEV team.

This allows contributors to create pull requests, and have workflows provide them (and maintainers) with feedback on the changes introduced by the pull request, and, in the same time preventing untrusted parties from changing a workflow, thus preventing any security issues that might arise.

WDYT?

ashb added a commit to ashb/runner that referenced this issue Jan 8, 2021
This addresses actions#494 at the runner level to give "just enough protection"
to allow using self-hosted runners with public repos.

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 added a commit to ashb/runner that referenced this issue Jan 8, 2021
This addresses actions#494 at the runner level to give "just enough protection"
to allow using self-hosted runners with public repos.

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 added a commit to ashb/runner that referenced this issue Jan 13, 2021
This addresses actions#494 at the runner level to give "just enough protection"
to allow using self-hosted runners with public repos.

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 added a commit to ashb/runner that referenced this issue Jan 15, 2021
This addresses actions#494 at the runner level to give "just enough protection"
to allow using self-hosted runners with public repos.

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 added a commit to ashb/runner that referenced this issue Jan 15, 2021
This addresses actions#494 at the runner level to give "just enough protection"
to allow using self-hosted runners with public repos.

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 added a commit to ashb/runner that referenced this issue Jan 15, 2021
This addresses actions#494 at the runner level to give "just enough protection"
to allow using self-hosted runners with public repos.

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.
AdamOlech pushed a commit to antmicro/runner that referenced this issue Jan 28, 2021
This addresses actions#494 at the runner level to give "just enough protection"
to allow using self-hosted runners with public repos.

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.
AdamOlech pushed a commit to antmicro/runner that referenced this issue Jan 28, 2021
This addresses actions#494 at the runner level to give "just enough protection"
to allow using self-hosted runners with public repos.

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.
themarpe pushed a commit to luxonis/runner that referenced this issue Jan 31, 2021
This addresses actions#494 at the runner level to give "just enough protection"
to allow using self-hosted runners with public repos.

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.
@c4deszes
Copy link

My use case would be running hardware tests using Github Actions, the only workaround I can see right now is using https://github.blog/changelog/2020-12-15-github-actions-environments-environment-protection-rules-and-environment-secrets-beta/ to have a protected environment. But these approvals depend on the service where you deploy rejecting the request if someone makes a PR where the environment section is removed from the workflow.

You could use this by having the runner under a restricted user and use the environment secrets to elevate it's access, but you would have to make sure they can't do anything malicious under the restricted user which is probably very hard.

@ashb
Copy link

ashb commented Feb 15, 2021

But these approvals depend on the service where you deploy rejecting the request if someone makes a PR where the environment section is removed from the workflow.

Different use case, but same worry is exactly the reason why I've been "carrying" my own patch for this for running Apache Airflow - #783

@chrispat
Copy link
Member

We have added some additional settings that may be a good solution to this problem. Please take a look at https://github.blog/changelog/2021-07-01-github-actions-new-settings-for-maintainers/ and see what you think.

@ashb
Copy link

ashb commented Jul 27, 2021

The run approval doesn't change anything about our worflow on the Apache Airflow team -- we are happy with the existing behaviour for PRs (run for non-committers on public runners) -- we just want to not have our release process held up in the shared queue with the apache org, and don't want to have to worry about doing detailed (security) review of a PR before running it on our self-hosted runners.

@aschleck
Copy link

It's really surprising to me that GitHub sets users up for failure here. All I really want is to have the workflow configuration always pulled from HEAD on my main branch and then run against the PR. I get that this makes it a little harder to iterate on workflows, but that's not the common case of workflow runs at all and I don't think it makes sense to optimize for it. Alternatively I'd take options to require approval when the PR modifies .github. As it is, I am actively looking to move my organization off of GitHub Actions which is really unfortunate.

@akikanellis
Copy link

From my understanding, the conditions for a potential security breach are like so:

  • The repo is public
  • Any self-hosted runner is accessible in the repo (either via repo or organisation settings)
  • The repo has defined at least one workflow file with pull_request trigger
  • The repo maintainer has not checked Require approval for all outside collaborators

Then a malicious actor can simply open a PR from a forked repo where they add a new workflow file like so:

name: Malicious Workflow

on:
  pull_request:
    branches: [ main ]

jobs:
  malicious-job:
    runs-on: self-hosted

    steps:
      - name: Run a malicious script
        run: |
          echo Malicious script running from **forked** repo

The script will run on the available self-hosted runner of the maintainer's repo.

The best (and most cumbersome) way to prevent this is to check Require approval for all outside collaborators in the repo settings as explained here. Then you have to manually approve workflows running on each PR after you have first checked the PR for any changes to workflow files.

Another way to mitigate the risk would be to have a private "companion" repo to your public repo that the self-hosted runner is assigned to as explained here but I haven't tried this myself.

I want to echo that it would be great if there was a way to lock down a self-hosted runner and not allow it to every run on PRs.

@domenkozar
Copy link

What about:

  • set a repo secret IS_TRUSTED to XXX
  • write ACTIONS_RUNNER_HOOK_JOB_STARTED script to check if IS_TRUSTED secret matches and if not, abort the job

@13013SwagR
Copy link

I opened a feature that would solve this and seems fairly simple to implement, see https://github.com/orgs/community/discussions/53430

@ruvceskistefan
Copy link
Contributor

Hey @MalloZup,

Thank you for your interest in the runner application and taking the time to provide your valuable feedback. We kindly ask you to redirect this feedback to the GitHub Community Support Forum which our team actively monitors and would be a better place to start a discussion for new feature requests in GitHub Actions. For more information on this policy please read our contribution guidelines.

@13013SwagR already opened the ticket here but you can add new one to the GitHub Community Support Forum.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
extra-large Service Feature Feature scope to the pipelines service and launch app
Projects
None yet
Development

Successfully merging a pull request may close this issue.