Open Stack

Hi Christophe,

I've checked and we do actually have a couple of GitHub actions using the "pull_request_target" event:

- Action to enforce the PR porting labels [1]
- Action to move all issues to "in progress" when a PR references them [2]

Note that both these actions are in *all* our repos.


# What is the "pull_request_target" event?

If you want a GitHub acction to trigger when a pull request is opened, the obvious choice seems to be the "pull_request" event. However, GitHub documents in [2] that:

> Pull request events for forked repositories
> Note: Workflows do not run on private base repositories when you open a pull request from a forked repository.

That's a problem because all the PRs the community raises are on "forked repositories".

Enter the "pull_request_target" event. The event *does* work with "forked repositories" and allows actions to run in the context of the PR rather than the main repo. However, quoting from [3],

> Warning: The pull_request_target event is granted a read/write repository token and can access secrets, even when it is triggered from a fork.

This seems somewhat odd given the event appears to have been created to make actions safer and you have to wonder if GitHub might introduce a new event that sits somewhere between "pull_request" and "pull_request_target" at some point.

______

Taking each of our actions in turn...


# Porting labels action

## What is it for?

The action checks to ensure that the PR is correctly labelled with respect to porting labels [3]. Note that the action is ultimately only checking to ensure the PR has a number of labels set.

## Can we survive without it?

I'd say yes. The action is only reminding the community of the need to perform the porting - it cannot enforce it due to the complexities of bi-directional porting and timing [3].

## Can the action be run without "pull_request_target"?

I think it could. I'm going to test this and see if we can change the event to "pull_request" instead...


# Move issue to in progress label

## What is it for?

The action is triggered when a PR is opened and if that PR has an associated (linked) issue number (aka a "fixes #XXX" comment), that issue is resolved and moved into the issue backlog GitHub board [1]. GitHub allows us to move newly opened issue into the project automatically, but the GitHub API does not support "linked (issues|prs)" well, hence the need for the action.

## Can we survive without it?

Possibly. This was added specifically for the project board [1]. But since creating it, we have been hampered somewhat by GitHub's very restricted set of queries (filters) that can be run in a project. In fact, the weekly issue backlog "scrub" doesn't use that project - it uses a simple GitHub query (which is not restriected in the filters that can be applied).

Ariel may wish to comment on this further.

## Can the action be run without "pull_request_target"?

afaik, no it cannot. That event is required to allow access to the PR. However, I'll re-test to see if that is still the case...


# TL;DR

The all important summary ;) fwics, a "rogue PR" that attempts to install a bad action that does naughty things won't actually get run until it is itself merged in the main branch. The bad code would only run on the *next* PR raised after the bad PR was merged. This is good and bad. However, looking on the positive side, it seems that if we want to keep these two actions there are two relatively easy changes we can make:

1) Adopt GitHub's suggestion to make our actions only trigger when labelled with something like `safe-to-test`

   This effectively makes the actions "manually triggered".

2) Rotate the GitHub secrets (effectively the "uid" the actions run as) and check they have the absolute minimum set of privileges.


Cheers,

James

[1] - https://github.com/orgs/kata-containers/projects/23
[2] - https://docs.github.com/en/actions/reference/events-that-trigger-workflows#pull_request
[3] - https://docs.github.com/en/actions/reference/events-that-trigger-workflows#pull_request_target

---
https://katacontainers.io/ | https://github.com/kata-containers
Open Source Technology Center
Intel Corporation (UK) Ltd. - Co. Reg. #1134945 - Pipers Way, Swindon SN3 1RJ.

________________________________________
From: Christophe de Dinechin <dinechin at redhat.com>
Sent: Tuesday, 19 January 2021 08:02
To: kata-dev at lists.katacontainers.io
Subject: [kata-dev] GitHub actions security recommendations

Hello,


I don't know if you are aware of the security recommendations in this paper: https://securitylab.github.com/research/github-actions-preventing-pwn-requests.

I thought this was interesting notably in the context of the discussion regarding unifying testing/CI and code repos. While it is true that a single repo makes it possible to commit a test along with the change, it also means a malicious PR now has a much larger attack surface, something that I personally was not aware of.

Food for thought.


Thanks,
Christophe


_______________________________________________
kata-dev mailing list
kata-dev at lists.katacontainers.io
http://lists.katacontainers.io/cgi-bin/mailman/listinfo/kata-dev
---------------------------------------------------------------------
Intel Corporation (UK) Limited
Registered No. 1134945 (England)
Registered Office: Pipers Way, Swindon SN3 1RJ
VAT No: 860 2173 47

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.