Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions .github/workflows/cleanup-staging.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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' }}
Expand Down
4 changes: 0 additions & 4 deletions scripts/gcp-import-dev-state.sh
Original file line number Diff line number Diff line change
Expand Up @@ -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}"
Expand Down
8 changes: 6 additions & 2 deletions terraform/environments/gcp/ci-cd-permissions/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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/<owner>/<repo> --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` (`<github_repo_id>:<deploy_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:<owner>@<owner_id>/<repo>@<repo_id>:...`),
Expand Down
29 changes: 26 additions & 3 deletions terraform/environments/gcp/ci-cd-permissions/github_oidc.tf
Original file line number Diff line number Diff line change
@@ -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

Expand Down Expand Up @@ -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.
Expand All @@ -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
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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"
}
}

Expand All @@ -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" {
Expand Down
42 changes: 37 additions & 5 deletions terraform/modules/compute/gcp/cloud-run/main.tf
Original file line number Diff line number Diff line change
Expand Up @@ -452,19 +452,51 @@ 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}"
}

# Commitment write access: custom project role cudlyCommitmentWriter
# 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} —
Expand Down
94 changes: 94 additions & 0 deletions terraform/modules/compute/gcp/runtime_compute_iam_test.go
Original file line number Diff line number Diff line change
@@ -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)
}
}
9 changes: 7 additions & 2 deletions terraform/modules/secrets/aws/main.tf
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand All @@ -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}"
}
]
})
Expand Down
Loading
Loading