sec(iac/gcp): scope gcp-sa-impersonation to least-privilege commitment roles

iac/federation/gcp-sa-impersonation/terraform/main.tf

@@ -21,17 +21,70 @@ resource "google_service_account_iam_member" "cudly_impersonate" {
 }
 
 # Grant the target SA permissions needed to manage CUDs/commitments.
-# commitmentAdmin: create/manage committed-use discounts.
-# billing.viewer:  read billing data (costs, invoices). Read-only —
-#                  billing.user is unnecessarily broad for CUDly's use case.
-resource "google_project_iam_member" "cudly_commitment_admin" {
+#
+# roles/commerceorgpolicy.commitmentAdmin and roles/billing.viewer are
+# organization- and billing-account-scoped respectively; granting either at
+# project scope via google_project_iam_member 400s (same failure mode
+# terraform/modules/compute/gcp/cloud-run/main.tf:484 documents for
+# billing.viewer, and federation/gcp-target/terraform/variables.tf:198-201
+# documents for commitmentAdmin). Neither can work here.
+#
+# Instead grant exactly what CUDly needs, mirroring the sibling
+# gcp-target bundle (iac/federation/gcp-target/terraform/main.tf) so the
+# two onboarding paths converge on the same permission set: a custom role
+# holding only the two commitment-write permissions, plus the built-in
+# compute.viewer role for read access (regions/zones/machineTypes/commitments
+# .list/.get). There is no billing-read path in this bundle (no
+# billing_account_id input to scope roles/billing.viewer to), so it is
+# dropped rather than reintroduced broken.
+#
+# var.create_custom_role gates creation: if the same project was already
+# onboarded through federation/gcp-target (or self-hosts CUDly via
+# terraform/modules/compute/gcp/cloud-run), that call site already created a
+# role with the same var.custom_role_id, and creating it again here 409s
+# (custom role IDs are unique per project, and each bundle applies from its
+# own Terraform state, so there's no shared state to reconcile against).
+# Default true preserves this bundle's own behavior for the common case
+# where it's the only onboarding path applied to the project.
+resource "google_project_iam_custom_role" "cudly" {
+  count   = var.create_custom_role ? 1 : 0
   project = var.project_id
-  role    = "roles/commerceorgpolicy.commitmentAdmin"
-  member  = "serviceAccount:${var.service_account_email}"
+  role_id = var.custom_role_id
+  title   = "CUDly Commitment Writer"
+  # Description matches terraform/modules/compute/gcp/cloud-run and
+  # federation/gcp-target exactly so applies from any of the three don't
+  # stomp each other.
+  description = "Minimum permissions for CUDly to purchase and update committed use discounts."
+  permissions = var.custom_role_permissions
+  stage       = "GA"
+}
+
+locals {
+  # Reference the role this bundle created, or (when var.create_custom_role
+  # is false) the identically-shaped role another call site already created
+  # in this project. google_project_iam_custom_role.id renders to exactly
+  # this format.
+  custom_role_id = var.create_custom_role ? google_project_iam_custom_role.cudly[0].id : "projects/${var.project_id}/roles/${var.custom_role_id}"
 }
 
-resource "google_project_iam_member" "cudly_billing_viewer" {
+resource "google_project_iam_member" "cudly_custom" {
   project = var.project_id
-  role    = "roles/billing.viewer"
+  role    = local.custom_role_id
   member  = "serviceAccount:${var.service_account_email}"
 }
+
+# Built-in read-side roles, mirroring federation/gcp-target's
+# service_account_project_roles: roles/compute.viewer (regions/zones/
+# machineTypes/commitments.list/.get) and roles/recommender.viewer. The
+# latter is required by the collection pipeline
+# (providers/gcp/services/computeengine.GetRecommendations calls the GCP
+# Recommender API, "google.billing.CostInsight.commitmentRecommender");
+# without it, every recommendation call 403s and is caught and warn-logged
+# by providers/gcp/recommendations.go, so an onboarded account would
+# silently surface zero GCP recommendations rather than erroring.
+resource "google_project_iam_member" "cudly_built_in" {
+  for_each = toset(var.built_in_project_roles)
+  project  = var.project_id
+  role     = each.value
+  member   = "serviceAccount:${var.service_account_email}"
+}

iac/federation/gcp-sa-impersonation/terraform/variables.tf

@@ -13,6 +13,75 @@ variable "source_service_account" {
   type        = string
 }
 
+variable "create_custom_role" {
+  description = <<-EOT
+    Whether to create the var.custom_role_id custom role in this project.
+    Default true is correct when this bundle is the only onboarding path
+    applied to the project.
+
+    Set to false ONLY after confirming the role already exists (e.g. `gcloud
+    iam roles describe var.custom_role_id --project var.project_id`):
+    either terraform/modules/compute/gcp/cloud-run (self-hosted CUDly,
+    unconditional) or federation/gcp-target (only when it created its own
+    service account, i.e. its var.service_account_email was left empty) can
+    have already created a role with the same ID in this project. If
+    neither applies, the role does not exist and setting this to false
+    makes the binding below target a missing role, which fails the apply.
+  EOT
+  type        = bool
+  default     = true
+}
+
+variable "custom_role_id" {
+  description = <<-EOT
+    Project-scoped custom role ID Terraform creates and binds to
+    var.service_account_email. The role carries the minimum permissions
+    required to purchase and manage Compute Engine CUDs on behalf of CUDly.
+    Matches the default in federation/gcp-target/terraform so applies from
+    either bundle stay idempotent against the same project.
+  EOT
+  type        = string
+  default     = "cudlyCommitmentWriter"
+}
+
+variable "custom_role_permissions" {
+  description = <<-EOT
+    Permissions bundled into the custom role granted to
+    var.service_account_email. Defaults match the definition in
+    terraform/modules/compute/gcp/cloud-run and federation/gcp-target, so all
+    three stay in lockstep and none can stomp another's role on apply.
+
+    Read-side permissions (regions/zones/machineTypes/commitments.list/.get)
+    come from roles/compute.viewer, granted separately in main.tf.
+  EOT
+  type        = list(string)
+  default = [
+    "compute.commitments.create",
+    "compute.commitments.update",
+  ]
+}
+
+variable "built_in_project_roles" {
+  description = <<-EOT
+    Built-in project-scoped roles granted to var.service_account_email.
+    Defaults match federation/gcp-target's service_account_project_roles, so
+    both onboarding paths grant the same read-side access:
+
+    - roles/compute.viewer: regions/zones/machineTypes/commitments.list/.get.
+    - roles/recommender.viewer: required by the collection pipeline
+      (providers/gcp/services/computeengine.GetRecommendations calls the GCP
+      Recommender API). Missing it doesn't fail the apply or any API call
+      visibly -- providers/gcp/recommendations.go catches the resulting 403
+      and only warn-logs it, so the account onboards successfully but
+      silently returns zero GCP recommendations.
+  EOT
+  type        = list(string)
+  default = [
+    "roles/compute.viewer",
+    "roles/recommender.viewer",
+  ]
+}
+
 variable "cudly_api_url" {
   description = "CUDly API base URL for automatic account registration. Leave empty to skip registration."
   type        = string

iac/federation/gcp-target/terraform/variables.tf

@@ -195,6 +195,13 @@ variable "service_account_project_roles" {
     the two modules' definitions of cudlyCommitmentWriter identical
     so one doesn't stomp the other on apply.
 
+    roles/recommender.viewer is required by the collection pipeline
+    (providers/gcp/services/computeengine.GetRecommendations calls the GCP
+    Recommender API). Missing it doesn't fail the apply or any API call
+    visibly -- providers/gcp/recommendations.go catches the resulting 403
+    and only warn-logs it, so the account onboards successfully but
+    silently returns zero GCP recommendations (#129).
+
     Note: GCP's built-in commitment/billing roles (e.g.
     roles/commerceorgpolicy.commitmentAdmin, roles/billing.viewer) are
     organization- or billing-account-scoped and will 400 if granted at
@@ -204,6 +211,7 @@ variable "service_account_project_roles" {
   type        = list(string)
   default = [
     "roles/compute.viewer",
+    "roles/recommender.viewer",
   ]
 }
 

iac/gcp_sa_impersonation_test.go

@@ -0,0 +1,116 @@
+package iac
+
+import (
+	"regexp"
+	"slices"
+	"testing"
+)
+
+// These tests assert that the two GCP onboarding bundles (gcp-sa-impersonation
+// and gcp-target) grant an equivalent permission set to the CUDly service
+// account, so they cannot drift back into granting different, organization-
+// or billing-account-scoped roles at project scope (#129), and that neither
+// silently drops a built-in role the other grants (also #129: missing
+// roles/recommender.viewer doesn't fail the apply or any visible API call --
+// providers/gcp/recommendations.go catches the resulting 403 and only
+// warn-logs it, so an onboarded account silently returns zero GCP
+// recommendations).
+
+const (
+	gcpSAImpersonationMainTF = "federation/gcp-sa-impersonation/terraform/main.tf"
+	gcpSAImpersonationVarsTF = "federation/gcp-sa-impersonation/terraform/variables.tf"
+	gcpTargetVarsTF          = "federation/gcp-target/terraform/variables.tf"
+)
+
+func readModuleFile(t *testing.T, path string) string {
+	t.Helper()
+	raw, err := Modules.ReadFile(path)
+	if err != nil {
+		t.Fatalf("read %s: %v", path, err)
+	}
+	return string(raw)
+}
+
+// extractVarDefaultList pulls the quoted string literals out of a
+// `variable "<name>" { ... default = [ ... ] ... }` block in a federation
+// module's variables.tf.
+func extractVarDefaultList(t *testing.T, tf, varName, path string) []string {
+	t.Helper()
+	varRe := regexp.MustCompile(`(?s)variable "` + regexp.QuoteMeta(varName) + `" \{.*?default = \[(.*?)\]`)
+	m := varRe.FindStringSubmatch(tf)
+	if m == nil {
+		t.Fatalf("%s: could not find variable %q default block", path, varName)
+	}
+	litRe := regexp.MustCompile(`"([^"]+)"`)
+	lits := litRe.FindAllStringSubmatch(m[1], -1)
+	values := make([]string, 0, len(lits))
+	for _, l := range lits {
+		values = append(values, l[1])
+	}
+	if len(values) == 0 {
+		t.Fatalf("%s: variable %q default has no values", path, varName)
+	}
+	return values
+}
+
+func assertEquivalentLists(t *testing.T, a, b []string, aName, bName string) {
+	t.Helper()
+	sortedA := slices.Clone(a)
+	slices.Sort(sortedA)
+	sortedB := slices.Clone(b)
+	slices.Sort(sortedB)
+	if !slices.Equal(sortedA, sortedB) {
+		t.Errorf("%s %v != %s %v; the two onboarding paths must grant the same permission set", aName, sortedA, bName, sortedB)
+	}
+}
+
+// TestGCPBundlesGrantEquivalentCommitmentPermissions guards #129: the
+// sa-impersonation and gcp-target bundles must converge on the same
+// commitment-write custom-role permission set rather than one granting a
+// broader, project-scope-invalid role.
+func TestGCPBundlesGrantEquivalentCommitmentPermissions(t *testing.T) {
+	saPerms := extractVarDefaultList(t, readModuleFile(t, gcpSAImpersonationVarsTF), "custom_role_permissions", gcpSAImpersonationVarsTF)
+	targetPerms := extractVarDefaultList(t, readModuleFile(t, gcpTargetVarsTF), "custom_role_permissions", gcpTargetVarsTF)
+	assertEquivalentLists(t, saPerms, targetPerms, "gcp-sa-impersonation custom_role_permissions", "gcp-target custom_role_permissions")
+}
+
+// TestGCPBundlesGrantEquivalentBuiltInRoles guards the same drift for the
+// built-in project-scoped roles (roles/compute.viewer,
+// roles/recommender.viewer): gcp-sa-impersonation's built_in_project_roles
+// must match gcp-target's service_account_project_roles.
+func TestGCPBundlesGrantEquivalentBuiltInRoles(t *testing.T) {
+	saRoles := extractVarDefaultList(t, readModuleFile(t, gcpSAImpersonationVarsTF), "built_in_project_roles", gcpSAImpersonationVarsTF)
+	targetRoles := extractVarDefaultList(t, readModuleFile(t, gcpTargetVarsTF), "service_account_project_roles", gcpTargetVarsTF)
+	assertEquivalentLists(t, saRoles, targetRoles, "gcp-sa-impersonation built_in_project_roles", "gcp-target service_account_project_roles")
+
+	for _, want := range []string{"roles/compute.viewer", "roles/recommender.viewer"} {
+		if !slices.Contains(saRoles, want) {
+			t.Errorf("gcp-sa-impersonation built_in_project_roles missing %s", want)
+		}
+	}
+}
+
+// TestGCPSAImpersonationDoesNotGrantInvalidProjectScopeRoles guards the two
+// broken grants #129 reported: roles/commerceorgpolicy.commitmentAdmin is
+// organization-scoped and roles/billing.viewer is billing-account-scoped, so
+// granting either via google_project_iam_member (project scope) 400s. Neither
+// may reappear in the bundle's main.tf.
+func TestGCPSAImpersonationDoesNotGrantInvalidProjectScopeRoles(t *testing.T) {
+	tf := readModuleFile(t, gcpSAImpersonationMainTF)
+
+	// Match an actual `role = "roles/..."` assignment, not prose (this file's
+	// own comments name both roles to explain why they were removed).
+	if regexp.MustCompile(`role\s*=\s*"roles/commerceorgpolicy\.commitmentAdmin"`).MatchString(tf) {
+		t.Errorf("%s: grants roles/commerceorgpolicy.commitmentAdmin, which is organization-scoped and 400s at project scope", gcpSAImpersonationMainTF)
+	}
+	if regexp.MustCompile(`role\s*=\s*"roles/billing\.viewer"`).MatchString(tf) {
+		t.Errorf("%s: grants roles/billing.viewer, which is billing-account-scoped and 400s at project scope (see terraform/modules/compute/gcp/cloud-run/main.tf for the correct google_billing_account_iam_member shape if a billing read path is ever added here)", gcpSAImpersonationMainTF)
+	}
+
+	if !regexp.MustCompile(`resource "google_project_iam_custom_role" "cudly" \{`).MatchString(tf) {
+		t.Errorf("%s: expected a google_project_iam_custom_role granting the minimum commitment-write permissions", gcpSAImpersonationMainTF)
+	}
+	if !regexp.MustCompile(`resource "google_project_iam_member" "cudly_built_in" \{`).MatchString(tf) {
+		t.Errorf("%s: expected a google_project_iam_member granting var.built_in_project_roles for read access", gcpSAImpersonationMainTF)
+	}
+}