Skip to content

fix(stages-aw): security hardening for Stage 3 KMS grants, VPC-SC dry-run, and script eval - #377

Open
LeonxLJX wants to merge 3 commits into
google:mainfrom
LeonxLJX:fix/371-stages-aw-3-security
Open

LeonxLJX wants to merge 3 commits into
google:mainfrom
LeonxLJX:fix/371-stages-aw-3-security

Conversation

@LeonxLJX

Copy link
Copy Markdown

Fixes #371.

Summary

This PR addresses security and functional defects in fast/stages-aw/3-security and repository automation scripts.

Changes Made

Stage 3 Security (Terraform)

  • variables.tf: Extended service_accounts object type with optional gke-dev, gke-prod, gcve-dev, gcve-prod keys using optional(string, null).

  • core-dev.tf: Added var.service_accounts.gke-dev and var.service_accounts.gcve-dev to dev_kms_restricted_admins list so GKE and GCVE service accounts receive KMS admin permissions on development keys.

  • core-prod.tf: Added var.service_accounts.gke-prod and var.service_accounts.gcve-prod to prod_kms_restricted_admins list so GKE and GCVE service accounts receive KMS admin permissions on production keys.

  • vpc-sc.tf: Changed status from (!dry_run ? local.vpc_sc_perimeter : null) to always local.vpc_sc_perimeter. This preserves the enforced perimeter status when dry_run mode is enabled, preventing accidental removal of VPC Service Controls protection.

  • outputs.tf: Changed file_permission from "0644" to "0600" on the local tfvars file, and added kms_key_name to the google_storage_bucket_object resource so uploaded tfvars are encrypted with CMEK.

Scripts (Bash)

  • scripts/destroy.sh: Replaced all 3 instances of eval with printf -v for safe variable assignment, preventing potential command injection from interactive user input.

  • tools/create_e2e_sandbox.sh: Fixed typo tocuh → touch.

Security Impact

  • Eliminates shell eval injection vector in destroy script
  • Prevents accidental VPC-SC perimeter removal during dry-run mode
  • Restricts local tfvars file permissions from world-readable to owner-only
  • Encrypts uploaded tfvars output with customer-managed KMS key
  • Completes cross-env KMS admin grant coverage for all workload service accounts

- core-dev.tf/core-prod.tf: add gke/gcve service accounts to KMS restricted admins
- variables.tf: extend service_accounts type with optional gke/gcve keys
- vpc-sc.tf: preserve enforced perimeter status when dry_run=true
- outputs.tf: restrict local file permissions to 0600, add kms_key_name to uploaded tfvars
- destroy.sh: replace eval injection with printf -v for safe variable assignment
- create_e2e_sandbox.sh: fix typo tocuh to touch
@google-cla

google-cla Bot commented Oct 28, 2026

Copy link
Copy Markdown

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@github-actions github-actions Bot added the Scope - Fast Related to Fabric FAST deployment stages in the fast directory label Oct 28, 2026
@LeonxLJX

Copy link
Copy Markdown
Author

刚 force push 了 erraform fmt 修复 Repository Lint(for_each map key 对齐)。CI 会重新跑。

@LeonxLJX

Copy link
Copy Markdown
Author

Hi, I just signed the Google Contributor License Agreement (CLA). Could someone please re-run the \cla/google\ check? Thanks!

@aghassemlouei aghassemlouei added Bug Something isn't working Level of Effort - Low Quick, well-defined tasks with no unknowns; takes a few hours up to one day to complete Priority - High Critical issues blocking development or users; urgent bugs or core features for release Security Something is insecure or can be secured labels Oct 29, 2026

