From 944b87eff09438fc146bc27d201545db3d218428 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 30 Sep 2026 03:55:19 +0200 Subject: [PATCH] fix(iac): enforce AWS CLI federation guard parity Reject invalid federation inputs before AWS calls and require an exact, readable OIDC provider with the requested audience before creating roles. Use AWS thumbprint retrieval and preserve rendered audience defaults. Exercise rendered Bash with local CLI stubs, including lookup failures, lookalike providers, invalid thumbprints, and literal JSON claims. Refs #91 --- .../iacfiles/templates/aws-wif-cli.sh.tmpl | 74 +++--- internal/iacfiles/templates_aws_wif_test.go | 210 ++++++++++++++++++ internal/iacfiles/templates_test.go | 13 +- 3 files changed, 264 insertions(+), 33 deletions(-) create mode 100644 internal/iacfiles/templates_aws_wif_test.go diff --git a/internal/iacfiles/templates/aws-wif-cli.sh.tmpl b/internal/iacfiles/templates/aws-wif-cli.sh.tmpl index ded3c0f0..0929ae15 100644 --- a/internal/iacfiles/templates/aws-wif-cli.sh.tmpl +++ b/internal/iacfiles/templates/aws-wif-cli.sh.tmpl @@ -6,18 +6,13 @@ # Usage: # bash {{.AccountSlug}}-aws-wif-cli.sh # -# Requirements: aws-cli v2, credentials for target account {{.AccountExternalID}}. +# Requirements: aws-cli v2, jq, credentials for target account {{.AccountExternalID}}. set -euo pipefail ROLE_NAME="${ROLE_NAME:-CUDly-{{.AccountSlug}}}" OIDC_ISSUER_URL="${OIDC_ISSUER_URL:-{{.OIDCIssuerURL}}}" -OIDC_AUDIENCE="${OIDC_AUDIENCE:-{{.OIDCAudience}}}" -# REQUIRED: restricts which OIDC subject can assume the role. Without a :sub -# condition the trust policy accepts every identity the issuer can mint — the -# same hole #1543/#1602 closed in the CloudFormation template — so this is -# validated below rather than left to build a subject-less policy silently. -# Matches the required TF variable oidc_subject_claim and the required -# CloudFormation OIDCSubjectClaim parameter. +OIDC_AUDIENCE="${OIDC_AUDIENCE-{{.OIDCAudience}}}" +OIDC_AUDIENCE="${OIDC_AUDIENCE:-sts.amazonaws.com}" OIDC_SUBJECT_CLAIM="${OIDC_SUBJECT_CLAIM:-{{.OIDCSubjectClaim}}}" if [[ -z "${OIDC_SUBJECT_CLAIM}" ]]; then echo "Error: OIDC_SUBJECT_CLAIM is required. Without it the trust policy has no" >&2 @@ -29,7 +24,7 @@ if [[ -z "${OIDC_SUBJECT_CLAIM}" ]]; then exit 1 fi case "${OIDC_SUBJECT_CLAIM}" in - *[[:space:]]*|*'$'*|*'*'*) + *[$' \t\n\r\v\f']*|*'$'*|*'*'*) echo "Error: OIDC_SUBJECT_CLAIM must not contain whitespace, '\$' or '*'." >&2 echo " IAM expands \${...} policy variables inside Condition values, so a" >&2 echo " value such as \${accounts.google.com:sub} would expand to the token's" >&2 @@ -39,45 +34,68 @@ case "${OIDC_SUBJECT_CLAIM}" in exit 1 ;; esac +case "${OIDC_AUDIENCE}" in + *[$' \t\n\r\v\f']*|*'$'*|*'*'*) + echo "Error: OIDC_AUDIENCE must not contain whitespace, '\$' or '*'." >&2 + exit 1 ;; +esac +# Keep this issuer check equivalent to aws-target/terraform/variables.tf. +if ! [[ "$OIDC_ISSUER_URL" =~ ^https://[^/]+(/[^/].*[^/]|/[^/]+)?$ ]] || [[ "$OIDC_ISSUER_URL" == */ ]]; then + echo "Error: OIDC_ISSUER_URL must start with https:// and must not end with a trailing slash." >&2 + exit 1 +fi +command -v jq >/dev/null || { echo "Error: jq is required." >&2; exit 1; } PROFILE_ARG="" if [[ -n "${AWS_PROFILE:-}" ]]; then PROFILE_ARG="--profile ${AWS_PROFILE}"; fi -OIDC_HOST=$(echo "${OIDC_ISSUER_URL}" | sed -E 's#^https://##; s#/$##') +OIDC_HOST="${OIDC_ISSUER_URL#https://}" echo "Creating/ensuring OIDC provider for ${OIDC_ISSUER_URL}..." # shellcheck disable=SC2086 -EXISTING_PROVIDER=$(aws iam list-open-id-connect-providers $PROFILE_ARG \ - --query "OpenIDConnectProviderList[?contains(Arn, '${OIDC_HOST}')].Arn | [0]" \ - --output text 2>/dev/null || echo "None") -if [[ "${EXISTING_PROVIDER}" == "None" || -z "${EXISTING_PROVIDER}" ]]; then +PROVIDERS=$(aws iam list-open-id-connect-providers $PROFILE_ARG --output json) +PROVIDER_ARN=$(jq -er --arg host "$OIDC_HOST" ' + if (.OpenIDConnectProviderList | type) != "array" then error("Invalid provider list") + else [.OpenIDConnectProviderList[].Arn | select(split(":oidc-provider/")[1] == $host)] + | if length > 1 then error("Multiple providers match the issuer") else .[0] // "" end + end' <<< "$PROVIDERS") +if [[ -z "$PROVIDER_ARN" ]]; then # shellcheck disable=SC2086 - aws iam create-open-id-connect-provider $PROFILE_ARG \ + CREATED_PROVIDER=$(aws iam create-open-id-connect-provider $PROFILE_ARG \ --url "${OIDC_ISSUER_URL}" \ --client-id-list "${OIDC_AUDIENCE}" \ - --thumbprint-list "0000000000000000000000000000000000000000" >/dev/null + --output json) + PROVIDER_ARN=$(jq -er '.OpenIDConnectProviderArn | select(type == "string" and length > 0)' <<< "$CREATED_PROVIDER") +fi +if ! [[ "$PROVIDER_ARN" =~ ^arn:[a-z0-9-]+:iam::[0-9]{12}:oidc-provider/ ]] || [[ "${PROVIDER_ARN#*:oidc-provider/}" != "$OIDC_HOST" ]]; then + echo "Error: returned provider ARN does not match OIDC_ISSUER_URL." >&2 + exit 1 fi # shellcheck disable=SC2086 -PROVIDER_ARN=$(aws iam list-open-id-connect-providers $PROFILE_ARG \ - --query "OpenIDConnectProviderList[?contains(Arn, '${OIDC_HOST}')].Arn | [0]" \ - --output text) +PROVIDER=$(aws iam get-open-id-connect-provider $PROFILE_ARG --open-id-connect-provider-arn "$PROVIDER_ARN" --output json) +if ! jq -e --arg host "$OIDC_HOST" --arg audience "$OIDC_AUDIENCE" ' + .Url == $host and + (.ClientIDList | type == "array" and all(.[]; type == "string") and index($audience) != null) and + (.ThumbprintList | type == "array" and all(.[]; type == "string" and + test("^[0-9a-fA-F]{40}$") and . != "0000000000000000000000000000000000000000")) + ' <<< "$PROVIDER" >/dev/null; then + echo "Error: provider URL, audience or thumbprints do not match. Inspect $PROVIDER_ARN before rerunning; shared providers are not modified." >&2 + exit 1 +fi -# Build trust policy. OIDC_SUBJECT_CLAIM is validated non-empty above, so the -# :sub condition below is always present — there is no branch that omits it. -TRUST_POLICY=$(cat <> %q +[[ "$1 $2" != "${FAIL_OPERATION:-}" ]] || exit 42 +case "$1 $2" in + 'iam list-open-id-connect-providers') + if [[ "$*" == *'--output text'* ]]; then + jq -r --arg host "$TEST_HOST" '[.OpenIDConnectProviderList[].Arn | select(contains($host))][0] // "None"' <<< "$LIST_RESPONSE" + else + printf '%%s\n' "$LIST_RESPONSE" + fi ;; + 'iam create-open-id-connect-provider') printf '%%s\n' "$CREATE_RESPONSE" ;; + 'iam get-open-id-connect-provider') printf '%%s\n' "$PROVIDER_RESPONSE" ;; + 'iam get-role') echo 'arn:aws:iam::123456789012:role/CUDly' ;; + 'sts get-caller-identity') echo 123456789012 ;; + 'iam create-role'|'iam put-role-policy') echo '{}' ;; + *) exit 43 ;; +esac +`, logPath) +} + +func runAWSWIF(t *testing.T, data testTemplateData, overrides map[string]string) (int, string, string, [][]string) { + t.Helper() + host := strings.TrimPrefix(data.OIDCIssuerURL, "https://") + arn := "arn:aws:iam::123456789012:oidc-provider/" + host + provider, err := json.Marshal(map[string]any{ + "Url": host, "ClientIDList": []string{data.OIDCAudience, "sts.amazonaws.com"}, + "ThumbprintList": []string{strings.Repeat("a", 40)}, + }) + require.NoError(t, err) + env := map[string]string{ + "TEST_HOST": host, "LIST_RESPONSE": `{"OpenIDConnectProviderList":[]}`, + "CREATE_RESPONSE": fmt.Sprintf(`{"OpenIDConnectProviderArn":%q}`, arn), + "PROVIDER_RESPONSE": string(provider), + } + for key, value := range overrides { + env[key] = value + } + code, stdout, stderr, lines := runRenderedScript(t, "aws-wif-cli.sh", + renderCLITemplate(t, "templates/aws-wif-cli.sh.tmpl", data), + func(logPath string) map[string]string { + return map[string]string{"aws": awsWIFStub(logPath), "curl": fmt.Sprintf("#!/usr/bin/env bash\njq -cn --args '[\"curl\"] + $ARGS.positional' -- \"$@\" >> %q\necho 409\n", logPath)} + }, env) + calls := make([][]string, 0, len(lines)) + for _, line := range lines { + var args []string + require.NoError(t, json.Unmarshal([]byte(line), &args)) + calls = append(calls, args) + } + return code, stdout, stderr, calls +} + +func awsWIFData() testTemplateData { + data := baseData() + data.OIDCIssuerURL = "https://accounts.google.com" + data.OIDCAudience = "sts.amazonaws.com" + data.OIDCSubjectClaim = "123456789012345678901" + return data +} + +func awsWIFOperation(calls [][]string, operation string) []string { + for _, call := range calls { + if len(call) > 1 && call[1] == operation { + return call + } + } + return nil +} + +func awsWIFArgument(t *testing.T, call []string, flag string) string { + t.Helper() + for i, arg := range call { + if arg == flag && i+1 < len(call) { + return call[i+1] + } + } + t.Fatalf("missing %s in %v", flag, call) + return "" +} + +func TestAWSWIFCLIProviderGuards(t *testing.T) { + listed := `{"OpenIDConnectProviderList":[{"Arn":"` + awsWIFProvider + `.evil.example"},{"Arn":"` + awsWIFProvider + `/other"},{"Arn":"` + awsWIFProvider + `"}]}` + cases := []struct { + name string + env map[string]string + create, reject bool + }{ + {"absent", nil, true, false}, + {"exact with lookalikes", map[string]string{"LIST_RESPONSE": listed}, false, false}, + {"only lookalike", map[string]string{"LIST_RESPONSE": `{"OpenIDConnectProviderList":[{"Arn":"` + awsWIFProvider + `.evil.example"}]}`}, true, false}, + {"list denied", map[string]string{"FAIL_OPERATION": "iam list-open-id-connect-providers"}, false, true}, + {"create denied", map[string]string{"FAIL_OPERATION": "iam create-open-id-connect-provider"}, true, true}, + {"get denied", map[string]string{"FAIL_OPERATION": "iam get-open-id-connect-provider"}, true, true}, + {"missing audience", map[string]string{"LIST_RESPONSE": listed, "PROVIDER_RESPONSE": `{"Url":"accounts.google.com","ClientIDList":["other"],"ThumbprintList":[]}`}, false, true}, + {"wrong URL", map[string]string{"PROVIDER_RESPONSE": `{"Url":"accounts.google.com.evil","ClientIDList":["sts.amazonaws.com"],"ThumbprintList":[]}`}, true, true}, + {"zero thumbprint", map[string]string{"PROVIDER_RESPONSE": `{"Url":"accounts.google.com","ClientIDList":["sts.amazonaws.com"],"ThumbprintList":["0000000000000000000000000000000000000000"]}`}, true, true}, + {"malformed list", map[string]string{"LIST_RESPONSE": `not-json`}, false, true}, + {"malformed provider", map[string]string{"PROVIDER_RESPONSE": `{}`}, true, true}, + {"wrong created ARN", map[string]string{"CREATE_RESPONSE": `{"OpenIDConnectProviderArn":"` + awsWIFProvider + `.evil"}`}, true, true}, + {"duplicate exact", map[string]string{"LIST_RESPONSE": `{"OpenIDConnectProviderList":[{"Arn":"` + awsWIFProvider + `"},{"Arn":"` + awsWIFProvider + `"}]}`}, false, true}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + code, stdout, stderr, calls := runAWSWIF(t, awsWIFData(), tc.env) + require.Equal(t, tc.create, awsWIFOperation(calls, "create-open-id-connect-provider") != nil, "%v", calls) + if tc.reject { + require.NotZero(t, code, "stdout=%s stderr=%s", stdout, stderr) + require.Nil(t, awsWIFOperation(calls, "create-role")) + require.Nil(t, awsWIFOperation(calls, "put-role-policy")) + require.NotContains(t, stdout, "=== Done ===") + for _, call := range calls { + require.NotEqual(t, "curl", call[0], "registration after failed provider verification") + } + return + } + require.Zero(t, code, "%s", stderr) + if tc.create { + require.NotContains(t, awsWIFOperation(calls, "create-open-id-connect-provider"), "--thumbprint-list") + } + require.Equal(t, awsWIFProvider, awsWIFArgument(t, awsWIFOperation(calls, "get-open-id-connect-provider"), "--open-id-connect-provider-arn")) + var policy map[string]any + require.NoError(t, json.Unmarshal([]byte(awsWIFArgument(t, awsWIFOperation(calls, "create-role"), "--assume-role-policy-document")), &policy)) + want := `{"Version":"2012-10-17","Statement":[{"Effect":"Allow","Principal":{"Federated":"` + awsWIFProvider + `"},"Action":"sts:AssumeRoleWithWebIdentity","Condition":{"StringEquals":{"accounts.google.com:aud":"sts.amazonaws.com","accounts.google.com:sub":"123456789012345678901"}}}]}` + actual, err := json.Marshal(policy) + require.NoError(t, err) + require.JSONEq(t, want, string(actual)) + }) + } +} + +func TestAWSWIFCLIInputGuards(t *testing.T) { + for _, key := range []string{"OIDC_AUDIENCE", "OIDC_SUBJECT_CLAIM"} { + for _, invalid := range []string{"a b", "a\tb", "a\nb", "a\rb", "a\vb", "a\fb", "a$b", "a*b"} { + t.Run(key+"/"+fmt.Sprintf("%q", invalid), func(t *testing.T) { + code, _, stderr, calls := runAWSWIF(t, awsWIFData(), map[string]string{key: invalid}) + require.NotZero(t, code) + require.Empty(t, calls) + require.Contains(t, stderr, key) + }) + } + } + for _, issuer := range []string{"http://accounts.google.com", "https://accounts.google.com/", "https://"} { + t.Run(issuer, func(t *testing.T) { + code, _, stderr, calls := runAWSWIF(t, awsWIFData(), map[string]string{"OIDC_ISSUER_URL": issuer}) + require.NotZero(t, code) + require.Empty(t, calls) + require.Contains(t, stderr, "OIDC_ISSUER_URL") + }) + } + t.Run("missing jq", func(t *testing.T) { + code, _, stderr, calls := runAWSWIF(t, awsWIFData(), map[string]string{"PATH": t.TempDir()}) + require.NotZero(t, code) + require.Empty(t, calls) + require.Contains(t, stderr, "jq is required") + }) +} + +func TestAWSWIFCLIAudienceAndLiteralClaims(t *testing.T) { + for _, tc := range []struct { + name, rendered, want string + override map[string]string + }{ + {"Azure default", "api://AzureADTokenExchange", "api://AzureADTokenExchange", nil}, + {"rendered empty", "", "sts.amazonaws.com", nil}, + {"explicit empty", "api://AzureADTokenExchange", "sts.amazonaws.com", map[string]string{"OIDC_AUDIENCE": ""}}, + {"literal override", "default", `a"b\c`, map[string]string{"OIDC_AUDIENCE": `a"b\c`}}, + } { + t.Run(tc.name, func(t *testing.T) { + data := awsWIFData() + data.OIDCAudience = tc.rendered + if tc.name == "Azure default" { + data.Source = "azure" + data.OIDCIssuerURL = "https://login.microsoftonline.com/11111111-1111-1111-1111-111111111111/v2.0" + } + host := strings.TrimPrefix(data.OIDCIssuerURL, "https://") + env := map[string]string{"OIDC_SUBJECT_CLAIM": `a"b\c`} + for k, v := range tc.override { + env[k] = v + } + body, err := json.Marshal(map[string]any{"Url": host, "ClientIDList": []string{tc.want}, "ThumbprintList": []string{strings.Repeat("a", 40)}}) + require.NoError(t, err) + env["PROVIDER_RESPONSE"] = string(body) + code, _, stderr, calls := runAWSWIF(t, data, env) + require.Zero(t, code, "%s", stderr) + require.Equal(t, tc.want, awsWIFArgument(t, awsWIFOperation(calls, "create-open-id-connect-provider"), "--client-id-list")) + var policy struct { + Statement []struct { + Condition struct{ StringEquals map[string]string } + } + } + require.NoError(t, json.Unmarshal([]byte(awsWIFArgument(t, awsWIFOperation(calls, "create-role"), "--assume-role-policy-document")), &policy)) + require.Len(t, policy.Statement, 1) + require.Equal(t, map[string]string{host + ":aud": tc.want, host + ":sub": `a"b\c`}, policy.Statement[0].Condition.StringEquals) + }) + } +} diff --git a/internal/iacfiles/templates_test.go b/internal/iacfiles/templates_test.go index 28d2e280..58c55158 100644 --- a/internal/iacfiles/templates_test.go +++ b/internal/iacfiles/templates_test.go @@ -102,7 +102,7 @@ func TestCLITemplatesAutoRegister(t *testing.T) { `TARGET_ACCOUNT_ID=$(aws sts get-caller-identity`, // #1640: the trust policy's :sub condition must be present // unconditionally — there is no longer a code path that omits it. - `"${OIDC_HOST}:sub": "${OIDC_SUBJECT_CLAIM}"`, + `($host + ":sub"): $subject`, }, mustNot: []string{ "/api/registrations", @@ -237,7 +237,7 @@ func TestAWSWIFCLI_SubjectClaimRequired(t *testing.T) { // Exactly one Condition block, and it always carries :sub — no branch // builds a StringEquals map with only :aud. - subCondition := `"${OIDC_HOST}:sub": "${OIDC_SUBJECT_CLAIM}"` + subCondition := `($host + ":sub"): $subject` if n := strings.Count(rendered, subCondition); n != 1 { t.Errorf("expected exactly one :sub condition in the rendered trust policy, found %d", n) } @@ -276,9 +276,12 @@ func awsStubScript(logPath string) string { // JSON, so newlines inside the arguments are folded to spaces first. "args=\"$*\"\n" + "printf '%s\\n' \"${args//$'\\n'/ }\" >> '" + logPath + "'\n" + - // "None" is what the script's provider-lookup branches expect when no - // OIDC provider exists yet, so the rest of the script proceeds. - "echo None\n" + "case \"$1 $2\" in\n" + + " 'iam list-open-id-connect-providers') echo '{\"OpenIDConnectProviderList\":[]}' ;;\n" + + " 'iam create-open-id-connect-provider') echo '{\"OpenIDConnectProviderArn\":\"" + awsWIFProvider + "\"}' ;;\n" + + " 'iam get-open-id-connect-provider') echo '{\"Url\":\"accounts.google.com\",\"ClientIDList\":[\"sts.amazonaws.com\"],\"ThumbprintList\":[]}' ;;\n" + + " *) echo None ;;\n" + + "esac\n" } // runRenderedWIFScript writes the rendered aws-wif-cli.sh to a temp file, puts a