Repository navigation
fix(iac/aws): deny path-bearing cudly-* IAM ARNs in deploy policy and boundary - #997
Conversation
… boundary In an IAM resource ARN `*` matches `/`, and the role/policy/instance-profile path is part of the ARN and chosen by its creator, so the cudly-* prefix scopes (role/cudly-*, policy/cudly-*, instance-profile/cudly-*) were also satisfied by e.g. role/cudly-x/EvilRole. - policy_iam.tf: add IAMDenyPathBearingCudlyResources, an unconditioned Deny of iam:* on role|policy|instance-profile/cudly-*/*. - policy_boundary.tf: add DenyPassPathBearingRole, an unconditioned Deny of iam:PassRole on role/cudly-*/*, covering PassRoleCeiling. - Every IAM resource this repo creates sits at path "/", so cudly-*/* matches no genuine resource; no rename or recreation is needed. - policy_path_guard_test.go: both-direction guards (impostor refused, genuine root-path ARNs still granted and not denied), a pinned IAM ARN matcher, and a check that no IAM role/policy/instance profile sets a non-root path. Closes LeanerCloud#197 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configuration
📒 Files selected for processing: 3
✨ Finishing Touches
Comment |
cristim
left a comment
There was a problem hiding this comment.
Independent review of e9970f0: draft: blocks ready
Findings:
- NON-BLOCKING: policy_iam.tf (new IAMDenyPathBearingCudlyResources comment) says TestDeployPolicyDeniesPathBearingCudlyARNs lives in policy_guard_test.go. It is in policy_path_guard_test.go:212. Fix the reference.
- NON-BLOCKING (informational, no change needed): the Deny only reaches resources whose ARN has the cudly- prefix right after
role/,policy/orinstance-profile/. A pathed role named cudly-* (for example the CloudFormation stack role at Path /lambda/, ARN role/lambda/...) was never matched by the cudly-* Allows and still is not. No behavior change there.
Does it change what the deploy role can do today? No. The two Denies match only ARNs of the form /cudly-/, which no existing resource has:
- cudly-terraform-deploy has no path (role.tf), so its ARN has no second
/after cudly-. - No aws_iam_role, aws_iam_policy or aws_iam_instance_profile in the deployed module set sets
path. TestCudlyIAMResourcesUseRootPath scans them (it also fails if the scan finds zero resources) and I broke it with a path on the lambda role: it failed. - Policy size: stripped of comments, policy_iam.tf grows 1223 to 1414 chars and policy_boundary.tf 1662 to 1772, far below the 6144 limit.
- Deploy flow: operator must re-apply this bootstrap root for the guard to go live. Until then nothing changes. Hand-made cudly-x/... resources become unreachable from the deploy role after the apply; Terraform does not manage them.
IAM semantics: * in an IAM resource ARN matches any run of characters including /, so role/cudly-* is satisfied by role/cudly-x/Evil (the bypass is real), while role/cudly-*/* needs a second / and so matches no path-less cudly-* role. The Deny is iam:* on role, policy and instance-profile, so it covers CreateRole, PutRolePolicy, AttachRolePolicy, PassRole, CreatePolicy, CreatePolicyVersion, CreateInstanceProfile and any later action. The boundary Deny is PassRole only on the role shape, which matches PassRoleCeiling (the only boundary Allow that uses role/cudly-*). Both Denies are unconditioned.
What I attacked:
- Go tests parse the real .tf files (findStatements/readPolicySource on policy_*.tf, plus the module walk), not copies.
go testin the ci-cd-permissions package passes. - Mutation probes, each reverted (3 files checked clean at the end except one I reverted explicitly): iam Deny to Allow (fails TestDeployPolicyDeniesPathBearingCudlyARNs); drop the policy resource (fails); drop the instance-profile resource (fails); over-deny
role/cudly-*(fails, the genuine-ARN half); narrow Action to iam:CreateRole (fails); boundary Deny to Allow (fails the new test and TestBoundaryScopesPassRoleToCudlyRoles); boundary resource changed to cudly-*/x (fails); set path on a module role (fails). No survivors. terraform fmt -check -recursiveclean;terraform init -backend=falseandterraform validatesucceed. No plan or apply was run.- The CrossAccountAssumeRoleCeiling exclusion is argued correctly: the targets are in customer accounts and a path Deny narrows nothing there.
Gate status (origin/main Review gate):
- Independent review: this comment names the SHA; the PR is still a draft.
- CI on the SHA: several jobs pending at review time (Unit Tests, Lint Code, Validate Terraform, Integration Tests); green is required before merge. CodeRabbit not pinged.
- Local verification: mutation probes plus validate above, macOS. Real-account verification (applying the bootstrap and listing
aws iam list-rolesfor /cudly- paths) is a human step and is not covered. - Labels mirror issue #197 (priority/p2, severity/medium, urgency/this-sprint, impact/internal, triaged, effort/m, type/security).
Blocks ready:
- policy_iam.tf IAMDenyPathBearingCudlyResources (deploy role).
- policy_boundary.tf DenyPassPathBearingRole (workload ceiling).
- policy_path_guard_test.go (matcher, two Deny guards, root-path scan).
- Fix the test-file reference in the policy_iam.tf comment before ready.
Addresses review note on LeanerCloud#997. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Fixed the reference at 1130c48, a comment-only commit. The I left your second note as is, since it needs no change: pathed roles such as 🤖 Generated with Claude Code |
cristim
left a comment
There was a problem hiding this comment.
Independent delta review of 1130c48 (previous: e9970f0): no actionable findings
Delta: OLD_SHA is an ancestor of NEW_SHA; one new commit (1130c48) changes one comment line in terraform/environments/aws/ci-cd-permissions/policy_iam.tf:207, naming policy_path_guard_test.go instead of policy_guard_test.go. No HCL, no IAM statement, no test changed.
Previous findings: the comment-name nit is RESOLVED (policy_iam.tf:207 now matches the real file name). No blocking findings existed.
Deploy role capabilities: unchanged versus OLD_SHA, since the delta is comment-only. Versus origin/main the PR only adds Deny statements (IAMDenyPathBearingCudlyResources on role, policy and instance-profile cudly-*/*, and DenyPassPathBearingRole in the boundary). It grants nothing new.
What I attacked at NEW_SHA (detached worktree, removed afterward):
- go test in the ci-cd-permissions module passes.
- terraform fmt -check -recursive passes; terraform init -backend=false and terraform validate succeed. No plan or apply was run.
- Mutation probes, each reverted: narrowing the deploy Deny to
cudly-x/*fails TestDeployPolicyDeniesPathBearingCudlyARNs; changing the boundary Deny tocudly-zz/*fails TestBoundaryDeniesPassingPathBearingRoles; flipping the deploy Deny to Allow fails TestDeployPolicyDeniesPathBearingCudlyARNs. - A merge of NEW_SHA with origin/main is conflict-free. The PR diff is 3 files, +343/-0.
Gate status: CI on NEW_SHA is still running (checks pending, no action_required run). The PR is still a draft with mergeStateStatus BLOCKED. Triage labels are present (priority/p2, severity/medium, urgency/this-sprint, impact/internal, effort/m, type/security, triaged). The PR touches the manually applied bootstrap module, so re-applying it is a human step. Real-account verification was not done; the Go tests and Terraform validate stand in for it.
|
@cristim Lint Code at 1130c48 is red because of an infrastructure flake, not a lint finding. golangci-lint's config validation timed out downloading 🤖 Generated with Claude Code |
|
Gate record for 1130c48 (origin/main CLAUDE.md "Review gate"):
Marking ready (owner-authorized) and merging at 1130c48 with --match-head-commit. |
Summary
Closes #197.
The bypass (confirmed). In an IAM resource ARN,
*matches any run of characters,/included, and the IAM path is part of the ARN (IAM identifiers: ARNs and paths; Resource element wildcards). Soarn:aws:iam::*:role/cudly-*is satisfied byarn:aws:iam::123456789012:role/cudly-x/EvilRole, a role whose path is/cudly-x/and whose name is anything. Whoever creates the role chooses the path. (The reverse shape,role/evil/cudly-x, never matched.) The same applies to thepolicy/cudly-*andinstance-profile/cudly-*scopes inIAMRolesAndPolicies.As the issue says, this was contained, not live:
iam:CreateRoleis gated on the boundary (#1705/#1722), so a path-bearing role the deploy role creates is itself boundaried. The open case was acudly-x/...role made out of band. This PR adds an independent guard so the containment no longer rests on that one control.Fix
All IAM roles, policies and instance profiles this repo creates sit at the default path
/: noaws_iam_role/aws_iam_policy/aws_iam_instance_profilesetspath, and names cannot contain/. A genuinecudly-*ARN therefore never has a second/after the prefix, so the impostor shape can be stated exactly ascudly-*/*and denied without touching real resources. This is fix option 1/2 from the issue: the expected path (root) is now checked, not just the prefix.policy_iam.tf(cudly-deploy-iam, on the deploy role). New statement:It denies
iam:*, not a copy of today's Allow lists, so acudly-*Allow added later can't reopen the bypass.policy_boundary.tf(cudly-deploy-boundary, the workload ceiling). New statement coveringPassRoleCeiling:Before, both documents had only the
cudly-*Allows, plus the exact-ARN Denies oncudly-terraform-deploy. The existing Allows are unchanged.Migration impact
cudly-*role, policy and instance profile is at path/and matches neither Deny.TestCudlyIAMResourcesUseRootPathnow fails CI if anyone sets a non-rootpath.ci-cd-permissionschanges. It is the manually applied bootstrap root (see CLAUDE.md, "CI/CD IAM: bootstrap vs runtime split"), so an operator must re-apply it before the guard is live. Until then, deployed accounts behave as before.cudly-*/...IAM resource. If one exists in an account (hand-made), Terraform does not manage it and the deploy path never touches it. Operators may want to list them first:aws iam list-roles --query "Roles[?starts_with(Path, '/cudly-')].Arn".cudly-deploy-iamabout 1.3 KB,cudly-deploy-boundaryabout 1.6 KB).Out of scope
CrossAccountAssumeRoleCeiling(role/CUDly*) and the runtimecross_account_stspolicies inmodules/compute/aws/{lambda,fargate}are left as they are, with a comment saying why. Their targets live in customer accounts whose owner picks the name as freely as the path, so a path guard narrows nothing. The real gap there is the missing per-account condition (chore(deps): Bump @babel/core from 7.29.7 to 8.0.9 in /frontend #1636). A path Deny could also break customers whose target role sits under a path.iac/federation/**,internal/iacfiles) and the CFN hub stack create roles in other trust contexts that these deploy policies don't govern. No action lists changed, so CFN/TF action parity is unaffected.Verification
Static evidence only. Nothing was applied, planned against a real account, or deployed.
policy_path_guard_test.go:TestIAMARNMatchesCrossesPathSeparatorpins the ARN matcher, including the*-crosses-/property.TestDeployPolicyDeniesPathBearingCudlyARNsandTestBoundaryDeniesPassingPathBearingRolescheck both directions. Every IAM action acudly--scoped Allow grants on an impostor (role/cudly-x/EvilRole,role/cudly-a/b/c/EvilRole,policy/cudly-x/...,instance-profile/cudly-x/...) must be covered by an unconditioned Deny. Those Denies must not match genuine root-path ARNs, which must still be granted by acudly-Allow.TestCudlyIAMResourcesUseRootPathasserts no IAM role, policy or instance profile on the deploy path or in the bootstrap root setspathother than"/", and fails if it finds zero resources of any type..tfchanges reverted,TestDeployPolicyDeniesPathBearingCudlyARNsandTestBoundaryDeniesPassingPathBearingRolesfail. With them, the package passes.role/cudly-*(refuses the real roles)iam:PassRolepolicy/cudly-*/*droppedpath = "/cudly-x/"added to the fck-nat roleAWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null go test ./terraform/environments/aws/ci-cd-permissions/ ./terraform/modules/compute/aws/... -count=1: okbash scripts/check-aws-iam-parity.sh: OK;bash scripts/test-aws-iam-parity.sh: 12 passed, 0 failedterraform fmt -check: clean;terraform init -backend=false && terraform validate(on a temp copy): validgo vet: okscripts/setup-git-secrets.sh): clean;scripts/test-git-secrets-allowlist.sh: 42 PASS, 0 FAIL🤖 Generated with Claude Code