terraform: store binary plans in GCS instead of committing them - #274
Conversation
…ad of committing them A binary plan embeds a full copy of the Terraform state, including sensitive values in cleartext, and the tip-delete cleanup never removed it from git history. generate-terraform-plan now uploads the binary to a GCS object keyed by a slug of the terraform path plus the plan's commit SHA, and the plan PR commits only the redacted plan text plus a pointer file recording that SHA. apply-terraform-plan reads the pointer, rebuilds the object path from its own inputs, downloads the exact reviewed plan, applies it, and best-effort deletes the object during cleanup.
🦋 Changeset detectedLatest commit: 17a88ad The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
This means whoever has access to the gcs bucket can see these secrets. How is that access controlled? |
We don't need to give anyone access to the gcs bucket, just the deploy service account, which already has access to the secrets anyway. |
Responds to review feedback on how bucket access is controlled: a binary plan holds the same sensitive data as the Terraform state, so the bucket must be restricted exactly like the state bucket (CI service accounts only, no human-facing grants).
There was a problem hiding this comment.
Nice - thanks for getting the quick fix in! Reviewing based on the similar mechanism we use on data-infra.
(might be worthwhile for others to chime in too - bearing in mind actions is a public repo)
We don't need to give anyone access to the gcs bucket, just the deploy service account, which already has access to the secrets anyway.
Yep this make sense. In some data-infra repos we use GCS to store our state also, using the TF GCS backend (doc: https://docs.cloud.google.com/storage/docs/samples/storage-remote-terraform-backend-template) - since we want this to work across different repos' Terraform config, this GHA is explicitly copying the state file, which makes sense.
I guess the trick is to ensure that we provide the right GCS bucket in the repos in the input to ensure minimal privileged access.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
- Delete the GCS object only after the cleanup PR merges, so a cleanup failure can't leave the base branch pointing at a missing object. - Record the plan's sha256 in the pointer and verify it after download, binding the applied bytes to the reviewed plan. - Hardcode the object key's filename segment to tfplan.binary on both sides so it can't depend on mismatched local filename inputs. - Suffix the object-path slug with a short hash of the raw terraform path so lossy slugs can't collide across stacks. - Pin the duplicated slug pipelines to each other with a unit test (published tags contain one action's directory, so the two actions can't share a helper file). - Emit a workflow warning when apply skips due to missing plan files, for consumers who half-migrate their trigger paths.
…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
Review Guidancegithub-actions (2 files)
|
| # the object at this key could in principle have been overwritten | ||
| # since the plan PR was approved (e.g. by a re-run of the plan | ||
| # workflow on the same commit after state drift). | ||
| actual_hash=$(sha256sum "${{ inputs.plan_file_path }}" | cut -d' ' -f1) |
There was a problem hiding this comment.
suggestion (non-blocking): The digest-verify-before-apply path — the security control this PR is built around — has no test. Nothing in utils/*.test.ts exercises the pointer parse (sed -n '1p'/'2p'), the ^[0-9a-f]{40}$/{64}$ validators, or this actual_hash != plan_hash comparison; CI runs only vitest. If the comparison were ever inverted or the pointer's two lines swapped, an unreviewed plan would apply and no test would catch it. The test file already extracts and execSync-es action shell lines for the slug pipeline, so this logic is testable the same way: round-trip a written pointer, assert the validators reject a 63-hex digest / 41-char SHA / whitespace, and assert the mismatch branch exits 1.
| # The object's filename segment is a fixed literal on both sides so | ||
| # the key can't depend on the two actions' independently configurable | ||
| # local filename inputs. | ||
| object="gs://${{ inputs.plan_bucket }}/${slug}/${{ github.sha }}/tfplan.binary" |
There was a problem hiding this comment.
suggestion (non-blocking): The object-key test pins the slug pipeline and the /tfplan.binary suffix, but not the full path skeleton. It only asserts each object= line ends in /tfplan.binary, so a reorder in one action (e.g. (redacted) here vs ${slug}/${sha}/...in apply) would keep all tests green yet make every apply reconstruct a different key and 404 — exactly the drift the suite exists to prevent. Consider asserting the twoobject= lines are structurally identical after normalizing the one intended difference (${{ github.sha }}vs${plan_sha}`).
| # the key can't depend on the two actions' independently configurable | ||
| # local filename inputs. | ||
| object="gs://${{ inputs.plan_bucket }}/${slug}/${plan_sha}/tfplan.binary" | ||
| gcloud storage cp "$object" "${{ inputs.plan_file_path }}" |
There was a problem hiding this comment.
note (non-blocking): Re-running this workflow on the same commit after a completed apply is not idempotent. Once apply succeeds, the cleanup PR merges, and the object is deleted, a GitHub "Re-run all jobs" re-checks out the same commit — whose committed pointer the cleanup PR never touched — so check_files sets skip_apply=false and this gcloud storage cp 404s on the deleted object. With no continue-on-error, the re-run fails here rather than skipping cleanly. This is inherent to the (correct) delete-after-apply ordering; the bucket lifecycle rule and a fresh plan are the intended recovery. Flagging only so operators know re-runs are not safe to retry blindly.
| # The object's filename segment is a fixed literal on both sides so | ||
| # the key can't depend on the two actions' independently configurable | ||
| # local filename inputs. | ||
| object="gs://${{ inputs.plan_bucket }}/${slug}/${plan_sha}/tfplan.binary" |
There was a problem hiding this comment.
suggestion (non-blocking): Optional design thought, take it or leave it: the object could be content-addressed by the plan's sha256 (`(redacted) instead of slug + commit SHA. The pointer's digest is already the trust anchor apply enforces, so this keeps the tamper property, subsumes the per-stack collision concern the slug hash-suffix exists for, and would let you drop the duplicated slug pipeline in both actions plus most of the pinning test. One tradeoff to weigh: a content-addressed key no longer scopes by stack, so a tampered pointer could select any plan ever uploaded to the bucket rather than only this stack's — the current "at worst another plan for this same stack" property would widen. Non-blocking; the current scheme is correct as written.
| @@ -0,0 +1,102 @@ | |||
| import {describe, it, expect} from "vitest"; | |||
| import * as realFs from "node:fs"; | |||
There was a problem hiding this comment.
nitpick (non-blocking): These builtin imports use the node: prefix, but every other file in utils/ imports builtins bare (e.g. build.test.ts: import * as fs from "fs"). Consider matching for consistency:
| import * as realFs from "node:fs"; | |
| import * as realFs from "fs"; | |
| import path from "path"; | |
| import {execSync} from "child_process"; |
…l + write-only (#37) ## Summary The scheduled-job module reads `Slack__API_token_for_alertlib` with a `data "google_secret_manager_secret_version"` to build each job's Slack notification channel. Terraform state stores the full data-source response, including `secret_data`, so the token value sits in every consumer's state and every saved plan file; this is how the token was exposed by the committed-tfplan incident (see Khan/actions#274 and the consumer migrations). ## Change The channel stays fully Terraform-managed; the leak is fixed with the ephemeral/write-only mechanism (https://developer.hashicorp.com/terraform/language/manage-sensitive-data/ephemeral): - the token is read with `ephemeral "google_secret_manager_secret_version"` (never persisted to state or plans), and - written to the channel via the write-only `sensitive_labels.auth_token_wo` + `auth_token_wo_version` arguments, added to `google_monitoring_notification_channel` in provider 7.19.0. Rotation becomes explicit: after adding a new secret version, bump the new `slack_token_rotation` variable; the next apply re-reads the latest version and rewrites the channel token. (The old data-source design also only picked up new versions on the next plan, so nothing is lost; the bump makes the cutover reviewable.) An earlier revision of this PR added a `notification_channel_ids` input for pre-created channels; that is dropped in favor of keeping channel management in Terraform. ## Requirements and rollout - Terraform >= 1.11 (write-only arguments) and hashicorp/google >= 7.19.0; the module's `required_providers` now enforces both. - Suggest releasing as `scheduled-job-v0.4.0`. - Consumer follow-ups (culture-cron, beep-boop, internal-webserver; five jobs total, all currently embedding the token in state): bump the module pin, and bump the google provider pin from `7.8.0` to `>= 7.19.0` in the two stacks that pin exactly (no committed lock files, so that is the whole change). The adoption plan should show the channel's sensitive labels updating in place; after apply plus a state refresh, the token value is gone from live state. - Rotating the alertlib token (rotation runbook item #4) is still required regardless, since historical plan files already leaked it; after consumers adopt this version, the runbook's #4 mechanics change from "drift plan" to "bump slack_token_rotation in each repo". Author: jwbron Reviewers: nathanjd Required Reviewers: Approved By: nathanjd Checks: ⏭️ 1 check has been skipped Pull Request URL: #37
3 participants
Summary
A Terraform binary plan file is a zip that embeds a full copy of the Terraform state, including sensitive attribute values in cleartext, regardless of what the plan actually changes.
generate-terraform-planhas been committing that file to consumer repos in the plan PR, and theapply-terraform-plancleanup only deletes it from the tip; every plan ever merged is still in git history. This PR stops committing the binary entirely and moves it to an access-controlled GCS object instead, while keeping the review-then-apply-on-merge flow intact.How it works now
generate-terraform-planuploads the binary togs://<plan_bucket>/<slug of terraform_path>/<commit SHA>/tfplan.binaryand deletes the local copy. The plan PR commits only the redactedtfplan.txt(unchanged, still the human review content) plus a new pointer file (defaulttfplan.commit) containing the commit SHA the plan was generated from.apply-terraform-planreads the SHA from the pointer, validates it, rebuilds the object path from its ownplan_bucketinput, downloads the exact plan that was reviewed, and applies it as before. The cleanup PR removes the pointer and text files, and the action best-effort deletes the GCS object.gs://URI. Apply reconstructs the path from workflow-configured inputs, so a tampered pointer can at worst select another plan previously uploaded for the same stack; it cannot point apply at an arbitrary object. This is strictly better than before, when a tampered PR could swap the entire binary.terraform_path(same slug logic as the per-stack cleanup branch) so multiple stacks in one repo cannot collide.auto_approvemode are unchanged and do not need a bucket.google-github-actions/authwith WIF, which configures the preinstalledgcloudCLI automatically.Breaking changes (major bump for both actions)
plan_bucketinput on both actions, required for the plan-PR/apply flow. Both actions must be upgraded together.tfplan.binarystill committed at a consumer repo's tip should be deleted manually.Follow-up (not in this PR)
This only stops new leaks. Consumer repos that ever merged a plan PR have binary plans, and therefore cleartext state, in git history today; those histories need scrubbing (git filter-repo/BFG) and rotation of any secrets present in the embedded state.