Skip to content

sec(iac): validate CUDLY_ISSUER_URL in the served GCP script (the ARM script already does), plus normalize_mapping's bash 3.2 empty-array abort #211

Description

@cristim

Two items surfaced during review of LeanerCloud/cloud-commitments-cli#1866 (the fix for LeanerCloud/cloud-commitments-cli#1661). Both are pre-existing and were deliberately left out of that PR to keep its diff on the reported defects. Recorded here so they do not live only in a review thread.

1. CUDLY_ISSUER_URL is unvalidated in the served GCP script, while the ARM script validates the same destination

internal/iacfiles/templates/gcp-wif-cli.sh.tmpl:30:

CUDLY_ISSUER_URL="${CUDLY_ISSUER_URL:-{{.CUDlyAPIURL}}/oidc}"

It is environment-overridable and flows straight into --issuer-uri on the provider create. arm/CUDly-CrossSubscription/setup-gcp-wif.sh validates the same value:

validate_principal_value "--issuer-uri" "$ISSUER_URI" '^https://[a-zA-Z0-9.-]+(:[0-9]+)?(/[-a-zA-Z0-9._~/]*)?$'

The served path has no equivalent. Whoever controls the issuer that ends up configured can mint a token with any subject, and GCP will fetch their /.well-known/openid-configuration and trust it. A typo or a pasted wrong URL is enough; there is no attacker-controlled input path, so this is a misconfiguration vector rather than a live bypass.

The asymmetry is the reason to close this deliberately. Two implementations of one control that disagree is the shape behind LeanerCloud/cloud-commitments-cli#1592 → LeanerCloud/cloud-commitments-cli#1820 → LeanerCloud/cloud-commitments-cli#1821. LeanerCloud/cloud-commitments-cli#1866 hardened the subject on this path (CUDLY_FEDERATED_SUBJECT) and left the issuer as the one value on the same line of defence still unguarded.

Suggested fix: apply the ARM script's regex to CUDLY_ISSUER_URL in the template, before the first gcloud call, alongside the subject guard added in LeanerCloud/cloud-commitments-cli#1866. Reject plain http:// explicitly. internal/iacfiles/templates_test.go has the harness for it (renders the script and runs it under bash against a recording gcloud stub); a reject case must assert zero gcloud calls, and an accept case must keep a normal run working.

2. normalize_mapping aborts with unbound variable on bash 3.2 when the mapping is empty

arm/CUDly-CrossSubscription/setup-gcp-wif.sh:

normalize_mapping() {
  local mapping="$1" pair_sep="$2"
  local -a pairs
  IFS="$pair_sep" read -ra pairs <<<"$mapping"
  printf '%s\n' "${pairs[@]}" | sort
}

"${pairs[@]}" on an empty array is an error under set -u in bash < 4.4, which is macOS's /bin/bash (3.2.57) and therefore the shell many operators will run this with. Reproduced: normalize_mapping "" ";" emits pairs[@]: unbound variable.

It is reached when an existing provider has an empty attribute mapping. The path still fails closed — the substitution runs inside [[ ]], so the empty result mismatches and die fires — but the operator sees a bash internal error instead of the "different attribute mapping" diagnostic that was written for exactly this case, and a reader cannot tell the closed outcome is accidental.

Suggested fix: printf '%s\n' ${pairs[@]+"${pairs[@]}"} | sort, or return early on [[ ${#pairs[@]} -eq 0 ]].

Triage note

Filed as one issue because both are small, both sit in the same pair of onboarding artifacts, and both were found in the same review. Split if they end up with different owners. Labels take the higher severity across the two and the union of types, per the multi-concern rule: item 1 drives severity/medium and type/security, item 2 is type/bug and low on its own.

Impact is impact/few rather than impact/all-users deliberately: the missing guard is on a path every GCP onboarding uses, but harm only materialises for an operator who supplies a wrong issuer, which is a small population. If that reading is wrong, the priority should go up with it.


3. CUDLY_FEDERATED_SUBJECT is overridable but unsatisfiable

Added after review of LeanerCloud/cloud-commitments-cli#1866 found it from the Go side rather than the script side.

internal/credentials/gcp_federated.go:20:

// gcpFederatedSubject is the fixed JWT subject CUDly uses when the
// target GCP project has a Workload Identity Pool provider bound to
// CUDly's own OIDC issuer. Changing this string is an incompatible
// change that requires every existing WIF provider's
// attribute_condition to be recreated.
const gcpFederatedSubject = "cudly-controller"

It is a compile-time constant, used directly as the sub claim at line 37, with no per-account override anywhere in the codebase. But internal/iacfiles/templates/gcp-wif-cli.sh.tmpl:31 honours an environment override:

CUDLY_FEDERATED_SUBJECT="${CUDLY_FEDERATED_SUBJECT:-cudly-controller}"

and feeds it into both the provider's attribute condition and the impersonation grant. An operator who sets it gets a provider and a grant pinned to a subject CUDly will never present. The script prints === Done ===, and with CUDlyAPIURL set it auto-registers the account. So the override is not merely unvalidated, it is unsatisfiable: no value other than the default can ever work.

The same shape exists on the Azure path: azure-wif-cli.sh.tmpl:26 and azure-wif-deploy.sh.tmpl:61 against the equivalent constant in internal/credentials/azure_federated.go:22. Whatever is decided here should be applied there in the same change.

Options, roughly in increasing order of cost: warn when the value differs from the default; reject the override outright; or make the subject genuinely per-account, which is the change the constant's own doc comment warns is incompatible with every existing provider.

LeanerCloud/cloud-commitments-cli#1866 deliberately did not touch the knob: it predates that range, appears only as diff context, and the Azure duplication puts it beyond that PR's scope. What LeanerCloud/cloud-commitments-cli#1866 did do is stop its own test suite from implying the override works, since a test asserting that google-oauth2|1234567890 configures a project end to end read as an endorsement of a configuration that cannot authenticate.

Why these three are one issue

All three items here are the same shape: a script promise the rest of the system does not keep. The served script accepts an issuer nobody validates, reports a mapping comparison it cannot always render, and offers a subject knob the backend cannot honour. Each is individually small; together they are a pattern worth fixing in one pass over the onboarding templates.

Findings from the 2026-09-02 codebase audit

Added by an automated audit of 3c0f8ac94048a2c36fce5ccddee54e6c4849a5cd (tip of origin/main). Each item below was reported by one reviewer and independently confirmed by a second that did not write it. Full report: docs/audits/codebase-audit-2026-09-02.md.

A14-033 (medium)

Same bash 3.2 class as the normalize_mapping half of this issue, in a second script. scripts/gcp-import-dev-state.sh:151 uses declare -A SECRET_TF_ADDR with no BASH_VERSINFO guard anywhere in the file; measured, /bin/bash 3.2.57 rejects declare -A with exit 2 and set -euo pipefail (line 17) turns that into an abort. The damage is that line 70 has already run terraform init -backend-config=... -reconfigure against the live GCS state, so a Mac operator is left with a reconfigured local backend and zero imports. scripts/lib/code-scan-awk.sh:74 and check-azure-role-parity.sh:118 both avoid bash-4 constructs for this reason. Finding A14-033.

No activity

Activity on this issue will appear here.

Activity

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

Metadata

Metadata

Assignees

No one assigned

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions