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

workflows/release-binaries: Stop using ccache #124415

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

tstellar
Copy link
Collaborator

@tstellar tstellar commented Jan 25, 2025

Using ccache relies on the GitHub Actions Cache, which may be susceptible to cache poisoning. See https://adnanthekhan.com/2024/05/06/the-monsters-in-your-build-cache-github-actions-cache-poisoning/

Even though these attacks may be difficult, it's better to err on the side of caution and ensure that the build environment for our releases is as isolated as possible. Additionally, ccache was only being used for the stage1 build, which is a small part of the overall build, so the speed up from using it was not that large.

Using ccache is a potential security risk, because the GitHub Actions
cache is writable by pull requests, which means that any GitHub user
could upload malicious data to the cache.
@llvmbot
Copy link
Member

llvmbot commented Jan 25, 2025

@llvm/pr-subscribers-github-workflow

Author: Tom Stellard (tstellar)

Changes

Using ccache is a potential security risk, because the GitHub Actions cache is writable by pull requests, which means that any GitHub user could upload malicious data to the cache.


Full diff: https://github.com/llvm/llvm-project/pull/124415.diff

1 Files Affected:

  • (modified) .github/workflows/release-binaries.yml (+1-11)
diff --git a/.github/workflows/release-binaries.yml b/.github/workflows/release-binaries.yml
index f9a264e7cf48f1..9e74610723f156 100644
--- a/.github/workflows/release-binaries.yml
+++ b/.github/workflows/release-binaries.yml
@@ -216,14 +216,6 @@ jobs:
       id: setup-stage
       uses: ./workflows-main/.github/workflows/release-binaries-setup-stage
 
-    - name: Setup sccache
-      uses: hendrikmuhs/ccache-action@ca3acd2731eef11f1572ccb126356c2f9298d35e # v1.2.9
-      with:
-        # Default to 2G to workaround: https://github.com/hendrikmuhs/ccache-action/issues/174
-        max-size: 2G
-        key: sccache-${{ runner.os }}-${{ runner.arch }}-release
-        variant: sccache
-
     - name: Configure
       id: build
       shell: bash
@@ -234,9 +226,7 @@ jobs:
             ${{ needs.prepare.outputs.target-cmake-flags }} \
             -C clang/cmake/caches/Release.cmake \
             -DBOOTSTRAP_LLVM_PARALLEL_LINK_JOBS=1 \
-            -DBOOTSTRAP_CPACK_PACKAGE_FILE_NAME="${{ needs.prepare.outputs.release-binary-basename }}" \
-            -DCMAKE_C_COMPILER_LAUNCHER=sccache \
-            -DCMAKE_CXX_COMPILER_LAUNCHER=sccache
+            -DBOOTSTRAP_CPACK_PACKAGE_FILE_NAME="${{ needs.prepare.outputs.release-binary-basename }}"
     - name: Build
       shell: bash
       run: |

Copy link
Member

@carlocab carlocab left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems fine to me, but note that GitHub Actions cache already has built-in protections against cache poisoning.

For example, caches from PR branches are quarantined from other branches so PRs generally cannot introduce malicious code into other PRs or onto the main branch until the PR is merged.

But we can merge this for now and revert when the cache has been audited properly.

@tstellar
Copy link
Collaborator Author

@carlocab Is there some documentation about how the quarantining works?

@tstellar
Copy link
Collaborator Author

@carlocab OK, thanks for the documentation links. So it sounds like the cache is a little more secure than I thought.

The benefit of using the cache is pretty low. It's only used for the stage1 builds, which is just a small part of the overall build time. So, I think it might be best to just not use it for the release binaries. In general, it's good practice to limit inputs into release artifacts and the cache is just another thing we need to try to secure.

@tstellar
Copy link
Collaborator Author

I've updated the commit message with a new justification for this change, since my previous message was misunderstanding how the caches work.

Copy link
Contributor

@boomanaiden154 boomanaiden154 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removing seems reasonable enough to me, but I'm also pretty sure this isn't a super fruitful attack vector given how PRs from forks I'm pretty sure push caches into the context of the fork.

Completely closing off the avenue is not a bid idea though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants