Skip to content

Commit

Permalink
Remove tokens from Circle CI config (#21058)
Browse files Browse the repository at this point in the history
Summary:
While these were intentionally used in the open, and never were abused, it has become a distraction whenever they are flagged.

We'll have to move this functionality to a service outside of Circle CI, as we cannot securely pass secrets to forks and PRs in Circle CI. By necessity, these PR analysis scripts must run alongside PRs.

The The controller you requested could not be found. token has already been revoked. The The controller you requested could not be found. token is not under our control, and is still valid as of this writing. The eslint token has public_repo scope, with no access to any private repos. It's no different than having a random account commenting on any public repo.

Unfortunately, revoking the The controller you requested could not be found. token affects React's use of this bot account as well.

 ---

Q: What does the React team need this token for?
A: It's used to analyze how a PR will impact the build size for React.

Q: What does the React Native team need this token for?
A: We do lightweight automated PR code reviews with it (eslint, flagging large PRs, etc)

Q: What can someone do with the access token?
A: The token was for the The controller you requested could not be found. GitHub account. The account has no privileged access to any organization, so in effect it's like having the token to a random GitHub account. The token has public_repo access scope, which allows it to interact with any public repository on GitHub. The attacker can leave comments on any issue, pull request, or commit, on any public open source repository on GitHub. They could spam or leave arbitrary messages. It's no different than using any random newly created GitHub account to do this, but the bot is named "React Linter", so people could have used it to make React look bad.

Q: Why didn't we just remove the token from the open source repositories? CircleCI allows you to use environment variables to keep secrets out of the repo.
A: We have configured CircleCI to hide environment variables from Circle CI jobs triggered by non-Facebook org forks and pull requests (otherwise, anyone could add a file to their fork that echoes $SUPER_SECRET and then read it from the Circle CI logs). This allows us to do things like publish to npm only on commits that actually land on the main repo, without letting random people do the same on their forks.

Q: Why can't we run these scripts on Circle CI jobs that do have access to secret environment variables?
A: It's by necessity. These scripts are meant to run on pull requests and forks. They're used to lint pull requests, after all.

Q: Why can't we run these scripts on internal Facebook infrastructure?
A: Automatic importing of arbitrary code from external sources into internal Facebook systems without a FB engineer's involvement  is disallowed. We're happy to let Circle CI run unvetted code in this manner.

Q: What do other projects do in similar situations?
A: A common solution for open source projects that need to run scripts with access to GitHub without exposing the access token on CI is to use a private cloud server (i.e. a droplet in Digital Ocean, an instance on AWS...).

Q: Why don't we use the same infra used by react-native-bot to run react-linter?
A: React-Native-Bot runs once an hour or so, querying for recent issues and PRs. It does not use webhooks, and instead performs the same kind of search queries you'd use on GitHub, therefore it's not great for picking up when a PR has been updated. Circle CI is great for running scripts whenever a PR is created or updated, as Circle outages aside, we can be fairly certain a script will run any time a PR is updated. If you want to track build sizes, you really want to make sure any new commit added to a PR will trigger a re-run.
Pull Request resolved: #21058

Differential Revision: D9809842

Pulled By: hramos

fbshipit-source-id: 6ca5d2f5b48e077ec822a3aea5237534bd828850
  • Loading branch information
hramos authored and facebook-github-bot committed Sep 13, 2018
1 parent cb87c3f commit 023d650
Showing 1 changed file with 8 additions and 4 deletions.
12 changes: 8 additions & 4 deletions .circleci/config.yml
Original file line number Diff line number Diff line change
Expand Up @@ -538,12 +538,18 @@ jobs:
# Analyze pull request and raise any lint/flow issues.
# Issues will be posted to the PR itself via GitHub bots.
# This workflow should only fail if the bots fail to run.
# The public github tokens are publicly visible by design
analyze_pr:
<<: *defaults
docker:
- image: circleci/node:10
environment:
- PATH: "/opt/yarn/yarn-v1.5.1/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin"
- PUBLIC_PULLBOT_GITHUB_TOKEN_A: "a6edf8e8d40ce4e8b11a"
- PUBLIC_PULLBOT_GITHUB_TOKEN_B: "150e1341f4dd9c944d2a"
- PUBLIC_ANALYSISBOT_GITHUB_TOKEN_A: "78a72af35445ca3f8180"
- PUBLIC_ANALYSISBOT_GITHUB_TOKEN_B: "b1a98e0bbd56ff1ccba1"

steps:
- checkout
- run: *setup-artifacts
Expand All @@ -554,10 +560,9 @@ jobs:
- run:
name: Analyze Code
command: |
# GITHUB_TOKEN=eslint-bot public_repo access token
if [ -n "$CIRCLE_PR_NUMBER" ]; then
echo -e "\\x1B[36mInstalling additional dependencies\\x1B[0m"; yarn add @octokit/rest@15.10.0
echo -e "\\x1B[36mAnalyzing code\\x1B[0m"; GITHUB_TOKEN="af6ef0d15709bc91d""06a6217a5a826a226fb57b7" ./scripts/circleci/analyze_code.sh
echo -e "\\x1B[36mAnalyzing code\\x1B[0m"; GITHUB_TOKEN="$PUBLIC_ANALYSISBOT_GITHUB_TOKEN_A""$PUBLIC_ANALYSISBOT_GITHUB_TOKEN_B" ./scripts/circleci/analyze_code.sh
else
echo "Skipping code analysis."
fi
Expand All @@ -567,11 +572,10 @@ jobs:
- run:
name: Analyze Pull Request
command: |
# DANGER_GITHUB_API_TOKEN=React-Linter public_repo access token
if [ -n "$CIRCLE_PR_NUMBER" ]; then
cd bots
yarn install --non-interactive --cache-folder ~/.cache/yarn
DANGER_GITHUB_API_TOKEN="80aa64c50f38a267e9ba""575d41d528f9c234edb8" yarn danger
DANGER_GITHUB_API_TOKEN="$PUBLIC_PULLBOT_GITHUB_TOKEN_A""$PUBLIC_PULLBOT_GITHUB_TOKEN_B" yarn danger
else
echo "Skipping pull request analysis."
fi
Expand Down

0 comments on commit 023d650

Please sign in to comment.