@aghassemlouei aghassemlouei left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Running terraform plan on fast/stages-aw/3-security and checking the shell updates for #371 surfaced three issues documented inline:

  1. fast/stages-aw/3-security/core-dev.tf still retains var.service_accounts.project-factory-prod inside local.dev_kms_restricted_admins, continuing to grant the production project-factory service account roles/cloudkms.admin on development KMS keys.
  2. fast/stages-aw/3-security/outputs.tf looks up a nonexistent default key on module.prod-sec-kms[keys(module.prod-sec-kms)[0]] for google_storage_bucket_object.tfvars, which silently evaluates to null and references Stage 3's security KMS keyring instead of Stage 0's automation outputs bucket KMS key.
  3. scripts/destroy.sh passes ${stage_id}_DESTROYED and $var_name to printf -v without validating identifier syntax, and items 4 and 5 of #371 in sa_lockdown.sh, validation.sh, scripts/restore.sh, and generate-mocks.sh remain open while the description uses Fixes #371. without the .github/pull_request_template.md sections.

Comment on lines 20 to +24
var.service_accounts.data-platform-dev,
var.service_accounts.project-factory-dev,
var.service_accounts.project-factory-prod
var.service_accounts.project-factory-prod,
var.service_accounts.gke-dev,
var.service_accounts.gcve-dev,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

var.service_accounts.project-factory-prod is still present in local.dev_kms_restricted_admins, which continues granting the production project-factory service account roles/cloudkms.admin on development KMS keys per item 1 of #371. Remove var.service_accounts.project-factory-prod from core-dev.tf so dev_kms_restricted_admins contains only development service accounts:

Suggested change
var.service_accounts.data-platform-dev,
var.service_accounts.project-factory-dev,
var.service_accounts.project-factory-prod
var.service_accounts.project-factory-prod,
var.service_accounts.gke-dev,
var.service_accounts.gcve-dev,
var.service_accounts.data-platform-dev,
var.service_accounts.project-factory-dev,
var.service_accounts.gke-dev,
var.service_accounts.gcve-dev,

Comment on lines +50 to +53
bucket = var.automation.outputs_bucket
name = "tfvars/3-security.auto.tfvars.json"
content = jsonencode(local.tfvars)
kms_key_name = try(module.prod-sec-kms[keys(module.prod-sec-kms)[0]].keys.default.id, null)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

try(module.prod-sec-kms[keys(module.prod-sec-kms)[0]].keys.default.id, null) has three issues:

  1. var.kms_keys defaults to {} and has no guaranteed default key entry, so kms_key_name silently evaluates to null during terraform plan.
  2. keys(module.prod-sec-kms)[0] selects whichever KMS location sorts first lexicographically, such as global or us, which mismatches the regional location of var.automation.outputs_bucket.
  3. var.automation.outputs_bucket lives in the Stage 0 automation project var.automation.project_id, whose Cloud Storage service agent holds roles/cloudkms.cryptoKeyEncrypterDecrypter on Stage 0's gcs key projects/${var.automation.project_id}/locations/${var.regions.primary}/keyRings/gcs/cryptoKeys/gcs used by 0-bootstrap and 1-resman, not on Stage 3's prod-sec-kms keys.

Comment thread scripts/destroy.sh
log_info "Marking $stage_id as complete..."
# Update current session variable
eval "${stage_id}_DESTROYED=true"
printf -v "${stage_id}_DESTROYED" "true"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Guard stage_id here and var_name in prompt_val at lines 424 and 428 with [[ "$stage_id" =~ ^[A-Za-z_][A-Za-z0-9_]*$ ]] before calling printf -v, since printf -v evaluates array subscripts inside non-validated variable names in Bash.

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

Labels

Bug Something isn't working Level of Effort - Low Quick, well-defined tasks with no unknowns; takes a few hours up to one day to complete Priority - High Critical issues blocking development or users; urgent bugs or core features for release Scope - Fast Related to Fabric FAST deployment stages in the fast directory Security Something is insecure or can be secured

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] fast/stages-aw/3-security, scripts: fix cross-env KMS admin grant, VPC-SC dry-run status, and script input handling

2 participants