Skip to content

ci: restrict self-hosted runner jobs to non-fork PRs#2096

Closed
adilburaksen wants to merge 1 commit into
GoogleContainerTools:mainfrom
adilburaksen:fix/ci-fork-pr-gate
Closed

ci: restrict self-hosted runner jobs to non-fork PRs#2096
adilburaksen wants to merge 1 commit into
GoogleContainerTools:mainfrom
adilburaksen:fix/ci-fork-pr-gate

Conversation

@adilburaksen
Copy link
Copy Markdown

Summary

Jobs ci-build-test and ci-images in ci.yaml, and diff in image-check.yaml, run on the distroless-ci-large-ubuntu-22.04 self-hosted runner. These jobs check out the PR author's code and execute it (Bazel BUILD rules, private/tools/diff/diff.bash) on the runner without any fork restriction.

An attacker who opens a PR from a fork can modify BUILD files to include a malicious genrule, or modify private/tools/diff/diff.bash, and have that code execute on the self-hosted runner. Self-hosted GCP runners have GCE service account credentials accessible via the metadata server (http://metadata.google.internal/).

Affected jobs (no fork guard at job level):

  • ci.yamlci-build-test (runs fork's Bazel targets + bazel test //...)
  • ci.yamlci-images (runs fork's Bazel image tests)
  • image-check.yamldiff (runs bazel build //:sign_and_push + ./private/tools/diff/diff.bash from fork)

Note: image-check.yaml has fork checks on comment-posting steps, but not on the build/execution steps where the actual risk exists.

Fix

Add if: github.event_name != 'pull_request' || github.event.pull_request.head.repo.full_name == github.repository to each affected job:

  • push and workflow_dispatch events are unaffected
  • Fork PRs skip the self-hosted runner jobs entirely

Test plan

  • Verify CI passes on a same-repo PR after this change
  • Verify that fork PRs skip ci-build-test, ci-images, and diff jobs (expected: jobs show as skipped in workflow run)

Jobs ci-build-test, ci-images (ci.yaml) and diff (image-check.yaml)
run on distroless-ci-large-ubuntu-22.04 self-hosted runners and execute
code from the checked-out repository (Bazel BUILD files, diff.bash).
Without a fork guard, fork PRs trigger these jobs, giving fork code
access to the runner's GCP credentials.

Add an if condition to each affected job so that pull_request events
from forks are skipped. workflow_dispatch and push events are unaffected.
@gemini-code-assist
Copy link
Copy Markdown
Contributor

Note

Gemini is unable to generate a review for this pull request due to the file types involved not being currently supported.

@loosebazooka
Copy link
Copy Markdown
Member

I think you have made incorrect assumptions on how our builds work and are gated.

@adilburaksen
Copy link
Copy Markdown
Author

Thank you for the feedback — could you clarify what mechanism you're referring to?

From the workflow YAML as currently written:

ci.yamlci-build-test and ci-images jobs have no fork guard at the job level and run unconditionally on distroless-ci-large-ubuntu-22.04 for any pull_request event, including from forks. They execute bazel build $targets, bazel test //..., and bazel build //examples/... against the fork's code.

image-check.yaml — the diff job runs on distroless-ci-large-ubuntu-22.04 unconditionally. The step-level github.event.pull_request.head.repo.fork checks (lines 76, 85, 92, 110) only gate PR comment posting, not the execution steps: bazel build //:sign_and_push and ./private/tools/diff/diff.bash both run for fork PRs.

If the distroless-ci-large-ubuntu-22.04 runner genuinely has no GCP credentials accessible to fork-submitted code (e.g., no GCE SA, no Workload Identity, no push access to gcr.io/distroless/), then our concern about credential theft is moot and we're happy to close this PR. We just want to understand which assumption is incorrect.

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