Skip to content

fix(iac/aws): deny path-bearing cudly-* IAM ARNs in deploy policy and boundary - #997

Merged
cristim merged 2 commits into
LeanerCloud:mainfrom
IAmJSD:fix/issue-197
Oct 16, 2026
Merged

cristim merged 2 commits into
LeanerCloud:mainfrom
IAmJSD:fix/issue-197

Conversation

@IAmJSD

@IAmJSD IAmJSD commented Oct 16, 2026

Copy link
Copy Markdown
Collaborator

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). So arn:aws:iam::*:role/cudly-* is satisfied by arn: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 the policy/cudly-* and instance-profile/cudly-* scopes in IAMRolesAndPolicies.

As the issue says, this was contained, not live: iam:CreateRole is gated on the boundary (#1705/#1722), so a path-bearing role the deploy role creates is itself boundaried. The open case was a cudly-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 /: no aws_iam_role/aws_iam_policy/aws_iam_instance_profile sets path, and names cannot contain /. A genuine cudly-* ARN therefore never has a second / after the prefix, so the impostor shape can be stated exactly as cudly-*/* 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:

Sid    = "IAMDenyPathBearingCudlyResources"
Effect = "Deny"
Action = ["iam:*"]
Resource = [
  "arn:aws:iam::*:role/cudly-*/*",
  "arn:aws:iam::*:policy/cudly-*/*",
  "arn:aws:iam::*:instance-profile/cudly-*/*",
]

It denies iam:*, not a copy of today's Allow lists, so a cudly-* Allow added later can't reopen the bypass.

policy_boundary.tf (cudly-deploy-boundary, the workload ceiling). New statement covering PassRoleCeiling:

Sid      = "DenyPassPathBearingRole"
Effect   = "Deny"
Action   = ["iam:PassRole"]
Resource = ["arn:aws:iam::*:role/cudly-*/*"]

Before, both documents had only the cudly-* Allows, plus the exact-ARN Denies on cudly-terraform-deploy. The existing Allows are unchanged.

Migration impact

  • Nothing is renamed or recreated. Every existing cudly-* role, policy and instance profile is at path / and matches neither Deny. TestCudlyIAMResourcesUseRootPath now fails CI if anyone sets a non-root path.
  • Only ci-cd-permissions changes. 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.
  • After the apply, the deploy role and the workload roles can no longer act on a path-bearing 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".
  • Policy sizes stay well under the 6144-char managed-policy limit (approx., whitespace and comments stripped: cudly-deploy-iam about 1.3 KB, cudly-deploy-boundary about 1.6 KB).

Out of scope

  • CrossAccountAssumeRoleCeiling (role/CUDly*) and the runtime cross_account_sts policies in modules/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.
  • The federation templates (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.

  • New policy_path_guard_test.go:
    • TestIAMARNMatchesCrossesPathSeparator pins the ARN matcher, including the *-crosses-/ property.
    • TestDeployPolicyDeniesPathBearingCudlyARNs and TestBoundaryDeniesPassingPathBearingRoles check both directions. Every IAM action a cudly--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 a cudly- Allow.
    • TestCudlyIAMResourcesUseRootPath asserts no IAM role, policy or instance profile on the deploy path or in the bootstrap root sets path other than "/", and fails if it finds zero resources of any type.
  • Fail before / pass after: with the two .tf changes reverted, TestDeployPolicyDeniesPathBearingCudlyARNs and TestBoundaryDeniesPassingPathBearingRoles fail. With them, the package passes.
  • Mutations, each caught:
    • Deny widened to role/cudly-* (refuses the real roles)
    • Deny action narrowed to iam:PassRole
    • policy/cudly-*/* dropped
    • boundary Deny made conditional
    • path = "/cudly-x/" added to the fck-nat role
  • AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null go test ./terraform/environments/aws/ci-cd-permissions/ ./terraform/modules/compute/aws/... -count=1: ok
  • bash scripts/check-aws-iam-parity.sh: OK; bash scripts/test-aws-iam-parity.sh: 12 passed, 0 failed
  • terraform fmt -check: clean; terraform init -backend=false && terraform validate (on a temp copy): valid
  • golangci-lint v2.10.1 on the package: 0 issues; go vet: ok
  • git-secrets scan of the changed files (temp repo, patterns from scripts/setup-git-secrets.sh): clean; scripts/test-git-secrets-allowlist.sh: 42 PASS, 0 FAIL

🤖 Generated with Claude Code

… 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>
@IAmJSD IAmJSD added priority/p2 Backlog-worthy severity/medium Moderate harm urgency/this-sprint Within the current sprint labels Oct 16, 2026
@coderabbitai

coderabbitai Bot commented Oct 16, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Note

Currently processing new changes in this PR. This may take a few minutes, please wait...

⚙️ Run configuration
  • Configuration used: Repository: LeanerCloud/cloud-commitments-platform/.coderabbit.yaml
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: e4b9f18b-433c-45d8-bff2-3b4a50ab36a4
📥 Commits

Reviewing files that changed from the base of the PR and between 8aae08d and 1130c48.

📒 Files selected for processing: 3
  • terraform/environments/aws/ci-cd-permissions/policy_boundary.tf
  • terraform/environments/aws/ci-cd-permissions/policy_iam.tf
  • terraform/environments/aws/ci-cd-permissions/policy_path_guard_test.go
+----------------------------------------------------------------------+
| If it “compiles on my machine”, I will make your machine my machine. |
+----------------------------------------------------------------------+
  \
   \   (\__/)
       (•ㅅ•)
       /   づ
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Comment @coderabbitai help to get the list of available commands.

@IAmJSD IAmJSD added impact/internal Team-internal only effort/m Days type/security Security finding triaged Item has been triaged labels Oct 16, 2026

@cristim cristim left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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/ or instance-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 test in 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 -recursive clean; terraform init -backend=false and terraform validate succeed. 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-roles for /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:

  1. policy_iam.tf IAMDenyPathBearingCudlyResources (deploy role).
  2. policy_boundary.tf DenyPassPathBearingRole (workload ceiling).
  3. policy_path_guard_test.go (matcher, two Deny guards, root-path scan).
  4. 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>
@IAmJSD

IAmJSD commented Oct 16, 2026

Copy link
Copy Markdown
Collaborator Author

Fixed the reference at 1130c48, a comment-only commit. The IAMDenyPathBearingCudlyResources comment in policy_iam.tf now points at policy_path_guard_test.go, where TestDeployPolicyDeniesPathBearingCudlyARNs lives (line 212). terraform fmt -check is clean. I checked the other policy_guard_test.go references in both files, and they're correct.

I left your second note as is, since it needs no change: pathed roles such as role/lambda/... were never matched by the cudly-* Allows, and still aren't.

🤖 Generated with Claude Code

@cristim cristim left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 to cudly-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.

@IAmJSD

IAmJSD commented Oct 16, 2026

Copy link
Copy Markdown
Collaborator Author

@cristim Lint Code at 1130c48 is red because of an infrastructure flake, not a lint finding. golangci-lint's config validation timed out downloading golangci.v2.10.jsonschema.json (context deadline exceeded), and the lint run itself reported 0 issues. It's the same schema-download flake as on #940. Could you re-run that job?

🤖 Generated with Claude Code

@cristim

cristim commented Oct 16, 2026

Copy link
Copy Markdown
Member

Gate record for 1130c48 (origin/main CLAUDE.md "Review gate"):

  • Independent review of e9970f0: no blocking findings.
    • IAM ARN * crosses /, so path-bearing cudly-* ARNs were a real bypass.
    • The new Deny matches only <type>/cudly-*/*. It covers iam:* on role, policy and instance-profile, and the boundary Deny covers PassRole.
    • No existing resource has a path (cudly-terraform-deploy has none, and no module sets path), so the deploy role's current capabilities are unchanged.
    • Policy size stays well under 6144 chars.
    • Nit: the comment in policy_iam.tf names policy_guard_test.go, but the test lives in policy_path_guard_test.go.
  • Independent delta review of 1130c48: no actionable findings. The one-line comment fix resolves the nit; no HCL or test change.
  • CI on 1130c48: CI Success and Run pre-commit hooks pass. The first Lint Code run failed only because golangci-lint could not fetch its jsonschema (infra, 0 issues); the failed job was rerun and passed.
  • Local verification (macOS, static):
    • The Go tests parse the real .tf files. 8 mutation probes each fail them.
    • terraform fmt -check and terraform validate pass.
    • No plan or apply against real AWS.
  • Operator note: this is the manually applied ci-cd-permissions bootstrap module. The guard takes effect only after a privileged human re-applies terraform/environments/aws/ci-cd-permissions.
  • CR skipped: draft and quota-limited; covered by the independent review plus local verification.
  • Labels mirrored from sec(iac/aws): role-prefix ARN scopes are satisfiable via the IAM path (arn:aws:iam::*:role/cudly-*) #197.

Marking ready (owner-authorized) and merging at 1130c48 with --match-head-commit.

@cristim
cristim marked this pull request as ready for review October 16, 2026 16:29
@cristim
cristim merged commit eb1739a into LeanerCloud:main Oct 16, 2026
44 of 46 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/m Days impact/internal Team-internal only priority/p2 Backlog-worthy severity/medium Moderate harm triaged Item has been triaged type/security Security finding urgency/this-sprint Within the current sprint

Projects

None yet

Development

Successfully merging this pull request may close these issues.

sec(iac/aws): role-prefix ARN scopes are satisfiable via the IAM path (arn:aws:iam::*:role/cudly-*)

2 participants