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

Improve security around ACTIONS_RUNTIME_TOKEN (Part 1) #1146

Merged
merged 2 commits into from
Aug 5, 2024

Conversation

aomarks
Copy link
Member

@aomarks aomarks commented Aug 3, 2024

Background

Wireit's GitHub Actions caching integration works by talking to the GitHub caching HTTP service, which is secured through a secret token. GitHub Actions automatically provides this token to re-usable actions like this one (google/wireit@setup-github-actions-caching) through the ACTIONS_RUNTIME_TOKEN environment variable. However, it does not expose this environment variable to the calling workflow, which is where Wireit is actually running, and is where we actually need it.

Previously, our solution was to read the ACTIONS_RUNTIME_TOKEN environment variable from the called-workflow, and simply expose it directly to the calling-workflow by writing it to the GITHUB_ENV file (see https://docs.github.com/en/actions/writing-workflows/choosing-what-your-workflow-does/variables).

This is not ideal because that token is sensitive, and we would like to minimize the chance of it leaking out. One way it can leak is if a process logs all of its environment variables.

(Note that the GitHub Actions runner is smart enough to know that this token is sensitive, so it masks out the value with ***s when it appears in the runner's own logs. However, this doesn't help if the environment variables are logged in some other way.)

Changes

Now, instead of exposing the ACTIONS_RUNTIME_TOKEN variable directly, we instead start up a "custodian" HTTP server, which lives for the duration of the workflow, and whose only role is to respond to any HTTP request with the token. We then expose the WIREIT_CACHE_GITHUB_CUSTODIAN_PORT environment variable to the calling-workflow (this provides a positive signal that the custodian is running, and lets us be dynamic with port assignment). Wireit processes can then make a fetch to the custodian service to read the token.

Alternatives considered

An alternative considered was to write the token to a known location on disk, like $RUNNER_TEMP/.wireit-caching-github-token. This would solve for the case where environment variables are being logged, but introduces another way the token could leak out (one can imagine a GitHub Action that uploads the temp directory somewhere for some kind of debugging reason). The HTTP approach should be much less liable to leak than either of the other approaches.

Plan

  • Land this PR (note that the GitHub Action lives on its own setup-github-actions-caching branch, which is the base for this PR).
  • Land Improve security around ACTIONS_RUNTIME_TOKEN (Part 2) #1147 which updates Wireit to support both the old and new approaches, and warns if the old method is detected.
  • Release -pre versions of each to test caching still works for real.
  • Release v2 of the action (which is done simply by Git tagging).
  • Release a patch version of Wireit.
  • Sometime in the future: remove support for the old approach.

@aomarks aomarks requested a review from rictic August 3, 2024 00:41
@aomarks aomarks changed the title Improve security around ACTIONS_RUNTIME_TOKEN (Part 1/2) Improve security around ACTIONS_RUNTIME_TOKEN (Part 1) Aug 3, 2024
@aomarks aomarks merged commit 729430a into setup-github-actions-caching Aug 5, 2024
1 check passed
@aomarks aomarks deleted the custodian-action branch August 5, 2024 19:11
aomarks added a commit that referenced this pull request Aug 5, 2024
The new GitHub caching setup system (see #1146) is not working on Windows. The Wireit processes are unable to fetch from the local custodian server.

I'm not sure why yet, but in the meantime this release will make it non-fatal when GitHub caching fails to set up.

Also shows more info when a fetch error occurs, and fixes an issue with environment variable inheritance in our integration tests.
aomarks added a commit that referenced this pull request Aug 13, 2024
Fixes Windows support for GitHub Actions Caching, which has been broken since the security improvement in #1146. (This only affected users who had upgraded to v2 of the action, and the failure was not fatal, it just disabled caching).

The problem was that under Windows, {detached: true} is not sufficient for keeping the server alive after the parent shell exits. The parent shell will exit as soon as the main launching script is done, because the action is invoked through a GitHub Actions uses: clause (as opposed to run: where we would be sharing our parent shell with subsequent steps).

We now use start so that the server is not killed when our parent shell exits: https://learn.microsoft.com/en-us/windows-server/administration/windows-commands/start.

Also listens only on localhost instead of all hosts (shouldn't matter because we should have strong network isolation for free through the GitHub Actions container, but seems wise, and good in case this code runs in some other context for some reason).
aomarks added a commit to breadboard-ai/breadboard that referenced this pull request Aug 13, 2024
aomarks added a commit to lit/lit that referenced this pull request Aug 13, 2024
Part 1 of bringing in the security improvement from google/wireit#1146. I will update the GitHub Action version in a follow-up PR, because doing both at once breaks the tachometer workflow (because tachometer is trying to build main but with this PR's GitHub Action version, which is incompatible until the wireit version is bumped).
aomarks added a commit to lit/lit that referenced this pull request Aug 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants