Repository navigation
sec(iac/aws): gate deploy-role IAM writes on a permissions boundary - #1722
Conversation
The cudly-terraform-deploy role held iam:CreateRole, iam:AttachRolePolicy and iam:PutRolePolicy on arn:aws:iam::*:role/cudly-* with no iam:PermissionsBoundary condition and no iam:PolicyARN condition. It could therefore create a role, attach AdministratorAccess to it, and either assume it (same-account, so a trust policy naming the deploy role suffices) or pass it to Lambda under the existing IAMPassRoleScopedByService grant: full account administrator from a GitHub Actions OIDC token, persisting past rotation of that token. Denying the three actions is not an option. The deploy path legitimately creates roles on every apply, so a blanket Deny 403s every deployment, and a name-based Deny cannot help because the escalation target is a role and any name the deploy role may legitimately use is one it may also abuse. What separates a legitimate role from an escalation vehicle is its ceiling, and a permissions boundary is the only AWS mechanism that caps a principal regardless of its identity policy. It is also the only thing that closes iam:PutRolePolicy, whose inline document supports no condition key at all. Every role in terraform/modules now carries cudly-deploy-boundary, and the three actions plus iam:PutRolePermissionsBoundary are re-granted in a new cudly-deploy-iam policy only under StringEquals on iam:PermissionsBoundary, so a role without the ceiling can be neither created nor written to. iam:AttachRolePolicy additionally carries an ArnNotEquals Deny allowlisting the four AWS managed policies the modules actually attach, which is what makes AdministratorAccess unattachable rather than merely capped. Stripping a boundary is denied outright, and the deploy role cannot boundary itself. iam:UpdateAssumeRolePolicy, iam:UpdateRole and iam:UpdateRoleDescription are deliberately left unconditioned even though they accept the key: the provider's resourceRoleUpdate issues all three BEFORE the boundary call in the same function, so conditioning them would 403 the first apply against a role that does not carry the boundary yet. The boundary is a ceiling, not a grant, so it removes no permission any role holds today: its allow list is derived from the IAM policy documents in terraform/modules plus the four attached AWS managed policies. It is per-service rather than per-action because a too-narrow boundary fails at runtime rather than at apply time. organizations and sts are the two exceptions, pinned to the exact actions and role prefix the modules use: organizations:* reaches CreateAccount and SCPs, and sts:AssumeRole on "*" escapes the boundary entirely, since the assumed session is a different principal. policy_guard_test.go turns the remaining drift into a CI failure rather than a production 403: it re-derives the service set, the attached managed-policy ARNs and the cross-account role prefix from the modules, asserts every module role is boundaried and that none is declared outside them, and asserts the structure of both new statements so the condition operators, the Deny effect and the wildcard-free ARN list cannot be weakened silently. Closes #1705
cristim
added
triaged
Adversarial review — independent, at
|
| Command | Result |
|---|---|
terraform fmt -recursive -check terraform/ |
exit 0 |
terraform init -backend=false && terraform validate in ci-cd-permissions |
Success, exit 0 |
terraform init -backend=false && terraform validate in environments/aws |
Success, exit 0 |
go test -v -count=1 ./terraform/environments/aws/ci-cd-permissions |
9/9 PASS, exit 0 |
| 10 independent mutations, each reverted (below) | 10/10 caught |
curl servicereference.us-east-1.amazonaws.com/v1/iam/iam.json + per-action condition-key extraction |
see §1 |
| Rendered-JSON size computation from the statement structures | see L2 |
Mutation testing (the four you cite, plus six I designed)
| # | Mutation | Caught by |
|---|---|---|
| M1 | StringEquals → StringEqualsIfExists |
TestDeployPolicyGatesRoleMutationOnBoundary |
| M2 | ArnNotEquals → ArnEquals |
TestDeployPolicyDeniesUnapprovedManagedPolicyAttachment |
| M3 | add arn:aws:iam::aws:policy/* to the allowlist |
same |
| M4 | that statement's Effect → Allow |
same |
| M5 | drop permissions_boundary from the Lambda role |
TestEveryModuleRoleHasPermissionsBoundary |
| M6 | rename cudly-deploy-boundary → cudly-workload-boundary |
TestBoundaryPolicyNameStaysInProtectedNamespace |
| M7 | re-add iam:CreateRole to the unconditioned IAMRolesAndPolicies |
TestBoundaryGatedActionsAreNotUnconditionallyGranted |
| M8 | add "iam:*" to WorkloadServiceCeiling |
TestBoundaryCoversWorkloadServices |
| M9 | drop kms:* from the ceiling |
same |
| M10 | module role grants dynamodb:GetItem (service absent from ceiling) |
same |
M5–M10 are mine, not from the PR body. The suite is genuinely mutation-resistant in both directions — it fails on weakening the guard and on a drift that would 403 at runtime. Working tree was verified clean after every revert.
1. Does the boundary actually cap an inline policy? — verified, the central claim holds
Pulled the AWS Service Authorization Reference and extracted ActionConditionKeys per action:
PutRolePolicy -> ['iam:PermissionsBoundary']
CreateRole -> ['aws:RequestTag/${TagKey}', 'aws:TagKeys', 'iam:PermissionsBoundary']
AttachRolePolicy -> ['iam:PermissionsBoundary', 'iam:PolicyARN']
PutRolePermissionsBoundary -> ['iam:PermissionsBoundary']
CreatePolicyVersion -> None
SetDefaultPolicyVersion -> None
DeletePolicy -> None
DeletePolicyVersion -> None
Both halves check out, and this was the one that could have taken the pipeline down rather than merely leaving a hole: iam:PermissionsBoundary is supported on all four actions in IAMRoleMutationRequiresBoundary, so the conditioned Allow will actually match at apply time. Had PutRolePolicy supported no condition key (as the PR body's prose says in one place), that Allow would never match and every apply would 403.
PutRolePolicy supports no key that constrains the inline document, so the boundary is indeed the only mechanism that caps it. And the four managed-policy mutation actions genuinely have zero condition keys, which is what makes the resource-scoped Deny in IAMDenyModifyDeployRoleAndPolicies the only way to protect the boundary document — policy_boundary.tf:26-29 is correct.
2. Bypass rather than break — enumerated independently, none found
grep -rn 'resource "aws_iam_role"' terraform/ returns 13: 12 under terraform/modules/**/aws, all 12 carrying permissions_boundary = var.permissions_boundary_arn, plus ci-cd-permissions/role.tf:3 (the deploy role itself), correctly excluded. No module was missed.
Also checked and cleared:
- No other root instantiates these modules. Only
environments/aws/{compute,secrets,networking,database}.tfsource them (5 invocations;cleanup-lambdais uninstantiated). Every new variable is required, no default, so a missed invocation failsterraform validate— apply-time, not runtime. Confirmed by running validate. - Role names are in scope. All names derive from
local.stack_name = "${var.project_name}-${var.environment}-${random_id.suffix.hex}"withproject_name = "cudly"in all threegithub-*.tfvarsand as the variable default. No role setspath =, so every ARN isrole/cudly-…and matchesrole/cudly-*. - Delete-and-recreate is closed.
iam:DeleteRolestays unconditioned, butCreateRolerequires the boundary, so the round-trip gains nothing. CreateServiceLinkedRoleis not a door.policy_data.tf:186-206scopes it to five specificaws-service-role/ARNs and aStringLikeoniam:AWSServiceName; SLR policies are AWS-owned and unmodifiable.UpdateAssumeRolePolicyunconditioned is safe. Rewriting the trust policy of acudly-*role yields a role that is itself boundaried; a role that does not exist yet cannot be created without one. The provider-ordering argument for leaving it unconditioned is sound and matchesresourceRoleUpdate's block order.- The deploy role cannot detach its own gate.
iam:DetachRolePolicyis inIAMDenyModifyDeployRoleAndPolicies' action list againstrole/cudly-terraform-deploy, socudly-deploy-iamcannot be detached. AddRoleToInstanceProfilesupports no condition keys and stays unconditioned — see L4.
3. Is the ARN allowlist exact? — verified exact, and complete
Derived the attachment set independently from the modules:
arn:aws:iam::aws:policy/AmazonSSMManagedInstanceCore networking (fck-nat)
arn:aws:iam::aws:policy/service-role/AWSLambdaBasicExecutionRole lambda, cleanup-lambda, secrets rotation
arn:aws:iam::aws:policy/service-role/AWSLambdaVPCAccessExecutionRole lambda, cleanup-lambda, secrets rotation
arn:aws:iam::aws:policy/service-role/AmazonECSTaskExecutionRolePolicy fargate task-execution
Exactly the four in the ArnNotEquals list, no wildcards, no patterns. aws_iam_policy.secret_read in modules/secrets/aws is indeed only exported via outputs and attached nowhere — the "created but never attached" claim holds. None of the four is escalation-capable (logs, ECR pull, ENI management, SSM agent core), and each is capped by the boundary anyway.
Bypass attempts on the operator itself, all fail closed: an attacker cannot mint a policy in the aws namespace; a different-partition ARN (arn:aws-us-gov:…) is not equal to any listed value so the Deny fires; ARN matching is case-sensitive.
4. Deny-vs-Allow evaluation order — each Deny matches the request it targets
IAMDenyAttachUnapprovedManagedPolicy—Resource: "*", so it cannot be sidestepped by a role name outsidecudly-*.iam:PolicyARNis always present onAttachRolePolicy, and if it somehow weren't, a negated operator returns true on an absent key, so the Deny still fires. Fails closed either way.IAMDenyStripRoleBoundary—DeleteRolePermissionsBoundary's resource type isrole, sorole/cudly-*matches. Note it is belt-and-braces: swapping the boundary for a weaker one is already blocked by the absence of any Allow (PutRolePermissionsBoundaryis granted only underStringEqualson the one ARN, and I confirmed by grep that no other attached policy grants that action).IAMDenyBoundaryOnDeployRole— separate statement, correctly. Folding it into the strip Deny would cross-productPutRolePermissionsBoundary×role/cudly-*and block the transition apply. That reasoning is right.- The "name is load-bearing" claim is correct and pinned.
cudly-deploy-boundarymatchespolicy/cudly-deploy-*inIAMDenyModifyDeployRoleAndPolicies(policy_data.tf:146-165), whose action list is the complete managed-policy mutation set. Mutation M6 confirmsTestBoundaryPolicyNameStaysInProtectedNamespacefails on a rename.
5. Will the deploy actually still work? — no runtime gap found in any declared grant
This is the one that breaks production, so I derived it from source rather than trusting the list. Every service prefix appearing in an Action/actions assignment anywhere under terraform/modules/**/aws:
ce, ec2, ecr, ecs, elasticache, es, kms, lambda, logs, memorydb,
organizations, rds, redshift, savingsplans, secretsmanager, ses, ssmmessages, sts, iam
Every one is covered by the ceiling. The three narrowed services match exactly what the modules grant: organizations → DescribeAccount / DescribeOrganization / ListAccounts (exactly the three in OrganizationsDiscoveryCeiling); sts → AssumeRole only; iam → PassRole only, covered by PassRoleCeiling. The four managed policies add logs, ec2, ecr, ssm, ssmmessages, ec2messages, s3 — all present.
cross_account_role_name_prefix = "CUDly" at compute.tf:105 and compute.tf:226 matches arn:aws:iam::*:role/CUDly* in CrossAccountAssumeRoleCeiling; TestBoundaryMatchesCrossAccountRolePrefix pins the module default.
I could not name a single action a module role policy grants that the boundary does not. Caveat stated plainly: this covers the declared grants. It cannot cover an AWS call the application makes that has no matching grant today — such a call is already failing on main, so the boundary does not regress it.
6. Managed-policy size limits — conclusion holds, the cited numbers do not reproduce (see L2)
7. The ordering constraint — verified, the PR body has it the right way round
.github/workflows/deploy-aws-lambda.yml on.push.paths includes terraform/environments/aws/**, terraform/modules/compute/aws/lambda/**, terraform/modules/database/aws/**, terraform/modules/secrets/aws/** and terraform/modules/networking/aws/**. This PR touches files under all five. Merging does trigger the deploy, that deploy does call iam:PutRolePermissionsBoundary against the pre-existing boundary-less roles, and that grant exists only in the new cudly-deploy-iam. Apply the bootstrap root first. Getting this backwards reddens every AWS deploy with AccessDenied until the apply happens.
The transition-order argument inside the apply is also sound: every aws_iam_role_policy / aws_iam_role_policy_attachment references its role through aws_iam_role.X.name, so the graph orders PutRolePermissionsBoundary ahead of the writes that require it.
Findings
F1 — MEDIUM. The escape analysis is stated as complete and is not: lambda:*, ssm:* and ecs:* at Resource: "*" are the same shape of escape as sts:*
terraform/environments/aws/ci-cd-permissions/policy_boundary.tf:100-116 asserts:
organizationsandstsare the two services that are NOT safe atservice:*granularity, because at that width each of them is a complete escape from this boundary rather than a widening within it
The stated criterion is right, and the reasoning for sts — "the assumed session is a DIFFERENT principal, so this boundary does not follow it" — is exactly right. But it is applied to only two of the services in the list. At least three more meet the same criterion, because a boundary follows a principal, and each of these lets a boundaried role execute code under a different principal:
lambda:*on*→lambda:UpdateFunctionCodeon any function in the account, then invoke. Noiam:PassRoleis required (the role is not being changed), soPassRoleCeiling'scudly-*scoping does not apply. Code then runs under that function's execution role.ssm:*on*→ssm:SendCommand/ssm:StartSessionto any SSM-managed instance. NoPassRole. Code runs under that instance profile's role.ecs:*on*→ecs:UpdateServiceonto an existing task-definition revision, orecs:ExecuteCommandinto a running task. Neither needsPassRolewhen the task role is already set.
Precondition in each case: a compute principal in the account that is not boundaried and holds more than the ceiling. Every Terraform-managed cudly-* role is boundaried after the transition apply, so this is account-state-dependent — the same class of precondition as the OrganizationAccountAccessRole reasoning that you (correctly) treated as decisive for sts.
The gap is wide relative to need. What the modules actually grant is:
lambda:InvokeFunction (×9), lambda:InvokeFunctionUrl, lambda:GetFunctionUrlConfig
ecs:RunTask (×4)
kms:Sign, kms:GetPublicKey, kms:DescribeKey
ec2:{Describe*,PurchaseReservedInstancesOffering,AcceptReservedInstancesExchangeQuote,
GetReservedInstancesExchangeQuote,ModifyNetworkInterfaceAttribute}
s3: nothing at all (s3 is in the ceiling solely for AmazonSSMManagedInstanceCore's s3:GetObject)
I am not asking you to narrow the ceiling in this PR — your runtime-vs-apply-time argument for landing the escalation fix first and narrowing separately is correct, and I'd make the same call. Two things I do think belong here:
- Fix the comment. As written it tells a future maintainer the escape analysis is finished. It isn't. Something like "these are the two the modules force us to narrow today;
lambda,ssmandecsat*are escapes of the same shape and are left wide deliberately, tracked in #NNNN" costs three lines and stops the next person from trusting a closed door that is ajar. - Amend "Left open, deliberately." It currently reads as data exposure ("read every secret, delete RDS instances, schedule KMS key deletion, commit RI/SP spend"). All true, and the RI/SP spend line is the money-path one. But it does not say that the ceiling still permits further privilege escalation out of the boundary when a non-boundaried compute principal exists. That is a materially different residual from "can read secrets", and sec(iac/aws): deploy role can create an unbounded role and attach AdministratorAccess to it #1705 should not be closed on the wider claim without it being written down.
If a follow-up narrows this, the minimum-blast-radius shape is a Deny inside the boundary on the code-execution verbs (lambda:UpdateFunctionCode, lambda:UpdateFunctionConfiguration, ssm:SendCommand, ssm:StartSession, ecs:UpdateService, ecs:ExecuteCommand, ecs:RegisterTaskDefinition) for resources outside cudly-* — a Deny cannot cause the silent runtime 403 a narrowed Allow can, because the deny surface is enumerable and none of those verbs appears in any module policy.
F2 — LOW. policy_iam.tf:14-16 overstates the condition-key claim for iam:CreatePolicy
The deploy role keeps
iam:CreatePolicyandiam:CreatePolicyVersion, neither of which supports any condition key at all (verified against the AWS Service Authorization Reference)
CreatePolicy supports aws:RequestTag/${TagKey} and aws:TagKeys (service reference, quoted in §1). CreatePolicyVersion genuinely supports none. The operative conclusion is unaffected — neither key constrains the policy document, and the minted policy is inert because attaching it is denied — but the sentence is cited as verified fact and a maintainer could reasonably rely on it. Narrow it to "neither supports a condition key that constrains the policy document; CreatePolicyVersion supports none at all."
The parallel claim at policy_boundary.tf:26-29 about the four managed-policy mutation actions is correct; I verified all four return no condition keys.
F3 — LOW. The cited rendered sizes do not reproduce
PR body: "New sizes are 1127 and 1223 characters." Rendering both documents from the statement structures as compact JSON (which is what jsonencode emits, and the 6144 limit excludes whitespace) with a 12-digit account ID substituted for the boundary ARN, I get:
cudly-deploy-boundary: 966 characters
cudly-deploy-iam: 1083 characters
Both are ~13% below the quoted figures and both are far under 6144, so the conclusion — these fit comfortably and the split was the right call — is unaffected. Flagging only because the numbers are presented as measured evidence and don't reproduce; if they came from an earlier draft of the documents, worth re-measuring before the body becomes the record. The "five of ten attached policies" count I confirmed directly: role.tf now has attachments for networking, compute, compute_b, data, iam.
F4 — LOW / informational. Two residuals the "Left open" section doesn't name
iam:AddRoleToInstanceProfilesupports no condition keys and stays unconditioned onrole/cudly-*+instance-profile/cudly-*. Not exploitable today: launching with the profile needsiam:PassRole, which is scoped tocudly-*with the deploy role explicitly denied, and every Terraform-managedcudly-*role is boundaried. But its precondition is identical to the hand-made-cudly-*-role residual you do admit, so it belongs in the same bullet.- The rollback caveat is real but narrower than stated.
rollback.ymltakescloud/environment/image_tag/reasonand has no ref input; itsactions/checkoutsteps carry noref:, so they check out whatever ref the dispatch targeted. A rollback dispatched frommain— the normal path, rolling the image back — applies current config and is completely unaffected. TheDeleteRolePermissionsBoundaryfailure only occurs if someone selects an old branch or tag in the Actions ref picker. The README wording ("a ref that predates the boundary has nopermissions_boundaryin config") is accurate, but reads as though rollback is broken in general when the default path is fine. One clause would fix it.
Hypotheses tested and cleared
Stated explicitly so the negative results are on the record: PutRolePolicy unsupported-condition-key pipeline break; a missed aws_iam_role; a second environment root instantiating the modules unguarded; a role name or path = outside role/cudly-*; a fifth managed-policy attachment or a secret_read attachment breaking the apply; CreateServiceLinkedRole as a role-creation door; delete-then-recreate around the CreateRole condition; detaching cudly-deploy-iam from the deploy role; swapping the boundary for a weaker one; UpdateAssumeRolePolicy on an unboundaried role; a partition or case variant defeating ArnNotEquals; a Deny whose Resource never matches its target request; a service granted by a module but missing from the ceiling; a cross_account_role_name_prefix override capping the cross-account read; the trigger-path/ordering claim being inverted. All cleared, each by the check named in the section above.
What I could not verify, and why
- No
terraform planand no live-state check. Out of scope by instruction and no credentials here, so nothing below the config layer is confirmed: whether the boundary policy already exists in the target account, whatcudly-*roles exist today that Terraform does not manage (F1's and F4's precondition, and the residual you already admit), and whether any non-boundaried Lambda / SSM-managed instance / ECS service exists in the account. F1's severity is a function of that account state, and someone with console access should check it before sec(iac/aws): deploy role can create an unbounded role and attach AdministratorAccess to it #1705 is closed on the wider claim. - Runtime coverage is verified against declared grants only. An AWS call the application makes with no matching IAM grant is already broken on
main; the ceiling neither fixes nor regresses it. - I did not attempt to render the documents through Terraform itself (that needs
init+ provider evaluation ofaws_iam_policy.workload_boundary.arn); F3's figures come from reconstructing the statement structures, which is why I report the discrepancy rather than asserting the PR's numbers are wrong.
Recommendation: land it, in the stated order — bootstrap root applied by hand first, then merge. F1's comment and PR-body corrections are worth folding in before merge since they are prose-only and this is the durable record for a p0; F2–F4 are equally prose-only and can ride along. The mechanism itself is correct, the guard tests are the strongest part of the change, and I found no way to reconstruct the #1705 escalation against the committed configuration.
|
Requesting a full review, paced to one request per hour across this repo. The CodeRabbit quota is per-developer per-organization and adaptive, tightening at the 95th percentile of recent review volume. Every open PR here draws on one budget, so bursting requests both consumes it faster and tightens the limit. Spacing them is what gets verdicts to land. Full form rather than incremental: this head was pushed while the quota was exhausted, so its automatic review never ran and is not retried retroactively. An incremental request would skip exactly those commits and report clean on a diff it never read. @coderabbitai full review |
|
✏️ Learnings added
|
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 34 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (23)
Comment |
… size Fix three inaccuracies flagged by adversarial review, all in comments only: - policy_boundary.tf: the organizations/sts comment read as a complete escape analysis. State the criterion, name organizations and sts as narrowed in this change, and note that lambda:*, ssm:* and ecs:* meet the same criterion and are tracked in #1723 instead. - policy_iam.tf: iam:CreatePolicy does support aws:RequestTag/${TagKey} and aws:TagKeys condition keys (it accepts an optional Tags parameter). iam:CreatePolicyVersion supports none. The conclusion is unchanged since neither constrains the policy document. - PR body: recompute the two new policies' compact-JSON sizes (966 and 1083, not 1127 and 1223), add the privilege-escalation residual and the unconditioned iam:AddRoleToInstanceProfile action to "Left open, deliberately". No policy logic, resources, or tests changed.
|
Addressed the four findings from the adversarial review at
Also added the Verification: Pushed at |
Bootstrap root APPLIED — merge this PR promptlyApplied Plan reviewed before applying: 0 destroys, nothing outside this root. Verified against live IAM, not the apply output
|
|
Merging without a CodeRabbit verdict, deliberately, with the reasoning recorded. CodeRabbit has not reviewed this PR at any head — it has posted no comments here at all, having been throttled under Fair Usage when the branch was pushed, and a push that lands during the quota window is never auto-reviewed retroactively. A paced full-review request was made at Waiting increases risk rather than reducing it. The bootstrap root is already applied, so What this PR does carry:
Residuals are tracked, not hidden: #1723 (the ceiling still permits escaping to another principal via #1705 should not be closed on the wider claim until live account state is checked for non-boundaried Lambda functions, SSM-managed instances or ECS services, which determines #1723 severity and could not be verified without account access at review time. |
cudly-deploy-boundary capped the deploy role's IAM writes (#1722) but still granted ecs, lambda and ssm at `service:*` on Resource "*". Each of those three lets a boundaried principal execute code as a DIFFERENT principal, which is not a widening within the ceiling but a complete exit from it: a permissions boundary constrains the principal it is attached to and does not follow the one the code ends up running as. - lambda:UpdateFunctionCode on any function, then invoke, runs as that function's execution role. - ssm:SendCommand / ssm:StartSession to any SSM-managed instance runs as that instance's profile. - ecs:UpdateService onto an existing task-definition revision, or ecs:ExecuteCommand into a running task, runs as that task's role. None of them needs iam:PassRole, so PassRoleCeiling's cudly-* scoping does not constrain them either. This is the same criterion #1722 already applied to organizations and sts; the enumeration was incomplete. The three are now pinned to what the workload roles actually need, re-derived from the tree rather than assumed: - ecs:RunTask, the only ecs action any module grants (the four EventBridge invoker roles in modules/compute/aws/fargate). - lambda:InvokeFunction and lambda:GetFunctionUrlConfig, the only two any module grants. The InvokeFunction / InvokeFunctionUrl in aws_lambda_permission blocks are resource policies granting a service principal, not grants to a workload role, so this ceiling never gated them. - the 15 ssm actions of AmazonSSMManagedInstanceCore v2 verbatim, which the fck-nat instance role carries for Session Manager. No module grants an ssm action of its own, and the operator-side verbs that reach a DIFFERENT instance are absent. Narrowing is safe against the runtime-403 risk that argues for per-service granularity everywhere else in this file: for these three the pinned lists are the union of what terraform/modules grants and what the attached AWS managed policies grant, i.e. a superset of the whole identity side. Effective permissions are identity AND ceiling, so a ceiling that already covers the identity side changes nothing for the legitimate path. Guarded in CI in both directions: - TestBoundaryDeniesCrossPrincipalEscapes fails if any of the six escape actions becomes permitted again. It keys off a matcher, not off the literal tokens "ecs:*" / "lambda:*" / "ssm:*", because "*", "lambda:Update*" and a bare "ssm:SendCommand" entry all reopen the hole while leaving a check written against the wildcard spellings green. - TestBoundaryCoversSSMManagedInstanceCore closes the blind spot the existing coverage test cannot see: a role's identity policy is its inline policies plus the AWS managed policies attached to it, and a managed policy's actions appear nowhere in this tree. - TestActionPermittedMatchesWholeActionCaseInsensitively pins the matcher the other two delegate to. Its dangerous failure mode is silent: a matcher that is too strict leaves the escape test passing against a wide-open ceiling, which reads exactly like a ceiling that grants none. Matching is case -insensitive because IAM action matching is, so "ecs:updateservice" cannot slip past. - The coverage test now resolves grants through the same matcher instead of substring-searching the file, so an action moved from an Allow into a Deny no longer reads as covered, and it rejects a bare "*" or any iam grant wider than iam:PassRole in an Allow statement. Mutation testing the above found a hole in the guard itself: actionStringPattern only matched service:Action literals, so a bare "*" never entered any derived action set. A ceiling of `Action = ["*"]` therefore passed every assertion in this file, including the "no bare star" check written to catch exactly that and the escape check, which found none of the six escapes in a set the star had never entered. The pattern now matches a lone "*" as well. Two residuals are documented rather than closed, both stated in the file: ecs:RunTask against an already-registered task definition still runs as that definition's task role, and cannot be resource-scoped here because the family is "<stack_name>-fargate" with stack_name defaulting to a random suffix, so no literal ARN pattern could be known to match; and iam:AddRoleToInstanceProfile supports no condition key, though it needs iam:PassRole to matter and that is scoped. Closes #1723
1 participant
Closes #1705
Nothing here changes live AWS state. The escalation stays fully open until a human applies
terraform/environments/aws/ci-cd-permissions. Noterraform applywas run and none should be run by CI: that root is human-applied by design.Apply the bootstrap root BEFORE merging, not after.
deploy-aws-lambda.ymltriggers on pushes tomainunderterraform/environments/aws/**and the AWS compute/database/secrets/networking modules, all of which this PR touches. On merge, the next deploy callsiam:PutRolePermissionsBoundaryto attach the boundary to the roles that already exist, and that grant lives in the newcudly-deploy-iampolicy in the bootstrap root. Merge first and every AWS deploy goes red withAccessDenieduntil the apply happens. Recoverable, not destructive, but avoidable. This is written intoci-cd-permissions/README.mdtoo so it is not only in a PR body.I could not produce a
terraform plan: no AWS credentials are reachable from this environment (aws sts get-caller-identityreturnsNoCredentials). What I could verify is below.The defect
cudly-terraform-deployheldiam:CreateRole,iam:AttachRolePolicyandiam:PutRolePolicyonarn:aws:iam::*:role/cudly-*with noiam:PermissionsBoundarycondition and noiam:PolicyARNcondition. Create a role, attachAdministratorAccess, then either assume it (same-account, so a trust policy naming the deploy role suffices with no identity-side grant) or pass it to Lambda under the existingIAMPassRoleScopedByService. Full account administrator from a GitHub Actions OIDC token, persisting past rotation of that token.ref:refs/heads/mainis a trusted subject andmaintakes direct pushes with red CI, so the precondition is repo write access.Why a permissions boundary and not a Deny
I evaluated three alternatives before settling:
iam:CreateRole/PutRolePolicy/AttachRolePolicy. Takes the pipeline down. The deploy path creates twelve roles acrossterraform/moduleson every apply (Lambda execution, Fargate task and task-execution, four EventBridge invokers, fck-nat, VPC flow logs, RDS proxy, secret rotation, cleanup Lambda).iam:PolicyARNallowlist alone. Closes the path the issue names and nothing else.iam:PutRolePolicywrites an inline document that supports no condition key at all, so{"Action":"*","Resource":"*"}inline on a new role reaches the same administrator with the managed-policy door shut.A permissions boundary is the only AWS mechanism that caps a principal regardless of what its identity policy says, and therefore the only thing that closes the inline path. Both layers are here, because the boundary alone would leave a created role capped-but-broad while the allowlist alone leaves the inline path open.
What changed
policy_boundary.tf(new)cudly-deploy-boundary, the ceiling. Per-service allow list derived from the IAM policy documents interraform/modulesplus the four AWS managed policies those modules attach, withiam:absent apart from acudly-*-scopedPassRolepolicy_iam.tf(new)cudly-deploy-iam. Re-grants the three actions plusiam:PutRolePermissionsBoundaryunderStringEqualsoniam:PermissionsBoundary;ArnNotEqualsDeny allowlisting the four attachable managed policies; Denies stripping a boundary and Denies the deploy role boundarying itselfpolicy_data.tfIAMRolesAndPoliciesAllow, with a comment explaining why they must not come backpermissions_boundary = var.permissions_boundary_arn, required variable with no defaultpolicy_guard_test.go(new)The new statements went into a separate managed policy rather than into
cudly-deploy-databecause of the 6144-character limit:cudly-deploy-datarenders to ~4.6 KB andcudly-deploy-computeis close enough to the ceiling that an earlier fix had to be split intopolicy_compute_b.tf. New sizes, reconstructed as compact JSON, are 966 and 1083 characters. Both are far under the 6144 limit. The role now carries five of the ten managed policies AWS permits.Escalation paths closed
AdministratorAccessIAMDenyAttachUnapprovedManagedPolicy—ArnNotEqualsoniam:PolicyARN, four exact ARNs, no wildcards*on*IAMRoleMutationRequiresBoundary— the role cannot exist without the ceiling, so the inline document is cappedcudly-*role (UpdateAssumeRolePolicy+PutRolePolicy)PutRolePolicyrequires the target to already carry the boundaryPassRoleto Lambda/ECS/EC2/EventBridge and run codeAddRoleToInstanceProfile+RunInstancesRunInstanceswith a profile needsPassRole, and the contained role is boundariedcudly-deploy-boundarymatchescudly-deploy-*, so the existingIAMDenyModifyDeployRoleAndPoliciesalready deniesCreatePolicyVersion/SetDefaultPolicyVersion/DeletePolicy/DeletePolicyVersionon it — the complete mutation set for a managed policy. The name is load-bearing and a test asserts itIAMDenyStripRoleBoundary;PutRolePermissionsBoundaryis pinned to the one ARNorganizationsnarrowed to the three read actions the modules use (organizations:*reachesCreateAccountand SCPs);sts:AssumeRolescoped toarn:aws:iam::*:role/CUDly*(on*it escapes the boundary outright, since the assumed session is a different principal andOrganizationAccountAccessRolein member accounts trusts the management account root)Left open, deliberately
Read this section before closing #1705 on the wider claim.
cudly-terraform-deploy's own reach. It grantss3:*,kms:*,rds:*,secretsmanager:*and similar onResource: "*", while the deploy role itself is scoped tocudly-*resources. A compromised deploy role can therefore still read every secret, delete RDS instances, schedule KMS key deletion and commit RI/SP spend. What this PR removes is IAM writes, and with them persistence and in-account administrator. Narrowing further is genuinely risky in the wrong direction: a too-narrow boundary fails at runtime, in production, whereas everything this PR can get wrong fails at apply time in CI. I would rather land the escalation fix and narrow the ceiling as a separate, separately-verified change. Happy to file that as a follow-up issue.lambda:*,ssm:*andecs:*are granted at full service width, and each lets a boundaried role run as a different principal with noiam:PassRoleinvolved (lambda:UpdateFunctionCode+ invoke runs as the function's execution role;ssm:SendCommand/StartSessionruns as an SSM-managed instance's instance profile;ecs:UpdateServiceonto an existing task definition, orecs:ExecuteCommand, runs inside a running task).PassRoleCeiling'scudly-*scoping does not apply to any of these, because none of them usesPassRole. This is a materially different residual from the data-exposure bullet above and sec(iac/aws): deploy role can create an unbounded role and attach AdministratorAccess to it #1705 should not be closed on the wider claim without it: tracked in sec(iac/aws): deploy boundary still permits escaping to another principal via lambda/ssm/ecs at service:* #1723.iam:CreatePolicy/iam:CreatePolicyVersionstay unconditioned.iam:CreatePolicysupportsaws:RequestTag/${TagKey}andaws:TagKeys(it accepts an optional Tags parameter), andiam:CreatePolicyVersionsupports no condition key at all. Neither constrains the policy document, which is the point here. The deploy role can still mint acudly-*managed policy whose document is*on*; it is inert, because attaching it to any role is denied by the ARN allowlist and noAttachUserPolicy/AttachGroupPolicyis granted anywhere. Denying policy creation would break the managed policymodules/secrets/awsdeclares.mainstill takes direct pushes with red CI while being a trusted OIDC subject. That is the reachability half of sec(iac/aws): deploy role can create an unbounded role and attach AdministratorAccess to it #1705 and needs a repo-settings change, not a Terraform one.cudly-*role created by hand or from the console carries no boundary and stays a legalPassRoletarget. Closing that needs a tag or a naming split; not worth the coupling.iam:AddRoleToInstanceProfilesupports no condition keys and stays unconditioned, same as the bullet above. Not exploitable today on its own: reaching it still needsiam:PassRole, which is scoped tocudly-*roles byPassRoleCeiling. Its precondition is identical to the hand-made-cudly-*-role residual just above, so it belongs in the same list rather than the escalation-paths-closed table.Evidence that legitimate deployments still work
aws_iam_roleunderterraform/modules/**/aws, 12permissions_boundarylines.ci-cd-permissions/role.tfis correctly the only uncovered one.TestEveryModuleRoleHasPermissionsBoundaryasserts it, requiring the value to be exactlyvar.permissions_boundary_arn, andTestNoIAMRolesOutsideGuardedModulesasserts none is declared outside those modules.iam:PermissionsBoundaryonPutRolePolicy/AttachRolePolicymeans the boundary must already be on the target role. Everyaws_iam_role_policyandaws_iam_role_policy_attachmentin the AWS modules references its role viaaws_iam_role.X.name/.id(all 42role =lines checked, zero hardcoded names), so Terraform's graph putsPutRolePermissionsBoundaryahead of them.internal/service/iam/role.go:resourceRoleCreatepopulatesCreateRoleInput.PermissionsBoundarydirectly, so there is no create-then-attach window that the condition would deny.Update*actions are deliberately unconditioned. Same source:resourceRoleUpdate'sd.HasChangeblocks runassume_role_policy→description→max_session_duration→permissions_boundary. Conditioning them would have 403'd the first apply against a role whose trust policy also changed. This was the one place where "condition everything that supports the key" would have broken the pipeline.DeleteRole,DeleteRolePolicyandDetachRolePolicyremain unconditioned, socleanup-staging.ymlanddestroy-fargate-dev.ymlare unaffected.TestBoundaryCoversWorkloadServicesre-derives that set from source and fails if a service is missing.terraform fmt -recursive -checkclean.terraform validatepasses in bothci-cd-permissionsandenvironments/aws.go test ./terraform/...green (17 assertions across 2 packages),go vetclean,golangci-lintat the CI-pinned v2.10.1 clean.One rollback caveat
rollback.ymlapplies the environment root from the dispatched ref's checkout. A ref predating this change has nopermissions_boundaryin config while the live roles have one, so the provider issuesDeleteRolePermissionsBoundaryandIAMDenyStripRoleBoundaryfails the rollback. Roll back to a ref at or after this change. Documented inci-cd-permissions/README.mdand in the policy comment next to that Deny.Guard tests
The reviewer's finding that mattered most was that a test suite can be green while the fix is gone. Four single-token edits reopened the escalation with the first draft passing:
StringEquals→StringEqualsIfExists,ArnNotEquals→ArnEquals, addingarn:aws:iam::aws:policy/*to the ARN list, and flipping that statement'sEffecttoAllow. The tests now assert both statements structurally (exact action sets, exact condition operators, wildcard-free ARNs, exactly one match per lookup so a renamed or split statement fails closed), and each of those four edits was empirically confirmed to fail the intended test and then reverted. The guarded-file list is globbed rather than hardcoded and everyStatement = [array in a file is parsed, so a newpolicy_*.tfor a second policy appended to an existing one cannot slip past.