Repository navigation
Conversation
…is on With compute_platform = "fargate" and enable_cdn = true, CloudFront fronts the ALB (frontend.tf), but the ALB security group still admitted HTTP/HTTPS from 0.0.0.0/0 and ::/0, so clients could call the ALB directly and bypass the distribution and any WAF on it. enable_cdn only gated the Lambda Function URL's auth type. - modules/networking/aws: new restrict_alb_ingress_to_cloudfront (default false). When set, the ALB group's only ingress is tcp/80 from the com.amazonaws.global.cloudfront.origin-facing managed prefix list (the frontend module reaches the Fargate ALB over http-only; one rule because a prefix-list rule's weight leaves no room for a second under the default quota). With it off, the four internet-facing rules are unchanged. New output alb_ingress_cloudfront_only, derived from the group's inline ingress rules. - environments/aws: pass compute_platform == "fargate" && enable_cdn, and add the non-blocking check "fargate_cdn_requires_cloudfront_only_alb". Every committed tfvars sets enable_cdn = false, so no current stack plans a change. - ci-cd-permissions: grant ec2:DescribeManagedPrefixLists and ec2:GetManagedPrefixListEntries for the prefix-list lookup. - Tests: terraform test for the module and the environment (mock providers), Go guard for the wiring and check condition, CI step running both with -lockfile=readonly, lock files for linux_amd64/darwin_arm64/ darwin_amd64, and an anchored .gitallowed entry for the mock account id. Closes LeanerCloud#412 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 |
|
Heads-up on merge order: #1090 (fck-nat persistent ENI, #716) also edits |
|
VERDICT: REQUEST_CHANGES 8ba439d Base: merge-base 8caed39, current origin/main b6a0649. One commit, 440+/35- = 475 changed lines (cap 400: non-blocking process nit). Findings
Checked and fine
Local results (Terraform 1.10.5, /opt/homebrew/bin/terraform, merged tree with conflicts resolved as both-sides)
Attacked:
|
|
Independent review 2 (claude-fleet, cold): REQUEST_CHANGES 8ba439d The fix itself is right for #412. Both blocking items come from main having moved. Findings:
Against #412:
Attacked:
Verified locally, with Terraform 1.10.5 (the CI pin) and no cloud credentials, under the heavy lock:
Reviewed by an independent agent with no author context (second reviewer). Review only: nothing was pushed, and the GitHub review state is unchanged. |
Resolve three additive conflicts by keeping both sides: the LeanerCloud#295 OIDC-thumbprint entry in .gitallowed, the LeanerCloud#714 "Test monitoring module" CI step, and the LeanerCloud#718 lambda_concurrency_bounds_db_connections check in terraform/environments/aws/checks.tf. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…oud#718 and pin the ALB guard's inputs - fargate_cdn_alb.tftest.hcl: set lambda_reserved_concurrency = 20 in the lambda run, so main's lambda_concurrency_bounds_db_connections check (LeanerCloud#718) stays quiet; its default of -1 failed the run on the merged tree. - TestAWSALBIngressOutputPredicatePinned: pin each per-rule clause of the networking module's alb_ingress_cloudfront_only output, which no tftest can drive false. - TestDeployPolicyAllowsManagedPrefixListLookup: pin the two read-only EC2 actions the prefix-list lookup needs in policy_networking.tf. - policy_networking.tf: move ec2:GetManagedPrefixListEntries out of the alphabetized Describe* run. Refs LeanerCloud#412 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Addressing both reviews of 8ba439d (VERDICT REQUEST_CHANGES, and Independent review 2). New head: bac8cff. I merged Blocking
Non-blocking
PR body: I replaced the "Known gap" paragraph with the two new guards and added a note on the post-merge #718 interaction. Verification on the merged tree (bac8cff)
Nothing ran against real AWS. The PR stays a draft. |
Root cause
enable_cdn = trueonly changed the Lambda path: it flips the Function URL toAWS_IAMbehind CloudFront OAC (compute.tf). On the Fargate path,frontend.tfalready puts CloudFront in front of the ALB (the origin is the ALB, or itsapi-<env>alias, overhttp-only). Butmodule.networking's ALB security group always admitted HTTP/HTTPS from0.0.0.0/0and::/0, whateverenable_cdnwas set to. So withcompute_platform = "fargate"andenable_cdn = true, clients could call the internet-facing ALB directly and skip CloudFront, along with any WAF attached to it. Theprod_requires_network_authenticated_ingresscheck from #401/#128 only looks at the Lambda auth type, and it had nothing to check for Fargate.Fix
modules/networking/awsrestrict_alb_ingress_to_cloudfront(bool, defaultfalse).com.amazonaws.global.cloudfront.origin-facingmanaged prefix list (data.aws_ec2_managed_prefix_list).origin_protocol = "http-only".alb_ingress_cloudfront_only. It is computed from the group's inline ingress rules, not just echoed from the variable.environments/aws/networking.tf: passesrestrict_alb_ingress_to_cloudfront = var.compute_platform == "fargate" && var.enable_cdn. This follows the same pattern aslocal.lambda_function_url_auth_type, which is also derived fromenable_cdn.environments/aws/checks.tf: adds a new non-blockingcheck "fargate_cdn_requires_cloudfront_only_alb"(following sec(iac/gcp): prod Cloud Run is unauthenticated and internet-facing, Cloud Armor sits unattached #128's convention). It asserts!(fargate && enable_cdn) || module.networking.alb_ingress_cloudfront_only.ci-cd-permissions/policy_networking.tf: grantsec2:DescribeManagedPrefixListsandec2:GetManagedPrefixListEntries, which the prefix-list lookup needs. Without them, the first Fargate+CDN plan would fail with AccessDenied. Both are read-only, and the lookup only runs whenfargate && enable_cdn.ci.yml, aws matrix): adds a new step, "Test AWS ALB ingress guard". It runsinit -lockfile=readonly,fmt -checkandterraform testformodules/networking/awsandenvironments/aws. Both lock files now include hashes for linux_amd64, darwin_arm64 and darwin_amd64. Provider versions are unchanged. The env lock'sconstraintsstrings were also refreshed to match the currentrequired_providers..gitallowed: adds one entry, anchored on path, line and the whole line, for the syntheticaccount_id = "123456789012"in the env test'saws_caller_identitymock. The provider rejects a boundary ARN whose account is not 12 digits. On Linux, git-secrets flags this line without the entry (verified below).Live-infrastructure impact (owner please confirm)
github-{dev,staging,prod}.tfvars,dev.tfvars.example) setsenable_cdn = false.deploy-aws-fargate.yml/destroy-fargate-dev.ymlonly overridecompute_platform=fargate, and the variable's default isfalse. With the restriction off, the planned ALB security group is identical toorigin/main(mock-plan comparison below). I did not run a realterraform plan.enable_cdn = trueon a Fargate stack. That is exactly what this issue asks for, but whoever flips it should know three things, and these are owner decisions:deploy-aws-fargate.yml's post-deploy gates andmodule.deployment_checkscall the ALB /api-<env>alias directly. With the restriction on, those calls will stop connecting.deployment_checksreports this as warnings (on_failure = continue/ check blocks). The workflow's test-deployment gate would need to target the CloudFront URL. The Lambda+CDN path has the same property today, because the Function URL becomesAWS_IAM.enable_httpsis on, the ALB's port-80 listener redirects to 443, but CloudFront talks to the Fargate originhttp-only. Fargate + CDN + HTTPS therefore already redirects clients to the ALB itself, and the ALB is now unreachable from them. The real fix is anhttps-onlyorigin for Fargate when the ALB has a certificate, plus a 443 prefix-list rule instead of 80. I left that out of scope.ci-cd-permissionsstack is applied by a human. The two read-only EC2 actions take effect only after that stack is re-applied, and they are only needed once a Fargate stack enables the CDN.Verification
All of this is unit/mock/fixture evidence. Nothing ran against a real AWS account, and there was no real
terraform plan/apply.Module test (
terraform/modules/networking/aws/tests/alb_ingress.tftest.hcl, mock provider,command = plan):0.0.0.0/0/::/0), no prefix-list lookup, outputfalse.com.amazonaws.global.cloudfront.origin-facing, there is exactly one rule (tcp/80, prefix list only, no CIDRs), the output istrue, and egress is unchanged.false.Environment test (
terraform/environments/aws/tests/fargate_cdn_alb.tftest.hcl): mocks all four providers, overrides every module exceptmodule.networking, and plans the real environment root.terraform testrun.Go guard (
TestAWSFargateCDNGuardPresentinprod_exposure_guard_test.go): pins the exact check condition and thenetworking.tfwiring. No mock plan can drive the environment into the failing state, so this test is what guards the condition.Fail-before / pass-after:
go test ./terraform/environments/aws/ -run TestAWSwithorigin/main'schecks.tf+networking.tf:FAIL: TestAWSFargateCDNGuardPresent ... checks.tf missing check "fargate_cdn_requires_cloudfront_only_alb" (#412). Passes after the fix.origin/maincopy of the tree (git archive): the module test fails withdata resource "aws_ec2_managed_prefix_list" "cloudfront_origin_facing" has not been declared, and the env test withmodule.networking ... does not have an attribute named "alb_ingress_cloudfront_only". Both pass after the fix.restrict_alb_ingress_to_cloudfront = false: the run fails withCheck block assertion failed ... check "fargate_cdn_requires_cloudfront_only_alb", plus the assertion.TestAWSALBIngressOutputPredicatePinned(prod_exposure_guard_test.go) pins each clause instead; dropping the IPv4 CIDR clause, replacing it withtrue, dropping theselfclause or making the security-group clausetrueeach fail it. Rules attached from outside the resource (aws_vpc_security_group_ingress_rule) are still not visible to the output.TestDeployPolicyAllowsManagedPrefixListLookup(ci-cd-permissions/policy_guard_test.go) fails ifpolicy_networking.tfstops allowing eitherec2:DescribeManagedPrefixListsorec2:GetManagedPrefixListEntrieswhile the module still declares the lookup (each removal probed).No-diff proof for current stacks: I mock-planned
aws_security_group.alb(ingress, egress, description, name_prefix) with the restriction off, from both theorigin/mainmodule and this branch's module. Comparing the JSON dumps from the two gaveIDENTICAL.After merging
origin/main(c8bdd46): main's #718 checklambda_concurrency_bounds_db_connectionsfires for Lambda with the defaultlambda_reserved_concurrency = -1, so the env test's Lambda run now setslambda_reserved_concurrency = 20(20 x 3 fits 80 - 10). Without it the run fails withCheck block assertion failed(2/3); with it 3/3, on macOS (1.15.8) and on Linux inhashicorp/terraform:1.10.5.Commands run:
The Go tests ran with
AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null, because this machine's default AWS profile is invalid. Not run locally: tflint (not installed), zizmor.Out of scope
http-onlyorigin vs. HTTPS-redirect mismatch (item 2 above).deploy-aws-fargate.yml's post-deploy gates at the CloudFront URL when the CDN is on (item 1 above).enable_cdncutover (sec(iac): flip dev/staging/prod to AWS_IAM + enable CloudFront OAC (runtime cut-over for #424) #16).Closes #412
Maintainers: please mirror the issue's triage labels onto this PR:
priority/p2,severity/medium,triaged,urgency/this-quarter,impact/few,effort/m,type/security.🤖 Generated with Claude Code