From 0878884d86d9197c1fb303cee09d2a8c3f526404 Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Fri, 23 Oct 2026 08:45:05 +0000 Subject: [PATCH 1/5] chore(iac): derive the CFN issuer host, create the GCP signing key first, read the dev Function URL from state Three low findings from the 2026-09-02 audit. A13b-014: the aws-target CloudFormation template took the trust policy's condition-key host as a second parameter, OIDCIssuerHost, that the operator had to keep equal to OIDCIssuerURL minus https://. The host is now derived with !Select [1, !Split ["https://", !Ref OIDCIssuerURL]], the same value as the Terraform module's local.oidc_condition_host, and the server no longer passes the parameter in the bundle's params JSON or deploy script (CFN would reject an undeclared parameter). A13c-024: the Cloud Run module's OIDC signing key now has create_before_destroy, matching its AWS twin, so a change that forces a new key cannot destroy the live one first. destroy_scheduled_duration becomes an input whose default keeps every deployed key unchanged (the provider marks it ForceNew). prevent_destroy cannot be an input (Terraform requires a literal) and staging cleanup runs terraform destroy, so the redundant `prevent_destroy = false` line is replaced by a comment saying so. A13c-017: github-dev.tfvars no longer pins the dev Lambda Function URL host. The deploy and rollback workflows read the previous apply's lambda_function_url output (scripts/read-lambda-function-url.sh) and pass it as TF_VAR_lambda_deployed_function_url; compute.tf appends its origin to the CORS allowlist. A post-apply step warns when the apply changed the URL. Closes #324 Co-Authored-By: Claude Opus 5.5 --- .github/workflows/ci.yml | 19 ++ .github/workflows/deploy-aws-lambda.yml | 25 ++ .github/workflows/rollback.yml | 5 + iac/aws_target_cfn_test.go | 220 ++++++++++++++++++ .../aws-target/cloudformation/template.yaml | 35 +-- internal/api/handler_federation.go | 8 +- .../api/handler_federation_cfn_params_test.go | 123 ++++++++++ .../handler_federation_controlchar_test.go | 1 - .../iacfiles/templates/aws-cfn-deploy.sh.tmpl | 1 - internal/iacfiles/templates_test.go | 1 - known_issues/13_iac_aws_target.md | 1 + scripts/read-lambda-function-url.sh | 45 ++++ scripts/test-read-lambda-function-url.sh | 75 ++++++ terraform/environments/aws/compute.tf | 9 +- terraform/environments/aws/github-dev.tfvars | 7 +- .../aws/lambda_cors_origin_test.go | 92 ++++++++ .../aws/tests/lambda_cors_origins.tftest.hcl | 109 +++++++++ terraform/environments/aws/variables.tf | 19 ++ terraform/environments/gcp/compute.tf | 3 + terraform/environments/gcp/variables.tf | 6 + .../compute/gcp/cloud-run/.terraform.lock.hcl | 1 + .../compute/gcp/cloud-run/signing-key.tf | 19 +- .../cloud-run/tests/signing_key.tftest.hcl | 105 +++++++++ .../compute/gcp/cloud-run/variables.tf | 21 ++ .../compute/gcp/signing_key_lifecycle_test.go | 41 ++++ 25 files changed, 959 insertions(+), 32 deletions(-) create mode 100644 iac/aws_target_cfn_test.go create mode 100644 internal/api/handler_federation_cfn_params_test.go create mode 100755 scripts/read-lambda-function-url.sh create mode 100755 scripts/test-read-lambda-function-url.sh create mode 100644 terraform/environments/aws/lambda_cors_origin_test.go create mode 100644 terraform/environments/aws/tests/lambda_cors_origins.tftest.hcl create mode 100644 terraform/modules/compute/gcp/cloud-run/tests/signing_key.tftest.hcl create mode 100644 terraform/modules/compute/gcp/signing_key_lifecycle_test.go diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index d8a230650..0f66f2543 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -723,6 +723,19 @@ jobs: terraform -chdir="$d" test fi + # The Cloud Run OIDC signing key's create-time retention input must keep + # deployed keys unchanged and reject values Cloud KMS refuses (#324). + - name: Test GCP compute modules + if: matrix.cloud == 'gcp' + run: | + for t in terraform/modules/compute/gcp/*/tests; do + [ -d "$t" ] || continue + d=$(dirname "$t") + terraform -chdir="$d" init -backend=false -lockfile=readonly -input=false + terraform -chdir="$d" fmt -check -recursive + terraform -chdir="$d" test + done + - name: Test CI/CD trust (immutable ids) # Skips a cloud until it has a tests dir. The lock file is gitignored, # so no -lockfile=readonly. @@ -1251,6 +1264,12 @@ jobs: - name: Run state namespace self-tests run: bash scripts/test-aws-tfstate-platform-key.sh + # The Lambda deploy and rollback read the Function URL back from state + # for its CORS origin (#324); the reader must reject anything that is + # not a Function URL. terraform is stubbed, so nothing reads real state. + - name: Run Function URL reader self-tests + run: bash scripts/test-read-lambda-function-url.sh + # Assert that no Terraform file declares an azurerm_key_vault_access_policy # resource. This project's only Key Vault sets enable_rbac_authorization = # true, and an RBAC-enabled vault ignores access policies entirely, so such a diff --git a/.github/workflows/deploy-aws-lambda.yml b/.github/workflows/deploy-aws-lambda.yml index a8055e468..098df965d 100644 --- a/.github/workflows/deploy-aws-lambda.yml +++ b/.github/workflows/deploy-aws-lambda.yml @@ -271,6 +271,17 @@ jobs: cd terraform/environments/aws terraform init -backend-config=/tmp/backend.tfbackend + # The Function URL's host is server-assigned, so its origin is read from + # the previous apply instead of being committed to github-*.tfvars, where + # it went stale whenever the URL was reissued (#324). Empty on the first + # deploy; "Check CORS covers the Function URL" below flags that case. + - name: Read deployed Function URL + id: deployed-url + run: | + set -euo pipefail + url=$(scripts/read-lambda-function-url.sh terraform/environments/aws) + echo "url=${url}" >> "$GITHUB_OUTPUT" + - name: Terraform Plan id: plan env: @@ -285,6 +296,7 @@ jobs: # accepted by the var (the Terraform local then falls back to # frontend_domain_names[0]). TF_VAR_dashboard_url: ${{ secrets.DASHBOARD_URL }} + TF_VAR_lambda_deployed_function_url: ${{ steps.deployed-url.outputs.url }} ENVIRONMENT: ${{ needs.prepare.outputs.target_environment }} run: | cd terraform/environments/aws @@ -334,6 +346,19 @@ jobs: echo "log_group=$(terraform output -raw lambda_log_group_name 2>/dev/null | grep -v '::' || echo "")" } >> "$GITHUB_OUTPUT" + # The allowlist this apply wrote holds the origin read before the plan. + # If the apply created or reissued the Function URL, the live origin is + # not in it until the next deploy, which reads the new URL back. + - name: Check CORS covers the Function URL + env: + PLANNED_URL: ${{ steps.deployed-url.outputs.url }} + run: | + set -euo pipefail + live=$(scripts/read-lambda-function-url.sh terraform/environments/aws) + if [ -n "$live" ] && [ "$live" != "$PLANNED_URL" ]; then + echo "::warning::The Function URL is now ${live}, but this apply's CORS allowlist was built from '${PLANNED_URL}'. Re-run this workflow so the next apply adds the new origin." + fi + - name: Save deployment info env: ENVIRONMENT: ${{ needs.prepare.outputs.target_environment }} diff --git a/.github/workflows/rollback.yml b/.github/workflows/rollback.yml index e754e7e8a..f16bfa576 100644 --- a/.github/workflows/rollback.yml +++ b/.github/workflows/rollback.yml @@ -278,6 +278,11 @@ jobs: cd terraform/environments/aws terraform init -backend-config=/tmp/backend.tfbackend + # Keep the deployed Function URL's origin in the CORS allowlist, as + # deploy-aws-lambda.yml does; it is not in github-*.tfvars (#324). + TF_VAR_lambda_deployed_function_url=$(../../../scripts/read-lambda-function-url.sh .) + export TF_VAR_lambda_deployed_function_url + terraform apply -auto-approve \ -var-file="github-${ENVIRONMENT}.tfvars" \ -var="image_uri=${IMAGE_URI}" \ diff --git a/iac/aws_target_cfn_test.go b/iac/aws_target_cfn_test.go new file mode 100644 index 000000000..0bc9127e7 --- /dev/null +++ b/iac/aws_target_cfn_test.go @@ -0,0 +1,220 @@ +package iac + +import ( + "fmt" + "regexp" + "sort" + "strconv" + "strings" + "testing" + + "github.com/stretchr/testify/require" + "gopkg.in/yaml.v3" +) + +// Regression tests for #324 (audit finding A13b-014). The aws-target +// CloudFormation template used to take the trust policy's condition-key host +// as a second free-text parameter, OIDCIssuerHost, that the operator had to +// keep equal to OIDCIssuerURL minus https:// by hand. A mismatch produced a +// stack that deployed cleanly and then denied every AssumeRoleWithWebIdentity. +// The host is now derived from OIDCIssuerURL, the way the Terraform sibling's +// local.oidc_condition_host derives it. + +const awsTargetCFNTemplate = "federation/aws-target/cloudformation/template.yaml" + +// mapValue returns the value node for key in a YAML mapping node, or nil. +func mapValue(n *yaml.Node, key string) *yaml.Node { + if n == nil || n.Kind != yaml.MappingNode { + return nil + } + for i := 0; i+1 < len(n.Content); i += 2 { + if n.Content[i].Kind == yaml.ScalarNode && n.Content[i].Value == key { + return n.Content[i+1] + } + } + return nil +} + +// mapPath follows keys from n through nested mappings, failing the test when +// any step is missing. +func mapPath(t *testing.T, n *yaml.Node, keys ...string) *yaml.Node { + t.Helper() + for _, k := range keys { + next := mapValue(n, k) + require.NotNil(t, next, "template has no %q under the path %v", k, keys) + n = next + } + return n +} + +func loadAWSTargetCFN(t *testing.T) *yaml.Node { + t.Helper() + raw, err := Modules.ReadFile(awsTargetCFNTemplate) + require.NoError(t, err) + var doc yaml.Node + require.NoError(t, yaml.Unmarshal(raw, &doc)) + require.Len(t, doc.Content, 1) + return doc.Content[0] +} + +// cfnEval evaluates the subset of CloudFormation intrinsics the trust +// policy's condition keys use (!Ref, !Select, !Split, !Sub) against the given +// parameter values. Anything else is an error, so a rewrite into a form this +// cannot model fails the test instead of passing unevaluated. +func cfnEval(n *yaml.Node, params map[string]string) (any, error) { + switch n.Tag { + case "!!str", "!!int": + return n.Value, nil + case "!Ref": + v, ok := params[n.Value] + if !ok { + return nil, fmt.Errorf("!Ref to %q, which is not a parameter value under test", n.Value) + } + return v, nil + case "!Split": + args, err := cfnArgs(n, 2, params) + if err != nil { + return nil, err + } + delim, s, ok := twoStrings(args) + if !ok { + return nil, fmt.Errorf("!Split arguments are not strings: %v", args) + } + return strings.Split(s, delim), nil + case "!Select": + args, err := cfnArgs(n, 2, params) + if err != nil { + return nil, err + } + idx, err := strconv.Atoi(fmt.Sprint(args[0])) + list, ok := args[1].([]string) + if err != nil || !ok || idx < 0 || idx >= len(list) { + return nil, fmt.Errorf("!Select %v out of range or not a list", args) + } + return list[idx], nil + case "!Sub": + return cfnSub(n, params) + } + return nil, fmt.Errorf("unsupported node tag %q (value %q)", n.Tag, n.Value) +} + +// cfnArgs evaluates the elements of a sequence-form intrinsic. +func cfnArgs(n *yaml.Node, want int, params map[string]string) ([]any, error) { + if n.Kind != yaml.SequenceNode || len(n.Content) != want { + return nil, fmt.Errorf("%s wants a %d-element list", n.Tag, want) + } + out := make([]any, 0, want) + for _, c := range n.Content { + v, err := cfnEval(c, params) + if err != nil { + return nil, err + } + out = append(out, v) + } + return out, nil +} + +func twoStrings(args []any) (string, string, bool) { + a, ok1 := args[0].(string) + b, ok2 := args[1].(string) + return a, b, ok1 && ok2 +} + +var subVarRe = regexp.MustCompile(`\$\{([^}!]+)\}`) + +// cfnSub renders both the string and the [string, {vars}] forms of !Sub. +func cfnSub(n *yaml.Node, params map[string]string) (any, error) { + format, vars := n, map[string]string{} + if n.Kind == yaml.SequenceNode { + if len(n.Content) != 2 || n.Content[1].Kind != yaml.MappingNode { + return nil, fmt.Errorf("!Sub list form wants [string, mapping]") + } + format = n.Content[0] + m := n.Content[1] + for i := 0; i+1 < len(m.Content); i += 2 { + v, err := cfnEval(m.Content[i+1], params) + if err != nil { + return nil, err + } + s, ok := v.(string) + if !ok { + return nil, fmt.Errorf("!Sub variable %q is not a string", m.Content[i].Value) + } + vars[m.Content[i].Value] = s + } + } + var missing error + out := subVarRe.ReplaceAllStringFunc(format.Value, func(ref string) string { + name := subVarRe.FindStringSubmatch(ref)[1] + if v, ok := vars[name]; ok { + return v + } + if v, ok := params[name]; ok { + return v + } + missing = fmt.Errorf("!Sub references %q, which is neither a variable nor a parameter value under test", name) + return ref + }) + return out, missing +} + +// terraformConditionHost mirrors local.oidc_condition_host in +// federation/aws-target/terraform/main.tf. +func terraformConditionHost(url string) string { + return strings.TrimSuffix(strings.TrimPrefix(url, "https://"), "/") +} + +func TestAWSTargetCFN_HasNoIssuerHostParameter(t *testing.T) { + root := loadAWSTargetCFN(t) + params := mapPath(t, root, "Parameters") + require.Nil(t, mapValue(params, "OIDCIssuerHost"), + "OIDCIssuerHost must not be a parameter: the condition-key host is derived from OIDCIssuerURL (#324)") + raw, err := Modules.ReadFile(awsTargetCFNTemplate) + require.NoError(t, err) + require.NotContains(t, string(raw), "${OIDCIssuerHost}", "nothing may still reference the removed parameter") +} + +// TestAWSTargetCFN_ConditionKeysDeriveFromIssuerURL evaluates every key of the +// trust policy's StringEquals block for real issuer URLs and checks it is the +// host Terraform derives, followed by :aud or :sub. +func TestAWSTargetCFN_ConditionKeysDeriveFromIssuerURL(t *testing.T) { + root := loadAWSTargetCFN(t) + urlPattern := regexp.MustCompile(mapPath(t, root, "Parameters", "OIDCIssuerURL", "AllowedPattern").Value) + statements := mapPath(t, root, "Resources", "CUDlyRole", "Properties", "AssumeRolePolicyDocument", "Statement") + require.Equal(t, yaml.SequenceNode, statements.Kind) + require.Len(t, statements.Content, 1, "the trust policy has exactly one statement") + stringEquals := mapPath(t, statements.Content[0], "Condition", "StringEquals") + + for _, url := range []string{ + "https://accounts.google.com", + "https://login.microsoftonline.com/11111111-1111-1111-1111-111111111111/v2.0", + } { + t.Run(url, func(t *testing.T) { + require.True(t, urlPattern.MatchString(url), "fixture must satisfy OIDCIssuerURL's AllowedPattern") + host := terraformConditionHost(url) + var keys []string + for i := 0; i+1 < len(stringEquals.Content); i += 2 { + v, err := cfnEval(stringEquals.Content[i], map[string]string{"OIDCIssuerURL": url}) + require.NoError(t, err, "condition key %d must be computed from OIDCIssuerURL alone", i/2) + keys = append(keys, v.(string)) + } + sort.Strings(keys) + require.Equal(t, []string{host + ":aud", host + ":sub"}, keys) + }) + } +} + +// TestCFNEval_Positive is the control for the evaluator above: it must +// resolve the derivation the template uses, and reject an unknown parameter, +// so the condition-key test cannot pass by evaluating nothing. +func TestCFNEval_Positive(t *testing.T) { + var n yaml.Node + require.NoError(t, yaml.Unmarshal([]byte(`!Sub ["${Host}:aud", {Host: !Select [1, !Split ["https://", !Ref OIDCIssuerURL]]}]`), &n)) + got, err := cfnEval(n.Content[0], map[string]string{"OIDCIssuerURL": "https://accounts.google.com"}) + require.NoError(t, err) + require.Equal(t, "accounts.google.com:aud", got) + + require.NoError(t, yaml.Unmarshal([]byte(`!Sub "${OIDCIssuerHost}:aud"`), &n)) + _, err = cfnEval(n.Content[0], map[string]string{"OIDCIssuerURL": "https://accounts.google.com"}) + require.Error(t, err) +} diff --git a/iac/federation/aws-target/cloudformation/template.yaml b/iac/federation/aws-target/cloudformation/template.yaml index 3b95d2fb6..4e2e1ce0c 100644 --- a/iac/federation/aws-target/cloudformation/template.yaml +++ b/iac/federation/aws-target/cloudformation/template.yaml @@ -9,25 +9,15 @@ Parameters: Type: String Description: > Full OIDC issuer URL (with https://) — used for the IAM OIDC provider Url property. - Must NOT contain a trailing slash; the value here must match OIDCIssuerHost - (minus the https:// prefix) exactly because AWS IAM condition keys are - case-sensitive and string-equal. + Must NOT contain a trailing slash. The trust policy's condition-key host + is derived from this value (the URL with https:// removed), so the two + cannot disagree; AWS IAM condition keys are case-sensitive and + string-equal. Azure AD: https://login.microsoftonline.com//v2.0 GCP: https://accounts.google.com AllowedPattern: "^https://[^/]+(/[^/]+)*$" ConstraintDescription: "OIDCIssuerURL must be https:// and must not contain a trailing slash." - OIDCIssuerHost: - Type: String - Description: > - OIDC issuer host and path WITHOUT the https:// prefix — used as the IAM - condition key. Must equal OIDCIssuerURL with the https:// stripped and no - trailing slash; the operator is responsible for keeping the two in sync. - Azure AD: login.microsoftonline.com//v2.0 - GCP: accounts.google.com - AllowedPattern: "^[^/]+(/[^/]+)*$" - ConstraintDescription: "OIDCIssuerHost must not start or end with a slash." - OIDCAudience: Type: String Description: > @@ -267,11 +257,22 @@ Resources: Principal: Federated: !GetAtt OIDCProvider.Arn Action: sts:AssumeRoleWithWebIdentity + # The condition-key host is OIDCIssuerURL minus its https:// + # prefix, derived here rather than taken as a second parameter so + # the two cannot disagree (#324). OIDCIssuerURL's AllowedPattern + # guarantees exactly one leading https:// and no trailing slash, so + # element 1 of the split is the whole host and path. This is the + # same value as local.oidc_condition_host in the Terraform module. Condition: StringEquals: - !Sub "${OIDCIssuerHost}:aud": - !If [HasAudience, !Ref OIDCAudience, sts.amazonaws.com] - !Sub "${OIDCIssuerHost}:sub": !Ref OIDCSubjectClaim + ? !Sub + - "${Host}:aud" + - Host: !Select [1, !Split ["https://", !Ref OIDCIssuerURL]] + : !If [HasAudience, !Ref OIDCAudience, sts.amazonaws.com] + ? !Sub + - "${Host}:sub" + - Host: !Select [1, !Split ["https://", !Ref OIDCIssuerURL]] + : !Ref OIDCSubjectClaim ManagedPolicyArns: - !Ref CUDlyPolicy diff --git a/internal/api/handler_federation.go b/internal/api/handler_federation.go index ead506cd4..09f8f885d 100644 --- a/internal/api/handler_federation.go +++ b/internal/api/handler_federation.go @@ -30,9 +30,8 @@ type federationIaCData struct { Source string SourceAccountID string // AWS account ID where CUDly runs (resolved via STS) // AWS WIF / cross-account - OIDCIssuerURL string - OIDCIssuerHost string // issuer URL without https:// prefix (used as IAM condition key) - OIDCAudience string + OIDCIssuerURL string + OIDCAudience string // OIDCSubjectClaim restricts the AWS trust policy to a single workload // subject. Deliberately left empty by every generic-bundle builder below — // CUDly's server has no generic way to know the calling workload's real @@ -338,7 +337,6 @@ func shellEscapeData(data federationIaCData) federationIaCData { d.AccountSlug = shellEscape(data.AccountSlug) d.Source = shellEscape(data.Source) d.OIDCIssuerURL = shellEscape(data.OIDCIssuerURL) - d.OIDCIssuerHost = shellEscape(data.OIDCIssuerHost) d.OIDCAudience = shellEscape(data.OIDCAudience) d.OIDCSubjectClaim = shellEscape(data.OIDCSubjectClaim) d.SubscriptionID = shellEscape(data.SubscriptionID) @@ -402,7 +400,6 @@ func buildGenericIaCData(target, source, dashboardURL string) federationIaCData switch target { case "aws": data.OIDCIssuerURL = awsOIDCIssuer(source, "") - data.OIDCIssuerHost = strings.TrimPrefix(data.OIDCIssuerURL, "https://") data.OIDCAudience = awsOIDCAudience(source) case "gcp": data.OIDCIssuerURI = gcpOIDCIssuerURI(source, "") @@ -853,7 +850,6 @@ func buildCFParamsJSON(data federationIaCData, source string) (string, error) { } else { params = []cfParam{ {ParameterKey: "OIDCIssuerURL", ParameterValue: data.OIDCIssuerURL}, - {ParameterKey: "OIDCIssuerHost", ParameterValue: strings.TrimPrefix(data.OIDCIssuerURL, "https://")}, {ParameterKey: "OIDCAudience", ParameterValue: data.OIDCAudience}, {ParameterKey: "OIDCSubjectClaim", ParameterValue: data.OIDCSubjectClaim}, {ParameterKey: "RoleName", ParameterValue: "CUDly-" + data.AccountSlug}, diff --git a/internal/api/handler_federation_cfn_params_test.go b/internal/api/handler_federation_cfn_params_test.go new file mode 100644 index 000000000..e231cd3af --- /dev/null +++ b/internal/api/handler_federation_cfn_params_test.go @@ -0,0 +1,123 @@ +package api + +import ( + "archive/zip" + "bytes" + "context" + "encoding/base64" + "encoding/json" + "io" + "regexp" + "sort" + "strings" + "testing" + + "github.com/stretchr/testify/require" + "gopkg.in/yaml.v3" +) + +// cfnOverrideRe matches one "Key=value" argument of the deploy script's +// --parameter-overrides list. +var cfnOverrideRe = regexp.MustCompile(`(?m)^\s+"([A-Za-z][A-Za-z0-9]*)=`) + +// cfnBundleFiles returns the template, the parameter keys from the params +// JSON, and the parameter keys the deploy script overrides, from a format=cfn +// download. +func cfnBundleFiles(t *testing.T, source string) (template []byte, paramsKeys, scriptKeys []string) { + t.Helper() + res, err := federationHandler().getFederationIaC(context.Background(), federationReq(map[string]string{ + "target": "aws", "source": source, "format": "cfn", + })) + require.NoError(t, err) + raw, err := base64.StdEncoding.DecodeString(res.Content) + require.NoError(t, err) + zr, err := zip.NewReader(bytes.NewReader(raw), int64(len(raw))) + require.NoError(t, err) + for _, f := range zr.File { + rc, err := f.Open() + require.NoError(t, err) + body, err := io.ReadAll(rc) + require.NoError(t, rc.Close()) + require.NoError(t, err) + switch { + case f.Name == "cloudformation/template.yaml": + template = body + case strings.HasSuffix(f.Name, "-cf-params.json"): + var params []map[string]string + require.NoError(t, json.Unmarshal(body, ¶ms)) + for _, p := range params { + paramsKeys = append(paramsKeys, p["ParameterKey"]) + } + case f.Name == "cloudformation/deploy-cfn.sh": + for _, m := range cfnOverrideRe.FindAllStringSubmatch(string(body), -1) { + scriptKeys = append(scriptKeys, m[1]) + } + } + } + require.NotEmpty(t, template, "bundle has no template") + require.NotEmpty(t, paramsKeys, "bundle has no params JSON entries") + require.NotEmpty(t, scriptKeys, "deploy script has no --parameter-overrides entries") + return template, paramsKeys, scriptKeys +} + +// cfnTemplateParameters returns the template's declared parameters, mapped to +// whether each one has a Default. +func cfnTemplateParameters(t *testing.T, template []byte) map[string]bool { + t.Helper() + var doc yaml.Node + require.NoError(t, yaml.Unmarshal(template, &doc)) + root := doc.Content[0] + var params *yaml.Node + for i := 0; i+1 < len(root.Content); i += 2 { + if root.Content[i].Value == "Parameters" { + params = root.Content[i+1] + } + } + require.NotNil(t, params, "template has no Parameters section") + out := map[string]bool{} + for i := 0; i+1 < len(params.Content); i += 2 { + hasDefault := false + body := params.Content[i+1] + for j := 0; j+1 < len(body.Content); j += 2 { + if body.Content[j].Value == "Default" { + hasDefault = true + } + } + out[params.Content[i].Value] = hasDefault + } + return out +} + +// TestCFNBundle_OverridesMatchTemplateParameters guards the contract between +// the CloudFormation template a bundle ships and the two ways the bundle +// passes it parameters. CloudFormation rejects a parameter the template does +// not declare, so a key left behind after a parameter is removed (as +// OIDCIssuerHost was, #324) breaks every deploy, and a missing required one +// fails it too. +func TestCFNBundle_OverridesMatchTemplateParameters(t *testing.T) { + // Both non-AWS sources ship the aws-target WIF template. + for _, source := range []string{"gcp", "azure"} { + t.Run(source, func(t *testing.T) { + template, paramsKeys, scriptKeys := cfnBundleFiles(t, source) + declared := cfnTemplateParameters(t, template) + for name, keys := range map[string][]string{"params JSON": paramsKeys, "deploy script": scriptKeys} { + passed := map[string]bool{} + for _, k := range keys { + require.Contains(t, declared, k, "%s passes %s, which the template does not declare", name, k) + passed[k] = true + } + var missing []string + for p, hasDefault := range declared { + if !hasDefault && !passed[p] { + missing = append(missing, p) + } + } + sort.Strings(missing) + require.Empty(t, missing, "%s omits required template parameters", name) + } + require.NotContains(t, declared, "OIDCIssuerHost") + require.Contains(t, paramsKeys, "OIDCIssuerURL") + require.Contains(t, scriptKeys, "OIDCIssuerURL") + }) + } +} diff --git a/internal/api/handler_federation_controlchar_test.go b/internal/api/handler_federation_controlchar_test.go index e18198e31..16fb0d47d 100644 --- a/internal/api/handler_federation_controlchar_test.go +++ b/internal/api/handler_federation_controlchar_test.go @@ -76,7 +76,6 @@ func benignFederationData() federationIaCData { Source: "azure", SourceAccountID: "000000000000", OIDCIssuerURL: "https://login.microsoftonline.com/00000000-0000-0000-0000-000000000002/v2.0", - OIDCIssuerHost: "login.microsoftonline.com/00000000-0000-0000-0000-000000000002/v2.0", OIDCAudience: "api://AzureADTokenExchange", OIDCSubjectClaim: "00000000-0000-0000-0000-000000000003", SubscriptionID: "00000000-0000-0000-0000-000000000001", diff --git a/internal/iacfiles/templates/aws-cfn-deploy.sh.tmpl b/internal/iacfiles/templates/aws-cfn-deploy.sh.tmpl index 2d594bfa4..bfee76081 100644 --- a/internal/iacfiles/templates/aws-cfn-deploy.sh.tmpl +++ b/internal/iacfiles/templates/aws-cfn-deploy.sh.tmpl @@ -33,7 +33,6 @@ aws cloudformation deploy \ --stack-name "$STACK_NAME" \ --parameter-overrides \ "OIDCIssuerURL={{.OIDCIssuerURL}}" \ - "OIDCIssuerHost={{.OIDCIssuerHost}}" \ "OIDCAudience={{.OIDCAudience}}" \ "OIDCSubjectClaim={{.OIDCSubjectClaim}}" \ "RoleName=CUDly-{{.AccountSlug}}" \ diff --git a/internal/iacfiles/templates_test.go b/internal/iacfiles/templates_test.go index e7bc2d64e..05f53cd5b 100644 --- a/internal/iacfiles/templates_test.go +++ b/internal/iacfiles/templates_test.go @@ -22,7 +22,6 @@ type testTemplateData struct { AccountSlug string Source string OIDCIssuerURL string - OIDCIssuerHost string OIDCAudience string OIDCSubjectClaim string SubscriptionID string diff --git a/known_issues/13_iac_aws_target.md b/known_issues/13_iac_aws_target.md index 14624ac97..4710e8964 100644 --- a/known_issues/13_iac_aws_target.md +++ b/known_issues/13_iac_aws_target.md @@ -228,6 +228,7 @@ other control on this trust policy. - Terraform now derives `local.oidc_issuer_url_normalized` (`trimsuffix(..., "/")`) and `local.oidc_condition_host` (`trimsuffix(trimprefix(..., "https://"), "/")`) once and uses them for both the OIDC provider URL and the trust-policy condition keys, so trailing slashes are handled consistently. - The `oidc_issuer_url` variable now has a strict regex validation rejecting trailing slashes and non-https URLs. - `OIDCIssuerURL` and `OIDCIssuerHost` parameters in CloudFormation now have explicit `AllowedPattern` + `ConstraintDescription` forbidding trailing slashes; the description calls out that the operator must keep the two values in sync. +- #324 then removed the `OIDCIssuerHost` parameter: the CloudFormation trust policy derives the condition-key host from `OIDCIssuerURL` (`!Select [1, !Split ["https://", ...]]`), the same value as Terraform's `local.oidc_condition_host`, and the bundle no longer passes the parameter. ### Original implementation plan diff --git a/scripts/read-lambda-function-url.sh b/scripts/read-lambda-function-url.sh new file mode 100755 index 000000000..6f7a30716 --- /dev/null +++ b/scripts/read-lambda-function-url.sh @@ -0,0 +1,45 @@ +#!/usr/bin/env bash +# read-lambda-function-url.sh +# +# Prints the Lambda Function URL recorded by the previous apply of the AWS +# environment, so a deploy can hand it back to Terraform as +# TF_VAR_lambda_deployed_function_url and keep that origin in the Function +# URL's CORS allowlist (#324). The URL ID is server-assigned and only known +# after an apply, which is why it is read from state instead of being pasted +# into github-.tfvars by hand. +# +# Usage: scripts/read-lambda-function-url.sh +# +# The directory must already be initialised against its backend. Prints: +# - the URL with any trailing slash removed, when state has one; +# - nothing (exit 0) when state has no lambda_function_url value yet, i.e. +# the first deploy, or a Fargate stack whose output is null. +# Exits 1 when state holds something that is not a Lambda Function URL, so a +# corrupted or unexpected value stops the deploy instead of reaching CORS. +# +# Read-only: runs `terraform output` and nothing else. + +set -euo pipefail + +if [[ $# -ne 1 || -z "$1" ]]; then + echo "usage: $0 " >&2 + exit 2 +fi + +# `terraform output -raw` fails when the output is missing (no state yet) or +# null, and both mean "no deployed URL". hashicorp/setup-terraform's wrapper +# adds ::debug:: lines to stdout, which are dropped the same way the deploy +# workflow's "Get Terraform outputs" step drops them. +raw=$(terraform -chdir="$1" output -raw lambda_function_url 2>/dev/null | grep -v '::' || true) + +if [[ -z "$raw" ]]; then + exit 0 +fi + +url_re='^https://[a-z0-9]+\.lambda-url\.[a-z0-9-]+\.on\.aws/?$' +if [[ "$raw" == *$'\n'* || ! "$raw" =~ $url_re ]]; then + echo "ERROR: lambda_function_url in state is not a Lambda Function URL; refusing to pass it to the CORS allowlist" >&2 + exit 1 +fi + +printf '%s\n' "${raw%/}" diff --git a/scripts/test-read-lambda-function-url.sh b/scripts/test-read-lambda-function-url.sh new file mode 100755 index 000000000..ad9ff15fb --- /dev/null +++ b/scripts/test-read-lambda-function-url.sh @@ -0,0 +1,75 @@ +#!/usr/bin/env bash +# test-read-lambda-function-url.sh +# +# Offline test for read-lambda-function-url.sh (#324): terraform is stubbed via +# PATH, so nothing reads real state or reaches AWS. Each case sets what the +# stubbed `terraform output -raw lambda_function_url` prints and how it exits. +# +# Exits 0 when all cases pass; exits 1 on any failure. + +set -euo pipefail + +SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +SUT="${SCRIPT_DIR}/read-lambda-function-url.sh" +WORK="$(mktemp -d)" +trap 'rm -rf "$WORK"' EXIT +mkdir -p "$WORK/bin" "$WORK/tf" + +# The stub checks it was asked for exactly the output the script needs, then +# prints $STUB_STDOUT and exits $STUB_EXIT. +cat > "$WORK/bin/terraform" <<'STUB' +#!/usr/bin/env bash +if [[ "$*" != "-chdir=$EXPECT_DIR output -raw lambda_function_url" ]]; then + echo "unexpected terraform invocation: $*" >&2 + exit 99 +fi +printf '%b' "$STUB_STDOUT" +exit "${STUB_EXIT:-0}" +STUB +chmod +x "$WORK/bin/terraform" + +pass=0 +fail=0 + +# run_case NAME STDOUT EXIT WANT_EXIT WANT_OUTPUT +run_case() { + local name="$1" stdout="$2" exit_code="$3" want_exit="$4" want_out="$5" + local out got_exit=0 + out=$(PATH="$WORK/bin:$PATH" EXPECT_DIR="$WORK/tf" STUB_STDOUT="$stdout" STUB_EXIT="$exit_code" \ + bash "$SUT" "$WORK/tf" 2>/dev/null) || got_exit=$? + if [[ "$got_exit" == "$want_exit" && "$out" == "$want_out" ]]; then + echo "PASS: $name" + pass=$((pass + 1)) + else + echo "FAIL: $name (exit $got_exit, want $want_exit; output '$out', want '$want_out')" + fail=$((fail + 1)) + fi +} + +url="https://abcdefghijklmnopqrstuvwxyz012345.lambda-url.us-east-1.on.aws" + +run_case "strips the trailing slash the output carries" "$url/" 0 0 "$url" +run_case "accepts a URL without a trailing slash" "$url" 0 0 "$url" +run_case "drops setup-terraform wrapper debug lines" "::debug::stdout\n$url/\n::debug::exitcode: 0" 0 0 "$url" +# `terraform output -raw` exits 1 with nothing on stdout both when the output +# does not exist yet (first deploy) and when it is null (a Fargate stack). +run_case "missing or null output is empty, not an error" "" 1 0 "" +run_case "rejects a non-Function-URL value" "https://evil.example.com/" 0 1 "" +run_case "rejects plain http" "http://abc.lambda-url.us-east-1.on.aws/" 0 1 "" +run_case "rejects a lookalike host suffix" "https://abc.lambda-url.us-east-1.on.aws.evil.example/" 0 1 "" +run_case "rejects a path after the host" "$url/x" 0 1 "" +run_case "rejects two URLs on separate lines" "$url/\n$url/" 0 1 "" + +usage_exit=0 +bash "$SUT" >/dev/null 2>&1 || usage_exit=$? +if [[ "$usage_exit" == 2 ]]; then + echo "PASS: no argument is a usage error" + pass=$((pass + 1)) +else + echo "FAIL: no argument exited $usage_exit, want 2" + fail=$((fail + 1)) +fi + +echo +echo "Results: $pass passed, $fail failed." +[[ "$fail" -eq 0 ]] diff --git a/terraform/environments/aws/compute.tf b/terraform/environments/aws/compute.tf index 92c659b43..27b090787 100644 --- a/terraform/environments/aws/compute.tf +++ b/terraform/environments/aws/compute.tf @@ -23,6 +23,13 @@ # to Function URL return 403 (no way to sign), site is unreachable. locals { lambda_function_url_auth_type = var.enable_cdn ? "AWS_IAM" : "NONE" + + # Function URL CORS origins: the static list from tfvars plus the deployed + # Function URL's own origin, which the workflows read from state (#324). + lambda_cors_origins = distinct(concat( + var.lambda_allowed_origins, + var.lambda_deployed_function_url == "" ? [] : [trimsuffix(var.lambda_deployed_function_url, "/")], + )) } module "compute_lambda" { @@ -62,7 +69,7 @@ module "compute_lambda" { # Function URL function_url_auth_type = local.lambda_function_url_auth_type - allowed_origins = var.lambda_allowed_origins + allowed_origins = local.lambda_cors_origins # Concurrency reserved_concurrent_executions = var.lambda_reserved_concurrency diff --git a/terraform/environments/aws/github-dev.tfvars b/terraform/environments/aws/github-dev.tfvars index c6ae7b485..40ca20970 100644 --- a/terraform/environments/aws/github-dev.tfvars +++ b/terraform/environments/aws/github-dev.tfvars @@ -25,11 +25,12 @@ lambda_log_retention_days = 7 # Function URL auth_type is derived from enable_cdn (local in compute.tf): # enable_cdn = false -> NONE (direct browser hits, app-layer auth) # enable_cdn = true -> AWS_IAM (CloudFront OAC signs every request) -# Current deployed dev origin (Lambda Function URL) + local Webpack dev server. +# Static CORS origins: the local Webpack dev server. The deployed Function +# URL's own origin is not listed here: its host is server-assigned, so the +# deploy workflow reads it from the previous apply and passes it as +# TF_VAR_lambda_deployed_function_url (see variables.tf, #324). # Wildcard is rejected by the module (allow_credentials=true + * = any-origin CSRF). -# Update the Lambda Function URL entry when the dev environment is redeployed. lambda_allowed_origins = [ - "https://33pz7pombdqwu3bdlxp4lqxyra0bsriy.lambda-url.us-east-1.on.aws", "http://localhost:3000", ] diff --git a/terraform/environments/aws/lambda_cors_origin_test.go b/terraform/environments/aws/lambda_cors_origin_test.go new file mode 100644 index 000000000..c60465862 --- /dev/null +++ b/terraform/environments/aws/lambda_cors_origin_test.go @@ -0,0 +1,92 @@ +// Regression tests for issue #324 (audit finding A13c-017): the dev Lambda +// Function URL's hostname was pasted into github-dev.tfvars by hand, so a +// redeploy that reissued the URL left the CORS allowlist pointing at a dead +// origin with no Terraform diff. The origin now comes from the previous +// apply's lambda_function_url output, read by +// scripts/read-lambda-function-url.sh and passed as +// TF_VAR_lambda_deployed_function_url. The merge itself is exercised by +// tests/lambda_cors_origins.tftest.hcl; these tests pin the wiring around it. +package aws_test + +import ( + "regexp" + "strings" + "testing" +) + +var functionURLHostRe = regexp.MustCompile(`[a-z0-9]+\.lambda-url\.[a-z0-9-]+\.on\.aws`) + +func TestTfvarsCarryNoFunctionURLHost(t *testing.T) { + for _, env := range []string{"dev", "staging", "prod"} { + path := "github-" + env + ".tfvars" + src := readFile(t, path) + if host := functionURLHostRe.FindString(src); host != "" { + t.Errorf("%s pins the server-assigned Function URL host %s; the deploy workflow supplies it from state (#324)", path, host) + } + if regexp.MustCompile(`(?m)^\s*lambda_deployed_function_url\s*=`).MatchString(src) { + t.Errorf("%s sets lambda_deployed_function_url; a -var-file value would override the workflow's TF_VAR_ (#324)", path) + } + } +} + +func TestLambdaModuleGetsMergedCORSOrigins(t *testing.T) { + src := readFile(t, "compute.tf") + start := strings.Index(src, `module "compute_lambda" {`) + if start < 0 { + t.Fatal(`compute.tf has no module "compute_lambda"`) + } + end := strings.Index(src[start:], "\n}\n") + if end < 0 { + t.Fatal(`unterminated module "compute_lambda" block`) + } + if !regexp.MustCompile(`(?m)^\s*allowed_origins\s*=\s*local\.lambda_cors_origins\s*$`).MatchString(src[start : start+end]) { + t.Error("module.compute_lambda must take allowed_origins from local.lambda_cors_origins, which adds the deployed Function URL's origin (#324)") + } +} + +// workflowStep returns the text of the step named name in a workflow file, +// from its "- name:" line up to the next step or job. +func workflowStep(t *testing.T, src, name string) string { + t.Helper() + loc := regexp.MustCompile(`(?m)^(\s*)- name: ` + regexp.QuoteMeta(name) + `\s*$`).FindStringSubmatchIndex(src) + if loc == nil { + t.Fatalf("no step named %q", name) + } + indent := src[loc[2]:loc[3]] + rest := src[loc[1]:] + next := regexp.MustCompile(`(?m)^(` + indent + `- name:| [A-Za-z0-9_-]+:\s*$)`).FindStringIndex(rest) + if next == nil { + return src[loc[0]:] + } + return src[loc[0] : loc[1]+next[0]] +} + +func TestLambdaDeployPassesDeployedFunctionURL(t *testing.T) { + src := readFile(t, "../../../.github/workflows/deploy-aws-lambda.yml") + + read := workflowStep(t, src, "Read deployed Function URL") + if !strings.Contains(read, "scripts/read-lambda-function-url.sh terraform/environments/aws") { + t.Errorf("the read step must use scripts/read-lambda-function-url.sh, got:\n%s", read) + } + plan := workflowStep(t, src, "Terraform Plan") + if !strings.Contains(plan, "TF_VAR_lambda_deployed_function_url: ${{ steps.deployed-url.outputs.url }}") { + t.Errorf("Terraform Plan must pass the deployed Function URL as TF_VAR_lambda_deployed_function_url, got:\n%s", plan) + } + if strings.Index(src, "- name: Read deployed Function URL") > strings.Index(src, "- name: Terraform Plan") { + t.Error("the Function URL must be read before Terraform Plan") + } +} + +func TestLambdaRollbackPassesDeployedFunctionURL(t *testing.T) { + src := readFile(t, "../../../.github/workflows/rollback.yml") + job := src[strings.Index(src, "\n rollback-aws-lambda:"):] + job = job[:strings.Index(job[1:], "\n rollback-")+1] + step := workflowStep(t, job, "Rollback with Terraform") + + read := strings.Index(step, "TF_VAR_lambda_deployed_function_url=$(../../../scripts/read-lambda-function-url.sh .)") + export := strings.Index(step, "export TF_VAR_lambda_deployed_function_url") + apply := strings.Index(step, "terraform apply") + if read < 0 || export < 0 || apply < 0 || read > apply || export > apply { + t.Errorf("the Lambda rollback must read and export TF_VAR_lambda_deployed_function_url before terraform apply, or it drops the origin from CORS; got:\n%s", step) + } +} diff --git a/terraform/environments/aws/tests/lambda_cors_origins.tftest.hcl b/terraform/environments/aws/tests/lambda_cors_origins.tftest.hcl new file mode 100644 index 000000000..77ab31feb --- /dev/null +++ b/terraform/environments/aws/tests/lambda_cors_origins.tftest.hcl @@ -0,0 +1,109 @@ +# The Function URL's own origin reaches its CORS allowlist from the previous +# apply's output, not from a host pasted into github-dev.tfvars (#324). +# +# Every provider is mocked, so nothing here reaches AWS. Each run targets +# module.compute_lambda, the allowlist's only consumer, and reads the merged +# list from local.lambda_cors_origins, the value compute.tf passes to the +# module's allowed_origins. +mock_provider "aws" { + mock_data "aws_caller_identity" { + defaults = { account_id = "123456789012" } + } + + # The networking module indexes one subnet per availability zone. + mock_data "aws_availability_zones" { + defaults = { names = ["us-east-1a", "us-east-1b", "us-east-1c"] } + } +} + +mock_provider "random" {} +mock_provider "http" {} +mock_provider "external" {} + +variables { + admin_email = "admin@example.com" + compute_platform = "lambda" + enable_docker_build = false + image_uri = "123456789012.dkr.ecr.us-east-1.amazonaws.com/cudly:test" + lambda_architecture = "arm64" # skips the builder-host probe (data.external) + lambda_allowed_origins = ["http://localhost:3000"] +} + +run "first_deploy_keeps_static_origins" { + command = plan + + plan_options { + target = [module.compute_lambda] + } + + assert { + condition = local.lambda_cors_origins == tolist(["http://localhost:3000"]) + error_message = "With no deployed URL yet, the allowlist must be exactly the tfvars list." + } +} + +run "adds_deployed_function_url_origin" { + command = plan + + plan_options { + target = [module.compute_lambda] + } + + variables { + lambda_deployed_function_url = "https://abcdefghijklmnopqrstuvwxyz012345.lambda-url.us-east-1.on.aws/" + } + + assert { + condition = local.lambda_cors_origins == tolist([ + "http://localhost:3000", + "https://abcdefghijklmnopqrstuvwxyz012345.lambda-url.us-east-1.on.aws", + ]) + error_message = "The deployed Function URL's origin (no trailing slash) must be appended to the tfvars list." + } +} + +run "does_not_duplicate_an_origin_already_listed" { + command = plan + + plan_options { + target = [module.compute_lambda] + } + + variables { + lambda_allowed_origins = ["https://abcdefghijklmnopqrstuvwxyz012345.lambda-url.us-east-1.on.aws"] + lambda_deployed_function_url = "https://abcdefghijklmnopqrstuvwxyz012345.lambda-url.us-east-1.on.aws/" + } + + assert { + condition = local.lambda_cors_origins == tolist(["https://abcdefghijklmnopqrstuvwxyz012345.lambda-url.us-east-1.on.aws"]) + error_message = "An origin present in both inputs must appear once." + } +} + +run "rejects_a_non_function_url" { + command = plan + + plan_options { + target = [module.compute_lambda] + } + + variables { + lambda_deployed_function_url = "https://evil.example.com" + } + + expect_failures = [var.lambda_deployed_function_url] +} + +run "rejects_a_lookalike_host" { + command = plan + + plan_options { + target = [module.compute_lambda] + } + + variables { + lambda_deployed_function_url = "https://abc.lambda-url.us-east-1.on.aws.evil.example" + } + + expect_failures = [var.lambda_deployed_function_url] +} diff --git a/terraform/environments/aws/variables.tf b/terraform/environments/aws/variables.tf index 1367045f8..61e2392c8 100644 --- a/terraform/environments/aws/variables.tf +++ b/terraform/environments/aws/variables.tf @@ -308,6 +308,25 @@ variable "lambda_allowed_origins" { default = [] } +# The Function URL's own origin is not in any tfvars file: its host is +# server-assigned and only known after the first apply, so a hand-pasted copy +# goes stale whenever the URL is reissued (#324). The deploy and rollback +# workflows read it back from the previous apply's lambda_function_url output +# (scripts/read-lambda-function-url.sh) and pass it as +# TF_VAR_lambda_deployed_function_url; compute.tf appends its origin to +# lambda_allowed_origins. A -var-file value would override a TF_VAR_*, which +# is why this is a separate variable that no tfvars file sets. +variable "lambda_deployed_function_url" { + description = "Lambda Function URL from the previous apply (empty on the first deploy). Its origin is added to the Function URL CORS allowlist. Set by the deploy workflows; leave unset in tfvars." + type = string + default = "" + + validation { + condition = var.lambda_deployed_function_url == "" || can(regex("^https://[a-z0-9]+\\.lambda-url\\.[a-z0-9-]+\\.on\\.aws/?$", var.lambda_deployed_function_url)) + error_message = "lambda_deployed_function_url must be empty or a Lambda Function URL (https://.lambda-url..on.aws)." + } +} + variable "lambda_reserved_concurrency" { description = "Reserved concurrent executions (-1 for unreserved). Also the cap on concurrent DB pools (see db_max_connections)." type = number diff --git a/terraform/environments/gcp/compute.tf b/terraform/environments/gcp/compute.tf index 5c29b7bb6..01ba3da45 100644 --- a/terraform/environments/gcp/compute.tf +++ b/terraform/environments/gcp/compute.tf @@ -101,6 +101,9 @@ module "compute_cloud_run" { # Billing account (for billing.viewer IAM binding at account level) billing_account_id = var.billing_account_id + # OIDC signing key: create-time only, see the variable (#324) + signing_key_destroy_scheduled_duration = var.signing_key_destroy_scheduled_duration + # Additional environment variables archera_org_id = var.archera_org_id archera_plan_id = var.archera_plan_id diff --git a/terraform/environments/gcp/variables.tf b/terraform/environments/gcp/variables.tf index 34ff88fec..ca8fdad7b 100644 --- a/terraform/environments/gcp/variables.tf +++ b/terraform/environments/gcp/variables.tf @@ -359,6 +359,12 @@ variable "auto_migrate" { default = true } +variable "signing_key_destroy_scheduled_duration" { + description = "Cloud Run only: how long the OIDC signing key's versions stay restorable (DESTROY_SCHEDULED) before Cloud KMS destroys them, as \"s\", 1 to 120 days. Applied only when the key is created: on a deployed key a different value plans a key replacement, which fails on the key-name collision without touching the live key. The default keeps every existing key unchanged (#324)." + type = string + default = "86400s" +} + # ============================================== # Additional Configuration # ============================================== diff --git a/terraform/modules/compute/gcp/cloud-run/.terraform.lock.hcl b/terraform/modules/compute/gcp/cloud-run/.terraform.lock.hcl index 3acc57c20..a158bd9c1 100644 --- a/terraform/modules/compute/gcp/cloud-run/.terraform.lock.hcl +++ b/terraform/modules/compute/gcp/cloud-run/.terraform.lock.hcl @@ -6,6 +6,7 @@ provider "registry.terraform.io/hashicorp/google" { constraints = "~> 5.0" hashes = [ "h1:iy2Q9VcnMu4z/bH3v/NmI/nEpgYY7bXgJmT/hVTAUS4=", + "h1:k8taQAdfHrv2F/AiGV5BZBZfI+1uaq8g6O8dWzjx42c=", "zh:0d09c8f20b556305192cdbe0efa6d333ceebba963a8ba91f9f1714b5a20c4b7a", "zh:117143fc91be407874568df416b938a6896f94cb873f26bba279cedab646a804", "zh:16ccf77d18dd2c5ef9c0625f9cf546ebdf3213c0a452f432204c69feed55081e", diff --git a/terraform/modules/compute/gcp/cloud-run/signing-key.tf b/terraform/modules/compute/gcp/cloud-run/signing-key.tf index 1f31809c1..4792fbf93 100644 --- a/terraform/modules/compute/gcp/cloud-run/signing-key.tf +++ b/terraform/modules/compute/gcp/cloud-run/signing-key.tf @@ -25,15 +25,30 @@ resource "google_kms_crypto_key" "signing" { name = "cudly-oidc-signing" key_ring = google_kms_key_ring.signing.id purpose = "ASYMMETRIC_SIGN" - destroy_scheduled_duration = "86400s" # 1 day — tests redeploy often + destroy_scheduled_duration = var.signing_key_destroy_scheduled_duration version_template { algorithm = "EC_SIGN_P256_SHA256" protection_level = "SOFTWARE" } + # Matches the AWS twin (modules/compute/aws/lambda/signing-key.tf, cli#1480): + # a change that forces a new key (purpose, the algorithm, the duration + # above, or the key ring's location via var.region) must not destroy the + # live signing key first. Destroying a crypto key schedules every version + # for destruction, which stops AsymmetricSign at once, so the default + # destroy-then-create order breaks client-assertion JWT minting for every + # federated target cloud. Create-first either builds the new key ring and + # key before the old key goes (a region move), or, because Cloud KMS never + # frees a key name, fails on the name collision before the old key is + # touched (any in-place replacement). Either way the live key keeps signing + # until there is a working replacement (#324). + # + # There is deliberately no prevent_destroy. Terraform only accepts a literal + # there, so it cannot vary per environment, and cleanup-staging.yml runs + # `terraform destroy` on staging, which prevent_destroy = true would fail. lifecycle { - prevent_destroy = false + create_before_destroy = true } } diff --git a/terraform/modules/compute/gcp/cloud-run/tests/signing_key.tftest.hcl b/terraform/modules/compute/gcp/cloud-run/tests/signing_key.tftest.hcl new file mode 100644 index 000000000..7f137fe58 --- /dev/null +++ b/terraform/modules/compute/gcp/cloud-run/tests/signing_key.tftest.hcl @@ -0,0 +1,105 @@ +# OIDC signing key retention (#324). destroy_scheduled_duration is a +# create-time-only (ForceNew) attribute, so the default must stay the value +# every deployed key was created with, and an out-of-range value must fail at +# plan time rather than at the Cloud KMS API. Every provider is mocked; nothing +# here reaches GCP. create_before_destroy is asserted on the source by +# TestGCPSigningKeyCreatesBeforeDestroy, since terraform test cannot see +# lifecycle settings. +mock_provider "google" {} + +variables { + project_id = "cudly-test-project" + service_name = "cudly-test" + environment = "dev" + region = "us-central1" + image_uri = "us-central1-docker.pkg.dev/cudly-test-project/cudly/cudly:test" + database_host = "10.0.0.5" + database_name = "cudly" + database_username = "cudly" + database_password_secret_id = "cudly-test-db-password" + admin_email = "admin@example.com" +} + +run "default_keeps_deployed_keys_unchanged" { + command = plan + + plan_options { + target = [google_kms_crypto_key.signing] + } + + assert { + condition = google_kms_crypto_key.signing.destroy_scheduled_duration == "86400s" + error_message = "The default must stay 86400s: every deployed key was created with it, and a different value forces the key's replacement." + } +} + +run "accepts_longer_window_for_new_keys" { + command = plan + + plan_options { + target = [google_kms_crypto_key.signing] + } + + variables { + signing_key_destroy_scheduled_duration = "2592000s" + } + + assert { + condition = google_kms_crypto_key.signing.destroy_scheduled_duration == "2592000s" + error_message = "The input must reach the crypto key." + } +} + +run "accepts_upper_bound" { + command = plan + + plan_options { + target = [google_kms_crypto_key.signing] + } + + variables { + signing_key_destroy_scheduled_duration = "10368000s" + } +} + +run "rejects_below_one_day" { + command = plan + + plan_options { + target = [google_kms_crypto_key.signing] + } + + variables { + signing_key_destroy_scheduled_duration = "86399s" + } + + expect_failures = [var.signing_key_destroy_scheduled_duration] +} + +run "rejects_above_120_days" { + command = plan + + plan_options { + target = [google_kms_crypto_key.signing] + } + + variables { + signing_key_destroy_scheduled_duration = "10368001s" + } + + expect_failures = [var.signing_key_destroy_scheduled_duration] +} + +run "rejects_non_seconds_format" { + command = plan + + plan_options { + target = [google_kms_crypto_key.signing] + } + + variables { + signing_key_destroy_scheduled_duration = "30d" + } + + expect_failures = [var.signing_key_destroy_scheduled_duration] +} diff --git a/terraform/modules/compute/gcp/cloud-run/variables.tf b/terraform/modules/compute/gcp/cloud-run/variables.tf index 065fb5af9..45e6a6ef3 100644 --- a/terraform/modules/compute/gcp/cloud-run/variables.tf +++ b/terraform/modules/compute/gcp/cloud-run/variables.tf @@ -275,3 +275,24 @@ variable "archera_api_key_secret_id" { type = string default = "" } + +# Cloud KMS only accepts destroy_scheduled_duration when a key is created: +# the provider marks it ForceNew, so changing it on a deployed key plans a +# replacement of the OIDC signing key. With create_before_destroy that apply +# fails on the key-name collision before the live key is touched (see +# signing-key.tf), so set a longer window only for a project whose key does +# not exist yet. The default keeps every deployed key unchanged (#324). +variable "signing_key_destroy_scheduled_duration" { + description = "How long the OIDC signing key's versions stay in DESTROY_SCHEDULED (restorable) before Cloud KMS destroys them, as \"s\". Between 1 day (86400s) and 120 days (10368000s). Only takes effect when the key is created; see the comment above." + type = string + default = "86400s" + + validation { + condition = ( + can(regex("^[0-9]+s$", var.signing_key_destroy_scheduled_duration)) && + try(tonumber(trimsuffix(var.signing_key_destroy_scheduled_duration, "s")) >= 86400, false) && + try(tonumber(trimsuffix(var.signing_key_destroy_scheduled_duration, "s")) <= 10368000, false) + ) + error_message = "signing_key_destroy_scheduled_duration must be a whole number of seconds with an \"s\" suffix, between \"86400s\" (1 day) and \"10368000s\" (120 days), the range Cloud KMS accepts." + } +} diff --git a/terraform/modules/compute/gcp/signing_key_lifecycle_test.go b/terraform/modules/compute/gcp/signing_key_lifecycle_test.go new file mode 100644 index 000000000..9ed900ca9 --- /dev/null +++ b/terraform/modules/compute/gcp/signing_key_lifecycle_test.go @@ -0,0 +1,41 @@ +// Regression test for issue #324 (audit finding A13c-024): the Cloud Run +// module's OIDC signing key had an explicit `prevent_destroy = false` and no +// create_before_destroy, so a change forcing a new key destroyed the live +// signing key first (scheduling its versions for destruction stops +// AsymmetricSign at once) and broke client-assertion JWT minting. Its AWS twin +// in modules/compute/aws/lambda/signing-key.tf already creates first (cli#1480). +// terraform test cannot observe lifecycle settings, so this reads the source; +// the retention input is covered by cloud-run/tests/signing_key.tftest.hcl. +package gcp_test + +import ( + "regexp" + "testing" +) + +func TestGCPSigningKeyCreatesBeforeDestroy(t *testing.T) { + src := codeLines(readTF(t, "cloud-run/signing-key.tf")) + key := probeBlock(t, src, `resource\s+"google_kms_crypto_key"\s+"signing"`) + lifecycle := probeBlock(t, key, `lifecycle`) + + if !regexp.MustCompile(`(?m)^\s*create_before_destroy\s*=\s*true\s*$`).MatchString(lifecycle) { + t.Errorf("google_kms_crypto_key.signing must set lifecycle { create_before_destroy = true }, got:\n%s", lifecycle) + } + if regexp.MustCompile(`(?m)^\s*prevent_destroy\s*=\s*false\s*$`).MatchString(lifecycle) { + t.Error("google_kms_crypto_key.signing restates prevent_destroy = false, Terraform's default; drop it (see the comment in signing-key.tf)") + } + if !regexp.MustCompile(`(?m)^\s*destroy_scheduled_duration\s*=\s*var\.signing_key_destroy_scheduled_duration\s*$`).MatchString(key) { + t.Error("google_kms_crypto_key.signing must take destroy_scheduled_duration from var.signing_key_destroy_scheduled_duration") + } +} + +// TestAWSSigningKeyCreatesBeforeDestroy pins the twin the GCP key is matched +// against, so the two cannot drift apart again from the other side. +func TestAWSSigningKeyCreatesBeforeDestroy(t *testing.T) { + src := codeLines(readTF(t, "../aws/lambda/signing-key.tf")) + key := probeBlock(t, src, `resource\s+"aws_kms_key"\s+"signing"`) + lifecycle := probeBlock(t, key, `lifecycle`) + if !regexp.MustCompile(`(?m)^\s*create_before_destroy\s*=\s*true\s*$`).MatchString(lifecycle) { + t.Errorf("aws_kms_key.signing must keep lifecycle { create_before_destroy = true }, got:\n%s", lifecycle) + } +} From 3ab67298a33729929ec3de5ac810655a9bda29c2 Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Fri, 23 Oct 2026 11:07:32 +0000 Subject: [PATCH 2/5] test(iac): allowlist the mocked caller-identity account ID in the CORS tftest The combined-tree git-secrets run flagged terraform/environments/aws/tests/lambda_cors_origins.tftest.hcl:10, a bare account_id = "123456789012" in the mocked aws_caller_identity. Add an anchored whole-line .gitallowed entry for that one line (bracketed digits so the entry does not match itself), placed before the other terraform/ entries so it does not collide with #1255's entry for its own tftest. Refs #324 Co-Authored-By: Claude Opus 5.5 --- .gitallowed | 1 + 1 file changed, 1 insertion(+) diff --git a/.gitallowed b/.gitallowed index c1aec0906..767808ce2 100644 --- a/.gitallowed +++ b/.gitallowed @@ -89,6 +89,7 @@ ^internal/database/postgres/migrations/migrate_security_test\.go:[0-9]+:[[:space:]]*rawDSN[[:space:]]+:=[[:space:]]+fmt\.Sprintf\("postgre[s]://user:pass@localhost:5432/db\?sslmode=%s",[[:space:]]+mode\)[[:space:]]*$ ^internal/database/postgres/migrations/migrate_security_test\.go:[0-9]+:[[:space:]]*poolCfg,[[:space:]]+err[[:space:]]+:=[[:space:]]+pgxpool\.ParseConfig\("postgre[s]://user:pass@localhost:5432/db\?sslmode=require"\)[[:space:]]*$ ^internal/database/postgres/migrations/migrate_security_test\.go:[0-9]+:[[:space:]]*cfg,[[:space:]]+err[[:space:]]+:=[[:space:]]+pgxpool\.ParseConfig\("postgre[s]://user:pass@127\.0\.0\.1:1/db\?sslmode=disable"\)[[:space:]]*$ +^terraform/environments/aws/tests/lambda_cors_origins\.tftest\.hcl:[0-9]+:[[:space:]]*defaults[[:space:]]+=[[:space:]]+\{[[:space:]]+account_id[[:space:]]+=[[:space:]]+"123[4]567[8]901[2]"[[:space:]]+\}[[:space:]]*$ ^terraform/environments/(aws|azure|gcp)/dev\.tfvars\.example:[0-9]+:[[:space:]]*#[[:space:]]+admin_passwor[d][[:space:]]+=[[:space:]]+"YourSecurePassword123!"[[:space:]]*$ ^terraform/modules/secrets/azure/main\.tf:[0-9]+:[[:space:]]*value[[:space:]]+=[[:space:]]+var\.smtp_passwor[d][[:space:]]+!=[[:space:]]+null[[:space:]]+\?[[:space:]]+var\.smtp_passwor[d][[:space:]]+:[[:space:]]+"PLACEHOLDER_GENERATE_IN_AZURE_PORTAL"[[:space:]]*$ ^terraform/modules/secrets/gcp/main\.tf:[0-9]+:[[:space:]]*secret_data[[:space:]]+=[[:space:]]+var\.sendgrid_api_ke[y][[:space:]]+!=[[:space:]]+null[[:space:]]+\?[[:space:]]+var\.sendgrid_api_ke[y][[:space:]]+:[[:space:]]+"PLACEHOLDER_REPLACE_ME"[[:space:]]*$ From 277dbb5f2cf1a65d2f254eaf496bc56ea577cece Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Mon, 26 Oct 2026 12:28:28 +0000 Subject: [PATCH 3/5] fix(scripts): drop OIDCIssuerHost from the standalone IaC generator #1256 added OIDCIssuerHost to iacData because aws-cfn-deploy.sh.tmpl passed it at the time. This branch removes the parameter from the CloudFormation template and the deploy script (the host is derived from OIDCIssuerURL), so the field is unused again and the drift test that compares iacData with federationIaCData fails until it goes. The bundle test now checks that the rendered deploy script passes OIDCIssuerURL and never OIDCIssuerHost, since CloudFormation rejects an override for an undeclared parameter. The field walker's control uses OIDCIssuerURL instead. Co-Authored-By: Claude Opus 5.5 --- scripts/generate-federation-iac.go | 7 +------ scripts/generate_federation_iac_drift_test.go | 20 +++++++++++-------- 2 files changed, 13 insertions(+), 14 deletions(-) diff --git a/scripts/generate-federation-iac.go b/scripts/generate-federation-iac.go index aab02e98e..55e0044ee 100644 --- a/scripts/generate-federation-iac.go +++ b/scripts/generate-federation-iac.go @@ -128,11 +128,7 @@ type iacData struct { SourceAccountID string // AWS WIF / cross-account OIDCIssuerURL string - // OIDCIssuerHost is OIDCIssuerURL without its https:// prefix, the form - // IAM uses in condition keys; aws-cfn-deploy.sh.tmpl passes it to - // CloudFormation. Derived the same way as the server does. - OIDCIssuerHost string - OIDCAudience string + OIDCAudience string // OIDCSubjectClaim restricts the AWS trust policy to a single workload // subject. Empty unless --oidc-subject-claim is passed; every AWS-WIF // template requires it (no working subject-less default, see #1640). @@ -673,7 +669,6 @@ func populateData(data *iacData, target, source, tenantID, projectID, saEmail, o switch target { case "aws": data.OIDCIssuerURL = awsOIDCIssuer(source, tenantID) - data.OIDCIssuerHost = strings.TrimPrefix(data.OIDCIssuerURL, "https://") data.OIDCAudience = awsOIDCAudience(source) data.OIDCSubjectClaim = oidcSubjectClaim if err := requireSourceAccountID(data, source, sourceAccountID, diff --git a/scripts/generate_federation_iac_drift_test.go b/scripts/generate_federation_iac_drift_test.go index 96f456f60..ac2c5ca91 100644 --- a/scripts/generate_federation_iac_drift_test.go +++ b/scripts/generate_federation_iac_drift_test.go @@ -4,7 +4,7 @@ package main // as the server's GET /api/federation/iac, from its own copy of the template // data struct (iacData, a copy of internal/api's federationIaCData). A comment // asking people to keep the two in sync did not: iacData went without -// OIDCIssuerHost, which aws-cfn-deploy.sh.tmpl references, so +// OIDCIssuerHost, which aws-cfn-deploy.sh.tmpl then referenced, so // `--format bundle` failed for every AWS target with a non-AWS source. // // The script carries //go:build ignore and internal/api is a large package, so @@ -160,7 +160,7 @@ func TestIaCData_CoversEveryTemplateField(t *testing.T) { // tests above: a parser that found no fields would make both pass vacuously. func TestIaCData_FieldWalkerSeesKnownFields(t *testing.T) { refs := iacTemplateFieldRefs(t) - for _, field := range []string{"AccountName", "CUDlyAPIURL", "OIDCIssuerHost", "SourceAccountID"} { + for _, field := range []string{"AccountName", "CUDlyAPIURL", "OIDCIssuerURL", "SourceAccountID"} { require.Contains(t, refs, field) } require.Contains(t, structFieldNames(t, scriptFile, "iacData"), "AccountName") @@ -168,15 +168,18 @@ func TestIaCData_FieldWalkerSeesKnownFields(t *testing.T) { // TestGenerator_AWSWIFBundleRendersDeployScript runs the real // aws-cfn-deploy.sh.tmpl through --format bundle for each non-AWS source. It -// only reads the rendered script; running it would call the AWS CLI. +// only reads the rendered script; running it would call the AWS CLI. The +// template no longer declares OIDCIssuerHost (it derives the condition-key +// host from OIDCIssuerURL, #324), and CloudFormation rejects an override for +// an undeclared parameter, so the script must not pass it. func TestGenerator_AWSWIFBundleRendersDeployScript(t *testing.T) { const tenant = "aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee" cases := []struct { - source, wantHost string - extra []string + source, wantURL string + extra []string }{ - {"gcp", "accounts.google.com", nil}, - {"azure", "login.microsoftonline.com/" + tenant + "/v2.0", []string{"--tenant-id", tenant}}, + {"gcp", "https://accounts.google.com", nil}, + {"azure", "https://login.microsoftonline.com/" + tenant + "/v2.0", []string{"--tenant-id", tenant}}, } for _, tc := range cases { t.Run(tc.source, func(t *testing.T) { @@ -189,7 +192,8 @@ func TestGenerator_AWSWIFBundleRendersDeployScript(t *testing.T) { res := runGenerator(t, args...) require.Zero(t, res.exitCode, "stderr: %s", res.stderr) script := readZipEntry(t, bundle, "cloudformation/deploy-cfn.sh") - require.Contains(t, script, `"OIDCIssuerHost=`+tc.wantHost+`"`) + require.Contains(t, script, `"OIDCIssuerURL=`+tc.wantURL+`"`) + require.NotContains(t, script, "OIDCIssuerHost") }) } } From 2b39b249a753bac44e026aef9cb6e52cf68990d6 Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Mon, 26 Oct 2026 12:28:47 +0000 Subject: [PATCH 4/5] docs(iac): note the key ring order and the URL reader's failure mode Review follow-ups on #1312. create_before_destroy on the signing key also applies to the key ring it depends on, and the Function URL reader treats any terraform output failure as "no URL". Both are intended, but neither was written down where the code is. Co-Authored-By: Claude Opus 5.5 --- scripts/read-lambda-function-url.sh | 5 ++++- terraform/modules/compute/gcp/cloud-run/signing-key.tf | 5 ++++- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/scripts/read-lambda-function-url.sh b/scripts/read-lambda-function-url.sh index 6f7a30716..98f411d8f 100755 --- a/scripts/read-lambda-function-url.sh +++ b/scripts/read-lambda-function-url.sh @@ -29,7 +29,10 @@ fi # `terraform output -raw` fails when the output is missing (no state yet) or # null, and both mean "no deployed URL". hashicorp/setup-terraform's wrapper # adds ::debug:: lines to stdout, which are dropped the same way the deploy -# workflow's "Get Terraform outputs" step drops them. +# workflow's "Get Terraform outputs" step drops them. `|| true` also turns +# any other `terraform output` failure, such as a transient state read +# error, into "no URL". The plan then drops the Function URL origin, and the +# deploy's post-apply CORS check warns that the live URL is not allowed. raw=$(terraform -chdir="$1" output -raw lambda_function_url 2>/dev/null | grep -v '::' || true) if [[ -z "$raw" ]]; then diff --git a/terraform/modules/compute/gcp/cloud-run/signing-key.tf b/terraform/modules/compute/gcp/cloud-run/signing-key.tf index 4792fbf93..255852b9b 100644 --- a/terraform/modules/compute/gcp/cloud-run/signing-key.tf +++ b/terraform/modules/compute/gcp/cloud-run/signing-key.tf @@ -42,7 +42,10 @@ resource "google_kms_crypto_key" "signing" { # key before the old key goes (a region move), or, because Cloud KMS never # frees a key name, fails on the name collision before the old key is # touched (any in-place replacement). Either way the live key keeps signing - # until there is a working replacement (#324). + # until there is a working replacement (#324). Terraform also applies + # create_before_destroy to google_kms_key_ring.signing, which this key + # depends on, so a region move creates the new ring first as well. That is + # the intended order. # # There is deliberately no prevent_destroy. Terraform only accepts a literal # there, so it cannot vary per environment, and cleanup-staging.yml runs From f5365ea4272f659f4f0b93bdb9f344a8e8c17c8a Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Mon, 26 Oct 2026 12:33:18 +0000 Subject: [PATCH 5/5] test(api): pass t to federationHandler in the CFN bundle test #1223 on main made federationHandler take *testing.T so it can register AssertExpectations on its mock. This branch's CFN bundle parameter test still called it with no arguments, so internal/api stopped compiling after the merge. Co-Authored-By: Claude Opus 5.5 --- internal/api/handler_federation_cfn_params_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/api/handler_federation_cfn_params_test.go b/internal/api/handler_federation_cfn_params_test.go index e231cd3af..b91074976 100644 --- a/internal/api/handler_federation_cfn_params_test.go +++ b/internal/api/handler_federation_cfn_params_test.go @@ -25,7 +25,7 @@ var cfnOverrideRe = regexp.MustCompile(`(?m)^\s+"([A-Za-z][A-Za-z0-9]*)=`) // download. func cfnBundleFiles(t *testing.T, source string) (template []byte, paramsKeys, scriptKeys []string) { t.Helper() - res, err := federationHandler().getFederationIaC(context.Background(), federationReq(map[string]string{ + res, err := federationHandler(t).getFederationIaC(context.Background(), federationReq(map[string]string{ "target": "aws", "source": source, "format": "cfn", })) require.NoError(t, err)