CSC + Github Actions secrets issue

[thinkl33t]

Problem

CSC is a public repository, mostly consisting of terraform files, which uses the standard github fork-and-pr workflow. This means that when a user wants to make a change, they fork the entire repository to their own github, make the changes there, then open a pull request to the finos CSC repository.

When a Pull Request is opened, we have Github Actions set up to perform a few different actions. First we do a terraform formatting check, to make sure all the formatting is correct, then we do a terraform verify, which makes sure all the terraform in the folder we're looking at is valid and makes sense.

Assuming these two pass, we then try to actually run the terraform against a live system to see if it works - this is where the problem lies. To access the live AWS instance we have to supply some secrets. These secrets give access to add and remove resources from the CSC AWS instance whice we are running terraform against. This means that these secrets have to remain... secret, otherwise they can be used by bad actors to perform actions like spinning up crypocurrency miners which FinOS end up paying for!

If any PR raised ran with access to these secrets, anyone would be able to exfiltrate the secrets trivially, by echoing out the secret's contents as part of thier pull request, and viewing the CI run log. To prevent this, github actions doesn't allow access to secrets when a pull request has been raised from a forked repository.

What we've tried

We have tried setting up github actions environments, which allows access to secrets to be gated behind a reviewer OKing the run. Unfortunately this has the same security settings, which cannot be changed, so even when access to the secrets has been allowed they still aren't supplied.

We have also tested the pull_request_target event. While this does allow the sharing secrets, it runs the workflow from the target of the pull request. Meaning any changes to the terraform definitions may be tested against an established workflow (i.e. from the 'main' branch), any contributions to the workflows from forked repositories are still unable to be tested.

Solution

Using the pull_request_target event type alongside environments, we can be fairly confident that attempted secret exfiltration would be noted in review before being run.

As a form of documentation / review safeguarding for this, we should make sure any MR that modifies the github actions explicitly prevents the apply / destroy type tasks from being run, and have something reviewers have to acknowledge so they know why this has happened.

If we wanted to go further with this we could block any PRs with modifications to both the actions AND the terraform files from passing CI.

[abdullahgarcia]

Hi @thinkl33t ,

Thanks for raising this issue.

Can you please have a look at this:

• https://github.com/aws-actions/configure-aws-credentials

Thanks!

Abdullah

[benjamb]

After a bit of reading around, this does seem quite promising.

[benjamb]

I've looked into this, and while it is cool that you can us it to avoid putting any AWS secrets in the repository settings, this still does not work as a solution for forks.

For this to work, the GITHUB_TOKEN needs to have write permissions on the id-token scope, however forks are only ever provided read-only access: https://docs.github.com/en/actions/security-guides/automatic-token-authentication#permissions-for-the-github_token

[maoo]

Hi @thinkl33t ! I've shared this already, but probably got lost somewhere in the Slack chat.

As you mention, this limitation is in reality a security "feature".

What I did in the past for other projects was to set a condition in the GitHub action build steps, based on the GitHub username (or org) where the PR os coming from. If it's not "finos", everything but the deployment can be executed, ie the first 2 build steps you mention. Here's some github action code that shows the syntax:

docusaurus.yml
        # Extract GitHub org/user and patch siteConfig.js only if not "finos"
        REPO="${{ github.repository }}"
        REPO_ORG=${REPO%"/"*}
        if [ "$REPO_ORG" != "finos" ]
        then echo "Repo organization is '$REPO_ORG', patching docusaurus configuration to work on a forked repo" ; curl ${{ env.PATCH_SCRIPT_URL }} |  bash -s -- ${{ env.REPO_NAME }} $REPO_ORG

Check https://github.com/finos/open-developer-platform/blob/master/.github/workflows/docusaurus.yml#L43-L48

By using a separate, dedicated branch, say deploy, the deployment build step (your third step) would only trigger if a change happens in this branch. The deploy branch can be protected by direct commits, in order to avoid unwanted builds.

As a result, "external" PRs will not trigger deployments (which is not necessarily a bad thing, but I'm eager to hear team's opinion), but only static validation; if and when that passes, a CSC team member will raise a new PR against the deploy branch, and that will trigger a deployment.

As soon as the change is deployed on deploy branch, and tests pass, the change can hit the main branch.

WDYT?

[eddie-knight]

+1 ...but with a caveat.

From what you wrote (@maoo), the flow looks like this:
contributor's fork ->PR1-> ??? ->PR2-> deploy ->automerge or PR-> main

We should answer where the first PR would go, prior to the internal PR to deploy.

Or alternatively, we could say that the validation branch is the target users will make PRs to.

We could call it anything... deploy, dev, validate, etc. Code reviews and basic CI would happen from fork to dev, then deployment and post-deploy tests would happen on the PR from dev to main.

The revised workflow would be:
contributor's fork ->PR1-> dev ->PR2-> main

[maoo]

Agreed! I'd suggest to make it easy and clear for the contributor, so, as you suggest, declaring one branch as the "PR target".

It's always easy for a CSC team member, while validating the PR, to test it against other branches; so, if there's need to re-apply a PR, the CSC team could do it.

[benjamb]

Just to play devil's advocate here: if the subsequent PR to main then fails to pass CI, what is the workflow? If a PR merged into dev breaks CI, then other open PRs are blocked.

One option is to revert the entire pull request that broke CI and then notify that author to perform any relevant fixes and open a new one. A downside of this being that the resulting git history for the project would become quite messy (no one gets it right first time 😄).

Another option would be for the maintainer that opened the dev -> main PR to make these amendments themselves. However I will note that reviewer time is currently in short supply, and this process will only increase their load.

[maoo]

