github-ci-bootstrap: require an explicit write_branch_patterns list - #35
Open
jwbron wants to merge 1 commit into
Open
github-ci-bootstrap: require an explicit write_branch_patterns list#35jwbron wants to merge 1 commit into
jwbron wants to merge 1 commit into
Conversation
The default trusted both main and master, but a repository only protects the branch it actually uses; the other name is an ordinary branch anyone with push access can create, and a workflow on it could impersonate the read/write service account and deploy to production without review. We verified this end to end in Khan/culture-cron (default branch master): a pushed branch named main authenticated with the rw workload identity provider and ran a real terraform apply. Making the variable required forces each caller to name its real, protected deploy branch. Breaking change: callers must now pass write_branch_patterns explicitly, e.g. ["master"].
jwbron
added a commit
that referenced
this pull request
Jul 20, 2026
…36) ## Summary Khan/actions#274 changes `generate-terraform-plan`/`apply-terraform-plan` to store the Terraform binary plan in GCS instead of committing it to git (a binary plan embeds a full copy of the Terraform state, including sensitive values in cleartext). Every consumer of those actions needs the same bucket with the same security posture, so this PR has the bootstrap module create it, next to the CI service accounts and WIF providers it belongs with, instead of each repo hand-rolling it. ## What the module now creates (opt-out via `create_terraform_plans_bucket = false`) - Bucket `terraform-plans-{org}-{repo}-{service}` (same normalization as the state bucket; overridable via `terraform_plans_bucket`) in `khan-internal-services`, with uniform bucket-level access and public access prevention enforced. - Always per-service, never shared: plan files contain that service's state, so a shared bucket would let each service's CI read the others' state. - `roles/storage.objectAdmin` on the bucket for the read/write service account only (it uploads on plan, downloads and deletes on apply). The read-only account used for PR-branch plans gets no grant. - Lifecycle rule deleting objects after `terraform_plans_expiration_days` (default 30); applied plans are already deleted by the apply action, this catches superseded plan PRs. - New output `terraform_plans_bucket` to feed the actions' `plan_bucket` input. ## Rollout Additive and enabled by default: consumers get the bucket on their next module bump plus a local bootstrap `terraform apply`. For culture-cron, which already created `khan-culture-cron-terraform-plans` by hand in its bootstrap (Khan/culture-cron#28) during testing, adoption means passing `terraform_plans_bucket = "khan-culture-cron-terraform-plans"` and moving the two resources into the module in its bootstrap state: terraform state mv google_storage_bucket.terraform_plans 'module.github_ci_bootstrap.google_storage_bucket.terraform_plans[0]' terraform state mv google_storage_bucket_iam_member.ci_rw_terraform_plans_object_admin 'module.github_ci_bootstrap.google_storage_bucket_iam_member.ci_plans_bucket_access_rw[0]' Related: #35 (make `write_branch_patterns` explicit); if both land together, tag a single `github-ci-bootstrap-v2.0.0`. Author: jwbron Reviewers: csilvers Required Reviewers: Approved By: csilvers Checks: ✅ 1 check was successful Pull Request URL: #36
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
github-ci-bootstrapdefaultedwrite_branch_patternsto["main", "master"], so the read/write workload identity provider trusted both branch names. A repository only protects the branch it actually uses; the other name is an ordinary branch that anyone with push access can create. A workflow on that branch can impersonate the read/write service account and deploy to production without any review.This is not theoretical: while testing Khan/actions#274 in Khan/culture-cron (default branch
master), we pushed a branch namedmain, and it authenticated with the rw provider and ran a realterraform applyagainst production state. That test exploited exactly this gap.Change
write_branch_patternsis now required with no default (plus a non-empty validation), and the docs/example spell out the rule: list only branches that are protected in the GitHub repository, normally just the default branch, and never bothmainandmaster.Breaking change / rollout
Callers must pass
write_branch_patternsexplicitly (e.g.["master"]) when they bump to the release containing this; existing pins are unaffected until then. Suggest tagging asgithub-ci-bootstrap-v2.0.0. Known callers to update when they bump: culture-cron, beep-boop, internal-services, internal-webserver bootstrap stacks (each also needs a localterraform applyof its bootstrap to tighten the existing WIF provider condition).Note: callers can close the gap today without waiting for this release by passing
write_branch_patterns = ["master"]with their current pin; this PR makes the safe configuration mandatory rather than optional.