From 69288ae2d6d671b6e52139d0fa7b5f98d3dfa985 Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Fri, 23 Oct 2026 06:53:09 +0000 Subject: [PATCH] sec(iac): narrow three hosted-infra least-privilege grants From the 2026-09-02 audit (issue #323): - A13-014: the GCP CI impersonation member pinned only the repo id, so any other provider in the pool that mapped attribute.repository_id could impersonate the deploy SA from any ref. Map repo id and ref into one attribute (attribute.repository_id_ref) and key the member on :, matching the provider's condition. The member is replaced after the provider gains the mapping and before the old member goes (depends_on + create_before_destroy). The pool rename is deferred: it replaces the pool and provider and changes GCP_WORKLOAD_IDENTITY_PROVIDER for every GCP workflow. - A13c-021: the rotation role's RDS grant was arn:aws:rds::*:db:*. Build it from the caller account and var.rds_cluster_id, and validate the id so a wildcard cannot re-widen it. - A13c-022: replace the Cloud Run SA's roles/compute.viewer with a custom role cudlyComputeReader holding only the reads the pinned GCP provider makes (regions.list, machineTypes.list, commitments.list/get, regionOperations.get for the purchase op.Wait, resourcemanager.projects.get for credential validation and the ambient project lookup). Drop the stale compute_viewer import from gcp-import-dev-state.sh. A13c-023 (Azure subscription Reader) is deferred: the scheduler's ambient Azure fallback collects from the host subscription by default, so gating Reader off by default could break that path. Refs #323 Co-Authored-By: Claude Opus 5.5 --- .github/workflows/cleanup-staging.yml | 8 +- scripts/gcp-import-dev-state.sh | 4 - .../gcp/ci-cd-permissions/README.md | 8 +- .../gcp/ci-cd-permissions/github_oidc.tf | 29 +++- .../tests/immutable_trust.tftest.hcl | 30 +++- .../modules/compute/gcp/cloud-run/main.tf | 42 +++++- .../compute/gcp/runtime_compute_iam_test.go | 94 +++++++++++++ terraform/modules/secrets/aws/main.tf | 9 +- .../secrets/aws/rotation_rds_iam_test.go | 131 ++++++++++++++++++ terraform/modules/secrets/aws/variables.tf | 7 +- 10 files changed, 339 insertions(+), 23 deletions(-) create mode 100644 terraform/modules/compute/gcp/runtime_compute_iam_test.go create mode 100644 terraform/modules/secrets/aws/rotation_rds_iam_test.go diff --git a/.github/workflows/cleanup-staging.yml b/.github/workflows/cleanup-staging.yml index cabff1944..f7b612f4f 100644 --- a/.github/workflows/cleanup-staging.yml +++ b/.github/workflows/cleanup-staging.yml @@ -350,10 +350,10 @@ jobs: # GCP: the WIF provider keys on assertion.repository_owner_id, # assertion.repository_id and assertion.ref (gcp/ci-cd-permissions/ # github_oidc.tf), and the impersonation binding is on - # attribute.repository_id -- nothing reads the subject. So this - # binding does NOT affect whether the token mints; it is here for parity - # and so protection rules can apply once they exist. GCP therefore stays - # main-only via assertion.ref regardless. + # attribute.repository_id_ref (repo id + ref) -- nothing reads the + # subject. So this binding does NOT affect whether the token mints; it + # is here for parity and so protection rules can apply once they exist. + # GCP therefore stays main-only via assertion.ref regardless. environment: staging env: GCP_REGION: ${{ vars.GCP_REGION || 'us-central1' }} diff --git a/scripts/gcp-import-dev-state.sh b/scripts/gcp-import-dev-state.sh index c5cbc61d7..81dc09865 100755 --- a/scripts/gcp-import-dev-state.sh +++ b/scripts/gcp-import-dev-state.sh @@ -339,10 +339,6 @@ if [ -n "$SA_CLOUDRUN_EXISTS" ]; then "module.compute_cloud_run[0].google_project_iam_member.secret_accessor" \ "${PROJECT} roles/secretmanager.secretAccessor ${SA_MEMBER}" - import_if_missing \ - "module.compute_cloud_run[0].google_project_iam_member.compute_viewer" \ - "${PROJECT} roles/compute.viewer ${SA_MEMBER}" - import_if_missing \ "module.compute_cloud_run[0].google_project_iam_member.compute_commitment_admin" \ "${PROJECT} roles/compute.admin ${SA_MEMBER}" diff --git a/terraform/environments/gcp/ci-cd-permissions/README.md b/terraform/environments/gcp/ci-cd-permissions/README.md index 81e16c36d..cd4b0bd8b 100644 --- a/terraform/environments/gcp/ci-cd-permissions/README.md +++ b/terraform/environments/gcp/ci-cd-permissions/README.md @@ -151,8 +151,12 @@ gcloud iam workload-identity-pools providers describe github-actions \ To change the allowed repo, update `github_owner_id` and `github_repo_id` in `terraform.tfvars` (`gh api repos// --jq '.owner.id, .id'`) and re-apply. The `principalSet` member is -keyed on `attribute.repository_id`; the `attribute.repository` mapping is kept so the old member -keeps resolving while the replacement is created first. `github_repo` is still the skip toggle +keyed on `attribute.repository_id_ref` (`:`), so the grant pins the +same repo and ref as the condition and changing `deploy_ref` moves both. Principal identifiers are +pool-scoped, so without the ref a second provider added to the `github-actions` pool could +satisfy the grant from any ref of the repo; keep the pool dedicated to this trust. The older +`attribute.repository` and `attribute.repository_id` mappings are kept so the previous member +keeps resolving while its replacement is created first. `github_repo` is still the skip toggle (empty disables WIF setup) but no longer feeds the condition or the member. AWS and Azure match the GitHub immutable OIDC subject (`repo:@/@:...`), diff --git a/terraform/environments/gcp/ci-cd-permissions/github_oidc.tf b/terraform/environments/gcp/ci-cd-permissions/github_oidc.tf index 00eef32e6..e1e34cadf 100644 --- a/terraform/environments/gcp/ci-cd-permissions/github_oidc.tf +++ b/terraform/environments/gcp/ci-cd-permissions/github_oidc.tf @@ -1,3 +1,9 @@ +# Principal identifiers are pool-scoped, not provider-scoped: any provider added +# to this pool that can mint the same attribute value satisfies the +# impersonation grant below. Keep this pool dedicated to CUDly's deploy trust; +# do not add a second provider to it. (Renaming the pool to a CUDly-scoped id +# replaces it and the provider, which changes GCP_WORKLOAD_IDENTITY_PROVIDER for +# every GCP workflow, so it is left to a planned bootstrap migration.) resource "google_iam_workload_identity_pool" "github" { count = var.github_repo != "" ? 1 : 0 @@ -30,6 +36,11 @@ resource "google_iam_workload_identity_pool_provider" "github" { # a name squatter cannot match. "attribute.repository_id" = "assertion.repository_id" "attribute.repository_owner_id" = "assertion.repository_owner_id" + + # Repo id and ref in one attribute, so the impersonation member below can + # pin both: a principalSet names a single attribute. repository_id is all + # digits, so the first ':' separates the two unambiguously. + "attribute.repository_id_ref" = "assertion.repository_id + ':' + assertion.ref" } # Restrict to the specific repo (by immutable ids, not name) AND only allow the designated deploy branch. @@ -43,15 +54,27 @@ resource "google_iam_workload_identity_pool_provider" "github" { } # Allow workflows from the permitted repo+ref to impersonate the deploy SA. +# +# The member pins the ref as well as the repo id, so the grant itself names the +# same repo+ref the provider's attribute_condition admits. Keyed on the repo id +# alone, any other provider in the pool that maps attribute.repository_id for +# this repo would satisfy the grant from any branch, PR or tag. resource "google_service_account_iam_member" "github_actions" { count = var.github_repo != "" ? 1 : 0 service_account_id = google_service_account.cudly_deploy.name role = "roles/iam.workloadIdentityUser" - member = "principalSet://iam.googleapis.com/${google_iam_workload_identity_pool.github[0].name}/attribute.repository_id/${var.github_repo_id}" + member = "principalSet://iam.googleapis.com/${google_iam_workload_identity_pool.github[0].name}/attribute.repository_id_ref/${var.github_repo_id}:${var.deploy_ref}" + + # Upgrade ordering. member is ForceNew, so changing it replaces this binding: + # depends_on: the provider gains the repository_id_ref mapping + # (an in-place update) before the new member exists. + # create_before_destroy: the new member is added while the old one (keyed + # on attribute.repository_id, whose mapping stays) is + # still present. + # Every step keeps at least one matching grant, so no access gap opens. + depends_on = [google_iam_workload_identity_pool_provider.github] - # The old member (keyed on attribute.repository) keeps resolving because that - # mapping stays, so the new binding is created first and no access gap opens. lifecycle { create_before_destroy = true } diff --git a/terraform/environments/gcp/ci-cd-permissions/tests/immutable_trust.tftest.hcl b/terraform/environments/gcp/ci-cd-permissions/tests/immutable_trust.tftest.hcl index d80072a90..84284fab6 100644 --- a/terraform/environments/gcp/ci-cd-permissions/tests/immutable_trust.tftest.hcl +++ b/terraform/environments/gcp/ci-cd-permissions/tests/immutable_trust.tftest.hcl @@ -34,9 +34,30 @@ run "trust_uses_immutable_ids" { error_message = "id mappings must exist and the repository mapping must stay" } + # Issue #323 (A13-014): the grant pins the ref, not just the repo id. assert { - condition = endswith(google_service_account_iam_member.github_actions[0].member, "/attribute.repository_id/1391259743") - error_message = "member must be keyed on attribute.repository_id" + condition = endswith(google_service_account_iam_member.github_actions[0].member, "/attribute.repository_id_ref/1391259743:refs/heads/main") + error_message = "member must be keyed on attribute.repository_id_ref with the repo id and deploy_ref" + } + + assert { + condition = !strcontains(google_service_account_iam_member.github_actions[0].member, "/attribute.repository_id/") + error_message = "the repo-only member (any ref of the repo) must not remain" + } + + assert { + condition = startswith(google_service_account_iam_member.github_actions[0].member, "principalSet://iam.googleapis.com/${google_iam_workload_identity_pool.github[0].name}/") + error_message = "member must be scoped to this module's pool" + } + + assert { + condition = lookup(google_iam_workload_identity_pool_provider.github[0].attribute_mapping, "attribute.repository_id_ref", "") == "assertion.repository_id + ':' + assertion.ref" + error_message = "attribute.repository_id_ref must map to the repo id and ref the member pins" + } + + assert { + condition = google_service_account_iam_member.github_actions[0].role == "roles/iam.workloadIdentityUser" + error_message = "the deploy trust must still grant workloadIdentityUser" } } @@ -51,6 +72,11 @@ run "condition_follows_deploy_ref" { condition = endswith(google_iam_workload_identity_pool_provider.github[0].attribute_condition, "assertion.ref == 'refs/heads/feature/x'") error_message = "condition must follow deploy_ref" } + + assert { + condition = endswith(google_service_account_iam_member.github_actions[0].member, "/attribute.repository_id_ref/1391259743:refs/heads/feature/x") + error_message = "member must follow deploy_ref, matching the condition" + } } run "rejects_bad_repo_id" { diff --git a/terraform/modules/compute/gcp/cloud-run/main.tf b/terraform/modules/compute/gcp/cloud-run/main.tf index 329bad278..452539b28 100644 --- a/terraform/modules/compute/gcp/cloud-run/main.tf +++ b/terraform/modules/compute/gcp/cloud-run/main.tf @@ -452,11 +452,43 @@ resource "google_cloud_run_service_iam_member" "scheduler_ri_exchange_invoker" { # Runtime IAM: CUDly application cloud API access # ============================================== -# Compute Viewer: grants compute.commitments.list and compute.machineTypes.list -# needed for listing CUDs and available instance types. -resource "google_project_iam_member" "compute_viewer" { +# Compute read access: custom project role cudlyComputeReader holding only the +# reads the pinned cloud-commitments-go GCP provider makes. Replaces +# roles/compute.viewer, which read every Compute Engine resource in the project, +# instance metadata and startup scripts included (issue #323). Each permission: +# compute.regions.list Regions.List (recommendations, GetRegions) +# compute.machineTypes.list MachineTypes.List (purchase validation) +# compute.commitments.list RegionCommitments.List (existing CUDs) +# compute.commitments.get RegionCommitments.Get (adopting a re-driven +# purchase whose insert hit a 409) +# compute.regionOperations.get the insert's op.Wait() polls the region +# operation; without it a successful purchase +# is reported as failed +# resourcemanager.projects.get ValidateCredentials' GetProject, and the +# ambient (no registered GCP account) default +# project lookup, which lists only projects the +# caller can get +# Like the custom roles below, creating it needs the deployer SA's +# roles/iam.roleAdmin from terraform/environments/gcp/ci-cd-permissions. +resource "google_project_iam_custom_role" "cudly_compute_reader" { + project = var.project_id + role_id = "cudlyComputeReader" + title = "CUDly Compute Reader" + description = "Minimum Compute Engine reads for CUDly to list regions, machine types and committed use discounts and confirm purchases." + permissions = [ + "compute.commitments.get", + "compute.commitments.list", + "compute.machineTypes.list", + "compute.regionOperations.get", + "compute.regions.list", + "resourcemanager.projects.get", + ] + stage = "GA" +} + +resource "google_project_iam_member" "compute_reader" { project = var.project_id - role = "roles/compute.viewer" + role = google_project_iam_custom_role.cudly_compute_reader.id member = "serviceAccount:${google_service_account.cloud_run.email}" } @@ -464,7 +496,7 @@ resource "google_project_iam_member" "compute_viewer" { # granting only compute.commitments.{create,update}. Replaces the original # roles/compute.admin grant (pre-eaa84c259), which conferred full VM/network/ # disk/firewall control — a Cloud Run compromise could then rewrite the -# entire project. Read-side permissions come from the roles/compute.viewer +# entire project. Read-side permissions come from the cudlyComputeReader # grant above. # # Requires the deployer SA to hold iam.roles.{get,create,update,delete} — diff --git a/terraform/modules/compute/gcp/runtime_compute_iam_test.go b/terraform/modules/compute/gcp/runtime_compute_iam_test.go new file mode 100644 index 000000000..7d078a26d --- /dev/null +++ b/terraform/modules/compute/gcp/runtime_compute_iam_test.go @@ -0,0 +1,94 @@ +// Regression test for issue #323 (A13c-022): the Cloud Run runtime SA held +// project-wide roles/compute.viewer, which reads every Compute Engine resource +// in the host project (instance metadata and startup scripts included). The +// runtime only needs the reads the pinned cloud-commitments-go GCP provider +// makes, so it must instead hold a project custom role with exactly those +// permissions. The helpers (codeLines, customRoleBlockRe, ...) live in +// runtime_recommender_iam_test.go. +package gcp_test + +import ( + "os" + "regexp" + "slices" + "testing" +) + +const computeReaderRoleID = "cudlyComputeReader" + +// computeReaderPermissions maps each permission to the provider call that +// needs it (providers/gcp at the version pinned in go.mod). +var computeReaderPermissions = []string{ + "compute.commitments.get", // services/computeengine: RegionCommitments.Get when adopting a re-driven purchase + "compute.commitments.list", // services/computeengine: RegionCommitments.List (existing CUDs) + "compute.machineTypes.list", // services/computeengine: MachineTypes.List (purchase validation) + "compute.regionOperations.get", // op.Wait after RegionCommitments.Insert polls RegionOperations.Get + "compute.regions.list", // provider.go / recommendations.go: Regions.List + "resourcemanager.projects.get", // ValidateCredentials GetProject, and the ambient default-project lookup via projects.list +} + +// Any predefined Compute Engine role (roles/compute.viewer, .admin, ...). +var predefinedComputeRe = regexp.MustCompile(`roles/compute\.`) + +func TestCloudRunComputeGrantIsNarrow(t *testing.T) { + const file = "cloud-run/main.tf" + const sa = "google_service_account.cloud_run.email" + + data, err := os.ReadFile(file) + if err != nil { + t.Fatalf("reading %s: %v", file, err) + } + code := codeLines(string(data)) + + if loc := predefinedComputeRe.FindStringIndex(code); loc != nil { + t.Errorf("%s must not grant a predefined roles/compute.* role (issue #323); found %q", file, code[loc[0]:min(loc[0]+40, len(code))]) + } + + var roleResource string + for _, m := range customRoleBlockRe.FindAllStringSubmatch(code, -1) { + id := roleIDRe.FindStringSubmatch(m[2]) + if id == nil || id[1] != computeReaderRoleID { + continue + } + if roleResource != "" { + t.Fatalf("%s defines %s more than once", file, computeReaderRoleID) + } + roleResource = m[1] + perms := permissionsRe.FindStringSubmatch(m[2]) + if perms == nil { + t.Fatalf("%s: custom role %s has no permissions list", file, computeReaderRoleID) + } + quoted := quotedRe.FindAllStringSubmatch(perms[1], -1) + got := make([]string, 0, len(quoted)) + for _, q := range quoted { + got = append(got, q[1]) + } + slices.Sort(got) + if !slices.Equal(got, computeReaderPermissions) { + t.Errorf("%s: custom role %s permissions = %v, want exactly %v", file, computeReaderRoleID, got, computeReaderPermissions) + } + } + if roleResource == "" { + t.Fatalf("%s must define google_project_iam_custom_role with role_id %q (issue #323)", file, computeReaderRoleID) + } + + roleRef := regexp.MustCompile(`(?m)^\s*role\s*=\s*google_project_iam_custom_role\.` + regexp.QuoteMeta(roleResource) + `\.id\s*$`) + memberRef := regexp.MustCompile(`(?m)^\s*member\s*=\s*"serviceAccount:\$\{` + regexp.QuoteMeta(sa) + `\}"\s*$`) + projectRef := regexp.MustCompile(`(?m)^\s*project\s*=\s*var\.project_id\s*$`) + bound := 0 + for _, m := range iamMemberBlockRe.FindAllStringSubmatch(code, -1) { + if !roleRef.MatchString(m[1]) { + continue + } + bound++ + if !memberRef.MatchString(m[1]) { + t.Errorf("%s: the %s binding must target the runtime SA %s", file, computeReaderRoleID, sa) + } + if !projectRef.MatchString(m[1]) { + t.Errorf("%s: the %s binding must be project-scoped (project = var.project_id)", file, computeReaderRoleID) + } + } + if bound != 1 { + t.Errorf("%s: want exactly one google_project_iam_member binding %s, got %d", file, computeReaderRoleID, bound) + } +} diff --git a/terraform/modules/secrets/aws/main.tf b/terraform/modules/secrets/aws/main.tf index 9362e1a6f..f33049358 100644 --- a/terraform/modules/secrets/aws/main.tf +++ b/terraform/modules/secrets/aws/main.tf @@ -371,7 +371,12 @@ resource "aws_iam_role_policy" "rotation_secrets" { }) } -# RDS permissions for rotation (to update password) +# Account segment of the rotation RDS ARN below. +data "aws_caller_identity" "current" {} + +# RDS permissions for rotation (to update password), scoped to the one +# instance this rotation serves in this account. A ":*:db:*" ARN would let the +# role modify every DB instance in the account (issue #323). resource "aws_iam_role_policy" "rotation_rds" { count = var.enable_secret_rotation && var.rds_cluster_id != null ? 1 : 0 @@ -387,7 +392,7 @@ resource "aws_iam_role_policy" "rotation_rds" { "rds:ModifyDBInstance", "rds:DescribeDBInstances" ] - Resource = "arn:aws:rds:${var.region}:*:db:*" + Resource = "arn:aws:rds:${var.region}:${data.aws_caller_identity.current.account_id}:db:${var.rds_cluster_id}" } ] }) diff --git a/terraform/modules/secrets/aws/rotation_rds_iam_test.go b/terraform/modules/secrets/aws/rotation_rds_iam_test.go new file mode 100644 index 000000000..89f0be578 --- /dev/null +++ b/terraform/modules/secrets/aws/rotation_rds_iam_test.go @@ -0,0 +1,131 @@ +// Package aws_test guards the RDS grant of the secret-rotation Lambda role. +// +// Regression test for issue #323 (A13c-021): the rotation role's RDS +// statement used Resource "arn:aws:rds:${var.region}:*:db:*", which let it +// call rds:ModifyDBInstance on every DB instance in the account, although the +// policy's own count already requires the one instance it serves +// (var.rds_cluster_id). The grant must name that instance in this account and +// still carry the two actions rotation needs. +package aws_test + +import ( + "os" + "regexp" + "slices" + "strings" + "testing" +) + +const wantRotationRDSResource = `"arn:aws:rds:${var.region}:${data.aws_caller_identity.current.account_id}:db:${var.rds_cluster_id}"` + +var ( + rotationRDSBlockRe = regexp.MustCompile(`(?s)resource\s+"aws_iam_role_policy"\s+"rotation_rds"\s*\{(.*?)\n\}`) + resourceLineRe = regexp.MustCompile(`(?m)^\s*Resource\s*=\s*(.+?)\s*$`) + actionListRe = regexp.MustCompile(`(?s)Action\s*=\s*\[(.*?)\]`) + quotedRe = regexp.MustCompile(`"([^"]*)"`) + callerIdentityRe = regexp.MustCompile(`(?m)^data\s+"aws_caller_identity"\s+"current"\s*\{\s*\}`) + rdsClusterIDBlockRe = regexp.MustCompile(`(?s)variable\s+"rds_cluster_id"\s*\{(.*?)\n\}`) + validationRegexRe = regexp.MustCompile(`can\(regex\("((?:[^"\\]|\\.)*)",\s*var\.rds_cluster_id\)\)`) +) + +// codeLines drops whole-line comments so prose naming the old ARN is not +// mistaken for the grant. +func codeLines(src string) string { + var out []string + for _, line := range strings.Split(src, "\n") { + trimmed := strings.TrimSpace(line) + if strings.HasPrefix(trimmed, "#") || strings.HasPrefix(trimmed, "//") { + continue + } + out = append(out, line) + } + return strings.Join(out, "\n") +} + +func readCode(t *testing.T, path string) string { + t.Helper() + data, err := os.ReadFile(path) + if err != nil { + t.Fatalf("reading %s: %v", path, err) + } + return codeLines(string(data)) +} + +func TestRotationRDSGrantNamesTheTargetInstance(t *testing.T) { + code := readCode(t, "main.tf") + + block := rotationRDSBlockRe.FindStringSubmatch(code) + if block == nil { + t.Fatal(`main.tf must define resource "aws_iam_role_policy" "rotation_rds"`) + } + + resources := resourceLineRe.FindAllStringSubmatch(block[1], -1) + if len(resources) != 1 { + t.Fatalf("rotation_rds: want exactly one Resource line, got %d", len(resources)) + } + if got := resources[0][1]; got != wantRotationRDSResource { + t.Errorf("rotation_rds Resource = %s, want %s (issue #323: no account or instance wildcard)", got, wantRotationRDSResource) + } + + // The rotation function still needs both actions on that instance. + actions := actionListRe.FindStringSubmatch(block[1]) + if actions == nil { + t.Fatal("rotation_rds: no Action list") + } + quoted := quotedRe.FindAllStringSubmatch(actions[1], -1) + got := make([]string, 0, len(quoted)) + for _, q := range quoted { + got = append(got, q[1]) + } + slices.Sort(got) + if want := []string{"rds:DescribeDBInstances", "rds:ModifyDBInstance"}; !slices.Equal(got, want) { + t.Errorf("rotation_rds actions = %v, want %v", got, want) + } + + // The ARN's account segment comes from the caller identity, so the data + // source must exist in the module. + if !callerIdentityRe.MatchString(code) { + t.Error(`main.tf must declare data "aws_caller_identity" "current" {} for the rotation RDS ARN`) + } + + // No RDS ARN anywhere in the module may wildcard the account segment. + if strings.Contains(code, "arn:aws:rds:${var.region}:*:") { + t.Error("main.tf still has an RDS ARN with a wildcard account segment") + } +} + +// The instance id is interpolated into the ARN, so an input such as "*" or +// "cudly-*" would re-widen the grant. variables.tf must reject them. +func TestRotationRDSClusterIDRejectsWildcards(t *testing.T) { + code := readCode(t, "variables.tf") + + block := rdsClusterIDBlockRe.FindStringSubmatch(code) + if block == nil { + t.Fatal(`variables.tf must define variable "rds_cluster_id"`) + } + if !strings.Contains(block[1], "var.rds_cluster_id == null ||") { + t.Error("rds_cluster_id validation must keep null valid (rotation disabled)") + } + m := validationRegexRe.FindStringSubmatch(block[1]) + if m == nil { + t.Fatal("rds_cluster_id must be validated with can(regex(..., var.rds_cluster_id))") + } + // Terraform's regex() and Go's regexp both use RE2 syntax; the HCL string + // escapes are the only translation needed. + pattern := strings.ReplaceAll(m[1], `\\`, `\`) + re, err := regexp.Compile(pattern) + if err != nil { + t.Fatalf("rds_cluster_id validation regex %q does not compile: %v", pattern, err) + } + + for _, ok := range []string{"cudly-dev-postgres", "a", "db1"} { + if !re.MatchString(ok) { + t.Errorf("rds_cluster_id validation rejects valid identifier %q", ok) + } + } + for _, bad := range []string{"*", "cudly-*", "db?", "a:b", "a/b", "1db", "", "-db", strings.Repeat("a", 64)} { + if re.MatchString(bad) { + t.Errorf("rds_cluster_id validation accepts %q, which would widen or break the ARN", bad) + } + } +} diff --git a/terraform/modules/secrets/aws/variables.tf b/terraform/modules/secrets/aws/variables.tf index 29ee2d574..c586c401f 100644 --- a/terraform/modules/secrets/aws/variables.tf +++ b/terraform/modules/secrets/aws/variables.tf @@ -124,9 +124,14 @@ variable "rotation_lambda_vpc_config" { } variable "rds_cluster_id" { - description = "RDS cluster ID for rotation (required if enable_secret_rotation is true)" + description = "RDS DB instance identifier for rotation (required if enable_secret_rotation is true). It is interpolated into the rotation role's RDS ARN, so it must be a literal identifier, not a pattern." type = string default = null + + validation { + condition = var.rds_cluster_id == null || can(regex("^[A-Za-z][A-Za-z0-9-]{0,62}$", var.rds_cluster_id)) + error_message = "rds_cluster_id must be null or an RDS identifier (a letter, then up to 62 letters, digits or hyphens); wildcards would widen the rotation role's RDS grant." + } } variable "tags" {