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

Restrict build_runner_image to pull_request instead of pull_request_t… #28745

Merged
merged 1 commit into from
Oct 2, 2023

Conversation

damccorm
Copy link
Contributor

…arget

Helps minimize risk of escalation of privileges (we may want to move to a dedicated secret for this in the future)

Fixes #27470


Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:

  • Mention the appropriate issue in your description (for example: addresses #123), if applicable. This will automatically add a link to the pull request in the issue. If you would like the issue to automatically close on merging the pull request, comment fixes #<ISSUE NUMBER> instead.
  • Update CHANGES.md with noteworthy changes.
  • If this contribution is large, please file an Apache Individual Contributor License Agreement.

See the Contributor Guide for more tips on how to make review process smoother.

To check the build health, please visit https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md

GitHub Actions Tests Status (on master branch)

Build python source distribution and wheels
Python tests
Java tests
Go tests

See CI.md for more information about GitHub Actions CI or the workflows README to see a list of phrases to trigger workflows.

…arget

Helps minimize risk of escalation of privileges (we may want to move to a dedicated secret for this in the future)
@github-actions github-actions bot added the build label Sep 29, 2023
@damccorm
Copy link
Contributor Author

R: @kennknowles @diogoteles08

@github-actions
Copy link
Contributor

Stopping reviewer notifications for this pull request: review requested by someone other than the bot, ceding control

@damccorm damccorm merged commit 2b20e26 into master Oct 2, 2023
2 checks passed
@damccorm damccorm deleted the users/damccorm/pull_request branch October 2, 2023 13:58
@diogoteles08
Copy link

@damccorm FWI, this change definitely solves the security concern I pointed at #27470 , but be aware that this might not work 100% as intended, as by default workflows triggered by pull_request don't have access to secrets, as pointed on github documentation. However, this other documentation specifies that secrets actually can be reached in case of pull requests made from an internal branch, and I don't know if that would be enough for your purposes.

@damccorm
Copy link
Contributor Author

damccorm commented Oct 2, 2023

I think that should be fine since everything we're doing in the pull request case is local (though, we may fail on the gcp auth step...). I'll take a look

@kennknowles
Copy link
Member

FWIW from my somewhat birds-eye-view:

  • it makes sense that our workflows primarily aimed at automation should run from master or release branches or tags and have higher privileges
  • workflows aimed at supporting code review & verification should be limited to arbitrary code execution, including sandboxed-but-fairly-arbitrary manipulation of cloud resources, namely apache-beam-testing project

This separation has fallen apart in the past mostly around testing PRs that change the automation-type workflows. But all the stuff like nighly builds and release automation that requires real privileges shouldn't need to run on PRs. And maybe it makes sense to have changes to those authored by committers who can open branches in the apache/beam repo.

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

Successfully merging this pull request may close these issues.

[Task]: CI: Review dangerous use of pull_request_target
3 participants