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:]]*$ 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) + } +}