Repository navigation
Conversation
…rst, 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 LeanerCloud#324 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…S 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 LeanerCloud#1255's entry for its own tftest. Refs LeanerCloud#324 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: true
Comment |
Bring the branch up to date with main. Conflicted files: - .github/workflows/ci.yml: this branch appended a "Run Function URL reader self-tests" step to the aws-tfstate-platform-key job; main (LeanerCloud#1112) added a new rollback-ecr-registry job right after that job. Kept both: the step stays at the end of aws-tfstate-platform-key and main's job follows it unchanged. rollback.yml merged cleanly: main's rewritten "Verify image exists" steps and this branch's TF_VAR_lambda_deployed_function_url export touch different steps. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Merged origin/main (2374b4e) into this branch. New head: cdf6a69. It still merges cleanly with the current main (bfb4625). Conflict resolution:
Checks on the merged head:
|
|
Independent review (IAmJSD's agent, fresh context, given only the diff, the issue and the PR text): VERDICT: REQUEST_CHANGES Blocking:
Reviewer: rE3. Bar while the maintainers are away (set by IAmJSD as acting owner): one approving independent review on the head, two for money, auth and security paths. |
Two textual conflicts, both resolved by keeping both sides: - ci.yml: main added an init-backend.sh self-test step to the same job this branch appends the Function URL reader self-tests to, plus a new tfstate-concurrency-group job after it. Both steps stay in the job, and main's job follows. - cloud-run/.terraform.lock.hcl: main's "~> 5.45" constraint with the union of both sides' h1: hashes for google 5.45.2. LeanerCloud#1256 (merged on main) added OIDCIssuerHost to the standalone generator; the next commit drops it again to match this branch. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
LeanerCloud#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 <noreply@anthropic.com>
Review follow-ups on LeanerCloud#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 <noreply@anthropic.com>
LeanerCloud#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 <noreply@anthropic.com>
Picks up LeanerCloud#1467, which also edits scripts/generate-federation-iac.go, so the generator tests are re-run on the merged tree. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Pushed 2d2705b (head 2d2705b), answering rE3's review of cdf6a69: Blocking: A13b-014 lands in a template CloudFormation can't load
Blocking: conflict with main
Non-blocking (
Body-stale: the "possible CloudFormation problem" bullet is replaced by the verified result and #1489. The customer-stacks note is marked moot until #1489. The #153 note now describes #1256 and Checks on 2d2705b
The new head needs a fresh review. |
|
Independent review (IAmJSD's agent, fresh context, given only the diff, the issue and the PR text): VERDICT: APPROVE Blocking:
Non-blocking:
Attacked:
Body-stale:
Reviewer: rE6. Bar while the maintainers are away (set by IAmJSD as acting owner): one approving independent review on the head, two for money, auth and security paths. |
Fixes two of the three low IaC findings from the 2026-09-02 audit that #324 groups together (A13c-024 and A13c-017), and makes the parameter change for the third (A13b-014). A13b-014 is not finished here: the AWS-target CloudFormation template it lands in can't be loaded by CloudFormation, on
mainas well, and that is now tracked in #1489. So this PR isRefs #324andRefs #1489, notCloses. I re-checked each finding against currentmain(a14dc8f2) before changing anything.OIDCIssuerHostparametertemplate.yamlstill declared it.handler_federation.goandaws-cfn-deploy.sh.tmplstill passed itOIDCIssuerURL, and the server stops passing it. Covered by #1489, not closed here: the template still can't be loaded (cfn-lintE0000, same as on main), and the loadable rewrite is #1489GET /api/federation/iac, so the AWS Lambda app deploy picks it up. CUDly instances hosted on GCP or Azure only pick it up once the deploy pause (in place since 2026-10-20) lifts. Customer stacks change only when a customer re-downloads the bundle and runs itsigning-key.tfstill hadprevent_destroy = false, a hard-coded86400sand nocreate_before_destroycreate_before_destroy = true.destroy_scheduled_durationbecomes an input whose default leaves deployed keys unchangeddeploy-gcp.yml(Cloud Run). That workflow is paused since 2026-10-20, so this only takes effect after the pause lifts. Expected plan diff: none (a lifecycle change, and the default equals today's value)github-dev.tfvars33pz…lambda-url.us-east-1.on.awswas still there. #16 (the CloudFront cut-over) is parked: the maintainer deferred it and the issue is unclaimed, so the issue's "fix here only if #16 stays parked" condition holdslambda_function_urloutput and passed asTF_VAR_lambda_deployed_function_urldeploy-aws-lambda.yml, which runs on its own when this merges (it touchesterraform/environments/aws/**and the workflow).rollback.ymldoes the same for Lambda rollbacks. Expected CORS diff on dev: none, as long as the URL in state is still the one that was pastedRoot cause and fix
A13b-014: the CloudFormation issuer host (covered by #1489)
Status: the change below is kept, but it does not close A13b-014.
cfn-lint1.58.0 rejects the template at this head (E0000 Unhashable type "{'Fn::Sub': ['${Host}:aud', ...]}", line 268) and onmain(E0000 Unhashable type "{'Fn::Sub': '${OIDCIssuerHost}:aud'}", line 272): the trust policy uses intrinsic functions as map keys, which CloudFormation does not accept. So the aws-target bundle can't create a stack before or after this PR. RebuildingAssumeRolePolicyDocumentas one!SubJSON string, the stricterAllowedPatterns that needs, and cfn-lint in CI are #1489. A13b-014 is complete once #1489 lands.iac/aws_target_cfn_test.gochecks the derivation with the repo's own evaluator, which accepts the mapping keys CloudFormation rejects, so it is not evidence that the template loads.iac/federation/aws-target/cloudformation/template.yamltook the trust policy's condition-key host as a second free-text parameter. Its own description said the operator had to keep it equal toOIDCIssuerURLminushttps://. If the two differ (a trailing slash, or a typo in the tenant ID), IAM never fills in<host>:aud/<host>:sub. The stack still reports success, and everyAssumeRoleWithWebIdentityis then denied.!Sub ["${Host}:aud|sub", {Host: !Select [1, !Split ["https://", !Ref OIDCIssuerURL]]}].OIDCIssuerURL's existingAllowedPattern(^https://[^/]+(/[^/]+)*$) guarantees exactly one leadinghttps://and no trailing slash. So element 1 is the whole host and path, the same value aslocal.oidc_condition_hostin the Terraform sibling.OIDCIssuerHostfrombuildCFParamsJSON, fromaws-cfn-deploy.sh.tmpl's--parameter-overrides, and fromfederationIaCData, where nothing else used it.A13c-024: the GCP OIDC signing key
terraform/modules/compute/gcp/cloud-run/signing-key.tfhad neither create-first ordering nor a working protection.create_before_destroy = true, matchingmodules/compute/aws/lambda/signing-key.tf(cli#1480). Destroying a Cloud KMS key schedules all of its versions for destruction, which stopsAsymmetricSignat once. So a change that forces a new key used to break client-assertion minting immediately. With create-first, one of two things happens:Either way the live key keeps signing.
destroy_scheduled_durationis now an input (signing_key_destroy_scheduled_duration, passed through fromterraform/environments/gcp). It is validated to the 1–120 day range Cloud KMS accepts. I checked the provider source: in google provider v5.45.2 (the version in the lock files) this attribute isForceNew. A new default would therefore plan a replacement of every deployed key, so the default stays86400s. A longer window only applies to a project whose key doesn't exist yet. I did not change any tfvars.prevent_destroycannot be an input. Terraform only accepts a literal for lifecycle meta-arguments. Setting it totruewould breakcleanup-staging.yml, which runsterraform destroyon staging. The line that restated Terraform's default (prevent_destroy = false) is replaced with a comment explaining this. Thedeletion_policy = "PREVENT"field that would allow per-environment protection only exists from provider v7. This deviates from the issue's suggested fix, which asked for both values as module inputs.A13c-017: the dev Function URL CORS origin
The Function URL ID is assigned by AWS. So
lambda_allowed_originsingithub-dev.tfvarscarried a pasted copy that went stale whenever the URL was reissued, with no Terraform diff.lambda_deployed_function_url, with validation^https://[a-z0-9]+\.lambda-url\.[a-z0-9-]+\.on\.aws/?$.compute.tfappends its origin (trailing slash removed, de-duplicated) tolambda_allowed_originsfor the Lambda module only. It is a separate variable rather thanTF_VAR_lambda_allowed_originsas the issue suggested, because a-var-filevalue overrides aTF_VAR_*. Sincegithub-dev.tfvarskeeps its staticlocalhost:3000entry, the issue's version would have been silently ignored.scripts/read-lambda-function-url.shreadsterraform output -raw lambda_function_urlfrom the already-initialized directory:$GITHUB_OUTPUT.deploy-aws-lambda.ymlruns the script before the plan and passes the value toterraform plan. A new step after the apply emits a::warning::when the apply created or reissued the URL: the allowlist only gains the new origin on the next deploy.rollback.yml's Lambda job exports the same variable before its directterraform apply. Without that, a rollback would drop the origin.github-dev.tfvarsnow lists onlyhttp://localhost:3000. Staging and prod keep their*.invalidplaceholders. Once deployed they will also get their own Function URL origin, which is the origin the SPA is served from.Refs #324: this PR finishes A13c-024 and A13c-017. #324 also lists A13b-014, which needs #1489 to be complete, so #324 should be closed when #1489 lands (or closed by the owner pointing at #1489), not by this merge.
Refs #1489
Verification
All of the evidence below is unit, mock or fixture evidence. Nothing ran against a real cloud account: no plan, apply or deploy, and no real credentials. Go runs used
AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null AWS_EC2_METADATA_DISABLED=true, and Terraform usedTF_CLI_CONFIG_FILE=/dev/null.Fail-before / pass-after. I put
main's versions of the implementation files back into the tree, kept the new tests, ran everything below, then restored the fix.go test ./iac/ -run 'TestAWSTargetCFN_|TestCFNEval_'OIDCIssuerHostis still a parameter, and the condition keys can't be evaluated fromOIDCIssuerURLalonego test ./internal/api/ -run TestCFNBundle_OIDCIssuerHostgo test ./terraform/modules/compute/gcp/ -run SigningKeycreate_before_destroy, a restatedprevent_destroy = false, a hard-coded durationgo test ./terraform/environments/aws/ -run 'FunctionURL|CORS'github-dev.tfvarspins33pz…lambda-url.us-east-1.on.aws, nothing is wired into the module, deploy or rollbackterraform testinterraform/modules/compute/gcp/cloud-run(mockedgoogle)terraform testinterraform/environments/aws(all providers mocked, targetmodule.compute_lambda)bash scripts/test-read-lambda-function-url.sh(stubbedterraform)What the new tests cover:
iac/aws_target_cfn_test.goparses the shipped template withyaml.v3. It evaluates eachStringEqualskey with a small!Ref/!Select/!Split/!Subevaluator for a Google and an Azure issuer URL, and compares the result with Terraform'strimsuffix(trimprefix(url, "https://"), "/").TestCFNEval_Positiveis the control, so the test cannot pass by evaluating nothing.TestCFNBundle_OverridesMatchTemplateParameterschecks every key in the downloaded bundle's params JSON and deploy script against the template the same bundle ships. Every passed key must be declared, and every parameter without a default must be passed.Other checks, all on this branch:
go build ./...,go vet ./...,go vet -tags integration ./...: clean.go test -count=1with no integration tag, one package at a time:./iac/,./internal/api/,./internal/iacfiles/,./scripts/,./terraform/environments/aws/,./terraform/environments/gcp/,./terraform/modules/compute/gcp/,./terraform/modules/compute/aws/,./internal/mocks/. All ok.golangci-lintv2.10.1run --allow-parallel-runnerson the touched Go packages: 0 issues.gocyclo -over 10oninternal/api/handler_federation.go: clean (that file only has removals).shellcheckon both new scripts andactionlintonci.yml,deploy-aws-lambda.ymlandrollback.yml: clean.terraform fmt -check -recursive terraform/: clean.terraform/modules/compute/gcp/cloud-run:init -backend=false -lockfile=readonlyandvalidate.terraform/environments/awsandterraform/environments/gcp:init -backend=falseandvalidate. Their lock files were restored afterwards, unchanged.bash scripts/check-aws-iam-parity.sh(ok) andbash scripts/test-aws-iam-parity.sh(16 passed).cfn-lint1.58.0 (venv) oniac/federation/aws-target/cloudformation/template.yaml:E0000at this head and onmain, as described under A13b-014 and tracked in fix(iac/federation): AWS-target CloudFormation template can't be loaded (intrinsic functions used as map keys) #1489.bash scripts/test-aws-tfstate-platform-key.sh: 26 passed.CI wiring added:
gcp) that runsterraform/modules/compute/gcp/*/tests. For it to work with-lockfile=readonlyon Ubuntu runners,cloud-run/.terraform.lock.hclgains thelinux_amd64h1:hash for the same google 5.45.2 package; without it, readonly init fails the checksum check. That hash was added withterraform providers lock -platform=linux_amd64.aws-tfstate-platform-keyjob.terraform/environments/aws/tests/has no CI runner onmainyet. The open fix(iac): require PostgreSQL 15+ in every environment's version input #1233 branch adds "Test environment inputs", which runsterraform testinterraform/environments/<cloud>whenever atestsdir exists, and that picks this file up. The Go tests interraform/environments/awsrun undergo test ./...in the meantime.Combined-tree verification (historical). The table below is from an earlier head; the branch has since been merged with
main, and the latest PR comment lists the current head and the checks run on it. This branch was tested together with the other queued branches on a local, never-pushed tree:queue/platform@b56afb2b= origin/maina14dc8f2plus these heads, merged in this order:fix/issue-324andfix/issue-204both edit.github/workflows/ci.ymlnext to theazure-kv-access-policyjob. The tree kept #324's new "Run Function URL reader self-tests" step and #204's updated job comment; whichever PR merges second takes that same resolution.Linux, Go 1.26.9,
AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null AWS_EC2_METADATA_DISABLED=true; Docker Postgres via testcontainers. The full suite ran oned0c7174, which differs fromb56afb2bonly by one.gitallowedline on fix/issue-324:go build ./...,go vet ./...,go vet -tags integration ./...and the fullgo test -race -short -count=1 ./....--build-tags integration.gocyclo -over 10is silent on the changed non-test files.-tags integration -count=1, 60-minute local timeout):internal/api,internal/purchase,internal/schedulerandinternal/serverall pass..gitallowedline. Re-run atb56afb2b: git-secrets, using the repo'sscripts/setup-git-secrets.shpatterns in a fresh repo with an isolated HOME, found nothing in the changed files, and a planted fake key was flagged.scripts/test-git-secrets-allowlist.sh: 137 PASS, 0 FAIL.Out of scope / for the owner
maintoo, so no stack can have been created from this template. The fix is fix(iac/federation): AWS-target CloudFormation template can't be loaded (intrinsic functions used as map keys) #1489: buildAssumeRolePolicyDocumentas a whole!SubJSON string, which needsOIDCAudience,OIDCSubjectClaimandOIDCIssuerURLto forbid"and\so those values can't break out of the JSON string.deploy-cfn.shor params JSON fails with "Parameters: [OIDCIssuerHost] do not exist in the template". Re-downloading the bundle gives a matching pair. The condition-key strings are unchanged, so the role itself doesn't change.cleanup-staging.yml'sterraform destroy, the KMS key ring and key names stay taken. So a fresh staging apply against the same project can't recreatecudly-oidc-signing. This predates this PR and is not touched.OIDCIssuerHosttoscripts/generate-federation-iac.go'siacData, with drift tests that assumed the field and the deploy-script argument. The merge ofmaininto this branch drops the field again (277dbb5f): the bundle test now asserts the rendered deploy script passesOIDCIssuerURLand neverOIDCIssuerHost, and the field walker's control usesOIDCIssuerURL.Maintainer: please mirror the issue's triage labels on this PR:
impact/internal,type/chore,triaged,priority/p3,severity/low,urgency/eventually,effort/s.🤖 Generated with Claude Code