From 8bc399cc1f82a60002d3b43776413943c60ebc53 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 19 Aug 2026 18:55:50 +0200 Subject: [PATCH 1/3] fix(iac): verify an existing GCP WIF provider instead of reusing it blindly The served onboarding script ran both creates with `|| echo "(may already exist)"`, so under `set -euo pipefail` every failure of a security-critical create was swallowed. A provider that already existed with a weaker attribute condition (or none) was reused without being looked at, the impersonation grant was made against it, and the script printed "=== Done ===" and exited 0. Each resource is now looked up before it is created, and an existing provider is compared field by field against what this script would have written (issuer URI, attribute condition, attribute mapping). A match is reused; any difference aborts before the grant with the found and expected values and a delete hint. A provider that is soft-deleted or disabled is refused too: describe returns it with its configuration intact, so every comparison would match while no token exchange can use it. Every other create failure now stops the run. The pool propagation wait moved ahead of the provider create, which needs the pool to be visible. Verifying this run's provider is not sufficient on its own, because the grant names the pool: any other provider in it that maps a token to the same subject satisfies the binding. The pool's providers are enumerated and the run aborts on any this script did not configure. That check runs before anything is created, so the refusal leaves the project untouched: customers onboarded before the provider default changed from cudly- to cudly-oidc hit it on every run, and running it after the create would leave another provider behind each time. CUDLY_FEDERATED_SUBJECT is validated before the first gcloud call. It is environment-overridable and lands inside the CEL string literal of --attribute-condition, so a value carrying a quote closed the literal and the rest rewrote the condition: "x' || true || '" produced a condition whose second disjunct was a bare true, admitting every subject the issuer signs. It also lands in the principal:// identifier of the grant, where '*' widens the member. Tests run the rendered script under bash against a recording gcloud stub, and cover both directions: a weaker condition, a foreign issuer, a different mapping, an unusable provider, a foreign provider in the pool and a failed create each abort without granting, while an empty project and a re-run over the previous run's state both still complete. Refs #1661 --- .../iacfiles/templates/gcp-wif-cli.sh.tmpl | 184 +++++- internal/iacfiles/templates_test.go | 542 +++++++++++++++++- 2 files changed, 696 insertions(+), 30 deletions(-) diff --git a/internal/iacfiles/templates/gcp-wif-cli.sh.tmpl b/internal/iacfiles/templates/gcp-wif-cli.sh.tmpl index 0dc364e0a..6c0de0da8 100644 --- a/internal/iacfiles/templates/gcp-wif-cli.sh.tmpl +++ b/internal/iacfiles/templates/gcp-wif-cli.sh.tmpl @@ -34,29 +34,56 @@ CUDLY_FEDERATED_SUBJECT="${CUDLY_FEDERATED_SUBJECT:-cudly-controller}" : "${SERVICE_ACCOUNT_EMAIL:?set SERVICE_ACCOUNT_EMAIL to the CUDly target service account in the project}" : "${CUDLY_ISSUER_URL:?set CUDLY_ISSUER_URL to the OIDC issuer URL of your CUDly deployment (base URL + /oidc)}" +# CUDLY_FEDERATED_SUBJECT is interpolated into the CEL string literal of the +# provider's --attribute-condition below and into the principal:// identifier of +# the impersonation grant. A quote closes that literal and the rest of the value +# rewrites the condition: "x' || true || '" yields assertion.sub == 'x' || true +# || '', which admits every subject the issuer will sign. A '*' widens the +# principal identifier the same way. Restrict the value to characters both +# destinations treat as inert; '|' is allowed because Auth0/Okta-style subjects +# use it (google-oauth2|123) and it carries no meaning in either place. +if ! [[ "${CUDLY_FEDERATED_SUBJECT}" =~ ^[A-Za-z0-9][-A-Za-z0-9._:/@=+~|]*$ ]]; then + echo "ERROR: CUDLY_FEDERATED_SUBJECT must start with a letter or digit and use only [-A-Za-z0-9._:/@=+~|] (got: ${CUDLY_FEDERATED_SUBJECT})." >&2 + echo " Quotes, backslashes, '*' and '\$' would escape the provider's attribute condition or widen the impersonation grant." >&2 + exit 1 +fi +# google.subject is capped at 127 characters by GCP; a longer value is rejected +# at token-exchange time, long after this script reported success. +if [[ ${#CUDLY_FEDERATED_SUBJECT} -gt 127 ]]; then + echo "ERROR: CUDLY_FEDERATED_SUBJECT must be at most 127 characters (google.subject limit); got ${#CUDLY_FEDERATED_SUBJECT}." >&2 + exit 1 +fi + gcloud config set project "${PROJECT_ID}" echo "Enabling required APIs..." gcloud services enable iam.googleapis.com iamcredentials.googleapis.com sts.googleapis.com -echo "Creating Workload Identity Pool ${POOL_ID}..." -gcloud iam workload-identity-pools create "${POOL_ID}" \ - --location=global \ - --display-name="CUDly WIF pool" \ - --project="${PROJECT_ID}" \ - || echo " (pool may already exist)" +# The provider is created with exactly these two values, and an existing +# provider is compared against them before it is reused. Kept in variables so +# the create path and the reuse check cannot drift apart. +# +# EXPECTED_MAPPING must stay a single entry: the reuse check compares it as a +# plain string against gcloud's describe output, which joins map entries with +# ';' rather than the ',' this flag takes. A second entry would make every +# legitimate re-run report a mapping mismatch. +EXPECTED_MAPPING="google.subject=assertion.sub" +EXPECTED_CONDITION="assertion.sub == '${CUDLY_FEDERATED_SUBJECT}'" -echo "Creating OIDC provider ${PROVIDER_ID} bound to CUDly issuer..." -gcloud iam workload-identity-pools providers create-oidc "${PROVIDER_ID}" \ - --location=global \ - --workload-identity-pool="${POOL_ID}" \ - --issuer-uri="${CUDLY_ISSUER_URL}" \ - --attribute-mapping="google.subject=assertion.sub" \ - --attribute-condition="assertion.sub == '${CUDLY_FEDERATED_SUBJECT}'" \ - --project="${PROJECT_ID}" \ - || echo " (provider may already exist)" +echo "Creating Workload Identity Pool ${POOL_ID}..." +if gcloud iam workload-identity-pools describe "${POOL_ID}" \ + --location=global --project="${PROJECT_ID}" >/dev/null 2>&1; then + echo " Reusing existing pool ${POOL_ID}" +else + gcloud iam workload-identity-pools create "${POOL_ID}" \ + --location=global \ + --display-name="CUDly WIF pool" \ + --project="${PROJECT_ID}" +fi -# WIF pool creation is eventually consistent — retry describe briefly +# WIF pool creation is eventually consistent — retry describe briefly. This runs +# before the provider is created: a provider create against a pool that has not +# propagated yet fails, and no failure here is tolerated any more. for _attempt in 1 2 3 4 5; do POOL_NAME=$(gcloud iam workload-identity-pools describe "${POOL_ID}" --location=global --project="${PROJECT_ID}" --format="value(name)" 2>/dev/null) && break echo " Waiting for WIF pool to propagate (attempt ${_attempt})..." @@ -69,6 +96,131 @@ done # provider resource, prefixed with //iam.googleapis.com/ WIF_AUDIENCE="//iam.googleapis.com/${POOL_NAME}/providers/${PROVIDER_ID}" +# The impersonation grant further down names the pool, not the provider: GCP +# principal identifiers are pool-scoped, so any other provider in ${POOL_ID} +# that maps a token to subject '${CUDLY_FEDERATED_SUBJECT}' satisfies it too. +# Verifying only ${PROVIDER_ID} would leave that door open, so the pool must +# hold nothing else. +# +# Checked here rather than next to the grant, because it is the one check that +# can refuse a project this script has not touched yet: run after the provider +# create, it would create a provider and only then abort, leaving one behind on +# every attempt. +# +# Soft-deleted providers are excluded (--show-deleted is not passed) so that one +# deleted earlier does not block a re-run. That is a deliberate gap, not a free +# choice: a soft-deleted sibling is restorable by whoever created it for 30 +# days, and the grant would already be in place when they restore it. Aborting +# on them instead would refuse every pool that has ever had a provider removed, +# which is the worse failure. Deliberately asymmetric with ${PROVIDER_ID} itself, +# where a DELETED state is a hard abort below: there the alternative is not a +# refused re-run but this script reporting success over a provider that cannot +# mint a token, which is a claim it controls. +# +# The list is fetched into a variable rather than piped: a failed list inside a +# pipeline would read as "no other providers" and wave through the case this +# check exists to catch. +POOL_PROVIDERS=$(gcloud iam workload-identity-pools providers list \ + --location=global \ + --workload-identity-pool="${POOL_ID}" \ + --project="${PROJECT_ID}" \ + --format="value(name)") +FOREIGN_PROVIDERS="" +while IFS= read -r _provider; do + [[ -n "${_provider}" ]] || continue + # Compared as a whole path segment, not with a substring test: a provider + # named "cudly-oidc-old" must not be read as this run's "cudly-oidc". + [[ "${_provider##*/}" == "${PROVIDER_ID}" ]] || FOREIGN_PROVIDERS+=" ${_provider}"$'\n' +done <<<"${POOL_PROVIDERS}" +if [[ -n "${FOREIGN_PROVIDERS}" ]]; then + echo "ERROR: pool '${POOL_ID}' holds providers this script did not configure:" >&2 + printf '%s' "${FOREIGN_PROVIDERS}" >&2 + echo " The impersonation grant names the pool, so any of them that maps a token" >&2 + echo " to subject '${CUDLY_FEDERATED_SUBJECT}' could impersonate ${SERVICE_ACCOUNT_EMAIL}." >&2 + echo " One named cudly-aws or cudly-azure was created by an earlier version of" >&2 + echo " this script and may still be the provider that account federates through," >&2 + echo " so do not delete it before checking. The safe move is to set POOL_ID to a" >&2 + echo " new pool for this run; the WIF audience changes with the pool, so register" >&2 + echo " the one that run prints at the end." >&2 + echo " No provider was created and no impersonation grant was made." >&2 + exit 1 +fi + +provider_field() { + gcloud iam workload-identity-pools providers describe "${PROVIDER_ID}" \ + --location=global \ + --workload-identity-pool="${POOL_ID}" \ + --project="${PROJECT_ID}" \ + --format="value($1)" +} + +reject_existing_provider() { + echo "ERROR: provider '${PROVIDER_ID}' already exists with a different $1, so this script" >&2 + echo " cannot vouch for which identities enter pool '${POOL_ID}'." >&2 + echo " found : ${2:-(none)}" >&2 + echo " expected: $3" >&2 + # Not "use a different PROVIDER_ID": that leaves this provider in the pool, + # which the enumeration above then refuses. The pool is the unit to move. + echo "Delete the provider and re-run, or set POOL_ID to a new pool:" >&2 + echo " gcloud iam workload-identity-pools providers delete ${PROVIDER_ID} --location=global --workload-identity-pool=${POOL_ID} --project=${PROJECT_ID}" >&2 + echo "Note that delete is a soft delete: the ID stays reserved for 30 days, so a" >&2 + echo "re-run in the same pool needs 'providers undelete' first." >&2 + exit 1 +} + +echo "Creating OIDC provider ${PROVIDER_ID} bound to CUDly issuer..." +if gcloud iam workload-identity-pools providers describe "${PROVIDER_ID}" \ + --location=global --workload-identity-pool="${POOL_ID}" --project="${PROJECT_ID}" >/dev/null 2>&1; then + # A provider that already exists keeps whatever it was created with, and the + # grant below admits everything that provider lets into the pool. Reusing one + # whose attribute condition is weaker (or absent) is therefore the failure + # this check exists to stop, not the failed create itself: a condition of + # "true" is non-empty and admits every subject the issuer signs. Every field + # below is compared against what this script would have written, and any + # difference aborts before the impersonation grant. + # + # describe returns a soft-deleted or disabled provider with its configuration + # intact, so those comparisons would all match while no token exchange can use + # it. An empty state means gcloud did not report the field, not that the + # provider is unusable. + EXISTING_STATE=$(provider_field state) + EXISTING_DISABLED=$(provider_field disabled) + # Both fields are compared against the one value that means "usable" rather + # than against the values that mean "not": testing EXISTING_DISABLED == True + # would pass a "true" or "1" the day gcloud renders the boolean differently. + if [[ -n "${EXISTING_STATE}" && "${EXISTING_STATE}" != "ACTIVE" ]] || [[ -n "${EXISTING_DISABLED}" && "${EXISTING_DISABLED}" != "False" ]]; then + echo "ERROR: provider '${PROVIDER_ID}' exists but cannot exchange tokens (state: ${EXISTING_STATE:-unreported}, disabled: ${EXISTING_DISABLED:-unreported})." >&2 + echo " Restore it and re-run, or set POOL_ID to a new pool:" >&2 + echo " gcloud iam workload-identity-pools providers undelete ${PROVIDER_ID} --location=global --workload-identity-pool=${POOL_ID} --project=${PROJECT_ID}" >&2 + echo " gcloud iam workload-identity-pools providers update-oidc ${PROVIDER_ID} --no-disabled --location=global --workload-identity-pool=${POOL_ID} --project=${PROJECT_ID}" >&2 + exit 1 + fi + + EXISTING_ISSUER=$(provider_field oidc.issuerUri) + EXISTING_CONDITION=$(provider_field attributeCondition) + # gcloud's value() printer renders a map field as "key=value" entries joined + # with ';'. This mapping has a single entry, so it compares directly against + # the form --attribute-mapping takes as input. + EXISTING_MAPPING=$(provider_field attributeMapping) + + [[ "${EXISTING_ISSUER}" == "${CUDLY_ISSUER_URL}" ]] \ + || reject_existing_provider "issuer URI" "${EXISTING_ISSUER}" "${CUDLY_ISSUER_URL}" + [[ "${EXISTING_CONDITION}" == "${EXPECTED_CONDITION}" ]] \ + || reject_existing_provider "attribute condition" "${EXISTING_CONDITION}" "${EXPECTED_CONDITION}" + [[ "${EXISTING_MAPPING}" == "${EXPECTED_MAPPING}" ]] \ + || reject_existing_provider "attribute mapping" "${EXISTING_MAPPING}" "${EXPECTED_MAPPING}" + + echo " Reusing existing provider ${PROVIDER_ID} (issuer, attribute condition and mapping match)" +else + gcloud iam workload-identity-pools providers create-oidc "${PROVIDER_ID}" \ + --location=global \ + --workload-identity-pool="${POOL_ID}" \ + --issuer-uri="${CUDLY_ISSUER_URL}" \ + --attribute-mapping="${EXPECTED_MAPPING}" \ + --attribute-condition="${EXPECTED_CONDITION}" \ + --project="${PROJECT_ID}" +fi + echo "Granting workloadIdentityUser on ${SERVICE_ACCOUNT_EMAIL} to ${CUDLY_FEDERATED_SUBJECT}..." gcloud iam service-accounts add-iam-policy-binding "${SERVICE_ACCOUNT_EMAIL}" \ --role=roles/iam.workloadIdentityUser \ diff --git a/internal/iacfiles/templates_test.go b/internal/iacfiles/templates_test.go index 03d7086ec..a8bc30392 100644 --- a/internal/iacfiles/templates_test.go +++ b/internal/iacfiles/templates_test.go @@ -2,6 +2,7 @@ package iacfiles import ( "bytes" + "cmp" "context" "errors" "os" @@ -160,12 +161,26 @@ func TestCLITemplatesAutoRegister(t *testing.T) { // deployment, subject is the fixed cudly-controller. `CUDLY_ISSUER_URL="${CUDLY_ISSUER_URL:-https://cudly.example.com/oidc}"`, `--issuer-uri="${CUDLY_ISSUER_URL}"`, - `--attribute-mapping="google.subject=assertion.sub"`, - `--attribute-condition="assertion.sub == '${CUDLY_FEDERATED_SUBJECT}'"`, + // #1661: the provider is created from these two variables and an + // existing provider is compared against the same ones, so the + // create path and the reuse check cannot drift apart. + `EXPECTED_MAPPING="google.subject=assertion.sub"`, + `EXPECTED_CONDITION="assertion.sub == '${CUDLY_FEDERATED_SUBJECT}'"`, + `--attribute-mapping="${EXPECTED_MAPPING}"`, + `--attribute-condition="${EXPECTED_CONDITION}"`, `principal://iam.googleapis.com/${POOL_NAME}/subject/${CUDLY_FEDERATED_SUBJECT}`, `WIF_AUDIENCE="//iam.googleapis.com/${POOL_NAME}/providers/${PROVIDER_ID}"`, }, mustNot: []string{ + // #1661: a swallowed create is what let the script grant the + // impersonation binding against a provider it never inspected. + "(pool may already exist)", + "(provider may already exist)", + // An abort message must not overstate what it left behind: + // `gcloud config set project` and `services enable` have both + // already run by then. Reassuring output that does not match + // what happened is the defect this issue is about. + "Nothing has been created or changed", "/api/registrations", `"service_account_email":`, // The old AWS-STS-ARN provider is gone. @@ -270,11 +285,31 @@ func awsStubScript(logPath string) string { // runRenderedWIFScript writes the rendered aws-wif-cli.sh to a temp file, puts a // recording `aws` stub first on PATH, runs the script under bash, and returns // its exit code, stderr and the list of `aws` invocations the stub saw. +func runRenderedWIFScript(t *testing.T, rendered string, env map[string]string) (exitCode int, stderr string, awsCalls []string) { + t.Helper() + // stdout is dropped: this script's progress chatter is not under test. + exitCode, _, stderr, awsCalls = runRenderedScript(t, "aws-wif-cli.sh", rendered, + func(logPath string) map[string]string { + return map[string]string{"aws": awsStubScript(logPath)} + }, env) + return exitCode, stderr, awsCalls +} + +// runRenderedScript writes a rendered script to a temp file, puts the given stub +// executables (name -> script body, built around the invocation log path this +// function owns) first on PATH, runs the script under bash, and returns its exit +// code, stderr and the invocations the stubs recorded. // // PATH is set on the child process only (exec.Cmd.Env). The parent test // process's environment is never mutated, so a run here cannot race with, or -// leak a stub `aws` into, any other test in this package. -func runRenderedWIFScript(t *testing.T, rendered string, env map[string]string) (exitCode int, stderr string, awsCalls []string) { +// leak a stub CLI into, any other test in this package. +func runRenderedScript( + t *testing.T, + scriptName string, + rendered string, + stubs func(logPath string) map[string]string, + env map[string]string, +) (exitCode int, stdout, stderr string, calls []string) { t.Helper() // Fatal, not Skip: this is a security regression test, and a green run that @@ -287,7 +322,7 @@ func runRenderedWIFScript(t *testing.T, rendered string, env map[string]string) } dir := t.TempDir() - scriptPath := filepath.Join(dir, "aws-wif-cli.sh") + scriptPath := filepath.Join(dir, scriptName) if err := os.WriteFile(scriptPath, []byte(rendered), 0o600); err != nil { t.Fatalf("write rendered script: %v", err) } @@ -296,9 +331,11 @@ func runRenderedWIFScript(t *testing.T, rendered string, env map[string]string) if err := os.Mkdir(stubDir, 0o700); err != nil { t.Fatalf("create stub dir: %v", err) } - logPath := filepath.Join(dir, "aws-invocations.log") - if err := os.WriteFile(filepath.Join(stubDir, "aws"), []byte(awsStubScript(logPath)), 0o755); err != nil { - t.Fatalf("write aws stub: %v", err) + logPath := filepath.Join(dir, "invocations.log") + for name, body := range stubs(logPath) { + if err := os.WriteFile(filepath.Join(stubDir, name), []byte(body), 0o755); err != nil { + t.Fatalf("write %s stub: %v", name, err) + } } ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) @@ -306,13 +343,11 @@ func runRenderedWIFScript(t *testing.T, rendered string, env map[string]string) cmd := exec.CommandContext(ctx, bashPath, scriptPath) // The real PATH is kept after stubDir so the script's `sed`/`echo` still - // resolve; stubDir comes first so `aws` resolves to the stub. + // resolve; stubDir comes first so the stubbed CLI resolves to the stub. cmd.Env = []string{"PATH=" + stubDir + string(os.PathListSeparator) + os.Getenv("PATH")} for k, v := range env { cmd.Env = append(cmd.Env, k+"="+v) } - // stdout is captured but not returned: the script's progress chatter is not - // under test, and capturing it keeps it out of the test log. var errBuf, outBuf strings.Builder cmd.Stderr = &errBuf cmd.Stdout = &outBuf @@ -327,15 +362,15 @@ func runRenderedWIFScript(t *testing.T, rendered string, env map[string]string) case os.IsNotExist(err): // The stub was never invoked, which is what the reject cases want. case err != nil: - t.Fatalf("read aws invocation log: %v", err) + t.Fatalf("read invocation log: %v", err) default: for _, line := range strings.Split(strings.TrimSpace(string(logBytes)), "\n") { if line != "" { - awsCalls = append(awsCalls, line) + calls = append(calls, line) } } } - return cmd.ProcessState.ExitCode(), errBuf.String(), awsCalls + return cmd.ProcessState.ExitCode(), outBuf.String(), errBuf.String(), calls } // TestAWSWIFCLI_SubjectClaimGuardBlocksAWSCalls executes the rendered script @@ -453,6 +488,485 @@ func TestAWSWIFCLI_SubjectClaimGuardBlocksAWSCalls(t *testing.T) { }) } +// gcloudStubScript returns a stand-in `gcloud` that records every invocation and +// answers describes from state files under $CUDLY_STUB_STATE. The state +// directory is the caller's, so a test can seed a pre-existing pool/provider +// before the run and can hand the same directory to two consecutive runs to +// exercise the re-run path a customer actually takes. +// +// Only the describe/create verbs the GCP WIF script branches on are modeled; +// everything else (config set, services enable, add-iam-policy-binding) +// succeeds, so a run that reaches them has proved the guards let it through. +func gcloudStubScript(logPath string) string { + return `#!/usr/bin/env bash +state="${CUDLY_STUB_STATE:?stub state dir}" +printf '%s\n' "gcloud $*" >> '` + logPath + `' + +# Flag values are read off the argument vector rather than parsed out of the +# joined string, so a value containing spaces or quotes is recorded verbatim. +issuer=""; condition=""; mapping=""; provider_id=""; prev="" +for a in "$@"; do + case "$a" in + --issuer-uri=*) issuer="${a#--issuer-uri=}" ;; + --attribute-condition=*) condition="${a#--attribute-condition=}" ;; + --attribute-mapping=*) mapping="${a#--attribute-mapping=}" ;; + esac + [[ "$prev" == "create-oidc" ]] && provider_id="$a" + prev="$a" +done + +pool_path="projects/000000000000/locations/global/workloadIdentityPools/cudly-target" + +case "$*" in + *"workload-identity-pools providers describe"*) + [[ -f "${state}/provider" ]] || exit 1 + case "$*" in + *"value(oidc.issuerUri)"*) cat "${state}/provider.issuer" ;; + *"value(attributeCondition)"*) cat "${state}/provider.condition" ;; + *"value(attributeMapping)"*) cat "${state}/provider.mapping" ;; + *"value(state)"*) cat "${state}/provider.state" ;; + *"value(disabled)"*) cat "${state}/provider.disabled" ;; + esac + ;; + *"workload-identity-pools providers list"*) + if [[ -n "${STUB_PROVIDER_LIST_ERROR:-}" ]]; then + echo "ERROR: (gcloud.iam.workload-identity-pools.providers.list) ${STUB_PROVIDER_LIST_ERROR}" >&2 + exit 1 + fi + # Soft-deleted providers are omitted here as they are by real gcloud + # without --show-deleted; the seeded list holds only live ones. + [[ -f "${state}/providers.list" ]] && cat "${state}/providers.list" + ;; + *"workload-identity-pools providers create-oidc"*) + if [[ -n "${STUB_PROVIDER_CREATE_ERROR:-}" ]]; then + echo "ERROR: (gcloud.iam.workload-identity-pools.providers.create-oidc) ${STUB_PROVIDER_CREATE_ERROR}" >&2 + exit 1 + fi + printf '%s\n' "$issuer" > "${state}/provider.issuer" + printf '%s\n' "$condition" > "${state}/provider.condition" + printf '%s\n' "$mapping" > "${state}/provider.mapping" + printf 'ACTIVE\n' > "${state}/provider.state" + printf 'False\n' > "${state}/provider.disabled" + printf '%s\n' "${pool_path}/providers/${provider_id}" >> "${state}/providers.list" + touch "${state}/provider" + ;; + *"workload-identity-pools describe"*) + [[ -f "${state}/pool" ]] || exit 1 + echo "projects/000000000000/locations/global/workloadIdentityPools/cudly-target" + ;; + *"workload-identity-pools create"*) + if [[ -n "${STUB_POOL_CREATE_ERROR:-}" ]]; then + echo "ERROR: (gcloud.iam.workload-identity-pools.create) ${STUB_POOL_CREATE_ERROR}" >&2 + exit 1 + fi + touch "${state}/pool" + ;; +esac +exit 0 +` +} + +const ( + // gcpStubIssuer is the issuer the rendered script is run with, i.e. the + // value an existing provider must carry to be reusable. + gcpStubIssuer = "https://cudly.example.com/oidc" + // gcpExpectedCondition/gcpExpectedMapping are what the script writes for the + // default subject, and therefore what it must demand of a provider it reuses. + gcpExpectedCondition = "assertion.sub == 'cudly-controller'" + gcpExpectedMapping = "google.subject=assertion.sub" +) + +// gcpStubState is the pre-existing GCP state a run starts from. The zero value +// is an empty project. +type gcpStubState struct { + pool bool + provider bool + // Set only when provider is true. + issuer string + condition string + mapping string + // state defaults to ACTIVE and disabled to False when provider is true. + state string + disabled string + // otherProviders are further live providers in the same pool, given as + // bare IDs. + otherProviders []string +} + +// seedGCPStubState materializes state in dir for the stub to read. +func seedGCPStubState(t *testing.T, dir string, s gcpStubState) { + t.Helper() + write := func(name, content string) { + if err := os.WriteFile(filepath.Join(dir, name), []byte(content+"\n"), 0o600); err != nil { + t.Fatalf("seed %s: %v", name, err) + } + } + if s.pool { + write("pool", "") + } + const poolPath = "projects/000000000000/locations/global/workloadIdentityPools/cudly-target" + var live []string + if s.provider { + write("provider", "") + write("provider.issuer", s.issuer) + write("provider.condition", s.condition) + write("provider.mapping", s.mapping) + write("provider.state", cmp.Or(s.state, "ACTIVE")) + write("provider.disabled", cmp.Or(s.disabled, "False")) + live = append(live, poolPath+"/providers/cudly-oidc") + } + for _, id := range s.otherProviders { + live = append(live, poolPath+"/providers/"+id) + } + if len(live) > 0 { + write("providers.list", strings.Join(live, "\n")) + } +} + +// runGCPWIFScript renders gcp-wif-cli.sh and runs it against the gcloud stub, +// with stateDir carrying the project state across runs. +func runGCPWIFScript(t *testing.T, stateDir string, env map[string]string) (exitCode int, stdout, stderr string, calls []string) { + t.Helper() + // CUDlyAPIURL is cleared so the auto-registration block (curl + jq) is not + // rendered: these tests are about the pool/provider/grant sequence. + data := baseData() + data.CUDlyAPIURL = "" + data.ProjectID = "cudly-demo-project" + data.ServiceAccountEmail = "cudly@cudly-demo-project.iam.gserviceaccount.com" + rendered := renderCLITemplate(t, "templates/gcp-wif-cli.sh.tmpl", data) + + full := map[string]string{ + "CUDLY_STUB_STATE": stateDir, + // Rendered from an empty CUDlyAPIURL above, so it must be supplied here + // for the issuer guard to pass. + "CUDLY_ISSUER_URL": gcpStubIssuer, + } + for k, v := range env { + full[k] = v + } + return runRenderedScript(t, "gcp-wif-cli.sh", rendered, func(logPath string) map[string]string { + return map[string]string{"gcloud": gcloudStubScript(logPath)} + }, full) +} + +// callsContaining returns the recorded gcloud invocations containing needle. +func callsContaining(calls []string, needle string) []string { + var out []string + for _, c := range calls { + if strings.Contains(c, needle) { + out = append(out, c) + } + } + return out +} + +// TestGCPWIFCLI_ProviderReuseIsVerified is the regression test for #1661. The +// served script used to run both creates with `|| echo "(may already exist)"`, +// so under `set -euo pipefail` every failure of a security-critical create was +// swallowed: a provider that already existed with a weaker attribute condition +// (or none) was silently reused, the impersonation grant was made against it, +// and the script printed "=== Done ===" and exited 0. +// +// Each reject case below asserts the run aborts *before* the grant, and the +// accept cases assert a fresh project and a legitimate re-run both still +// succeed, so the fix cannot be a blanket refusal. +func TestGCPWIFCLI_ProviderReuseIsVerified(t *testing.T) { + const grantCall = "add-iam-policy-binding" + + reject := []struct { + name string + state gcpStubState + env map[string]string + // wantText must appear in stderr. + wantText string + // absentCall, when set, must not appear among the recorded gcloud + // invocations. It carries the cases where aborting at the right point + // matters as much as aborting at all. + absentCall string + }{ + { + // The actual vulnerability: "true" is non-empty and admits every + // subject the issuer will sign. + name: "existing provider with an always-true condition", + state: gcpStubState{pool: true, provider: true, issuer: gcpStubIssuer, condition: "true", mapping: gcpExpectedMapping}, + wantText: "different attribute condition", + }, + { + name: "existing provider with no condition at all", + state: gcpStubState{pool: true, provider: true, issuer: gcpStubIssuer, condition: "", mapping: gcpExpectedMapping}, + wantText: "different attribute condition", + }, + { + name: "existing provider pinned to a different subject", + state: gcpStubState{pool: true, provider: true, issuer: gcpStubIssuer, condition: "assertion.sub == 'someone-else'", mapping: gcpExpectedMapping}, + wantText: "different attribute condition", + }, + { + // A matching condition on a provider trusting someone else's issuer + // admits whoever that issuer signs a cudly-controller token for. + name: "existing provider bound to a foreign issuer", + state: gcpStubState{pool: true, provider: true, issuer: "https://evil.example.com/oidc", condition: gcpExpectedCondition, mapping: gcpExpectedMapping}, + wantText: "different issuer URI", + }, + { + name: "existing provider with a different attribute mapping", + state: gcpStubState{pool: true, provider: true, issuer: gcpStubIssuer, condition: gcpExpectedCondition, mapping: "google.subject=assertion.email"}, + wantText: "different attribute mapping", + }, + { + // Not an "already exists" case: any other create failure (quota, + // permission, a concurrent run winning the race) must stop the run + // rather than fall through to the grant. + name: "provider create fails", + state: gcpStubState{pool: true}, + env: map[string]string{"STUB_PROVIDER_CREATE_ERROR": "PERMISSION_DENIED: caller lacks iam.workloadIdentityPoolProviders.create"}, + wantText: "PERMISSION_DENIED", + }, + { + // The pre-fix script swallowed this one too and went on to create + // the provider; the assertion that no provider create is attempted + // is what makes this case a regression witness rather than a + // restatement of the propagation loop's eventual abort. + name: "pool create fails", + env: map[string]string{"STUB_POOL_CREATE_ERROR": "PERMISSION_DENIED: caller lacks iam.workloadIdentityPools.create"}, + wantText: "PERMISSION_DENIED", + absentCall: "providers create-oidc", + }, + { + // describe returns a soft-deleted provider with its configuration + // intact, so every comparison matches and the run would otherwise + // report success against a provider that cannot exchange tokens. + name: "existing provider is soft-deleted", + state: gcpStubState{ + pool: true, provider: true, issuer: gcpStubIssuer, + condition: gcpExpectedCondition, mapping: gcpExpectedMapping, state: "DELETED", + }, + wantText: "cannot exchange tokens", + }, + { + name: "existing provider is disabled", + state: gcpStubState{ + pool: true, provider: true, issuer: gcpStubIssuer, + condition: gcpExpectedCondition, mapping: gcpExpectedMapping, disabled: "True", + }, + wantText: "cannot exchange tokens", + }, + { + // The guard must not depend on gcloud rendering the boolean as + // Python's "True": a lowercase or numeric rendering still means + // disabled, and testing for the one usable value keeps it that way. + name: "existing provider is disabled, rendered lowercase", + state: gcpStubState{ + pool: true, provider: true, issuer: gcpStubIssuer, + condition: gcpExpectedCondition, mapping: gcpExpectedMapping, disabled: "true", + }, + wantText: "cannot exchange tokens", + }, + { + // The grant names the pool, not the provider, so a second provider + // in the same pool that maps a token to the same subject satisfies + // it. Verifying only this run's provider would leave that open. + name: "pool holds a provider this script did not configure", + state: gcpStubState{ + pool: true, provider: true, issuer: gcpStubIssuer, + condition: gcpExpectedCondition, mapping: gcpExpectedMapping, + otherProviders: []string{"someone-elses-oidc"}, + }, + wantText: "did not configure", + }, + { + // The fail-closed case for the enumeration itself: a list this run + // could not read says nothing about what the pool holds, so it must + // stop rather than proceed on an empty result. This is what the + // fetch-into-a-variable form buys over piping into a filter. + name: "listing the pool's providers fails", + state: gcpStubState{pool: true}, + env: map[string]string{"STUB_PROVIDER_LIST_ERROR": "PERMISSION_DENIED: caller lacks iam.workloadIdentityPoolProviders.list"}, + wantText: "PERMISSION_DENIED", + absentCall: "providers create-oidc", + }, + { + // The shape a customer onboarded before the provider default + // changed from cudly- to cudly-oidc lands in: the pool + // holds only the old provider. The refusal has to come before the + // create, or every attempt leaves another provider behind. + name: "pool holds only a legacy provider, ours not yet created", + state: gcpStubState{ + pool: true, + otherProviders: []string{"cudly-aws"}, + }, + wantText: "did not configure", + absentCall: "providers create-oidc", + }, + } + + for _, tc := range reject { + t.Run("reject/"+tc.name, func(t *testing.T) { + stateDir := t.TempDir() + seedGCPStubState(t, stateDir, tc.state) + + exitCode, stdout, stderr, calls := runGCPWIFScript(t, stateDir, tc.env) + + if exitCode == 0 { + t.Errorf("the run must abort, but it exited 0; stdout:\n%s", stdout) + } + if grants := callsContaining(calls, grantCall); len(grants) != 0 { + t.Errorf("the impersonation grant must not be made, but %s was invoked: %v", grantCall, grants) + } + if strings.Contains(stdout, "=== Done ===") { + t.Errorf("an aborted run must not report success; stdout:\n%s", stdout) + } + if !strings.Contains(stderr, tc.wantText) { + t.Errorf("stderr must explain the abort with %q, got:\n%s", tc.wantText, stderr) + } + if tc.absentCall != "" { + if got := callsContaining(calls, tc.absentCall); len(got) != 0 { + t.Errorf("the run must abort before %q, but it was invoked: %v", tc.absentCall, got) + } + } + }) + } + + // Positive control: an empty project must still be configured end to end. + // Without it, a fix that refused every run would satisfy every case above. + t.Run("accept/empty project", func(t *testing.T) { + stateDir := t.TempDir() + exitCode, stdout, stderr, calls := runGCPWIFScript(t, stateDir, nil) + + if exitCode != 0 { + t.Fatalf("a fresh project must be configured successfully, got exit %d; stderr:\n%s", exitCode, stderr) + } + if len(calls) == 0 { + t.Fatal("the gcloud stub was never invoked, so this case proves nothing") + } + created := callsContaining(calls, "providers create-oidc") + if len(created) != 1 { + t.Fatalf("expected exactly one provider create, got %d: %v", len(created), calls) + } + if !strings.Contains(created[0], "--attribute-condition="+gcpExpectedCondition) { + t.Errorf("the provider must be created with the pinned condition %q, got:\n%s", gcpExpectedCondition, created[0]) + } + if grants := callsContaining(calls, grantCall); len(grants) != 1 { + t.Errorf("expected exactly one impersonation grant, got %d: %v", len(grants), calls) + } + if !strings.Contains(stdout, "=== Done ===") { + t.Errorf("a successful run must report the values CUDly needs; stdout:\n%s", stdout) + } + }) + + // The resumable case: a customer re-running the script over the state the + // first run left behind. The second run must reuse the provider it created + // rather than refuse it, so onboarding is not broken by the fix. + t.Run("accept/re-run over the previous run's state", func(t *testing.T) { + stateDir := t.TempDir() + if exitCode, _, stderr, _ := runGCPWIFScript(t, stateDir, nil); exitCode != 0 { + t.Fatalf("first run failed with exit %d; stderr:\n%s", exitCode, stderr) + } + + exitCode, stdout, stderr, calls := runGCPWIFScript(t, stateDir, nil) + if exitCode != 0 { + t.Fatalf("re-running over an unchanged project must succeed, got exit %d; stderr:\n%s", exitCode, stderr) + } + if created := callsContaining(calls, "providers create-oidc"); len(created) != 0 { + t.Errorf("the second run must not re-create the provider, got: %v", created) + } + if !strings.Contains(stdout, "Reusing existing provider") { + t.Errorf("the second run must report the reuse; stdout:\n%s", stdout) + } + if grants := callsContaining(calls, grantCall); len(grants) != 1 { + t.Errorf("the re-run must still (idempotently) apply the grant, got %d: %v", len(grants), calls) + } + if !strings.Contains(stdout, "=== Done ===") { + t.Errorf("the re-run must complete; stdout:\n%s", stdout) + } + }) +} + +// TestGCPWIFCLI_SubjectValidated covers #1661 item 2: CUDLY_FEDERATED_SUBJECT is +// environment-overridable and lands inside the CEL string literal of +// --attribute-condition, so a value carrying a quote closes the literal and the +// remainder rewrites the condition: the value "x' || true || '" renders a +// condition whose second disjunct is a bare "true", admitting every subject the +// issuer will sign. It also lands in the principal:// identifier of the grant, +// where a '*' widens the member. +func TestGCPWIFCLI_SubjectValidated(t *testing.T) { + reject := []struct { + name string + subject string + }{ + {"CEL break-out", `x' || true || '`}, + {"double quote", `x" || true || "`}, + {"backslash", `x\'`}, + {"backtick", "x`id`"}, + {"wildcard", "*"}, + {"embedded wildcard", "cudly-*"}, + {"unexpanded placeholder", "${CUDLY_SUBJECT}"}, + {"embedded space", "cudly controller"}, + {"leading punctuation", "-cudly-controller"}, + {"over the 127-character google.subject limit", strings.Repeat("a", 128)}, + } + + for _, tc := range reject { + t.Run("reject/"+tc.name, func(t *testing.T) { + stateDir := t.TempDir() + exitCode, _, stderr, calls := runGCPWIFScript(t, stateDir, + map[string]string{"CUDLY_FEDERATED_SUBJECT": tc.subject}) + + if exitCode == 0 { + t.Errorf("CUDLY_FEDERATED_SUBJECT %q was accepted (exit 0)", tc.subject) + } + // The guard must run before the first gcloud call, so a rejected run + // leaves no pool, provider or grant behind. + if len(calls) != 0 { + t.Errorf("no gcloud call may be made before the subject is validated, got %d: %v", len(calls), calls) + } + if !strings.Contains(stderr, "CUDLY_FEDERATED_SUBJECT") { + t.Errorf("stderr must name the rejected variable, got:\n%s", stderr) + } + }) + } + + // Positive controls: the default, and a subject in the Auth0/Okta shape the + // charset deliberately admits. Without these, a guard rejecting everything + // would pass every case above. + accept := []struct { + name string + subject string + // wantPinned is the subject the provider must end up pinned to, which + // differs from subject only where the script substitutes its default. + wantPinned string + }{ + {"default subject", "cudly-controller", "cudly-controller"}, + {"identity-provider-prefixed subject", "google-oauth2|1234567890", "google-oauth2|1234567890"}, + {"exactly 127 characters", strings.Repeat("a", 127), strings.Repeat("a", 127)}, + // An empty override is not a hole: ${VAR:-default} substitutes the + // default for an empty value, so the provider is still pinned to + // cudly-controller rather than to the empty subject. + {"empty falls back to the default", "", "cudly-controller"}, + } + for _, tc := range accept { + t.Run("accept/"+tc.name, func(t *testing.T) { + stateDir := t.TempDir() + exitCode, stdout, stderr, calls := runGCPWIFScript(t, stateDir, + map[string]string{"CUDLY_FEDERATED_SUBJECT": tc.subject}) + + if exitCode != 0 { + t.Fatalf("CUDLY_FEDERATED_SUBJECT %q was rejected (exit %d); stderr:\n%s", tc.subject, exitCode, stderr) + } + created := callsContaining(calls, "providers create-oidc") + if len(created) != 1 { + t.Fatalf("expected exactly one provider create, got %d: %v", len(created), calls) + } + if want := "--attribute-condition=assertion.sub == '" + tc.wantPinned + "'"; !strings.Contains(created[0], want) { + t.Errorf("the provider must be pinned to the supplied subject with %q, got:\n%s", want, created[0]) + } + if !strings.Contains(stdout, "=== Done ===") { + t.Errorf("a valid subject must configure the project end to end; stdout:\n%s", stdout) + } + }) + } +} + // TestCLITemplatesShellMetacharsPassThrough confirms that text/template does // not shell-escape interpolated values. This is intentional: the templates are // designed to be downloaded and inspected by an operator, not auto-executed. From b7a1873f0821e6f5e62ff4c172163e5f382c1696 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 19 Aug 2026 18:56:02 +0200 Subject: [PATCH 2/3] fix(iac): stop calling create-cred-config with no credential source setup-gcp-wif.sh's OIDC branch called gcloud iam workload-identity-pools create-cred-config \ --service-account= --output-file=/dev/stdout with none of the credential sources gcloud requires. It rejects the invocation ("Exactly one of (--aws | --azure | --credential-source-file | --credential-source-url | --executable-command) must be specified"), so under `set -e` the run aborted at its last step, after the pool, the provider and the impersonation grant had all been created: a configured project, a non-zero exit and nothing to register. The credential source says where the CUDly deployment reads its subject token from, which an OIDC provider does not imply, so it is now an input: --oidc-credential-source takes an absolute path (file-sourced) or an https URL (URL-sourced) and the gcloud flag is derived from the value, so the two cannot contradict each other. --oidc-credential-source-field adds the JSON format and field name for an endpoint that wraps the token in an object. Both are refused under --provider-type aws, whose source is AWS IMDS, rather than accepted and ignored. Without a source the credential config is skipped rather than attempted, and the run reports the WIF audience to register as gcp_wif_audience, which is all CUDly needs when it signs its own subject token. Tests execute the script against a gcloud stub that reproduces the argparse rule, since reading the script cannot distinguish a command gcloud accepts from one it refuses. Closes #1661 --- arm/CUDly-CrossSubscription/setup-gcp-wif.sh | 98 ++++++- iac/gcp_setup_script_test.go | 269 +++++++++++++++++++ 2 files changed, 354 insertions(+), 13 deletions(-) create mode 100644 iac/gcp_setup_script_test.go diff --git a/arm/CUDly-CrossSubscription/setup-gcp-wif.sh b/arm/CUDly-CrossSubscription/setup-gcp-wif.sh index c61ea49e5..2eb122b67 100755 --- a/arm/CUDly-CrossSubscription/setup-gcp-wif.sh +++ b/arm/CUDly-CrossSubscription/setup-gcp-wif.sh @@ -1,8 +1,12 @@ #!/usr/bin/env bash # setup-gcp-wif.sh — Configure a GCP Workload Identity Pool and Provider so that # CUDly (running on AWS or with an OIDC token) can access GCP without a service -# account key file. Outputs the external-account credential config JSON that -# should be stored as gcp_workload_identity_config in CUDly. +# account key file. For an AWS provider it outputs the external-account +# credential config JSON to store as gcp_workload_identity_config in CUDly. For +# an OIDC provider that config can only be built once you say where the CUDly +# deployment reads its token from (--oidc-credential-source); without it the +# script prints the WIF audience to register as gcp_wif_audience instead, which +# is all CUDly needs when it signs its own subject token. # # SECURITY: the provider is always created with an --attribute-condition and the # service account impersonation grant is always scoped to a single principal. @@ -31,7 +35,15 @@ # --provider-type oidc \ # --issuer-uri https://token.actions.githubusercontent.com \ # --oidc-subject 'repo:my-org/my-repo:ref:refs/heads/main' \ -# --sa-email cudly@my-gcp-project.iam.gserviceaccount.com +# --sa-email cudly@my-gcp-project.iam.gserviceaccount.com \ +# --oidc-credential-source /var/run/secrets/cudly/token +# +# --oidc-credential-source is where the CUDly deployment reads its OIDC token +# from: an absolute path (file-sourced) or an https:// URL (URL-sourced). It is +# only needed to emit the credential config JSON; omit it when CUDly signs its +# own subject token, and register the printed gcp_wif_audience instead. Add +# --oidc-credential-source-field when the URL returns the token inside a +# JSON object rather than as the raw response body. # # Upgrading from an earlier run: versions of this script before the # --aws-role-name / --oidc-subject requirement granted @@ -51,6 +63,8 @@ AWS_ACCOUNT_ID="" AWS_ROLE_NAME="" ISSUER_URI="" OIDC_SUBJECT="" +OIDC_CREDENTIAL_SOURCE="" +OIDC_CREDENTIAL_SOURCE_FIELD="" REMOVE_LEGACY_POOL_BINDING="false" die() { echo "Error: $*" >&2; exit 1; } @@ -67,6 +81,8 @@ while [[ $# -gt 0 ]]; do --aws-role-name) AWS_ROLE_NAME="$2"; shift 2 ;; --issuer-uri) ISSUER_URI="$2"; shift 2 ;; --oidc-subject) OIDC_SUBJECT="$2"; shift 2 ;; + --oidc-credential-source) OIDC_CREDENTIAL_SOURCE="$2"; shift 2 ;; + --oidc-credential-source-field) OIDC_CREDENTIAL_SOURCE_FIELD="$2"; shift 2 ;; --remove-legacy-pool-binding) REMOVE_LEGACY_POOL_BINDING="true"; shift ;; # Removed on purpose: --subject-condition took a raw CEL expression, so a # value such as "true" or "assertion.sub != ''" looked like a restriction @@ -112,6 +128,12 @@ validate_principal_value "--provider-id" "$PROVIDER_ID" '^[a-z0-9][-a-z0-9]{2,30 validate_principal_value "--sa-email" "$SA_EMAIL" '^[a-zA-Z0-9][-a-zA-Z0-9._]*@[a-z0-9.-]+\.gserviceaccount\.com$' if [[ "$PROVIDER_TYPE" == "aws" ]]; then + # An AWS provider's credential source is AWS IMDS (--aws below). Silently + # ignoring these would emit an IMDS-sourced config while the operator believes + # they pinned a token file, and would skip the shape validation the OIDC + # branch applies to them. + [[ -z "$OIDC_CREDENTIAL_SOURCE" && -z "$OIDC_CREDENTIAL_SOURCE_FIELD" ]] \ + || die "--oidc-credential-source/--oidc-credential-source-field do not apply to --provider-type aws, whose credential source is AWS IMDS" [[ -n "$AWS_ACCOUNT_ID" ]] || die "--aws-account-id is required for --provider-type aws" # Mandatory: without a role to pin, the provider has no attribute condition and # every IAM principal in the AWS account can federate as the service account. @@ -164,6 +186,23 @@ else [[ ${#OIDC_SUBJECT} -le 127 ]] || die "--oidc-subject must be at most 127 characters (google.subject limit); got ${#OIDC_SUBJECT}" EXPECTED_CONDITION="google.subject == '${OIDC_SUBJECT}'" EXPECTED_MAPPING="google.subject=assertion.sub" + # --oidc-credential-source says where the CUDly deployment reads its subject + # token from, which is what create-cred-config below needs and what an OIDC + # provider does not imply. The two forms cannot be confused (a URL cannot + # start with '/' and a path cannot start with 'https://'), so the gcloud flag + # is derived from the value rather than from a second flag that could + # contradict it. Plain http:// is refused: the token would cross the network + # in cleartext. + CRED_SOURCE_FLAG="" + if [[ -n "$OIDC_CREDENTIAL_SOURCE" ]]; then + case "$OIDC_CREDENTIAL_SOURCE" in + /*) CRED_SOURCE_FLAG="--credential-source-file" ;; + https://*) CRED_SOURCE_FLAG="--credential-source-url" ;; + *) die "--oidc-credential-source must be an absolute path (file-sourced) or an https:// URL (URL-sourced); got: $OIDC_CREDENTIAL_SOURCE" ;; + esac + elif [[ -n "$OIDC_CREDENTIAL_SOURCE_FIELD" ]]; then + die "--oidc-credential-source-field only applies to a credential source; pass --oidc-credential-source as well" + fi fi PROJECT_NUMBER=$(gcloud projects describe "$PROJECT" --format='value(projectNumber)') @@ -411,24 +450,51 @@ if [[ -n "$OTHER_GRANTS" ]]; then echo " Remove any that are no longer expected." >&2 fi -# ── Generate external account credential config (no secrets) ────────────────── +# ── Report the values CUDly needs (no secrets) ──────────────────────────────── PROVIDER_RESOURCE="${POOL_RESOURCE}/providers/${PROVIDER_ID}" +WIF_AUDIENCE="//iam.googleapis.com/${PROVIDER_RESOURCE}" echo "" -echo "══════════════════════════════════════════════════════════════" -echo " External account credential config (store in CUDly as" -echo " gcp_workload_identity_config — contains no secrets)" -echo "══════════════════════════════════════════════════════════════" if [[ "$PROVIDER_TYPE" == "aws" ]]; then - gcloud iam workload-identity-pools create-cred-config \ - "$PROVIDER_RESOURCE" \ - --service-account="$SA_EMAIL" \ - --aws \ - --output-file=/dev/stdout + CRED_CONFIG_ARGS=(--aws) +elif [[ -n "$OIDC_CREDENTIAL_SOURCE" ]]; then + CRED_CONFIG_ARGS=("${CRED_SOURCE_FLAG}=${OIDC_CREDENTIAL_SOURCE}") + if [[ -n "$OIDC_CREDENTIAL_SOURCE_FIELD" ]]; then + # A JSON response needs both the format and the key holding the token; + # gcloud's default (text) would treat the whole response body as the token. + CRED_CONFIG_ARGS+=(--credential-source-type=json "--credential-source-field-name=${OIDC_CREDENTIAL_SOURCE_FIELD}") + fi else + CRED_CONFIG_ARGS=() +fi + +if [[ ${#CRED_CONFIG_ARGS[@]} -gt 0 ]]; then + echo "══════════════════════════════════════════════════════════════" + echo " External account credential config (store in CUDly as" + echo " gcp_workload_identity_config — contains no secrets)" + echo "══════════════════════════════════════════════════════════════" gcloud iam workload-identity-pools create-cred-config \ "$PROVIDER_RESOURCE" \ --service-account="$SA_EMAIL" \ + "${CRED_CONFIG_ARGS[@]}" \ --output-file=/dev/stdout +else + # No credential source, so no credential config. create-cred-config requires + # exactly one of (--aws | --azure | --credential-source-file | + # --credential-source-url | --executable-command); this branch used to call it + # with none, which gcloud rejects, and under `set -e` that aborted the run + # here, after the pool, the provider and the impersonation grant had all been + # created. Nothing is lost by skipping it: CUDly needs the credential config + # only when something other than CUDly holds the token. When CUDly signs its + # own subject token it needs the audience below, which is what the served + # onboarding script (internal/iacfiles/templates/gcp-wif-cli.sh.tmpl) emits. + echo "══════════════════════════════════════════════════════════════" + echo " No credential config generated: none was requested." + echo " Pass --oidc-credential-source to emit" + echo " one, or register the gcp_wif_audience below, which is all CUDly" + echo " needs when it signs its own subject token. In that case the" + echo " trusted subject '${OIDC_SUBJECT}' must be the one your CUDly" + echo " deployment signs, and ${ISSUER_URI} must be its OIDC issuer." + echo "══════════════════════════════════════════════════════════════" fi echo "" echo "══════════════════════════════════════════════════════════════" @@ -437,4 +503,10 @@ echo " provider : gcp" echo " gcp_auth_mode : workload_identity_federation" echo " gcp_project_id : ${PROJECT}" echo " gcp_client_email (optional) : ${SA_EMAIL}" +if [[ "$PROVIDER_TYPE" != "aws" ]]; then + # OIDC only: an AWS-federated account registers the credential config printed + # above. Registering the audience instead would have CUDly present a + # self-signed assertion to a provider that only trusts AWS role ARNs. + echo " gcp_wif_audience : ${WIF_AUDIENCE}" +fi echo "══════════════════════════════════════════════════════════════" diff --git a/iac/gcp_setup_script_test.go b/iac/gcp_setup_script_test.go new file mode 100644 index 000000000..06ee2911e --- /dev/null +++ b/iac/gcp_setup_script_test.go @@ -0,0 +1,269 @@ +package iac + +import ( + "context" + "errors" + "os" + "os/exec" + "path/filepath" + "strings" + "testing" + "time" +) + +// These tests execute arm/CUDly-CrossSubscription/setup-gcp-wif.sh against a +// stub `gcloud` rather than reading it, because the defect they cover (#1661 +// item 3) is an invocation gcloud rejects: reading the script cannot tell a +// command gcloud accepts from one it refuses, and the refusal landed after every +// mutation had been made. + +// gcloudCredConfigStub is a stand-in `gcloud` that records every invocation and +// reproduces the one argparse rule under test: create-cred-config requires +// exactly one credential source. Every other verb succeeds, so a run that +// reaches them has proved the script got that far. +const gcloudCredConfigStub = `#!/usr/bin/env bash +printf '%s\n' "gcloud $*" >> "${STUB_LOG}" +case "$*" in + *"projects describe"*) + echo "000000000000" + ;; + *create-cred-config*) + sources=0 + for a in "$@"; do + case "$a" in + --aws|--azure|--credential-source-file=*|--credential-source-url=*|--executable-command=*) + sources=$((sources + 1)) ;; + esac + done + if [[ $sources -ne 1 ]]; then + echo "ERROR: (gcloud.iam.workload-identity-pools.create-cred-config) Exactly one of (--aws | --azure | --credential-source-file | --credential-source-url | --executable-command) must be specified." >&2 + exit 1 + fi + echo '{"type":"external_account","audience":"stub"}' + ;; + *"workload-identity-pools providers describe"*|*"workload-identity-pools describe"*) + # Nothing exists yet, so the script takes its create paths. + exit 1 + ;; +esac +exit 0 +` + +// runSetupScript runs setup-gcp-wif.sh with the stub `gcloud` first on PATH and +// returns its exit code, stdout, stderr and the recorded gcloud invocations. +// +// PATH is set on the child process only, so a run here cannot leak a stub +// `gcloud` into any other test. +func runSetupScript(t *testing.T, args ...string) (exitCode int, stdout, stderr string, calls []string) { + t.Helper() + + // Fatal, not Skip: a green run that never executed the script would hide + // exactly the regression this test exists to catch. + bashPath, lookErr := exec.LookPath("bash") + if lookErr != nil { + t.Fatalf("bash is required to execute %s: %v", setupScript, lookErr) + } + + dir := t.TempDir() + stubDir := filepath.Join(dir, "bin") + if err := os.Mkdir(stubDir, 0o700); err != nil { + t.Fatalf("create stub dir: %v", err) + } + if err := os.WriteFile(filepath.Join(stubDir, "gcloud"), []byte(gcloudCredConfigStub), 0o755); err != nil { + t.Fatalf("write gcloud stub: %v", err) + } + logPath := filepath.Join(dir, "gcloud-invocations.log") + + ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + defer cancel() + + cmd := exec.CommandContext(ctx, bashPath, append([]string{setupScript}, args...)...) + cmd.Env = []string{ + "PATH=" + stubDir + string(os.PathListSeparator) + os.Getenv("PATH"), + "STUB_LOG=" + logPath, + } + var outBuf, errBuf strings.Builder + cmd.Stdout = &outBuf + cmd.Stderr = &errBuf + + var exitErr *exec.ExitError + if err := cmd.Run(); err != nil && !errors.As(err, &exitErr) { + t.Fatalf("run %s: %v (stderr: %s)", setupScript, err, errBuf.String()) + } + + logBytes, err := os.ReadFile(logPath) + switch { + case os.IsNotExist(err): + // The stub was never invoked, which is what the reject case wants. + case err != nil: + t.Fatalf("read gcloud invocation log: %v", err) + default: + for _, line := range strings.Split(strings.TrimSpace(string(logBytes)), "\n") { + if line != "" { + calls = append(calls, line) + } + } + } + return cmd.ProcessState.ExitCode(), outBuf.String(), errBuf.String(), calls +} + +// setupScriptCommonArgs are the flags every case below shares. +func setupScriptCommonArgs() []string { + return []string{ + "--project", "my-gcp-project", + "--sa-email", "cudly@my-gcp-project.iam.gserviceaccount.com", + } +} + +func setupScriptOIDCArgs(extra ...string) []string { + return append(append(setupScriptCommonArgs(), + "--provider-type", "oidc", + "--issuer-uri", "https://cudly.example.com/oidc", + "--oidc-subject", "cudly-controller", + ), extra...) +} + +// credConfigCalls returns the recorded create-cred-config invocations. +func credConfigCalls(calls []string) []string { + var out []string + for _, c := range calls { + if strings.Contains(c, "create-cred-config") { + out = append(out, c) + } + } + return out +} + +// TestSetupScriptOIDCCompletesWithoutACredentialSource is the regression test +// for #1661 item 3. The OIDC branch called create-cred-config with no credential +// source; gcloud requires exactly one, so the command was rejected and `set -e` +// aborted the run at its last step, after the pool, the provider and the +// impersonation grant had all been created. The customer was left with a +// configured project, a non-zero exit and nothing to register. +func TestSetupScriptOIDCCompletesWithoutACredentialSource(t *testing.T) { + exitCode, stdout, stderr, calls := runSetupScript(t, setupScriptOIDCArgs()...) + + if exitCode != 0 { + t.Fatalf("an OIDC run without a credential source must complete, got exit %d; stderr:\n%s", exitCode, stderr) + } + if len(calls) == 0 { + t.Fatal("the gcloud stub was never invoked, so this case proves nothing") + } + if got := credConfigCalls(calls); len(got) != 0 { + t.Errorf("create-cred-config must not be called without a credential source, got: %v", got) + } + // The audience is the whole point of the run once the credential config is + // not produced: without it the operator has nothing to paste into CUDly. + if !strings.Contains(stdout, "gcp_wif_audience : //iam.googleapis.com/projects/000000000000/locations/global/workloadIdentityPools/cudly-pool/providers/cudly-provider") { + t.Errorf("the run must print the WIF audience to register; stdout:\n%s", stdout) + } +} + +// TestSetupScriptEmitsCredentialConfigWhenSourced covers the paths that do +// produce a credential config, so the fix above cannot be "stop emitting one". +func TestSetupScriptEmitsCredentialConfigWhenSourced(t *testing.T) { + cases := []struct { + name string + args []string + wantFlags []string + }{ + { + name: "aws provider", + args: append(setupScriptCommonArgs(), + "--provider-type", "aws", + "--aws-account-id", "123456789012", + "--aws-role-name", "CUDly-Execution"), + wantFlags: []string{"--aws"}, + }, + { + name: "oidc with a file source", + args: setupScriptOIDCArgs("--oidc-credential-source", "/var/run/secrets/cudly/token"), + wantFlags: []string{"--credential-source-file=/var/run/secrets/cudly/token"}, + }, + { + // A JSON response needs the format and the field holding the token; + // with neither, gcloud treats the whole body as the token. + name: "oidc with a JSON url source", + args: setupScriptOIDCArgs( + "--oidc-credential-source", "https://cudly.example.com/oidc/token", + "--oidc-credential-source-field", "value"), + wantFlags: []string{ + "--credential-source-url=https://cudly.example.com/oidc/token", + "--credential-source-type=json", + "--credential-source-field-name=value", + }, + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + exitCode, _, stderr, calls := runSetupScript(t, tc.args...) + + if exitCode != 0 { + t.Fatalf("run failed with exit %d; stderr:\n%s", exitCode, stderr) + } + got := credConfigCalls(calls) + if len(got) != 1 { + t.Fatalf("expected exactly one create-cred-config invocation, got %d: %v", len(got), calls) + } + for _, flag := range tc.wantFlags { + if !strings.Contains(got[0], flag) { + t.Errorf("create-cred-config must carry %q, got:\n%s", flag, got[0]) + } + } + }) + } +} + +// TestSetupScriptRejectsCredentialSourceInAWSMode pins the credential source +// flags to the provider type they configure. An AWS provider's source is AWS +// IMDS, so accepting these and ignoring them would emit an IMDS-sourced config +// while the operator believes they pinned a token file. +func TestSetupScriptRejectsCredentialSourceInAWSMode(t *testing.T) { + args := append(setupScriptCommonArgs(), + "--provider-type", "aws", + "--aws-account-id", "123456789012", + "--aws-role-name", "CUDly-Execution", + "--oidc-credential-source", "/var/run/secrets/cudly/token") + + exitCode, _, stderr, calls := runSetupScript(t, args...) + + if exitCode == 0 { + t.Error("--oidc-credential-source was accepted under --provider-type aws (exit 0)") + } + if len(calls) != 0 { + t.Errorf("the rejection must happen before the first gcloud call, got %d: %v", len(calls), calls) + } + if !strings.Contains(stderr, "do not apply to --provider-type aws") { + t.Errorf("stderr must explain why the flag is refused, got:\n%s", stderr) + } +} + +// TestSetupScriptRejectsInsecureCredentialSource pins the two shapes a +// credential source may take. A plain http:// endpoint would carry the subject +// token in cleartext, and anything else is neither a path gcloud can read nor a +// URL it can fetch. +func TestSetupScriptRejectsInsecureCredentialSource(t *testing.T) { + for _, source := range []string{ + "http://insecure.example.com/token", + "relative/path/token", + "token", + } { + t.Run(source, func(t *testing.T) { + exitCode, _, stderr, calls := runSetupScript(t, + setupScriptOIDCArgs("--oidc-credential-source", source)...) + + if exitCode == 0 { + t.Errorf("credential source %q was accepted (exit 0)", source) + } + // Rejected before the first gcloud call, so a bad invocation leaves + // no pool, provider or grant behind. + if len(calls) != 0 { + t.Errorf("no gcloud call may be made before the credential source is validated, got %d: %v", len(calls), calls) + } + if !strings.Contains(stderr, "--oidc-credential-source") { + t.Errorf("stderr must name the rejected flag, got:\n%s", stderr) + } + }) + } +} From dee5961bce54956524fc9385a3bedfbe7a58ce5d Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 19 Aug 2026 19:35:52 +0200 Subject: [PATCH 3/3] fix(iac): reject a reused GCP WIF provider whose audience list excludes ours An existing provider was compared on issuer, condition and mapping but not on oidc.allowedAudiences. That list restricts which `aud` the provider accepts, so one that omits this pool's audience rejects every token CUDly mints while the script reports success over it. Same class as the soft-deleted and disabled cases already handled here. Empty is accepted rather than treated as unrestricted: GCP defaults an unset list to the provider's own resource name, which is exactly what WIF_AUDIENCE is. A non-empty list must contain it. gcloud's value() printer joins a repeated field with ';', verified against the bundled SDK's printer rather than assumed. Also in this round: - Corrects the charset comment on CUDLY_FEDERATED_SUBJECT and the test case that read as an endorsement of it. The comment justified '|' with Auth0/Okta-style subjects, a caller that cannot exist here, and the accept case asserted that google-oauth2|1234567890 "configures the project end to end". gcpFederatedSubject in internal/credentials/gcp_federated.go is a compile-time constant, so CUDly always presents 'cudly-controller' and any override pins the provider to a subject it will never sign. The case now asserts what it actually demonstrates, that the value round-trips through the template unmangled, and the character is documented as being in the set for parity with validate_principal_value in the ARM script. - Adds the missing test for --oidc-credential-source-field without a source, which the ARM script rejects but nothing covered. - Moves the GCP WIF stub and tests to templates_gcp_wif_test.go. The project caps files at 500 lines and templates_test.go had reached 1122; it is now 577 and the new file 557. Kept in one commit with the changes above because the moved and original definitions collide in any intermediate state. Refs #1661 --- iac/gcp_setup_script_test.go | 20 + .../iacfiles/templates/gcp-wif-cli.sh.tmpl | 24 +- internal/iacfiles/templates_gcp_wif_test.go | 557 ++++++++++++++++++ internal/iacfiles/templates_test.go | 480 --------------- 4 files changed, 599 insertions(+), 482 deletions(-) create mode 100644 internal/iacfiles/templates_gcp_wif_test.go diff --git a/iac/gcp_setup_script_test.go b/iac/gcp_setup_script_test.go index 06ee2911e..29a18d01e 100644 --- a/iac/gcp_setup_script_test.go +++ b/iac/gcp_setup_script_test.go @@ -239,6 +239,26 @@ func TestSetupScriptRejectsCredentialSourceInAWSMode(t *testing.T) { } } +// TestSetupScriptRejectsCredentialFieldWithoutSource covers the other half of +// the credential-source validation: the field name says how to read a token out +// of a JSON response, so on its own it configures nothing. Accepting it silently +// would emit a credential config with no credential source, which is the +// invocation gcloud rejects in the first place. +func TestSetupScriptRejectsCredentialFieldWithoutSource(t *testing.T) { + exitCode, _, stderr, calls := runSetupScript(t, + setupScriptOIDCArgs("--oidc-credential-source-field", "value")...) + + if exitCode == 0 { + t.Error("--oidc-credential-source-field was accepted without a source (exit 0)") + } + if len(calls) != 0 { + t.Errorf("the rejection must happen before the first gcloud call, got %d: %v", len(calls), calls) + } + if !strings.Contains(stderr, "--oidc-credential-source-field") { + t.Errorf("stderr must name the rejected flag, got:\n%s", stderr) + } +} + // TestSetupScriptRejectsInsecureCredentialSource pins the two shapes a // credential source may take. A plain http:// endpoint would carry the subject // token in cleartext, and anything else is neither a path gcloud can read nor a diff --git a/internal/iacfiles/templates/gcp-wif-cli.sh.tmpl b/internal/iacfiles/templates/gcp-wif-cli.sh.tmpl index 6c0de0da8..9a84ee412 100644 --- a/internal/iacfiles/templates/gcp-wif-cli.sh.tmpl +++ b/internal/iacfiles/templates/gcp-wif-cli.sh.tmpl @@ -40,8 +40,13 @@ CUDLY_FEDERATED_SUBJECT="${CUDLY_FEDERATED_SUBJECT:-cudly-controller}" # rewrites the condition: "x' || true || '" yields assertion.sub == 'x' || true # || '', which admits every subject the issuer will sign. A '*' widens the # principal identifier the same way. Restrict the value to characters both -# destinations treat as inert; '|' is allowed because Auth0/Okta-style subjects -# use it (google-oauth2|123) and it carries no meaning in either place. +# destinations treat as inert. The set is the one validate_principal_value +# applies to --oidc-subject in arm/CUDly-CrossSubscription/setup-gcp-wif.sh, +# kept identical so one charset governs the subject on both onboarding paths; +# '|' is inert in both destinations and is in the set for that parity, not for +# any caller here. CUDly itself always signs the default: gcpFederatedSubject in +# internal/credentials/gcp_federated.go is a compile-time constant, so an +# override pins the provider to a subject CUDly will not present. if ! [[ "${CUDLY_FEDERATED_SUBJECT}" =~ ^[A-Za-z0-9][-A-Za-z0-9._:/@=+~|]*$ ]]; then echo "ERROR: CUDLY_FEDERATED_SUBJECT must start with a letter or digit and use only [-A-Za-z0-9._:/@=+~|] (got: ${CUDLY_FEDERATED_SUBJECT})." >&2 echo " Quotes, backslashes, '*' and '\$' would escape the provider's attribute condition or widen the impersonation grant." >&2 @@ -203,6 +208,21 @@ if gcloud iam workload-identity-pools providers describe "${PROVIDER_ID}" \ # the form --attribute-mapping takes as input. EXISTING_MAPPING=$(provider_field attributeMapping) + # An audience list restricts which `aud` the provider accepts. Empty means + # unset, and GCP then defaults to the provider's own resource name, which is + # exactly what WIF_AUDIENCE is, so empty is correct rather than unrestricted. + # A non-empty list that omits WIF_AUDIENCE rejects every token CUDly mints. + # value() joins a repeated field with ';'. + EXISTING_AUDIENCES=$(provider_field oidc.allowedAudiences) + if [[ -n "${EXISTING_AUDIENCES}" ]]; then + _audience_ok="" + while IFS= read -r _audience; do + [[ "${_audience}" == "${WIF_AUDIENCE}" ]] && _audience_ok=yes + done <<<"${EXISTING_AUDIENCES//;/$'\n'}" + [[ -n "${_audience_ok}" ]] \ + || reject_existing_provider "allowed audience list" "${EXISTING_AUDIENCES}" "a list containing ${WIF_AUDIENCE}" + fi + [[ "${EXISTING_ISSUER}" == "${CUDLY_ISSUER_URL}" ]] \ || reject_existing_provider "issuer URI" "${EXISTING_ISSUER}" "${CUDLY_ISSUER_URL}" [[ "${EXISTING_CONDITION}" == "${EXPECTED_CONDITION}" ]] \ diff --git a/internal/iacfiles/templates_gcp_wif_test.go b/internal/iacfiles/templates_gcp_wif_test.go new file mode 100644 index 000000000..3833b1eae --- /dev/null +++ b/internal/iacfiles/templates_gcp_wif_test.go @@ -0,0 +1,557 @@ +package iacfiles + +// GCP WIF onboarding-script tests. Split out of templates_test.go, which the +// project's 500-line file limit had outgrown. runRenderedScript stays there and +// is shared with the AWS WIF tests. + +import ( + "cmp" + "os" + "path/filepath" + "strings" + "testing" +) + +// gcloudStubScript returns a stand-in `gcloud` that records every invocation and +// answers describes from state files under $CUDLY_STUB_STATE. The state +// directory is the caller's, so a test can seed a pre-existing pool/provider +// before the run and can hand the same directory to two consecutive runs to +// exercise the re-run path a customer actually takes. +// +// Only the describe/create verbs the GCP WIF script branches on are modeled; +// everything else (config set, services enable, add-iam-policy-binding) +// succeeds, so a run that reaches them has proved the guards let it through. +func gcloudStubScript(logPath string) string { + return `#!/usr/bin/env bash +state="${CUDLY_STUB_STATE:?stub state dir}" +printf '%s\n' "gcloud $*" >> '` + logPath + `' + +# Flag values are read off the argument vector rather than parsed out of the +# joined string, so a value containing spaces or quotes is recorded verbatim. +issuer=""; condition=""; mapping=""; provider_id=""; prev="" +for a in "$@"; do + case "$a" in + --issuer-uri=*) issuer="${a#--issuer-uri=}" ;; + --attribute-condition=*) condition="${a#--attribute-condition=}" ;; + --attribute-mapping=*) mapping="${a#--attribute-mapping=}" ;; + esac + [[ "$prev" == "create-oidc" ]] && provider_id="$a" + prev="$a" +done + +pool_path="projects/000000000000/locations/global/workloadIdentityPools/cudly-target" + +case "$*" in + *"workload-identity-pools providers describe"*) + [[ -f "${state}/provider" ]] || exit 1 + case "$*" in + *"value(oidc.issuerUri)"*) cat "${state}/provider.issuer" ;; + *"value(attributeCondition)"*) cat "${state}/provider.condition" ;; + *"value(attributeMapping)"*) cat "${state}/provider.mapping" ;; + *"value(oidc.allowedAudiences)"*) cat "${state}/provider.audiences" ;; + *"value(state)"*) cat "${state}/provider.state" ;; + *"value(disabled)"*) cat "${state}/provider.disabled" ;; + esac + ;; + *"workload-identity-pools providers list"*) + if [[ -n "${STUB_PROVIDER_LIST_ERROR:-}" ]]; then + echo "ERROR: (gcloud.iam.workload-identity-pools.providers.list) ${STUB_PROVIDER_LIST_ERROR}" >&2 + exit 1 + fi + # Soft-deleted providers are omitted here as they are by real gcloud + # without --show-deleted; the seeded list holds only live ones. + [[ -f "${state}/providers.list" ]] && cat "${state}/providers.list" + ;; + *"workload-identity-pools providers create-oidc"*) + if [[ -n "${STUB_PROVIDER_CREATE_ERROR:-}" ]]; then + echo "ERROR: (gcloud.iam.workload-identity-pools.providers.create-oidc) ${STUB_PROVIDER_CREATE_ERROR}" >&2 + exit 1 + fi + printf '%s\n' "$issuer" > "${state}/provider.issuer" + printf '%s\n' "$condition" > "${state}/provider.condition" + printf '%s\n' "$mapping" > "${state}/provider.mapping" + printf '\n' > "${state}/provider.audiences" + printf 'ACTIVE\n' > "${state}/provider.state" + printf 'False\n' > "${state}/provider.disabled" + printf '%s\n' "${pool_path}/providers/${provider_id}" >> "${state}/providers.list" + touch "${state}/provider" + ;; + *"workload-identity-pools describe"*) + [[ -f "${state}/pool" ]] || exit 1 + echo "projects/000000000000/locations/global/workloadIdentityPools/cudly-target" + ;; + *"workload-identity-pools create"*) + if [[ -n "${STUB_POOL_CREATE_ERROR:-}" ]]; then + echo "ERROR: (gcloud.iam.workload-identity-pools.create) ${STUB_POOL_CREATE_ERROR}" >&2 + exit 1 + fi + touch "${state}/pool" + ;; +esac +exit 0 +` +} + +const ( + // gcpStubIssuer is the issuer the rendered script is run with, i.e. the + // value an existing provider must carry to be reusable. + gcpStubIssuer = "https://cudly.example.com/oidc" + // gcpExpectedCondition/gcpExpectedMapping are what the script writes for the + // default subject, and therefore what it must demand of a provider it reuses. + gcpExpectedCondition = "assertion.sub == 'cudly-controller'" + gcpExpectedMapping = "google.subject=assertion.sub" + // gcpStubAudience is WIF_AUDIENCE for the stub's pool: the provider's own + // resource name, which is what GCP defaults an unset audience list to and + // what CUDly presents as `aud`. + gcpStubAudience = "//iam.googleapis.com/projects/000000000000/locations/global/workloadIdentityPools/cudly-target/providers/cudly-oidc" +) + +// gcpStubState is the pre-existing GCP state a run starts from. The zero value +// is an empty project. +type gcpStubState struct { + pool bool + provider bool + // Set only when provider is true. + issuer string + condition string + mapping string + // state defaults to ACTIVE and disabled to False when provider is true. + state string + disabled string + // audiences is the ';'-joined oidc.allowedAudiences list. Empty means the + // field is unset, which is how a provider created without --allowed-audiences + // reports, and which GCP treats as "the provider's own resource name". + audiences string + // otherProviders are further live providers in the same pool, given as + // bare IDs. + otherProviders []string +} + +// seedGCPStubState materializes state in dir for the stub to read. +func seedGCPStubState(t *testing.T, dir string, s gcpStubState) { + t.Helper() + write := func(name, content string) { + if err := os.WriteFile(filepath.Join(dir, name), []byte(content+"\n"), 0o600); err != nil { + t.Fatalf("seed %s: %v", name, err) + } + } + if s.pool { + write("pool", "") + } + const poolPath = "projects/000000000000/locations/global/workloadIdentityPools/cudly-target" + var live []string + if s.provider { + write("provider", "") + write("provider.issuer", s.issuer) + write("provider.condition", s.condition) + write("provider.mapping", s.mapping) + write("provider.audiences", s.audiences) + write("provider.state", cmp.Or(s.state, "ACTIVE")) + write("provider.disabled", cmp.Or(s.disabled, "False")) + live = append(live, poolPath+"/providers/cudly-oidc") + } + for _, id := range s.otherProviders { + live = append(live, poolPath+"/providers/"+id) + } + if len(live) > 0 { + write("providers.list", strings.Join(live, "\n")) + } +} + +// runGCPWIFScript renders gcp-wif-cli.sh and runs it against the gcloud stub, +// with stateDir carrying the project state across runs. +func runGCPWIFScript(t *testing.T, stateDir string, env map[string]string) (exitCode int, stdout, stderr string, calls []string) { + t.Helper() + // CUDlyAPIURL is cleared so the auto-registration block (curl + jq) is not + // rendered: these tests are about the pool/provider/grant sequence. + data := baseData() + data.CUDlyAPIURL = "" + data.ProjectID = "cudly-demo-project" + data.ServiceAccountEmail = "cudly@cudly-demo-project.iam.gserviceaccount.com" + rendered := renderCLITemplate(t, "templates/gcp-wif-cli.sh.tmpl", data) + + full := map[string]string{ + "CUDLY_STUB_STATE": stateDir, + // Rendered from an empty CUDlyAPIURL above, so it must be supplied here + // for the issuer guard to pass. + "CUDLY_ISSUER_URL": gcpStubIssuer, + } + for k, v := range env { + full[k] = v + } + return runRenderedScript(t, "gcp-wif-cli.sh", rendered, func(logPath string) map[string]string { + return map[string]string{"gcloud": gcloudStubScript(logPath)} + }, full) +} + +// callsContaining returns the recorded gcloud invocations containing needle. +func callsContaining(calls []string, needle string) []string { + var out []string + for _, c := range calls { + if strings.Contains(c, needle) { + out = append(out, c) + } + } + return out +} + +// TestGCPWIFCLI_ProviderReuseIsVerified is the regression test for #1661. The +// served script used to run both creates with `|| echo "(may already exist)"`, +// so under `set -euo pipefail` every failure of a security-critical create was +// swallowed: a provider that already existed with a weaker attribute condition +// (or none) was silently reused, the impersonation grant was made against it, +// and the script printed "=== Done ===" and exited 0. +// +// Each reject case below asserts the run aborts *before* the grant, and the +// accept cases assert a fresh project and a legitimate re-run both still +// succeed, so the fix cannot be a blanket refusal. +func TestGCPWIFCLI_ProviderReuseIsVerified(t *testing.T) { + const grantCall = "add-iam-policy-binding" + + reject := []struct { + name string + state gcpStubState + env map[string]string + // wantText must appear in stderr. + wantText string + // absentCall, when set, must not appear among the recorded gcloud + // invocations. It carries the cases where aborting at the right point + // matters as much as aborting at all. + absentCall string + }{ + { + // The actual vulnerability: "true" is non-empty and admits every + // subject the issuer will sign. + name: "existing provider with an always-true condition", + state: gcpStubState{pool: true, provider: true, issuer: gcpStubIssuer, condition: "true", mapping: gcpExpectedMapping}, + wantText: "different attribute condition", + }, + { + name: "existing provider with no condition at all", + state: gcpStubState{pool: true, provider: true, issuer: gcpStubIssuer, condition: "", mapping: gcpExpectedMapping}, + wantText: "different attribute condition", + }, + { + name: "existing provider pinned to a different subject", + state: gcpStubState{pool: true, provider: true, issuer: gcpStubIssuer, condition: "assertion.sub == 'someone-else'", mapping: gcpExpectedMapping}, + wantText: "different attribute condition", + }, + { + // A matching condition on a provider trusting someone else's issuer + // admits whoever that issuer signs a cudly-controller token for. + name: "existing provider bound to a foreign issuer", + state: gcpStubState{pool: true, provider: true, issuer: "https://evil.example.com/oidc", condition: gcpExpectedCondition, mapping: gcpExpectedMapping}, + wantText: "different issuer URI", + }, + { + name: "existing provider with a different attribute mapping", + state: gcpStubState{pool: true, provider: true, issuer: gcpStubIssuer, condition: gcpExpectedCondition, mapping: "google.subject=assertion.email"}, + wantText: "different attribute mapping", + }, + { + // An audience list that omits this pool's audience rejects every + // token CUDly mints, so the run would otherwise report success over + // a provider that cannot accept it. + name: "existing provider restricted to another audience", + state: gcpStubState{ + pool: true, provider: true, issuer: gcpStubIssuer, + condition: gcpExpectedCondition, mapping: gcpExpectedMapping, + audiences: "//iam.googleapis.com/projects/000000000000/locations/global/workloadIdentityPools/other/providers/other", + }, + wantText: "different allowed audience list", + }, + { + // Not an "already exists" case: any other create failure (quota, + // permission, a concurrent run winning the race) must stop the run + // rather than fall through to the grant. + name: "provider create fails", + state: gcpStubState{pool: true}, + env: map[string]string{"STUB_PROVIDER_CREATE_ERROR": "PERMISSION_DENIED: caller lacks iam.workloadIdentityPoolProviders.create"}, + wantText: "PERMISSION_DENIED", + }, + { + // The pre-fix script swallowed this one too and went on to create + // the provider; the assertion that no provider create is attempted + // is what makes this case a regression witness rather than a + // restatement of the propagation loop's eventual abort. + name: "pool create fails", + env: map[string]string{"STUB_POOL_CREATE_ERROR": "PERMISSION_DENIED: caller lacks iam.workloadIdentityPools.create"}, + wantText: "PERMISSION_DENIED", + absentCall: "providers create-oidc", + }, + { + // describe returns a soft-deleted provider with its configuration + // intact, so every comparison matches and the run would otherwise + // report success against a provider that cannot exchange tokens. + name: "existing provider is soft-deleted", + state: gcpStubState{ + pool: true, provider: true, issuer: gcpStubIssuer, + condition: gcpExpectedCondition, mapping: gcpExpectedMapping, state: "DELETED", + }, + wantText: "cannot exchange tokens", + }, + { + name: "existing provider is disabled", + state: gcpStubState{ + pool: true, provider: true, issuer: gcpStubIssuer, + condition: gcpExpectedCondition, mapping: gcpExpectedMapping, disabled: "True", + }, + wantText: "cannot exchange tokens", + }, + { + // The guard must not depend on gcloud rendering the boolean as + // Python's "True": a lowercase or numeric rendering still means + // disabled, and testing for the one usable value keeps it that way. + name: "existing provider is disabled, rendered lowercase", + state: gcpStubState{ + pool: true, provider: true, issuer: gcpStubIssuer, + condition: gcpExpectedCondition, mapping: gcpExpectedMapping, disabled: "true", + }, + wantText: "cannot exchange tokens", + }, + { + // The grant names the pool, not the provider, so a second provider + // in the same pool that maps a token to the same subject satisfies + // it. Verifying only this run's provider would leave that open. + name: "pool holds a provider this script did not configure", + state: gcpStubState{ + pool: true, provider: true, issuer: gcpStubIssuer, + condition: gcpExpectedCondition, mapping: gcpExpectedMapping, + otherProviders: []string{"someone-elses-oidc"}, + }, + wantText: "did not configure", + }, + { + // The fail-closed case for the enumeration itself: a list this run + // could not read says nothing about what the pool holds, so it must + // stop rather than proceed on an empty result. This is what the + // fetch-into-a-variable form buys over piping into a filter. + name: "listing the pool's providers fails", + state: gcpStubState{pool: true}, + env: map[string]string{"STUB_PROVIDER_LIST_ERROR": "PERMISSION_DENIED: caller lacks iam.workloadIdentityPoolProviders.list"}, + wantText: "PERMISSION_DENIED", + absentCall: "providers create-oidc", + }, + { + // The shape a customer onboarded before the provider default + // changed from cudly- to cudly-oidc lands in: the pool + // holds only the old provider. The refusal has to come before the + // create, or every attempt leaves another provider behind. + name: "pool holds only a legacy provider, ours not yet created", + state: gcpStubState{ + pool: true, + otherProviders: []string{"cudly-aws"}, + }, + wantText: "did not configure", + absentCall: "providers create-oidc", + }, + } + + for _, tc := range reject { + t.Run("reject/"+tc.name, func(t *testing.T) { + stateDir := t.TempDir() + seedGCPStubState(t, stateDir, tc.state) + + exitCode, stdout, stderr, calls := runGCPWIFScript(t, stateDir, tc.env) + + if exitCode == 0 { + t.Errorf("the run must abort, but it exited 0; stdout:\n%s", stdout) + } + if grants := callsContaining(calls, grantCall); len(grants) != 0 { + t.Errorf("the impersonation grant must not be made, but %s was invoked: %v", grantCall, grants) + } + if strings.Contains(stdout, "=== Done ===") { + t.Errorf("an aborted run must not report success; stdout:\n%s", stdout) + } + if !strings.Contains(stderr, tc.wantText) { + t.Errorf("stderr must explain the abort with %q, got:\n%s", tc.wantText, stderr) + } + if tc.absentCall != "" { + if got := callsContaining(calls, tc.absentCall); len(got) != 0 { + t.Errorf("the run must abort before %q, but it was invoked: %v", tc.absentCall, got) + } + } + }) + } + + // Positive control: an empty project must still be configured end to end. + // Without it, a fix that refused every run would satisfy every case above. + t.Run("accept/empty project", func(t *testing.T) { + stateDir := t.TempDir() + exitCode, stdout, stderr, calls := runGCPWIFScript(t, stateDir, nil) + + if exitCode != 0 { + t.Fatalf("a fresh project must be configured successfully, got exit %d; stderr:\n%s", exitCode, stderr) + } + if len(calls) == 0 { + t.Fatal("the gcloud stub was never invoked, so this case proves nothing") + } + created := callsContaining(calls, "providers create-oidc") + if len(created) != 1 { + t.Fatalf("expected exactly one provider create, got %d: %v", len(created), calls) + } + if !strings.Contains(created[0], "--attribute-condition="+gcpExpectedCondition) { + t.Errorf("the provider must be created with the pinned condition %q, got:\n%s", gcpExpectedCondition, created[0]) + } + if grants := callsContaining(calls, grantCall); len(grants) != 1 { + t.Errorf("expected exactly one impersonation grant, got %d: %v", len(grants), calls) + } + if !strings.Contains(stdout, "=== Done ===") { + t.Errorf("a successful run must report the values CUDly needs; stdout:\n%s", stdout) + } + }) + + // An audience list is a restriction only when it omits ours. Both of these + // are legitimate reuse: unset (GCP defaults to the provider's own resource + // name, which is what CUDly presents) and a list that includes it among + // others. Without them the audience check could reject every provider and + // the case above would still pass. + for _, tc := range []struct { + name string + audiences string + }{ + {"unset audience list", ""}, + {"audience list containing ours", "https://other.example/aud;" + gcpStubAudience}, + } { + t.Run("accept/"+tc.name, func(t *testing.T) { + stateDir := t.TempDir() + seedGCPStubState(t, stateDir, gcpStubState{ + pool: true, provider: true, issuer: gcpStubIssuer, + condition: gcpExpectedCondition, mapping: gcpExpectedMapping, + audiences: tc.audiences, + }) + + exitCode, stdout, stderr, calls := runGCPWIFScript(t, stateDir, nil) + if exitCode != 0 { + t.Fatalf("reuse must be allowed, got exit %d; stderr:\n%s", exitCode, stderr) + } + if grants := callsContaining(calls, grantCall); len(grants) != 1 { + t.Errorf("expected exactly one impersonation grant, got %d: %v", len(grants), calls) + } + if !strings.Contains(stdout, "=== Done ===") { + t.Errorf("the run must complete; stdout:\n%s", stdout) + } + }) + } + + // The resumable case: a customer re-running the script over the state the + // first run left behind. The second run must reuse the provider it created + // rather than refuse it, so onboarding is not broken by the fix. + t.Run("accept/re-run over the previous run's state", func(t *testing.T) { + stateDir := t.TempDir() + if exitCode, _, stderr, _ := runGCPWIFScript(t, stateDir, nil); exitCode != 0 { + t.Fatalf("first run failed with exit %d; stderr:\n%s", exitCode, stderr) + } + + exitCode, stdout, stderr, calls := runGCPWIFScript(t, stateDir, nil) + if exitCode != 0 { + t.Fatalf("re-running over an unchanged project must succeed, got exit %d; stderr:\n%s", exitCode, stderr) + } + if created := callsContaining(calls, "providers create-oidc"); len(created) != 0 { + t.Errorf("the second run must not re-create the provider, got: %v", created) + } + if !strings.Contains(stdout, "Reusing existing provider") { + t.Errorf("the second run must report the reuse; stdout:\n%s", stdout) + } + if grants := callsContaining(calls, grantCall); len(grants) != 1 { + t.Errorf("the re-run must still (idempotently) apply the grant, got %d: %v", len(grants), calls) + } + if !strings.Contains(stdout, "=== Done ===") { + t.Errorf("the re-run must complete; stdout:\n%s", stdout) + } + }) +} + +// TestGCPWIFCLI_SubjectValidated covers #1661 item 2: CUDLY_FEDERATED_SUBJECT is +// environment-overridable and lands inside the CEL string literal of +// --attribute-condition, so a value carrying a quote closes the literal and the +// remainder rewrites the condition: the value "x' || true || '" renders a +// condition whose second disjunct is a bare "true", admitting every subject the +// issuer will sign. It also lands in the principal:// identifier of the grant, +// where a '*' widens the member. +func TestGCPWIFCLI_SubjectValidated(t *testing.T) { + reject := []struct { + name string + subject string + }{ + {"CEL break-out", `x' || true || '`}, + {"double quote", `x" || true || "`}, + {"backslash", `x\'`}, + {"backtick", "x`id`"}, + {"wildcard", "*"}, + {"embedded wildcard", "cudly-*"}, + {"unexpanded placeholder", "${CUDLY_SUBJECT}"}, + {"embedded space", "cudly controller"}, + {"leading punctuation", "-cudly-controller"}, + {"over the 127-character google.subject limit", strings.Repeat("a", 128)}, + } + + for _, tc := range reject { + t.Run("reject/"+tc.name, func(t *testing.T) { + stateDir := t.TempDir() + exitCode, _, stderr, calls := runGCPWIFScript(t, stateDir, + map[string]string{"CUDLY_FEDERATED_SUBJECT": tc.subject}) + + if exitCode == 0 { + t.Errorf("CUDLY_FEDERATED_SUBJECT %q was accepted (exit 0)", tc.subject) + } + // The guard must run before the first gcloud call, so a rejected run + // leaves no pool, provider or grant behind. + if len(calls) != 0 { + t.Errorf("no gcloud call may be made before the subject is validated, got %d: %v", len(calls), calls) + } + if !strings.Contains(stderr, "CUDLY_FEDERATED_SUBJECT") { + t.Errorf("stderr must name the rejected variable, got:\n%s", stderr) + } + }) + } + + // Positive controls: the default, and a subject in the Auth0/Okta shape the + // charset deliberately admits. Without these, a guard rejecting everything + // would pass every case above. + accept := []struct { + name string + subject string + // wantPinned is the subject the provider must end up pinned to, which + // differs from subject only where the script substitutes its default. + wantPinned string + }{ + // The only subject CUDly can actually present: gcpFederatedSubject in + // internal/credentials/gcp_federated.go is a compile-time constant. + {"default subject", "cudly-controller", "cudly-controller"}, + // The cases below assert the charset admits these values and that they + // reach gcloud unmangled. They do NOT assert a usable configuration: + // pinning the provider to any subject other than the default binds it + // to one CUDly will never sign. + {"pipe-bearing subject reaches gcloud unmangled", "google-oauth2|1234567890", "google-oauth2|1234567890"}, + {"exactly 127 characters", strings.Repeat("a", 127), strings.Repeat("a", 127)}, + // An empty override is not a hole: ${VAR:-default} substitutes the + // default for an empty value, so the provider is still pinned to + // cudly-controller rather than to the empty subject. + {"empty falls back to the default", "", "cudly-controller"}, + } + for _, tc := range accept { + t.Run("accept/"+tc.name, func(t *testing.T) { + stateDir := t.TempDir() + exitCode, stdout, stderr, calls := runGCPWIFScript(t, stateDir, + map[string]string{"CUDLY_FEDERATED_SUBJECT": tc.subject}) + + if exitCode != 0 { + t.Fatalf("CUDLY_FEDERATED_SUBJECT %q was rejected (exit %d); stderr:\n%s", tc.subject, exitCode, stderr) + } + created := callsContaining(calls, "providers create-oidc") + if len(created) != 1 { + t.Fatalf("expected exactly one provider create, got %d: %v", len(created), calls) + } + if want := "--attribute-condition=assertion.sub == '" + tc.wantPinned + "'"; !strings.Contains(created[0], want) { + t.Errorf("the provider must be pinned to the supplied subject with %q, got:\n%s", want, created[0]) + } + // The run completes rather than aborting on the charset guard. + // Whether the resulting provider is usable is a separate question, + // and for every subject but the default the answer is no. + if !strings.Contains(stdout, "=== Done ===") { + t.Errorf("an accepted subject must not abort the run; stdout:\n%s", stdout) + } + }) + } +} diff --git a/internal/iacfiles/templates_test.go b/internal/iacfiles/templates_test.go index a8bc30392..28d2e2809 100644 --- a/internal/iacfiles/templates_test.go +++ b/internal/iacfiles/templates_test.go @@ -2,7 +2,6 @@ package iacfiles import ( "bytes" - "cmp" "context" "errors" "os" @@ -488,485 +487,6 @@ func TestAWSWIFCLI_SubjectClaimGuardBlocksAWSCalls(t *testing.T) { }) } -// gcloudStubScript returns a stand-in `gcloud` that records every invocation and -// answers describes from state files under $CUDLY_STUB_STATE. The state -// directory is the caller's, so a test can seed a pre-existing pool/provider -// before the run and can hand the same directory to two consecutive runs to -// exercise the re-run path a customer actually takes. -// -// Only the describe/create verbs the GCP WIF script branches on are modeled; -// everything else (config set, services enable, add-iam-policy-binding) -// succeeds, so a run that reaches them has proved the guards let it through. -func gcloudStubScript(logPath string) string { - return `#!/usr/bin/env bash -state="${CUDLY_STUB_STATE:?stub state dir}" -printf '%s\n' "gcloud $*" >> '` + logPath + `' - -# Flag values are read off the argument vector rather than parsed out of the -# joined string, so a value containing spaces or quotes is recorded verbatim. -issuer=""; condition=""; mapping=""; provider_id=""; prev="" -for a in "$@"; do - case "$a" in - --issuer-uri=*) issuer="${a#--issuer-uri=}" ;; - --attribute-condition=*) condition="${a#--attribute-condition=}" ;; - --attribute-mapping=*) mapping="${a#--attribute-mapping=}" ;; - esac - [[ "$prev" == "create-oidc" ]] && provider_id="$a" - prev="$a" -done - -pool_path="projects/000000000000/locations/global/workloadIdentityPools/cudly-target" - -case "$*" in - *"workload-identity-pools providers describe"*) - [[ -f "${state}/provider" ]] || exit 1 - case "$*" in - *"value(oidc.issuerUri)"*) cat "${state}/provider.issuer" ;; - *"value(attributeCondition)"*) cat "${state}/provider.condition" ;; - *"value(attributeMapping)"*) cat "${state}/provider.mapping" ;; - *"value(state)"*) cat "${state}/provider.state" ;; - *"value(disabled)"*) cat "${state}/provider.disabled" ;; - esac - ;; - *"workload-identity-pools providers list"*) - if [[ -n "${STUB_PROVIDER_LIST_ERROR:-}" ]]; then - echo "ERROR: (gcloud.iam.workload-identity-pools.providers.list) ${STUB_PROVIDER_LIST_ERROR}" >&2 - exit 1 - fi - # Soft-deleted providers are omitted here as they are by real gcloud - # without --show-deleted; the seeded list holds only live ones. - [[ -f "${state}/providers.list" ]] && cat "${state}/providers.list" - ;; - *"workload-identity-pools providers create-oidc"*) - if [[ -n "${STUB_PROVIDER_CREATE_ERROR:-}" ]]; then - echo "ERROR: (gcloud.iam.workload-identity-pools.providers.create-oidc) ${STUB_PROVIDER_CREATE_ERROR}" >&2 - exit 1 - fi - printf '%s\n' "$issuer" > "${state}/provider.issuer" - printf '%s\n' "$condition" > "${state}/provider.condition" - printf '%s\n' "$mapping" > "${state}/provider.mapping" - printf 'ACTIVE\n' > "${state}/provider.state" - printf 'False\n' > "${state}/provider.disabled" - printf '%s\n' "${pool_path}/providers/${provider_id}" >> "${state}/providers.list" - touch "${state}/provider" - ;; - *"workload-identity-pools describe"*) - [[ -f "${state}/pool" ]] || exit 1 - echo "projects/000000000000/locations/global/workloadIdentityPools/cudly-target" - ;; - *"workload-identity-pools create"*) - if [[ -n "${STUB_POOL_CREATE_ERROR:-}" ]]; then - echo "ERROR: (gcloud.iam.workload-identity-pools.create) ${STUB_POOL_CREATE_ERROR}" >&2 - exit 1 - fi - touch "${state}/pool" - ;; -esac -exit 0 -` -} - -const ( - // gcpStubIssuer is the issuer the rendered script is run with, i.e. the - // value an existing provider must carry to be reusable. - gcpStubIssuer = "https://cudly.example.com/oidc" - // gcpExpectedCondition/gcpExpectedMapping are what the script writes for the - // default subject, and therefore what it must demand of a provider it reuses. - gcpExpectedCondition = "assertion.sub == 'cudly-controller'" - gcpExpectedMapping = "google.subject=assertion.sub" -) - -// gcpStubState is the pre-existing GCP state a run starts from. The zero value -// is an empty project. -type gcpStubState struct { - pool bool - provider bool - // Set only when provider is true. - issuer string - condition string - mapping string - // state defaults to ACTIVE and disabled to False when provider is true. - state string - disabled string - // otherProviders are further live providers in the same pool, given as - // bare IDs. - otherProviders []string -} - -// seedGCPStubState materializes state in dir for the stub to read. -func seedGCPStubState(t *testing.T, dir string, s gcpStubState) { - t.Helper() - write := func(name, content string) { - if err := os.WriteFile(filepath.Join(dir, name), []byte(content+"\n"), 0o600); err != nil { - t.Fatalf("seed %s: %v", name, err) - } - } - if s.pool { - write("pool", "") - } - const poolPath = "projects/000000000000/locations/global/workloadIdentityPools/cudly-target" - var live []string - if s.provider { - write("provider", "") - write("provider.issuer", s.issuer) - write("provider.condition", s.condition) - write("provider.mapping", s.mapping) - write("provider.state", cmp.Or(s.state, "ACTIVE")) - write("provider.disabled", cmp.Or(s.disabled, "False")) - live = append(live, poolPath+"/providers/cudly-oidc") - } - for _, id := range s.otherProviders { - live = append(live, poolPath+"/providers/"+id) - } - if len(live) > 0 { - write("providers.list", strings.Join(live, "\n")) - } -} - -// runGCPWIFScript renders gcp-wif-cli.sh and runs it against the gcloud stub, -// with stateDir carrying the project state across runs. -func runGCPWIFScript(t *testing.T, stateDir string, env map[string]string) (exitCode int, stdout, stderr string, calls []string) { - t.Helper() - // CUDlyAPIURL is cleared so the auto-registration block (curl + jq) is not - // rendered: these tests are about the pool/provider/grant sequence. - data := baseData() - data.CUDlyAPIURL = "" - data.ProjectID = "cudly-demo-project" - data.ServiceAccountEmail = "cudly@cudly-demo-project.iam.gserviceaccount.com" - rendered := renderCLITemplate(t, "templates/gcp-wif-cli.sh.tmpl", data) - - full := map[string]string{ - "CUDLY_STUB_STATE": stateDir, - // Rendered from an empty CUDlyAPIURL above, so it must be supplied here - // for the issuer guard to pass. - "CUDLY_ISSUER_URL": gcpStubIssuer, - } - for k, v := range env { - full[k] = v - } - return runRenderedScript(t, "gcp-wif-cli.sh", rendered, func(logPath string) map[string]string { - return map[string]string{"gcloud": gcloudStubScript(logPath)} - }, full) -} - -// callsContaining returns the recorded gcloud invocations containing needle. -func callsContaining(calls []string, needle string) []string { - var out []string - for _, c := range calls { - if strings.Contains(c, needle) { - out = append(out, c) - } - } - return out -} - -// TestGCPWIFCLI_ProviderReuseIsVerified is the regression test for #1661. The -// served script used to run both creates with `|| echo "(may already exist)"`, -// so under `set -euo pipefail` every failure of a security-critical create was -// swallowed: a provider that already existed with a weaker attribute condition -// (or none) was silently reused, the impersonation grant was made against it, -// and the script printed "=== Done ===" and exited 0. -// -// Each reject case below asserts the run aborts *before* the grant, and the -// accept cases assert a fresh project and a legitimate re-run both still -// succeed, so the fix cannot be a blanket refusal. -func TestGCPWIFCLI_ProviderReuseIsVerified(t *testing.T) { - const grantCall = "add-iam-policy-binding" - - reject := []struct { - name string - state gcpStubState - env map[string]string - // wantText must appear in stderr. - wantText string - // absentCall, when set, must not appear among the recorded gcloud - // invocations. It carries the cases where aborting at the right point - // matters as much as aborting at all. - absentCall string - }{ - { - // The actual vulnerability: "true" is non-empty and admits every - // subject the issuer will sign. - name: "existing provider with an always-true condition", - state: gcpStubState{pool: true, provider: true, issuer: gcpStubIssuer, condition: "true", mapping: gcpExpectedMapping}, - wantText: "different attribute condition", - }, - { - name: "existing provider with no condition at all", - state: gcpStubState{pool: true, provider: true, issuer: gcpStubIssuer, condition: "", mapping: gcpExpectedMapping}, - wantText: "different attribute condition", - }, - { - name: "existing provider pinned to a different subject", - state: gcpStubState{pool: true, provider: true, issuer: gcpStubIssuer, condition: "assertion.sub == 'someone-else'", mapping: gcpExpectedMapping}, - wantText: "different attribute condition", - }, - { - // A matching condition on a provider trusting someone else's issuer - // admits whoever that issuer signs a cudly-controller token for. - name: "existing provider bound to a foreign issuer", - state: gcpStubState{pool: true, provider: true, issuer: "https://evil.example.com/oidc", condition: gcpExpectedCondition, mapping: gcpExpectedMapping}, - wantText: "different issuer URI", - }, - { - name: "existing provider with a different attribute mapping", - state: gcpStubState{pool: true, provider: true, issuer: gcpStubIssuer, condition: gcpExpectedCondition, mapping: "google.subject=assertion.email"}, - wantText: "different attribute mapping", - }, - { - // Not an "already exists" case: any other create failure (quota, - // permission, a concurrent run winning the race) must stop the run - // rather than fall through to the grant. - name: "provider create fails", - state: gcpStubState{pool: true}, - env: map[string]string{"STUB_PROVIDER_CREATE_ERROR": "PERMISSION_DENIED: caller lacks iam.workloadIdentityPoolProviders.create"}, - wantText: "PERMISSION_DENIED", - }, - { - // The pre-fix script swallowed this one too and went on to create - // the provider; the assertion that no provider create is attempted - // is what makes this case a regression witness rather than a - // restatement of the propagation loop's eventual abort. - name: "pool create fails", - env: map[string]string{"STUB_POOL_CREATE_ERROR": "PERMISSION_DENIED: caller lacks iam.workloadIdentityPools.create"}, - wantText: "PERMISSION_DENIED", - absentCall: "providers create-oidc", - }, - { - // describe returns a soft-deleted provider with its configuration - // intact, so every comparison matches and the run would otherwise - // report success against a provider that cannot exchange tokens. - name: "existing provider is soft-deleted", - state: gcpStubState{ - pool: true, provider: true, issuer: gcpStubIssuer, - condition: gcpExpectedCondition, mapping: gcpExpectedMapping, state: "DELETED", - }, - wantText: "cannot exchange tokens", - }, - { - name: "existing provider is disabled", - state: gcpStubState{ - pool: true, provider: true, issuer: gcpStubIssuer, - condition: gcpExpectedCondition, mapping: gcpExpectedMapping, disabled: "True", - }, - wantText: "cannot exchange tokens", - }, - { - // The guard must not depend on gcloud rendering the boolean as - // Python's "True": a lowercase or numeric rendering still means - // disabled, and testing for the one usable value keeps it that way. - name: "existing provider is disabled, rendered lowercase", - state: gcpStubState{ - pool: true, provider: true, issuer: gcpStubIssuer, - condition: gcpExpectedCondition, mapping: gcpExpectedMapping, disabled: "true", - }, - wantText: "cannot exchange tokens", - }, - { - // The grant names the pool, not the provider, so a second provider - // in the same pool that maps a token to the same subject satisfies - // it. Verifying only this run's provider would leave that open. - name: "pool holds a provider this script did not configure", - state: gcpStubState{ - pool: true, provider: true, issuer: gcpStubIssuer, - condition: gcpExpectedCondition, mapping: gcpExpectedMapping, - otherProviders: []string{"someone-elses-oidc"}, - }, - wantText: "did not configure", - }, - { - // The fail-closed case for the enumeration itself: a list this run - // could not read says nothing about what the pool holds, so it must - // stop rather than proceed on an empty result. This is what the - // fetch-into-a-variable form buys over piping into a filter. - name: "listing the pool's providers fails", - state: gcpStubState{pool: true}, - env: map[string]string{"STUB_PROVIDER_LIST_ERROR": "PERMISSION_DENIED: caller lacks iam.workloadIdentityPoolProviders.list"}, - wantText: "PERMISSION_DENIED", - absentCall: "providers create-oidc", - }, - { - // The shape a customer onboarded before the provider default - // changed from cudly- to cudly-oidc lands in: the pool - // holds only the old provider. The refusal has to come before the - // create, or every attempt leaves another provider behind. - name: "pool holds only a legacy provider, ours not yet created", - state: gcpStubState{ - pool: true, - otherProviders: []string{"cudly-aws"}, - }, - wantText: "did not configure", - absentCall: "providers create-oidc", - }, - } - - for _, tc := range reject { - t.Run("reject/"+tc.name, func(t *testing.T) { - stateDir := t.TempDir() - seedGCPStubState(t, stateDir, tc.state) - - exitCode, stdout, stderr, calls := runGCPWIFScript(t, stateDir, tc.env) - - if exitCode == 0 { - t.Errorf("the run must abort, but it exited 0; stdout:\n%s", stdout) - } - if grants := callsContaining(calls, grantCall); len(grants) != 0 { - t.Errorf("the impersonation grant must not be made, but %s was invoked: %v", grantCall, grants) - } - if strings.Contains(stdout, "=== Done ===") { - t.Errorf("an aborted run must not report success; stdout:\n%s", stdout) - } - if !strings.Contains(stderr, tc.wantText) { - t.Errorf("stderr must explain the abort with %q, got:\n%s", tc.wantText, stderr) - } - if tc.absentCall != "" { - if got := callsContaining(calls, tc.absentCall); len(got) != 0 { - t.Errorf("the run must abort before %q, but it was invoked: %v", tc.absentCall, got) - } - } - }) - } - - // Positive control: an empty project must still be configured end to end. - // Without it, a fix that refused every run would satisfy every case above. - t.Run("accept/empty project", func(t *testing.T) { - stateDir := t.TempDir() - exitCode, stdout, stderr, calls := runGCPWIFScript(t, stateDir, nil) - - if exitCode != 0 { - t.Fatalf("a fresh project must be configured successfully, got exit %d; stderr:\n%s", exitCode, stderr) - } - if len(calls) == 0 { - t.Fatal("the gcloud stub was never invoked, so this case proves nothing") - } - created := callsContaining(calls, "providers create-oidc") - if len(created) != 1 { - t.Fatalf("expected exactly one provider create, got %d: %v", len(created), calls) - } - if !strings.Contains(created[0], "--attribute-condition="+gcpExpectedCondition) { - t.Errorf("the provider must be created with the pinned condition %q, got:\n%s", gcpExpectedCondition, created[0]) - } - if grants := callsContaining(calls, grantCall); len(grants) != 1 { - t.Errorf("expected exactly one impersonation grant, got %d: %v", len(grants), calls) - } - if !strings.Contains(stdout, "=== Done ===") { - t.Errorf("a successful run must report the values CUDly needs; stdout:\n%s", stdout) - } - }) - - // The resumable case: a customer re-running the script over the state the - // first run left behind. The second run must reuse the provider it created - // rather than refuse it, so onboarding is not broken by the fix. - t.Run("accept/re-run over the previous run's state", func(t *testing.T) { - stateDir := t.TempDir() - if exitCode, _, stderr, _ := runGCPWIFScript(t, stateDir, nil); exitCode != 0 { - t.Fatalf("first run failed with exit %d; stderr:\n%s", exitCode, stderr) - } - - exitCode, stdout, stderr, calls := runGCPWIFScript(t, stateDir, nil) - if exitCode != 0 { - t.Fatalf("re-running over an unchanged project must succeed, got exit %d; stderr:\n%s", exitCode, stderr) - } - if created := callsContaining(calls, "providers create-oidc"); len(created) != 0 { - t.Errorf("the second run must not re-create the provider, got: %v", created) - } - if !strings.Contains(stdout, "Reusing existing provider") { - t.Errorf("the second run must report the reuse; stdout:\n%s", stdout) - } - if grants := callsContaining(calls, grantCall); len(grants) != 1 { - t.Errorf("the re-run must still (idempotently) apply the grant, got %d: %v", len(grants), calls) - } - if !strings.Contains(stdout, "=== Done ===") { - t.Errorf("the re-run must complete; stdout:\n%s", stdout) - } - }) -} - -// TestGCPWIFCLI_SubjectValidated covers #1661 item 2: CUDLY_FEDERATED_SUBJECT is -// environment-overridable and lands inside the CEL string literal of -// --attribute-condition, so a value carrying a quote closes the literal and the -// remainder rewrites the condition: the value "x' || true || '" renders a -// condition whose second disjunct is a bare "true", admitting every subject the -// issuer will sign. It also lands in the principal:// identifier of the grant, -// where a '*' widens the member. -func TestGCPWIFCLI_SubjectValidated(t *testing.T) { - reject := []struct { - name string - subject string - }{ - {"CEL break-out", `x' || true || '`}, - {"double quote", `x" || true || "`}, - {"backslash", `x\'`}, - {"backtick", "x`id`"}, - {"wildcard", "*"}, - {"embedded wildcard", "cudly-*"}, - {"unexpanded placeholder", "${CUDLY_SUBJECT}"}, - {"embedded space", "cudly controller"}, - {"leading punctuation", "-cudly-controller"}, - {"over the 127-character google.subject limit", strings.Repeat("a", 128)}, - } - - for _, tc := range reject { - t.Run("reject/"+tc.name, func(t *testing.T) { - stateDir := t.TempDir() - exitCode, _, stderr, calls := runGCPWIFScript(t, stateDir, - map[string]string{"CUDLY_FEDERATED_SUBJECT": tc.subject}) - - if exitCode == 0 { - t.Errorf("CUDLY_FEDERATED_SUBJECT %q was accepted (exit 0)", tc.subject) - } - // The guard must run before the first gcloud call, so a rejected run - // leaves no pool, provider or grant behind. - if len(calls) != 0 { - t.Errorf("no gcloud call may be made before the subject is validated, got %d: %v", len(calls), calls) - } - if !strings.Contains(stderr, "CUDLY_FEDERATED_SUBJECT") { - t.Errorf("stderr must name the rejected variable, got:\n%s", stderr) - } - }) - } - - // Positive controls: the default, and a subject in the Auth0/Okta shape the - // charset deliberately admits. Without these, a guard rejecting everything - // would pass every case above. - accept := []struct { - name string - subject string - // wantPinned is the subject the provider must end up pinned to, which - // differs from subject only where the script substitutes its default. - wantPinned string - }{ - {"default subject", "cudly-controller", "cudly-controller"}, - {"identity-provider-prefixed subject", "google-oauth2|1234567890", "google-oauth2|1234567890"}, - {"exactly 127 characters", strings.Repeat("a", 127), strings.Repeat("a", 127)}, - // An empty override is not a hole: ${VAR:-default} substitutes the - // default for an empty value, so the provider is still pinned to - // cudly-controller rather than to the empty subject. - {"empty falls back to the default", "", "cudly-controller"}, - } - for _, tc := range accept { - t.Run("accept/"+tc.name, func(t *testing.T) { - stateDir := t.TempDir() - exitCode, stdout, stderr, calls := runGCPWIFScript(t, stateDir, - map[string]string{"CUDLY_FEDERATED_SUBJECT": tc.subject}) - - if exitCode != 0 { - t.Fatalf("CUDLY_FEDERATED_SUBJECT %q was rejected (exit %d); stderr:\n%s", tc.subject, exitCode, stderr) - } - created := callsContaining(calls, "providers create-oidc") - if len(created) != 1 { - t.Fatalf("expected exactly one provider create, got %d: %v", len(created), calls) - } - if want := "--attribute-condition=assertion.sub == '" + tc.wantPinned + "'"; !strings.Contains(created[0], want) { - t.Errorf("the provider must be pinned to the supplied subject with %q, got:\n%s", want, created[0]) - } - if !strings.Contains(stdout, "=== Done ===") { - t.Errorf("a valid subject must configure the project end to end; stdout:\n%s", stdout) - } - }) - } -} - // TestCLITemplatesShellMetacharsPassThrough confirms that text/template does // not shell-escape interpolated values. This is intentional: the templates are // designed to be downloaded and inspected by an operator, not auto-executed.