Thanks @benjamb for the feedback, you raise valid points. As I was getting a bit lost, I wrapped up a bit the conversation, hopefully we can better pinpoint what we agree upon and where we need more conversations.

Branches:

• main - latest and greatest CSC version; all code in here passed all static and end-to-end tests. Only accepts PRs from the deploy branch. Master is also the target branch for contributors to build PRs
• deploy - never behind master; used to trigger deployments and "runtime checks" of incoming PRs

PR workflow:

1. A contributor checks out CSC main branch
2. A contributor makes and submits changes via PR against the main branch
3. A GitHub action automatically checks syntax, CLA and more, without involving any new deployment or runtime check
4. If checks pass, a CSC team member runs a manual review and approves the PR, by editing it and rebasing it against deploy (since deploy is never behind main, this operation won't cause any conflict)
   4.1. If checks don't pass, the reviewer will help contributor adapting the contribution
5. A GitHub action is triggered when the deploy branch is updated (see step 4). If the PR succeeds, the CSC reviewer must submit a PR from deploy to main (to make sure they're not out of sync).
   5.1 If the PR on step 5 doesn't succeed, we'd either need to revert the code (to avoid blocking other PRs that are running in parallel), or work closely with the contributor

@benjamb - you're referring to step 5.1, correct? I feel that it may be too premature to deal with this issue, as we never faced it; I'd vouch for building the workflow, testing it out and see how to better deal with these cases while we face them. I know it sounds a bit naive, but there are many factors to take in consideration.

WDYT?

[benjamb]

Yes, that's what I was referring to. While it perhaps may not be an issue for this repository right now, I don't think it's at all uncommon for untested code to have bugs. IMHO in order to better accommodate a growing community, these things are worth thinking about sooner rather than later.

[benjamb]

Another couple of potential options for enabling CI for forks:

• Self-hosted runner: with either the credentials baked into the instance, or run as an EC2 instance that has an IAM role applied.
• GitHub bot: a helper bot could be used to trigger the job (ideally still requiring an approval process from reviewers), passing the required credentials as inputs (via the GitHub REST API).

[maoo]

Interesting alternatives, thanks for sharing!

They both require some sort of runtime, so I guess these activities will have to be designed, planned and prioritized by the CSC team. I reckon they could bring an additional level of automation to process incoming PRs (ie, skipping the PR steps against the deploy branch), though it could be overkill if the volume of incoming PRs is low (would be interesting to know the amount of expected "external" PR).

Regardless of the solution, I think it would be useful (and maybe required?) to provide the CSC team with the ability to decide, for each PR, whether to run the tests against cloud providers or not. I'd be happy to hear your (and CSC team's) opinion on this, because this choice will impact the design of the solution.

Thanks!

[benjamb]

I think in the ideal world, the above options would make the deploy branch redundant, but yes would involve some level of effort to implement.

Absolutely! I think any changes to the terraform definitions or GitHub workflows for example ought to be vetted by a CSC member prior to being run. While you could opt to require approval for all workflows triggered by pull requests from forks via the repository setting, there is the potential for finer control with the GitHub bot.

[thinkl33t]

I agree with Ben here, a process where as much as possible of it can be automated using a bot makes a lot of sense. It removes a number of manual steps, thereby helping reduce mistakes and making the process of getting a pull request reviewed and approved a lot quicker and easier from a reviewer's point of view.

Of course, there would still have to be manual gates in place, such as the bot only attempting a deployment once a reviewer has given the go ahead to do so, but once this review has happened there shouldn't need to be any more requirement for the reviewer to do any manual steps to land the PR in main

[peterrhysthomas]

Hi all a few extra points to throw in:

1. FINOS have some standards around contribution, CLA for example which needs to be conformed with - @maoo can probably comment more on how that runs, I don't know if this would work outside of the FINOS repos. This is usually performed on any PR prior to merge.
2. Do we actually want to run all our tests against any branch/fork. If we apply a normal SDLC model - we don't usually run all our tests on a branch - rather we run a minimal set of tests and then after the merge run the remaining on the main branch. That way we get the idea of 'code stable' rather than 'system stable' for the branch*. So in that case we would run the terraform verifications and maybe terraform unit tests (like OPA testing) if we add those, but any live-environment testing we don't do until it goes to master.
3. Another concern isn't exfiltration of credentials, but rather just exploitation - for example, could I fork the repo and modify the terraform and use the credentials to build my own system which mines bitcoin or more simply, I could cause a DoS on the environment by just continually raising PRs. Or maybe just a minor typo could spin up the environment and not tear it down properly.

• ideally you would run all tests on the branch, but in practise that management of fully ephemeral environments for each branch isn't always possible.

Any thoughts?

[eddie-knight]

I hope these comments shed light on some of your concerns above.

Regarding point 2:
- Running tests on main is sufficient if we have some sort of badge showing that main is green / passing all validation tests
- It should be noted that in this approach, main will not always be green, because some changes may pass the pre-deploy tests, but fail the deployment or compliance validation tests.
- If we need main to always be green, we should shift the testing to a pre-main branch where the tests can be executed by a maintainer

Regarding point 3:
- Whether the deployment tests run on main or pre-main, we will ideally ensure that those tests only run as a post-merge action on a specific protected branch
- As a post-merge action, the tests will only be possible after the pre-merge tests pass, code has been approved by a maintainer, and the PR has merged. As such, there is little opportunity for malicious activity (DoS, exfil, etc)

Regarding the final bullet:
- pre-merge CI should happen for all PRs, as it is low-cost
- post-merge CI to create ephemeral environments should only happen for PRs that have been successfully merged to a specific protected branch