From 7214ba0ffdc3d9217f510158db3cfefbd30b913c Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Thu, 13 Aug 2026 23:41:01 +0200 Subject: [PATCH 1/5] sec(iac/aws): pin ecs, lambda and ssm per action in the deploy boundary 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 "-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 --- .../aws/ci-cd-permissions/policy_boundary.tf | 152 +++++++--- .../ci-cd-permissions/policy_guard_test.go | 283 ++++++++++++++++-- .../aws/ci-cd-permissions/policy_iam.tf | 13 + 3 files changed, 385 insertions(+), 63 deletions(-) diff --git a/terraform/environments/aws/ci-cd-permissions/policy_boundary.tf b/terraform/environments/aws/ci-cd-permissions/policy_boundary.tf index cdc486c6c..794bc55ab 100644 --- a/terraform/environments/aws/ci-cd-permissions/policy_boundary.tf +++ b/terraform/environments/aws/ci-cd-permissions/policy_boundary.tf @@ -32,23 +32,36 @@ # # THE ALLOW LIST IS A CEILING, NOT A GRANT. A boundary grants nothing on its # own; effective permissions are the intersection of it and the role's identity -# policy. Every service listed below is already reachable by at least one -# workload role today, so this document removes no permission any role currently -# has. Granularity is deliberately per-service rather than per-action: a -# too-narrow boundary fails at RUNTIME (a Lambda 403s in production) rather than -# at apply time, which is a far worse failure mode than the apply-time 403s this -# module has produced before (#1496, #1514, #1671, #1698). Per-service keeps the -# blast radius of a miss to "a whole new AWS service was added", which is a -# conscious change, rather than "someone added one more action". +# policy. Everything listed below is already reachable by at least one workload +# role today, so this document removes no permission any role currently has. # -# DRIFT IS GUARDED IN CI. Granting a workload role an action in a service that -# is not listed here would deploy cleanly and then 403 at runtime. That is -# exactly the invisible failure the per-service granularity is meant to make -# rare, and TestBoundaryCoversWorkloadServices in -# terraform/environments/aws/ci-cd-permissions/policy_guard_test.go turns what -# is left of it into a CI failure: it re-derives the service set from the IAM -# policy documents in terraform/modules and fails if any of them is missing -# here. Add the service to the list below in the same change. +# Granularity defaults to per-service rather than per-action: a too-narrow +# boundary fails at RUNTIME (a Lambda 403s in production) rather than at apply +# time, which is a far worse failure mode than the apply-time 403s this module +# has produced before (#1496, #1514, #1671, #1698). Per-service keeps the blast +# radius of a miss to "a whole new AWS service was added", which is a conscious +# change, rather than "someone added one more action". +# +# ecs, lambda and ssm are the exception and are pinned per action (#1723), +# because at `service:*` width each one lets a boundaried principal execute code +# as a DIFFERENT principal, which is not a widening within the ceiling but a +# complete exit from it: see the escape criterion spelled out in +# OrganizationsDiscoveryCeiling below. The runtime-403 risk that argues for +# per-service granularity does not apply to those three, because their action +# lists are the union of what terraform/modules grants and what the AWS managed +# policies those modules attach grant, i.e. a superset of every identity policy +# any workload role can carry. Effective permissions are identity AND ceiling, +# so a ceiling that already covers the whole identity side changes nothing. +# +# DRIFT IS GUARDED IN CI, in both directions, by policy_guard_test.go in this +# directory. TestBoundaryCoversWorkloadServices re-derives the action set from +# the IAM policy documents in terraform/modules and fails if this document does +# not cover it; TestBoundaryCoversSSMManagedInstanceCore does the same for the +# one AWS managed policy whose contents this document enumerates rather than +# wildcards; TestBoundaryDeniesCrossPrincipalEscapes fails if ecs, lambda or ssm +# is ever widened back to a form that permits one of the escape actions. A grant +# added to a module without the matching entry here would otherwise deploy +# cleanly and then 403 at runtime; make both edits in the same change. resource "aws_iam_policy" "workload_boundary" { name = "cudly-deploy-boundary" description = "CUDly Terraform deploy: permissions ceiling for every role the deploy role creates or manages" @@ -57,12 +70,50 @@ resource "aws_iam_policy" "workload_boundary" { Version = "2012-10-17" Statement = [ { - # The services CUDly workload roles actually use. Derived from the IAM - # policy documents in terraform/modules plus the four AWS managed - # policies those modules attach: AWSLambdaBasicExecutionRole (logs), + # What CUDly workload roles actually use. Derived from the IAM policy + # documents in terraform/modules plus the four AWS managed policies + # those modules attach: AWSLambdaBasicExecutionRole (logs), # AWSLambdaVPCAccessExecutionRole (ec2, logs), # AmazonECSTaskExecutionRolePolicy (ecr, logs) and - # AmazonSSMManagedInstanceCore (ssm, ssmmessages, ec2messages, s3, ec2). + # AmazonSSMManagedInstanceCore (ssm, ssmmessages, ec2messages). + # + # The three per-action services, and where each entry comes from: + # + # - ecs:RunTask is the only ecs action any module grants (the four + # EventBridge invoker roles in modules/compute/aws/fargate). None of + # the attached managed policies grants an ecs action. The ecs:cluster + # condition and the task-definition Resource stay on the module + # policy. RunTask keeps a RESIDUAL this ceiling does not close: + # running an ALREADY-REGISTERED task definition unchanged runs as + # that definition's task role, and the ceiling's Resource is "*". + # Overriding overrides.taskRoleArn / overrides.executionRoleArn + # needs iam:PassRole, which PassRoleCeiling scopes to cudly-*, so + # only the no-override form is reachable, and only against a task + # definition that already exists and already carries a role more + # privileged than this ceiling. It is deliberately NOT closed by + # resource-scoping the way CrossAccountAssumeRoleCeiling is: the + # family is local.name_prefix, i.e. "-fargate", and + # stack_name defaults to project_name-environment-, so + # no literal ARN pattern here can be known to match the definition + # the deploy actually creates. A pattern that missed would 403 the + # scheduled tasks at RUNTIME, which is the failure mode this file + # is organised to avoid. RegisterTaskDefinition, UpdateService and + # ExecuteCommand, the paths that would let a caller CHOOSE the role + # it lands on, are all absent. + # - lambda:InvokeFunction (the API Lambda self-invoking for the async + # refresh path, #257) and lambda:GetFunctionUrlConfig (OIDC issuer + # lookup at cold start, modules/compute/aws/lambda/signing-key.tf) + # are the only two any module grants. The lambda:InvokeFunction and + # lambda:InvokeFunctionUrl in aws_lambda_permission blocks are + # RESOURCE policies granting a service principal, not grants to a + # workload role, so this ceiling never gates them. + # - the ssm entries are AmazonSSMManagedInstanceCore v2 verbatim (the + # fck-nat instance role in modules/networking/aws is the only role + # that carries it, for Session Manager access). No module grants an + # ssm action of its own. Every one of them is an agent reporting on + # the instance it already runs on; the operator-side verbs that + # reach a DIFFERENT instance, ssm:SendCommand and ssm:StartSession + # above all, are deliberately absent. # # `iam:` is absent on purpose and is the whole point of the statement: # because a boundary caps by intersection, omitting a service denies it @@ -79,11 +130,12 @@ resource "aws_iam_policy" "workload_boundary" { "ec2:*", "ec2messages:*", "ecr:*", - "ecs:*", + "ecs:RunTask", "elasticache:*", "es:*", "kms:*", - "lambda:*", + "lambda:GetFunctionUrlConfig", + "lambda:InvokeFunction", "logs:*", "memorydb:*", "rds:*", @@ -92,18 +144,33 @@ resource "aws_iam_policy" "workload_boundary" { "savingsplans:*", "secretsmanager:*", "ses:*", - "ssm:*", + "ssm:DescribeAssociation", + "ssm:DescribeDocument", + "ssm:GetDeployablePatchSnapshotForInstance", + "ssm:GetDocument", + "ssm:GetManifest", + "ssm:GetParameter", + "ssm:GetParameters", + "ssm:ListAssociations", + "ssm:ListInstanceAssociations", + "ssm:PutComplianceItems", + "ssm:PutConfigurePackageResult", + "ssm:PutInventory", + "ssm:UpdateAssociationStatus", + "ssm:UpdateInstanceAssociationStatus", + "ssm:UpdateInstanceInformation", "ssmmessages:*", ] Resource = "*" }, { - # organizations and sts are the two services narrowed in THIS change, - # because at `service:*` granularity each of them is a complete escape - # from this boundary rather than a widening within it: the criterion is - # "does this let a boundaried role keep running as a DIFFERENT - # principal, with no iam:PassRole involved" (PassRoleCeiling below is - # what scopes PassRole itself, so it does not help here). + # organizations and sts are narrowed for the same reason ecs, lambda and + # ssm are pinned per action in WorkloadServiceCeiling above: at + # `service:*` granularity each of them is a complete escape from this + # boundary rather than a widening within it. The criterion is "does this + # let a boundaried role keep running as a DIFFERENT principal, with no + # iam:PassRole involved" (PassRoleCeiling below is what scopes PassRole + # itself, so it does not help here). # # - organizations:* includes CreateAccount (mints a fresh account that # trusts this one), AttachPolicy/DetachPolicy (rewrites SCPs) and @@ -116,17 +183,22 @@ resource "aws_iam_policy" "workload_boundary" { # account trusts the management account root) and the ceiling is # simply gone. # - # THIS IS NOT THE COMPLETE SET, and the statement below should not be - # read as a finished escape analysis. At least three more services meet - # the same criterion and are left at full service width in - # WorkloadServiceCeiling above: lambda:* (UpdateFunctionCode on any - # function, then invoke, runs as that function's execution role), - # ssm:* (SendCommand / StartSession to any SSM-managed instance, runs - # as its instance profile) and ecs:* (UpdateService onto an existing - # task definition revision, or ExecuteCommand into a running task). - # None of those three needs iam:PassRole, so PassRoleCeiling's - # cudly-*-only scoping does not constrain them either. Narrowing them - # is out of scope for this change and tracked in #1723. + # The three services that used to be left at full width here have been + # narrowed in WorkloadServiceCeiling above (#1723): lambda + # (UpdateFunctionCode on any function, then invoke, runs as that + # function's execution role), ssm (SendCommand / StartSession to any + # SSM-managed instance, runs as its instance profile) and ecs + # (UpdateService onto an existing task definition revision, or + # ExecuteCommand into a running task). None of the three needs + # iam:PassRole, so PassRoleCeiling's cudly-*-only scoping does not + # constrain them either, which is why the action lists and not a + # PassRole scope are what closes them. + # + # THIS IS STILL NOT A FINISHED ESCAPE ANALYSIS. The criterion applies to + # every service in WorkloadServiceCeiling, and the ones left at + # `service:*` are there because no cross-principal execution path + # through them is known, not because one was ruled out. Apply the + # criterion again before adding a service at full width. # # organizations and sts are therefore pinned to exactly what the # modules grant. organizations is action-scoped rather than diff --git a/terraform/environments/aws/ci-cd-permissions/policy_guard_test.go b/terraform/environments/aws/ci-cd-permissions/policy_guard_test.go index 4a21b3c43..39cdef12a 100644 --- a/terraform/environments/aws/ci-cd-permissions/policy_guard_test.go +++ b/terraform/environments/aws/ci-cd-permissions/policy_guard_test.go @@ -168,7 +168,17 @@ var actionAssignmentPattern = regexp.MustCompile(`(?m)^[ \t]*(?:Action|actions)\ // actionStringPattern extracts individual "service:Action" literals from the // value captured by actionAssignmentPattern. -var actionStringPattern = regexp.MustCompile(`"([A-Za-z0-9]+:[A-Za-z0-9_*]+)"`) +// +// The lone `"*"` alternative is load-bearing and not a stylistic nicety. A +// service:Action-only pattern cannot see a bare "*", which is the single widest +// grant IAM has, so it went missing from every derived action set: the "no bare +// star" guard in TestBoundaryCoversWorkloadServices could never fire, and +// TestBoundaryDeniesCrossPrincipalEscapes reported a ceiling of +// `Action = ["*"]` as permitting none of the escapes, because none of them was +// in a set the star never entered. Both read exactly like a clean ceiling. +// Mutation M6 in the #1723 verification caught this; it is the reason the +// pattern is an alternation rather than one branch. +var actionStringPattern = regexp.MustCompile(`"([A-Za-z0-9]+:[A-Za-z0-9_*]+|\*)"`) // resourceAssignmentPattern matches a `Resource = [...]` or `Resource = "..."` // assignment inside a single statement block and captures the value. @@ -498,6 +508,223 @@ func equalStringSets(got, want []string) bool { return true } +// boundaryAllowedActions returns every action named by an Allow statement in +// policy_boundary.tf, comments stripped. +// +// Callers ask "does this document permit X" through actionPermitted rather than +// by substring-searching the file, which is what the coverage check used to do. +// A substring check answers a weaker question than it looks like it does: an +// action named inside a DENY statement satisfies it just as well as a real +// grant, so a boundary that had moved ecs:RunTask from its Allow list into a +// Deny would still have read as "covered". Splitting into statement objects and +// keeping only the Allow ones is what makes the answer mean what the caller +// needs it to mean. +// +// Deny statements are deliberately NOT subtracted here. The one Deny this +// document carries (DenyPassDeployRole) is resource-scoped, so "the action +// appears in a Deny" does not imply the action is unreachable, and pretending +// otherwise would produce a wrong answer in the direction that hides a gap. +func boundaryAllowedActions(t *testing.T) []string { + t.Helper() + + stmts := extractStatementBlocks(readPolicySource(t, boundaryFile)) + if len(stmts) == 0 { + t.Fatalf("%s: found zero Statement objects; the statement splitter in this test is broken and would otherwise pass vacuously", boundaryFile) + } + + var allowed []string + for _, stmt := range stmts { + if statementEffect(stmt) != "Allow" { + continue + } + allowed = append(allowed, extractActionListActions(stmt)...) + } + if len(allowed) == 0 { + t.Fatalf("%s: parsed zero actions out of its Allow statements; every coverage assertion below would otherwise report the whole ceiling as missing, or (in the escape direction) report every escape as closed", boundaryFile) + } + sort.Strings(allowed) + return allowed +} + +// actionPermitted reports whether any entry in allowed matches action, treating +// "*" in an entry as a wildcard over any run of characters. Matching is a whole +// -string anchored comparison, never a substring or prefix test: "ecs:Run" must +// not match "ecs:RunTask", and "ecs:*" must match "ecs:RunTask" but not +// "ecsx:RunTask". +// +// Matching is case-INSENSITIVE because IAM action matching is, and the direction +// that matters is the one that hides a hole rather than the one that invents +// one. A ceiling entry spelled "ecs:updateservice" grants the escape exactly as +// "ecs:UpdateService" does, so a case-sensitive comparison here would let +// TestBoundaryDeniesCrossPrincipalEscapes report a wide-open ceiling as closed. +// TestActionPermittedMatchesWholeActionCaseInsensitively pins both halves. +func actionPermitted(allowed []string, action string) bool { + for _, entry := range allowed { + parts := strings.Split(entry, "*") + for i, p := range parts { + parts[i] = regexp.QuoteMeta(p) + } + if regexp.MustCompile(`(?i)^` + strings.Join(parts, `.*`) + `$`).MatchString(action) { + return true + } + } + return false +} + +// TestActionPermittedMatchesWholeActionCaseInsensitively pins the matcher every +// other assertion in this file delegates to. It is worth its own test because +// its failure modes are silent in the dangerous direction: a matcher that is too +// STRICT (an == comparison, or a case-sensitive one) leaves +// TestBoundaryDeniesCrossPrincipalEscapes passing against a ceiling that grants +// every escape, which reads exactly like a ceiling that grants none. +func TestActionPermittedMatchesWholeActionCaseInsensitively(t *testing.T) { + cases := []struct { + name string + allowed []string + action string + want bool + }{ + {"exact match", []string{"ecs:RunTask"}, "ecs:RunTask", true}, + {"different action in the same service", []string{"ecs:RunTask"}, "ecs:UpdateService", false}, + {"entry is a prefix of the action", []string{"ecs:Run"}, "ecs:RunTask", false}, + {"action is a prefix of the entry", []string{"ssm:GetParameters"}, "ssm:GetParameter", false}, + {"service wildcard covers the service", []string{"ecs:*"}, "ecs:RunTask", true}, + {"service wildcard does not bleed into a longer service", []string{"ecs:*"}, "ecsx:RunTask", false}, + {"service wildcard does not bleed into a shorter service", []string{"ecs:*"}, "ec:RunTask", false}, + {"verb wildcard covers the verb", []string{"lambda:Update*"}, "lambda:UpdateFunctionCode", true}, + {"verb wildcard does not cover another verb", []string{"lambda:Update*"}, "lambda:InvokeFunction", false}, + {"bare star covers everything", []string{"*"}, "ecs:UpdateService", true}, + {"lowercased entry still permits the action", []string{"ecs:updateservice"}, "ecs:UpdateService", true}, + {"uppercased entry still permits the action", []string{"ECS:UPDATESERVICE"}, "ecs:UpdateService", true}, + {"empty ceiling permits nothing", nil, "ecs:RunTask", false}, + {"match is found anywhere in the list", []string{"s3:*", "ecs:RunTask"}, "ecs:RunTask", true}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + if got := actionPermitted(tc.allowed, tc.action); got != tc.want { + t.Errorf("actionPermitted(%q, %q) = %v, want %v", tc.allowed, tc.action, got, tc.want) + } + }) + } +} + +// crossPrincipalEscapeActions are actions that let whoever holds them execute +// code as a DIFFERENT principal, which is a complete exit from this boundary +// rather than a widening within it: a permissions boundary constrains the +// principal it is attached to and does not follow the principal the code ends +// up running as. None of them needs iam:PassRole, so PassRoleCeiling's cudly-* +// scoping does not constrain them either (#1723). +// +// The value is the principal the escape lands on, quoted in the failure so the +// next reader does not have to rediscover why the entry is here. +var crossPrincipalEscapeActions = map[string]string{ + "ecs:ExecuteCommand": "the task role of the running task the command lands in", + "ecs:UpdateService": "the task role of the task definition revision the service is pointed at", + "lambda:UpdateFunctionCode": "the execution role of the function whose code is replaced", + "lambda:UpdateFunctionConfiguration": "the execution role of the function whose handler, layers or environment is replaced", + "ssm:SendCommand": "the instance profile of the SSM-managed instance the command runs on", + "ssm:StartSession": "the instance profile of the SSM-managed instance the session opens on", +} + +// TestBoundaryDeniesCrossPrincipalEscapes asserts policy_boundary.tf permits +// none of crossPrincipalEscapeActions. It is the negative half of the coverage +// check above: that one fails when the ceiling is too narrow for what the +// modules need, this one fails when it is wide enough to leave the boundary. +// +// It keys off actionPermitted rather than off the literal strings "ecs:*", +// "lambda:*" and "ssm:*", because the ways to reopen this are not limited to +// restoring those three tokens: "*", "lambda:Update*" and a plain +// "ssm:SendCommand" entry all permit an escape while leaving a check written +// against the wildcard spellings green. +func TestBoundaryDeniesCrossPrincipalEscapes(t *testing.T) { + allowed := boundaryAllowedActions(t) + + escapes := make([]string, 0, len(crossPrincipalEscapeActions)) + for action := range crossPrincipalEscapeActions { + escapes = append(escapes, action) + } + sort.Strings(escapes) + + for _, action := range escapes { + if actionPermitted(allowed, action) { + t.Errorf("%s permits %s, which runs code as %s. A permissions boundary caps the principal it is attached to and does not follow a different one, so this is not a widening of the ceiling but an exit from it, and it needs no iam:PassRole, so PassRoleCeiling does not constrain it either. The ceiling currently allows %v (#1723)", boundaryFile, action, crossPrincipalEscapeActions[action], allowed) + } + } +} + +// ssmManagedInstanceCorePolicyARN is the one AWS managed policy whose contents +// policy_boundary.tf enumerates action by action instead of covering with a +// service wildcard, because ssm at service width is a cross-principal escape +// (see crossPrincipalEscapeActions). It is attached to the fck-nat instance +// role in terraform/modules/networking/aws. +const ssmManagedInstanceCorePolicyARN = "arn:aws:iam::aws:policy/AmazonSSMManagedInstanceCore" + +// ssmManagedInstanceCoreActions is AmazonSSMManagedInstanceCore v2 (the current +// default version; created 2019-03-15, last edited 2019-05-23) verbatim, per +// https://docs.aws.amazon.com/aws-managed-policy/latest/reference/AmazonSSMManagedInstanceCore.html +// +// This list is hardcoded because its source of truth is AWS, not this repo, so +// unlike every other assertion in this file it cannot be re-derived from the +// tree. That makes it the one thing here that can go stale without any local +// edit: if AWS adds an action to the policy, the SSM agent on the fck-nat +// instance calls it, the boundary does not cover it, and the call 403s at +// RUNTIME with `terraform apply` still green. Re-check the document above when +// touching this. +var ssmManagedInstanceCoreActions = []string{ + "ec2messages:AcknowledgeMessage", + "ec2messages:DeleteMessage", + "ec2messages:FailMessage", + "ec2messages:GetEndpoint", + "ec2messages:GetMessages", + "ec2messages:SendReply", + "ssm:DescribeAssociation", + "ssm:DescribeDocument", + "ssm:GetDeployablePatchSnapshotForInstance", + "ssm:GetDocument", + "ssm:GetManifest", + "ssm:GetParameter", + "ssm:GetParameters", + "ssm:ListAssociations", + "ssm:ListInstanceAssociations", + "ssm:PutComplianceItems", + "ssm:PutConfigurePackageResult", + "ssm:PutInventory", + "ssm:UpdateAssociationStatus", + "ssm:UpdateInstanceAssociationStatus", + "ssm:UpdateInstanceInformation", + "ssmmessages:CreateControlChannel", + "ssmmessages:CreateDataChannel", + "ssmmessages:OpenControlChannel", + "ssmmessages:OpenDataChannel", +} + +// TestBoundaryCoversSSMManagedInstanceCore closes the one hole +// TestBoundaryCoversWorkloadServices cannot see. That test derives what the +// ceiling must cover from the Action lists in terraform/modules, but a role's +// identity policy is the union of its inline policies AND the AWS managed +// policies attached to it, and a managed policy's actions appear nowhere in +// this tree. ssm is the only service where that matters, because it is the only +// one whose entire need comes from a managed policy and which is pinned per +// action rather than covered by a wildcard. +func TestBoundaryCoversSSMManagedInstanceCore(t *testing.T) { + if attached := moduleAttachedManagedPolicyARNs(t); !attached[ssmManagedInstanceCorePolicyARN] { + t.Fatalf("no module under %s attaches %s any more, so the ssm: entries pinned in %s and the list in this test are now a grant nothing needs. Drop both in the same change as the attachment", modulesDir, ssmManagedInstanceCorePolicyARN, boundaryFile) + } + + allowed := boundaryAllowedActions(t) + + var missing []string + for _, action := range ssmManagedInstanceCoreActions { + if !actionPermitted(allowed, action) { + missing = append(missing, action) + } + } + if len(missing) > 0 { + t.Errorf("%s does not cover %v, granted by %s which %s/networking/aws attaches to the fck-nat instance role. A permissions boundary caps effective permissions to the intersection of the role's identity policy and this ceiling, so `terraform apply` stays green and the SSM agent 403s at RUNTIME, taking Session Manager access to the NAT instance with it. The ceiling currently allows %v", boundaryFile, missing, ssmManagedInstanceCorePolicyARN, modulesDir, allowed) + } +} + // sortedKeys returns the keys of set, sorted, for stable failure messages. func sortedKeys(set map[string]bool) []string { out := make([]string, 0, len(set)) @@ -508,15 +735,19 @@ func sortedKeys(set map[string]bool) []string { return out } -// TestBoundaryCoversWorkloadServices re-derives the set of AWS service -// prefixes granted to workload roles by the Terraform modules under -// terraform/modules/**/aws and asserts policy_boundary.tf's -// WorkloadServiceCeiling covers every one of them (via a ":*" wildcard -// or an explicit ":" grant for that exact action), except iam -// (see iamServiceExemption). A service missing here deploys cleanly and then +// TestBoundaryCoversWorkloadServices re-derives every action granted to a +// workload role by the Terraform modules under terraform/modules/**/aws and +// asserts policy_boundary.tf's Allow statements permit each one, whether +// through a ":*" wildcard or an entry for that exact action, except iam +// (see iamServiceExemption). An action missing here deploys cleanly and then // 403s the first time the workload role calls it, because a permissions // boundary caps effective permissions to the intersection of the role's // identity policy and this ceiling (#1705). +// +// It sees only what this tree declares. The other half of a workload role's +// identity policy is the AWS managed policies the modules attach, whose actions +// appear nowhere in the tree; TestBoundaryCoversSSMManagedInstanceCore covers +// the one of those the ceiling enumerates rather than wildcards. func TestBoundaryCoversWorkloadServices(t *testing.T) { files := findAwsModuleFiles(t) if len(files) == 0 { @@ -540,15 +771,27 @@ func TestBoundaryCoversWorkloadServices(t *testing.T) { t.Fatalf("parsed zero IAM actions out of %d aws module files; the Action/actions extraction in this test is broken and would otherwise pass vacuously", len(files)) } - boundaryContent := readPolicySource(t, boundaryFile) + allowed := boundaryAllowedActions(t) - // iam is exempt from the loop below (see iamServiceExemption), but - // granting "iam:*" in the ceiling would silently defeat the entire - // boundary: a boundaried role's effective IAM permissions would then be - // whatever its identity policy allows, with no cap at all. Assert the - // exemption stays narrow (PassRoleCeiling only), not wide open. - if strings.Contains(boundaryContent, `"iam:*"`) { - t.Errorf("%s grants \"iam:*\" in the WorkloadServiceCeiling statement; that defeats the boundary entirely, because iam is otherwise deliberately omitted from the ceiling so no workload role can create a role, attach a policy, or touch a permissions boundary, no matter what its identity policy says. iam:PassRole is meant to be covered only by the scoped PassRoleCeiling statement", boundaryFile) + // iam is exempt from the loop below (see iamServiceExemption), but an + // iam grant wider than iam:PassRole in the ceiling would silently defeat + // the entire boundary: a boundaried role's effective IAM permissions + // would then be whatever its identity policy allows, with no cap at all. + // Assert the exemption stays narrow (PassRoleCeiling only), not wide + // open. A bare "*" is checked with it because it is the same hole + // spelled shorter, and it would also make every other assertion in this + // file pass vacuously. + for _, action := range allowed { + if action == "*" { + t.Errorf("%s grants \"*\" in an Allow statement; that is not a ceiling at all, it caps nothing and makes every other coverage and escape assertion in this file pass vacuously", boundaryFile) + continue + } + // Case-folded for the reason actionPermitted is: IAM action + // matching is case-insensitive, so "IAM:CreateRole" is the same + // grant as "iam:CreateRole" and must not slip past this check. + if strings.HasPrefix(strings.ToLower(action), iamServiceExemption+":") && !strings.EqualFold(action, "iam:PassRole") { + t.Errorf("%s grants %s; iam is deliberately omitted from the ceiling so no workload role can create a role, attach a policy, or touch a permissions boundary, no matter what its identity policy says. iam:PassRole is the one exception and is meant to be covered only by the scoped PassRoleCeiling statement", boundaryFile, action) + } } services := make([]string, 0, len(serviceActions)) @@ -562,21 +805,15 @@ func TestBoundaryCoversWorkloadServices(t *testing.T) { continue } - wildcard := `"` + svc + `:*"` - if strings.Contains(boundaryContent, wildcard) { - continue - } - var missing []string for action := range serviceActions[svc] { - literal := `"` + action + `"` - if !strings.Contains(boundaryContent, literal) { + if !actionPermitted(allowed, action) { missing = append(missing, action) } } if len(missing) > 0 { sort.Strings(missing) - t.Errorf("policy_boundary.tf's WorkloadServiceCeiling does not cover service %q: found neither %s nor an explicit grant for %v (used by terraform/modules). Add the service to WorkloadServiceCeiling in %s in the same change, or the workload role that calls one of these actions will deploy cleanly and then 403 at runtime, because a permissions boundary caps effective permissions to the intersection of the role's identity policy and this ceiling", svc, wildcard, missing, boundaryFile) + t.Errorf("%s does not cover %v (granted to a workload role by terraform/modules): its Allow statements permit %v, which matches neither %q:* nor those actions individually. Add them to WorkloadServiceCeiling in the same change, or the workload role that calls one will deploy cleanly and then 403 at runtime, because a permissions boundary caps effective permissions to the intersection of the role's identity policy and this ceiling", boundaryFile, missing, allowed, svc) } } } diff --git a/terraform/environments/aws/ci-cd-permissions/policy_iam.tf b/terraform/environments/aws/ci-cd-permissions/policy_iam.tf index 081389723..d50ac783f 100644 --- a/terraform/environments/aws/ci-cd-permissions/policy_iam.tf +++ b/terraform/environments/aws/ci-cd-permissions/policy_iam.tf @@ -23,6 +23,19 @@ # reached from a workload role because those are capped by the boundary. # Denying policy creation instead would break every apply that manages the # module-level managed policy in modules/secrets/aws. +# +# The deploy role also keeps iam:AddRoleToInstanceProfile (policy_data.tf, +# IAMRolesAndPolicies, scoped to arn:aws:iam::*:instance-profile/cudly-* and +# arn:aws:iam::*:role/cudly-*). That action supports no condition key at all, so +# it cannot be gated on the target role carrying the boundary the way +# iam:AttachRolePolicy is below, and it is left unconditioned. It is not +# exploitable on its own: putting a role into an instance profile only matters +# once that profile reaches an instance, and that needs iam:PassRole, which +# IAMPassRoleScopedByService in policy_data.tf scopes to cudly-* roles. Its +# residual is the same one PassRoleCeiling in policy_boundary.tf already admits, +# namely a cudly-* role created by hand or from the console that carries no +# boundary; every cudly-* role Terraform manages is boundaried after the first +# apply (#1723). resource "aws_iam_policy" "iam" { name = "cudly-deploy-iam" description = "CUDly Terraform deploy: IAM role mutation gated on the cudly-deploy-boundary permissions boundary" From 6073a1a6cab8d0b312bf268baf4921bc18b644bd Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 14 Aug 2026 01:13:32 +0200 Subject: [PATCH 2/5] test(iac/aws): fail closed on unparseable boundary statements; pin PassRole scope The boundary guard's Allow-statement parser failed OPEN on three statement forms: a NotAction ceiling, a whole statement written on one line, and a non-literal `Action = local.x`. Each contributed zero actions to the derived set and was silently treated as granting nothing, so appending one `Effect = "Allow"` statement in any of those forms left all four boundary tests passing while the ceiling granted everything AWS has except IAM. `terraform fmt -check` accepts all three, so pre-commit did not compensate, and AWS treats NotAction as a first-class element. boundaryAllowedActions now treats every statement that is not positively a Deny as part of the ceiling, and fails outright when one uses NotAction or yields zero actions, so "I could not parse this" and "this grants nothing" become the same loud failure instead of a silent widening. That closes the class rather than the three enumerated evasions. Also: - pin PassRoleCeiling's Resource and DenyPassDeployRole's Effect. Both are load-bearing premises with no guard: widening the former to "*" and flipping the latter to Allow each left all 12 tests green. - add the hyphen to the service half of actionStringPattern. Every hyphenated AWS service prefix (application-autoscaling, execute-api, aws-marketplace) was dropped from the derived set, under-reporting what the ceiling must cover, which is the runtime-403 direction. - add the hyphen to iamRoleResourcePattern; Terraform resource names permit one, so an unboundaried aws_iam_role with a hyphenated name was invisible to TestEveryModuleRoleHasPermissionsBoundary. - add ecs:RegisterTaskDefinition and the ssm association/automation verbs to crossPrincipalEscapeActions. policy_boundary.tf names the first as deliberately absent, and the others reach a different instance profile with no iam:PassRole exactly as ssm:SendCommand does. - correct four comments: ecs:RunTask requires iam:PassRole in every form, not only the override form (the four EventBridge invoker roles do a no-override RunTask and all carry it, which would be dead code otherwise); document the lambda:InvokeFunction residual the way the ecs:RunTask one is; record that all four attached managed policies were checked, not just one; and narrow an over-broad condition-key claim in policy_iam.tf. Refs #1723 --- .../aws/ci-cd-permissions/policy_boundary.tf | 44 ++++- .../ci-cd-permissions/policy_guard_test.go | 157 +++++++++++++++++- .../aws/ci-cd-permissions/policy_iam.tf | 6 +- 3 files changed, 192 insertions(+), 15 deletions(-) diff --git a/terraform/environments/aws/ci-cd-permissions/policy_boundary.tf b/terraform/environments/aws/ci-cd-permissions/policy_boundary.tf index 794bc55ab..aeee4534f 100644 --- a/terraform/environments/aws/ci-cd-permissions/policy_boundary.tf +++ b/terraform/environments/aws/ci-cd-permissions/policy_boundary.tf @@ -53,6 +53,20 @@ # any workload role can carry. Effective permissions are identity AND ceiling, # so a ceiling that already covers the whole identity side changes nothing. # +# The managed-policy half of that claim was checked against all four documents +# the modules attach, not inferred: AmazonSSMManagedInstanceCore (ssm, +# ssmmessages, ec2messages), AWSLambdaBasicExecutionRole (logs), +# AWSLambdaVPCAccessExecutionRole (logs, ec2) and +# AmazonECSTaskExecutionRolePolicy (ecr, logs, and no ecs action despite the +# name). Only the first grants an ecs, lambda or ssm action, and it is the one +# enumerated verbatim below; logs, ec2 and ecr stay wildcarded here, so no +# managed-policy path can 403 as a result of the #1723 narrowing. AWS owns +# those documents, so only AmazonSSMManagedInstanceCore is guarded in CI (see +# TestBoundaryCoversSSMManagedInstanceCore); for the other three the exposure +# is "AWS edits an existing policy", since a NEW attachment is caught by +# TestDeployPolicyAllowsAttachedManagedPolicies. Re-check them when touching +# this. +# # DRIFT IS GUARDED IN CI, in both directions, by policy_guard_test.go in this # directory. TestBoundaryCoversWorkloadServices re-derives the action set from # the IAM policy documents in terraform/modules and fails if this document does @@ -86,11 +100,17 @@ resource "aws_iam_policy" "workload_boundary" { # policy. RunTask keeps a RESIDUAL this ceiling does not close: # running an ALREADY-REGISTERED task definition unchanged runs as # that definition's task role, and the ceiling's Resource is "*". - # Overriding overrides.taskRoleArn / overrides.executionRoleArn - # needs iam:PassRole, which PassRoleCeiling scopes to cudly-*, so - # only the no-override form is reachable, and only against a task - # definition that already exists and already carries a role more - # privileged than this ceiling. It is deliberately NOT closed by + # RunTask requires iam:PassRole on the task definition's task and + # execution roles in EVERY form, not just when + # overrides.taskRoleArn / overrides.executionRoleArn are supplied: + # the four EventBridge invoker roles in + # modules/compute/aws/fargate do a plain no-override RunTask (their + # ecs_target passes only containerOverrides.command) and every one + # of them carries iam:PassRole on both role ARNs, which would be + # dead code otherwise. So PassRoleCeiling confines the residual to + # task definitions whose task and execution roles are already + # cudly-*, i.e. already boundaried by everything above; the + # no-override form is not exempt. It is deliberately NOT closed by # resource-scoping the way CrossAccountAssumeRoleCeiling is: the # family is local.name_prefix, i.e. "-fargate", and # stack_name defaults to project_name-environment-, so @@ -107,6 +127,20 @@ resource "aws_iam_policy" "workload_boundary" { # lambda:InvokeFunctionUrl in aws_lambda_permission blocks are # RESOURCE policies granting a service principal, not grants to a # workload role, so this ceiling never gates them. + # lambda:InvokeFunction keeps a RESIDUAL of the same shape as + # ecs:RunTask's, stated here so it is not read as fully closed: + # this statement's Resource is "*", so it permits invoking ANY + # function in the account, which runs that function's code as its + # execution role with an attacker-chosen payload and needs no + # iam:PassRole. Functions this deploy role did not create carry no + # boundary. It is narrower than it looks, because the module grants + # are themselves scoped (modules/compute/aws/lambda/main.tf uses + # aws_lambda_function.main.arn, signing-key.tf uses + # arn:aws:lambda:*:*:function:${stack_name}-api*) and effective + # permissions are the intersection, so the residual is only + # reachable by a role whose own identity policy is wider. Scoping + # this entry to arn:aws:lambda:*:*:function:cudly-* would close it + # and is tracked separately rather than folded into #1723. # - the ssm entries are AmazonSSMManagedInstanceCore v2 verbatim (the # fck-nat instance role in modules/networking/aws is the only role # that carries it, for Session Manager access). No module grants an diff --git a/terraform/environments/aws/ci-cd-permissions/policy_guard_test.go b/terraform/environments/aws/ci-cd-permissions/policy_guard_test.go index 39cdef12a..a936346de 100644 --- a/terraform/environments/aws/ci-cd-permissions/policy_guard_test.go +++ b/terraform/environments/aws/ci-cd-permissions/policy_guard_test.go @@ -178,7 +178,18 @@ var actionAssignmentPattern = regexp.MustCompile(`(?m)^[ \t]*(?:Action|actions)\ // in a set the star never entered. Both read exactly like a clean ceiling. // Mutation M6 in the #1723 verification caught this; it is the reason the // pattern is an alternation rather than one branch. -var actionStringPattern = regexp.MustCompile(`"([A-Za-z0-9]+:[A-Za-z0-9_*]+|\*)"`) +// +// The hyphen in the SERVICE half is load-bearing for the same reason, in the +// same direction. AWS service prefixes are not all alphanumeric: +// application-autoscaling (the ECS service auto-scaling grant, on the Fargate +// deploy path), execute-api, aws-marketplace, s3-object-lambda, +// network-firewall and resource-groups all carry one. Without it, a module +// granting application-autoscaling:RegisterScalableTarget to a workload role +// contributes nothing to TestBoundaryCoversWorkloadServices' derived set, so +// the ceiling is never required to cover it and the role 403s at RUNTIME with +// the apply green: the under-reporting direction, which is the failure mode +// this file is organised to avoid. +var actionStringPattern = regexp.MustCompile(`"([A-Za-z0-9-]+:[A-Za-z0-9_*]+|\*)"`) // resourceAssignmentPattern matches a `Resource = [...]` or `Resource = "..."` // assignment inside a single statement block and captures the value. @@ -524,6 +535,31 @@ func equalStringSets(got, want []string) bool { // document carries (DenyPassDeployRole) is resource-scoped, so "the action // appears in a Deny" does not imply the action is unreachable, and pretending // otherwise would produce a wrong answer in the direction that hides a gap. +// +// IT FAILS CLOSED, WHICH IS THE WHOLE DESIGN. Every statement that is not +// positively identified as a Deny is treated as part of the ceiling and must +// yield at least one action, and no such statement may use NotAction: +// +// - NotAction expresses a ceiling as "everything except", which no assertion +// in this file can check. It is also the FIRST thing a future author asked +// to stop maintaining a 34-entry allow list would reach for, and +// `NotAction = ["iam:*"]` with `Resource = "*"` grants every service AWS +// has. terraform fmt and terraform validate both accept it (inside +// jsonencode these are ordinary map keys) and AWS treats it as a +// first-class IAM element, so CI is the only place it can be refused. +// - a statement yielding zero actions is either a real grant this file's +// patterns could not parse (a whole statement written on one line, past the +// ^-anchored Action/Effect patterns; `Action = local.something`, past the +// literal-only extractor) or a statement that grants nothing. Both must be +// the same loud failure, because the parser cannot tell them apart and the +// "could not parse" half silently widens the ceiling to whatever the +// statement says. Collapsing the two is what makes this closed against +// forms nobody has thought of yet, rather than against the three that have +// been enumerated. +// +// Refusing an unparseable statement outright costs nothing legitimate: every +// statement in this document is one attribute per line with a literal Action +// list, which is the form terraform fmt produces. func boundaryAllowedActions(t *testing.T) []string { t.Helper() @@ -534,13 +570,20 @@ func boundaryAllowedActions(t *testing.T) []string { var allowed []string for _, stmt := range stmts { - if statementEffect(stmt) != "Allow" { + if statementEffect(stmt) == "Deny" { continue } - allowed = append(allowed, extractActionListActions(stmt)...) + if strings.Contains(stmt, "NotAction") { + t.Fatalf("%s: statement %q uses NotAction, which caps nothing this test can check: a NotAction ceiling grants every service except the ones it names, so `NotAction = [\"iam:*\"]` on Resource \"*\" reopens every cross-principal escape #1723, #1722 and #1705 closed while every other assertion in this file stays green. Express the ceiling as an allow list. Statement: %q", boundaryFile, statementSid(stmt), stmt) + } + actions := extractActionListActions(stmt) + if len(actions) == 0 { + t.Fatalf("%s: statement %q is not a Deny and yields zero actions, so it either grants something this test cannot parse or grants nothing at all, and there is no way to tell which. Both are refused: an unparsed grant is a silent widening of the ceiling. Write it as one attribute per line with a literal Action list (an inline one-line statement hides Effect and Action from the ^-anchored patterns above, and `Action = local.x` hides them from the literal extractor). Statement: %q", boundaryFile, statementSid(stmt), stmt) + } + allowed = append(allowed, actions...) } if len(allowed) == 0 { - t.Fatalf("%s: parsed zero actions out of its Allow statements; every coverage assertion below would otherwise report the whole ceiling as missing, or (in the escape direction) report every escape as closed", boundaryFile) + t.Fatalf("%s: every statement in it is a Deny, so it grants nothing and is not a ceiling; every coverage assertion below would otherwise report the whole ceiling as missing, or (in the escape direction) report every escape as closed", boundaryFile) } sort.Strings(allowed) return allowed @@ -613,18 +656,32 @@ func TestActionPermittedMatchesWholeActionCaseInsensitively(t *testing.T) { // code as a DIFFERENT principal, which is a complete exit from this boundary // rather than a widening within it: a permissions boundary constrains the // principal it is attached to and does not follow the principal the code ends -// up running as. None of them needs iam:PassRole, so PassRoleCeiling's cudly-* -// scoping does not constrain them either (#1723). +// up running as (#1723). +// +// All but one need no iam:PassRole, so PassRoleCeiling's cudly-* scoping does +// not constrain them at all. ecs:RegisterTaskDefinition is the exception: a +// revision naming a task or execution role needs PassRole for it, which is why +// the deploy role's own iam:PassRole is scoped to ecs-tasks.amazonaws.com in +// policy_data.tf beside its ecs:RegisterTaskDefinition grant in +// policy_compute.tf. So PassRoleCeiling confines that one to cudly-* rather +// than closing it, and a cudly-* role created by hand or from the console +// carries no boundary (the residual PassRoleCeiling's own comment admits). It +// is on this list because policy_boundary.tf states outright that it is +// absent, and a stated invariant with no guard is one edit from being untrue. // // The value is the principal the escape lands on, quoted in the failure so the // next reader does not have to rediscover why the entry is here. var crossPrincipalEscapeActions = map[string]string{ "ecs:ExecuteCommand": "the task role of the running task the command lands in", + "ecs:RegisterTaskDefinition": "the task role named by the new revision, which the ceiling's own ecs:RunTask then runs", "ecs:UpdateService": "the task role of the task definition revision the service is pointed at", "lambda:UpdateFunctionCode": "the execution role of the function whose code is replaced", "lambda:UpdateFunctionConfiguration": "the execution role of the function whose handler, layers or environment is replaced", + "ssm:CreateAssociation": "the instance profile of every instance the association binds its document to", "ssm:SendCommand": "the instance profile of the SSM-managed instance the command runs on", + "ssm:StartAutomationExecution": "the instance profile of the instances the automation document targets", "ssm:StartSession": "the instance profile of the SSM-managed instance the session opens on", + "ssm:UpdateAssociation": "the instance profile of every instance the retargeted association now runs on", } // TestBoundaryDeniesCrossPrincipalEscapes asserts policy_boundary.tf permits @@ -648,7 +705,7 @@ func TestBoundaryDeniesCrossPrincipalEscapes(t *testing.T) { for _, action := range escapes { if actionPermitted(allowed, action) { - t.Errorf("%s permits %s, which runs code as %s. A permissions boundary caps the principal it is attached to and does not follow a different one, so this is not a widening of the ceiling but an exit from it, and it needs no iam:PassRole, so PassRoleCeiling does not constrain it either. The ceiling currently allows %v (#1723)", boundaryFile, action, crossPrincipalEscapeActions[action], allowed) + t.Errorf("%s permits %s, which runs code as %s. A permissions boundary caps the principal it is attached to and does not follow a different one, so this is not a widening of the ceiling but an exit from it, and PassRoleCeiling does not close it (see crossPrincipalEscapeActions above for which entries need no iam:PassRole at all and which one it merely confines). The ceiling currently allows %v (#1723)", boundaryFile, action, crossPrincipalEscapeActions[action], allowed) } } } @@ -1087,7 +1144,13 @@ func TestBoundaryPolicyNameStaysInProtectedNamespace(t *testing.T) { // iamRoleResourcePattern matches an `resource "aws_iam_role" "" {` // block header and captures the role's local name. -var iamRoleResourcePattern = regexp.MustCompile(`resource\s+"aws_iam_role"\s+"([A-Za-z0-9_]+)"\s*\{`) +// +// The name class includes "-" because Terraform resource names permit it. The +// repo happens to use underscores everywhere today, which is exactly why the +// omission never fired: a role named "sneaky-role" with no permissions_boundary +// was invisible to TestEveryModuleRoleHasPermissionsBoundary, and the +// roleCount vacuity guard could not help because the other roles still counted. +var iamRoleResourcePattern = regexp.MustCompile(`resource\s+"aws_iam_role"\s+"([A-Za-z0-9_-]+)"\s*\{`) // permissionsBoundaryArgumentPattern matches the exact // `permissions_boundary = var.permissions_boundary_arn` argument a module role @@ -1291,3 +1354,81 @@ func TestBoundaryMatchesCrossAccountRolePrefix(t *testing.T) { t.Errorf("%s: CrossAccountAssumeRoleCeiling caps sts:AssumeRole at %v, but %s defaults to %q in %v, so the modules grant %q. A permissions boundary caps by intersection: this mismatch is INVISIBLE at `terraform apply` (both documents are written exactly as configured, the apply is green) and surfaces as a RUNTIME AccessDenied the first time CUDly assumes a role in a linked account, i.e. cross-account cost collection silently stops working in the deployed environment. Change both in the same commit", boundaryFile, got, crossAccountPrefixVariable, prefix, crossAccountModuleVariableFiles, want) } } + +// passRoleAction is the one IAM action policy_boundary.tf grants, in +// PassRoleCeiling, and the one it denies, in DenyPassDeployRole. Both +// statements are located by their Action set rather than by their Sid, for the +// reason boundaryGatedRoleMutationActions gives: a Sid is free text and +// renaming one must not silently skip the check. +const passRoleAction = "iam:PassRole" + +// passRoleCeilingResource is the Resource PassRoleCeiling must be scoped to. +// It is spelled out here rather than shared with boundaryGatedRoleMutationResource +// (which happens to be the same string) because they are separate invariants in +// separate documents: one is what the deploy role may mutate, this one is what +// a boundaried workload role may pass. +const passRoleCeilingResource = "arn:aws:iam::*:role/cudly-*" + +// deployRoleResource is the exact ARN of the deploy role DenyPassDeployRole +// protects. Unlike passRoleCeilingResource it carries no wildcard, which is +// what stops the IAM-path trick (arn:aws:iam::123:role/cudly-x/EvilRole +// satisfies cudly-*) from evading the Deny. +const deployRoleResource = "arn:aws:iam::*:role/cudly-terraform-deploy" + +// TestBoundaryScopesPassRoleToCudlyRoles pins the Resource of PassRoleCeiling, +// which is the premise the rest of this boundary reasons from rather than an +// ordinary drift check. +// +// Three separate arguments in policy_boundary.tf and policy_iam.tf are only +// valid while iam:PassRole is capped at cudly-*: that the ecs:RunTask residual +// reaches only task definitions whose roles are already boundaried; that the +// three services narrowed in #1723 are the ones PassRole scoping does NOT +// constrain (implying it constrains the others); and that +// iam:AddRoleToInstanceProfile is safe unconditioned because reaching an +// instance needs a scoped PassRole. Widening this Resource to "*" invalidates +// all three at once and re-opens the escalation the boundary exists to close: +// a boundaried role could hand an unrelated administrator role to a new Lambda +// and run as it. Every other test in this file stays green while that happens, +// because they are action-set guards. +func TestBoundaryScopesPassRoleToCudlyRoles(t *testing.T) { + allows := findStatements(t, boundaryFile, func(stmt string) bool { + return statementEffect(stmt) == "Allow" && equalStringSets(statementActions(stmt), []string{passRoleAction}) + }) + if len(allows) != 1 { + t.Fatalf("%s: found %d Allow statements whose Action set is exactly [%q], want exactly 1 (PassRoleCeiling). A second one widens the cap by union, and none at all takes down the four EventBridge invoker roles that pass the task and task-execution roles to ecs:RunTask", boundaryFile, len(allows), passRoleAction) + } + + if got := statementResources(allows[0]); !equalStringSets(got, []string{passRoleCeilingResource}) { + t.Errorf("%s: statement %q caps %s at Resource %v, want exactly [%q]. Widening it (to \"*\" above all) lets a boundaried workload role pass ANY role in the account to a new Lambda or task and run as it, which is a complete exit from this boundary and not a widening within it; narrowing it 403s the EventBridge invoker roles at RUNTIME with `terraform apply` still green", boundaryFile, statementSid(allows[0]), passRoleAction, got, passRoleCeilingResource) + } +} + +// TestBoundaryDeniesPassingTheDeployRole pins the Deny that stops PassRoleCeiling +// covering cudly-terraform-deploy itself, which is a cudly-* role and therefore +// matches the Allow above. Without it a boundaried workload role could pass the +// deploy role to a Lambda and inherit it: a workload -> deploy escalation, the +// same shape as the #542 self-pass loop. +// +// It locates the statement by Action set AND Resource rather than by Effect, so +// flipping Deny to Allow leaves it matched and fails on the Effect assertion +// instead of quietly matching nothing. +func TestBoundaryDeniesPassingTheDeployRole(t *testing.T) { + denies := findStatements(t, boundaryFile, func(stmt string) bool { + if !equalStringSets(statementActions(stmt), []string{passRoleAction}) { + return false + } + for _, r := range statementResources(stmt) { + if r == deployRoleResource { + return true + } + } + return false + }) + if len(denies) != 1 { + t.Fatalf("%s: found %d statements covering %s on %q, want exactly 1 (DenyPassDeployRole). PassRoleCeiling allows cudly-*, and the deploy role is a cudly-* role, so without this statement a boundaried workload role may pass cudly-terraform-deploy to a Lambda and run as it", boundaryFile, len(denies), passRoleAction, deployRoleResource) + } + + if effect := statementEffect(denies[0]); effect != "Deny" { + t.Errorf("%s: statement %q has Effect %q, want \"Deny\". As an Allow it is not merely inert, it is redundant with PassRoleCeiling and removes the one thing stopping a workload -> deploy escalation; an explicit Deny is what beats the cudly-* Allow", boundaryFile, statementSid(denies[0]), effect) + } +} diff --git a/terraform/environments/aws/ci-cd-permissions/policy_iam.tf b/terraform/environments/aws/ci-cd-permissions/policy_iam.tf index d50ac783f..60e5c0a7d 100644 --- a/terraform/environments/aws/ci-cd-permissions/policy_iam.tf +++ b/terraform/environments/aws/ci-cd-permissions/policy_iam.tf @@ -26,8 +26,10 @@ # # The deploy role also keeps iam:AddRoleToInstanceProfile (policy_data.tf, # IAMRolesAndPolicies, scoped to arn:aws:iam::*:instance-profile/cudly-* and -# arn:aws:iam::*:role/cudly-*). That action supports no condition key at all, so -# it cannot be gated on the target role carrying the boundary the way +# arn:aws:iam::*:role/cudly-*). That action supports no SERVICE-SPECIFIC +# condition key (the global keys, aws:RequestedRegion, aws:PrincipalTag and +# aws:ResourceTag among them, apply to every IAM action, but none of them can +# express "the target role carries this boundary"), so it cannot be gated the way # iam:AttachRolePolicy is below, and it is left unconditioned. It is not # exploitable on its own: putting a role into an instance profile only matters # once that profile reaches an instance, and that needs iam:PassRole, which From 5eba1c3c9a62a26e7467a052f7c4dccc0153d310 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sat, 15 Aug 2026 18:29:09 +0200 Subject: [PATCH 3/5] test(iac/aws): refuse boundary statements with unparsed action strings The zero-action guard only fires when a statement parses to nothing at all, so a mixed Action list walks through it. Action = ["s3:GetObject", "*:*"] parses to one S3 read because "*:*" fails the extractor's SERVICE half and is dropped silently, and the ceiling that grants everything is then measured as granting one action: TestBoundaryDeniesCrossPrincipalEscapes reports every escape closed. Action = ["s3:GetObject", "iam*"] is the same hole against the iam narrowness check, which is prefix-based on "iam:" and never sees an entry the extractor already threw away. Count the quoted strings in each Action assignment and refuse any statement where fewer were parsed than were written. Counting rather than widening the pattern is deliberate: neither form belongs in a ceiling, so the test must refuse them, not learn to read them. --- .../ci-cd-permissions/policy_guard_test.go | 43 ++++++++++++++++++- 1 file changed, 41 insertions(+), 2 deletions(-) diff --git a/terraform/environments/aws/ci-cd-permissions/policy_guard_test.go b/terraform/environments/aws/ci-cd-permissions/policy_guard_test.go index a936346de..6d1fab486 100644 --- a/terraform/environments/aws/ci-cd-permissions/policy_guard_test.go +++ b/terraform/environments/aws/ci-cd-permissions/policy_guard_test.go @@ -228,6 +228,29 @@ func extractActionListActions(content string) []string { return actions } +// anyQuotedStringPattern matches every quoted string in an Action list, +// including the forms actionStringPattern deliberately refuses. +var anyQuotedStringPattern = regexp.MustCompile(`"[^"]*"`) + +// countActionListStrings returns how many quoted strings a statement's Action +// assignments contain, parseable by actionStringPattern or not. +// +// It exists so callers can compare it against len(extractActionListActions) and +// refuse any statement where the two disagree. Widening actionStringPattern +// until it accepts every form somebody might write is the losing half of that +// trade: "*:*" and "iam*" are grants that must never appear in a ceiling, so the +// fix is for the test to REFUSE them loudly, not to learn to parse them. +// Comparing the counts turns "the pattern did not recognise this entry" into a +// failure rather than a silent omission, which closes the class instead of the +// two forms currently known. +func countActionListStrings(stmt string) int { + n := 0 + for _, m := range actionAssignmentPattern.FindAllStringSubmatch(stmt, -1) { + n += len(anyQuotedStringPattern.FindAllString(m[1], -1)) + } + return n +} + // findAwsModuleFiles returns every *.tf file under modulesDir whose // directory path contains an "aws" path segment, i.e. the AWS-specific // Terraform modules and not their Azure/GCP siblings. @@ -537,8 +560,9 @@ func equalStringSets(got, want []string) bool { // otherwise would produce a wrong answer in the direction that hides a gap. // // IT FAILS CLOSED, WHICH IS THE WHOLE DESIGN. Every statement that is not -// positively identified as a Deny is treated as part of the ceiling and must -// yield at least one action, and no such statement may use NotAction: +// positively identified as a Deny is treated as part of the ceiling, must have +// every one of its action strings parsed, and no such statement may use +// NotAction: // // - NotAction expresses a ceiling as "everything except", which no assertion // in this file can check. It is also the FIRST thing a future author asked @@ -556,6 +580,18 @@ func equalStringSets(got, want []string) bool { // statement says. Collapsing the two is what makes this closed against // forms nobody has thought of yet, rather than against the three that have // been enumerated. +// - a statement whose Action list holds MORE quoted strings than the extractor +// parsed is refused for the same reason, one entry at a time rather than one +// statement at a time. The zero-action guard above only sees a statement +// that parsed to nothing at all, so `Action = ["s3:GetObject", "*:*"]` walks +// straight through it: "*:*" fails actionStringPattern's SERVICE half, is +// dropped, and a ceiling that grants literally everything is measured as +// granting one S3 read. `Action = ["s3:GetObject", "iam*"]` is the same hole +// against the iam narrowness check in TestBoundaryCoversWorkloadServices, +// which is prefix-based on "iam:" and therefore never sees a colon-less +// entry the extractor already threw away. Counting is the check rather than +// a wider pattern on purpose: neither form belongs in a ceiling, so the test +// must refuse them, not learn to read them. // // Refusing an unparseable statement outright costs nothing legitimate: every // statement in this document is one attribute per line with a literal Action @@ -577,6 +613,9 @@ func boundaryAllowedActions(t *testing.T) []string { t.Fatalf("%s: statement %q uses NotAction, which caps nothing this test can check: a NotAction ceiling grants every service except the ones it names, so `NotAction = [\"iam:*\"]` on Resource \"*\" reopens every cross-principal escape #1723, #1722 and #1705 closed while every other assertion in this file stays green. Express the ceiling as an allow list. Statement: %q", boundaryFile, statementSid(stmt), stmt) } actions := extractActionListActions(stmt) + if parsed, listed := len(actions), countActionListStrings(stmt); parsed != listed { + t.Fatalf("%s: statement %q lists %d action strings but this test parsed only %d of them; the unparsed entries are treated as absent from the ceiling, so a form like \"*:*\" or \"iam*\" widens the boundary while every assertion in this file stays green. Statement: %q", boundaryFile, statementSid(stmt), listed, parsed, stmt) + } if len(actions) == 0 { t.Fatalf("%s: statement %q is not a Deny and yields zero actions, so it either grants something this test cannot parse or grants nothing at all, and there is no way to tell which. Both are refused: an unparsed grant is a silent widening of the ceiling. Write it as one attribute per line with a literal Action list (an inline one-line statement hides Effect and Action from the ^-anchored patterns above, and `Action = local.x` hides them from the literal extractor). Statement: %q", boundaryFile, statementSid(stmt), stmt) } From c22efd43be7c827f6f049fdfc20a5b0e077a6ec8 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sat, 15 Aug 2026 19:26:55 +0200 Subject: [PATCH 4/5] test(iac/aws): close the same partial-drop hole in the condition and resource extractors The Action-list fix has two siblings of the same class, both confirmed live by mutation: an extractor filters a candidate set through a restrictive pattern, drops what does not match, and the caller treats the remainder as complete. conditionOperatorPattern is line-anchored, so an operator written after the previous one's closing brace is dropped and singleConditionOperator still sees exactly one. IAM ANDs the operators inside a Condition, so on a Deny the invisible one decides when the Deny fires: appending `}, StringEquals = { "aws:username" = "nobody-ever" }` to IAMDenyAttachUnapprovedManagedPolicy stops it ever firing and re-admits AdministratorAccess onto any cudly-* role. It survives terraform fmt, terraform validate and every assertion in the file. Compare the count of nested objects the Condition opens against the count parsed as operators. statementResources reads only the quoted entries of a Resource list, so a referenced element is invisible to the equality checks that pin every Resource in both documents: Resource = ["arn:aws:iam::*:role/cudly-*", local.x] compares EQUAL to the expected one-element set while the grant reaches local.x too. Action lists have the same hole, which counting quoted strings cannot see because a non-string element contributes to neither count. Assert the invariant once, over every statement in every document, rather than at each of the call sites that all consume the same blind extraction. --- .../ci-cd-permissions/policy_guard_test.go | 129 +++++++++++++++++- 1 file changed, 124 insertions(+), 5 deletions(-) diff --git a/terraform/environments/aws/ci-cd-permissions/policy_guard_test.go b/terraform/environments/aws/ci-cd-permissions/policy_guard_test.go index 6d1fab486..30659fe19 100644 --- a/terraform/environments/aws/ci-cd-permissions/policy_guard_test.go +++ b/terraform/environments/aws/ci-cd-permissions/policy_guard_test.go @@ -215,6 +215,13 @@ var conditionAssignmentPattern = regexp.MustCompile(`Condition\s*=\s*\{`) // colon and which must therefore not slip past a bare-identifier pattern. var conditionOperatorPattern = regexp.MustCompile(`(?m)^[ \t]*"?([A-Za-z0-9]+(?::[A-Za-z0-9]+)?)"?[ \t]*=[ \t]*\{`) +// anyBlockOpenPattern matches every `= {` inside a Condition body, including +// the ones conditionOperatorPattern refuses because they do not begin a line. +// It is the condition-block half of the same completeness check +// countActionListStrings performs on Action lists: see singleConditionOperator +// for what a dropped operator costs. +var anyBlockOpenPattern = regexp.MustCompile(`=[ \t]*\{`) + // extractActionListActions returns every "service:Action" literal found // strictly inside Action/actions assignments in content, per // actionAssignmentPattern above. @@ -251,6 +258,38 @@ func countActionListStrings(stmt string) int { return n } +// listStructurePattern matches the brackets, commas and whitespace an HCL list +// is built from, i.e. everything in a captured Action or Resource value that is +// not an element. +var listStructurePattern = regexp.MustCompile(`[\[\],\s]`) + +// unreadListElements returns the elements of the values assignPattern captures +// in stmt that are not quoted literals: local.x, var.y, a function call, +// anything whose value this test cannot know. It returns "" when every element +// is a literal. +// +// It is the other half of the completeness check countActionListStrings starts. +// Counting quoted strings catches an element written as a string the extractor +// refuses ("*:*", "iam*"); it cannot catch an element that is not a string at +// all, because such an element contributes to neither count. +// `Action = ["sts:GetCallerIdentity", var.x]` therefore parses one action out of +// two elements with both counts reading 1, and whatever var.x resolves to is +// absent from the derived ceiling. Resource lists have the same hole in the +// direction that matters more: statementResources reads only the quoted entries, +// so `Resource = ["arn:aws:iam::*:role/cudly-*", local.y]` compares EQUAL to the +// one-element set every Resource assertion in this file expects, while the +// grant it describes is whatever local.y adds. Both forms survive terraform fmt +// and terraform validate. +func unreadListElements(stmt string, assignPattern *regexp.Regexp) string { + var refs []string + for _, m := range assignPattern.FindAllStringSubmatch(stmt, -1) { + if r := listStructurePattern.ReplaceAllString(anyQuotedStringPattern.ReplaceAllString(m[1], ""), ""); r != "" { + refs = append(refs, r) + } + } + return strings.Join(refs, " ") +} + // findAwsModuleFiles returns every *.tf file under modulesDir whose // directory path contains an "aws" path segment, i.e. the AWS-specific // Terraform modules and not their Azure/GCP siblings. @@ -447,6 +486,17 @@ func statementResources(stmt string) []string { return resources } +// statementConditionBody returns a statement's Condition block verbatim, +// braces included, or "" if it has no Condition. +func statementConditionBody(stmt string) string { + loc := conditionAssignmentPattern.FindStringIndex(stmt) + if loc == nil { + return "" + } + open := loc[1] - 1 // the "{" the match ends on + return stmt[open:balancedBraceEnd(stmt, open)] +} + // conditionOperator is one `Operator = { ... }` entry inside a statement's // Condition block: its name verbatim (including any ForAllValues:/ForAnyValue: // qualifier) and the body it introduces. @@ -461,12 +511,10 @@ type conditionOperator struct { // a check that merely found "StringEquals" somewhere would stay green if a // second, permissive operator were added beside it. func statementConditionOperators(stmt string) []conditionOperator { - loc := conditionAssignmentPattern.FindStringIndex(stmt) - if loc == nil { + body := statementConditionBody(stmt) + if body == "" { return nil } - open := loc[1] - 1 // the "{" the match ends on - body := stmt[open:balancedBraceEnd(stmt, open)] var ops []conditionOperator for _, m := range conditionOperatorPattern.FindAllStringSubmatchIndex(body, -1) { @@ -484,10 +532,26 @@ func statementConditionOperators(stmt string) []conditionOperator { // is not want. want is compared verbatim, so StringEqualsIfExists, // ForAllValues:StringEquals and ArnEquals are all rejected against a want of // StringEquals / ArnNotEquals respectively. +// +// "Exactly one" is only worth as much as the extractor's completeness, so this +// also refuses a Condition body holding more `= {` openings than +// conditionOperatorPattern turned into operators. That pattern is ^-anchored, +// so an operator written on a line that starts with the previous operator's +// closing brace is dropped, and dropping one is a fail-open on a DENY: IAM ANDs +// the operators inside a Condition, so a second one the extractor cannot see +// narrows when the Deny fires without changing what this test measures. +// `}, StringEquals = { "aws:username" = "nobody-ever" }` appended to +// IAMDenyAttachUnapprovedManagedPolicy stops that Deny ever firing, which +// re-admits AdministratorAccess onto any cudly-* role (#1705 verbatim), and it +// survives terraform fmt, terraform validate and every assertion in this file. func singleConditionOperator(t *testing.T, stmt, want, why string) conditionOperator { t.Helper() ops := statementConditionOperators(stmt) + body := statementConditionBody(stmt) + if opens := len(anyBlockOpenPattern.FindAllString(body, -1)); opens != len(ops) { + t.Fatalf("%s: statement %q has a Condition block opening %d nested objects but this test parsed only %d of them as operators; an operator it cannot see is still ANDed into the condition by IAM, so it changes when this statement applies while every assertion here stays green. Write one operator per line. Condition: %q", iamFile, statementSid(stmt), opens, len(ops), body) + } if len(ops) != 1 { names := make([]string, 0, len(ops)) for _, op := range ops { @@ -591,7 +655,10 @@ func equalStringSets(got, want []string) bool { // which is prefix-based on "iam:" and therefore never sees a colon-less // entry the extractor already threw away. Counting is the check rather than // a wider pattern on purpose: neither form belongs in a ceiling, so the test -// must refuse them, not learn to read them. +// must refuse them, not learn to read them. An element that is not a quoted +// string at all (`Action = ["s3:GetObject", local.x]`) escapes both counts, +// since it contributes to neither, and is refused separately by +// unreadListElements. // // Refusing an unparseable statement outright costs nothing legitimate: every // statement in this document is one attribute per line with a literal Action @@ -613,6 +680,9 @@ func boundaryAllowedActions(t *testing.T) []string { t.Fatalf("%s: statement %q uses NotAction, which caps nothing this test can check: a NotAction ceiling grants every service except the ones it names, so `NotAction = [\"iam:*\"]` on Resource \"*\" reopens every cross-principal escape #1723, #1722 and #1705 closed while every other assertion in this file stays green. Express the ceiling as an allow list. Statement: %q", boundaryFile, statementSid(stmt), stmt) } actions := extractActionListActions(stmt) + if refs := unreadListElements(stmt, actionAssignmentPattern); refs != "" { + t.Fatalf("%s: statement %q has Action list elements that are not literal strings (%s); this test cannot know what they resolve to, so they are absent from the derived ceiling and every coverage and escape assertion below measures a narrower document than the one AWS will enforce. Write the ceiling as literal actions. Statement: %q", boundaryFile, statementSid(stmt), refs, stmt) + } if parsed, listed := len(actions), countActionListStrings(stmt); parsed != listed { t.Fatalf("%s: statement %q lists %d action strings but this test parsed only %d of them; the unparsed entries are treated as absent from the ceiling, so a form like \"*:*\" or \"iam*\" widens the boundary while every assertion in this file stays green. Statement: %q", boundaryFile, statementSid(stmt), listed, parsed, stmt) } @@ -1471,3 +1541,52 @@ func TestBoundaryDeniesPassingTheDeployRole(t *testing.T) { t.Errorf("%s: statement %q has Effect %q, want \"Deny\". As an Allow it is not merely inert, it is redundant with PassRoleCeiling and removes the one thing stopping a workload -> deploy escalation; an explicit Deny is what beats the cudly-* Allow", boundaryFile, statementSid(denies[0]), effect) } } + +// TestPolicyDocumentsUseLiteralActionAndResourceLists is the precondition every +// other assertion in this file rests on: that reading the quoted strings out of +// an Action or Resource list reads the whole list. +// +// It exists because the assertions above are all EQUALITY checks against a set +// this file extracts, and an element expressed as a reference rather than a +// literal is invisible to that extraction. It does not make an assertion fail, +// it makes it measure a smaller document: +// `Resource = ["arn:aws:iam::*:role/cudly-*", local.x]` compares equal to +// [passRoleCeilingResource], so TestBoundaryScopesPassRoleToCudlyRoles reports +// PassRole as capped at cudly-* while the grant reaches local.x as well. There +// is no per-assertion fix for that, because every one of them consumes the same +// blind extraction, so the invariant is asserted once here over every statement +// in every document rather than at each call site. +// +// Scanning both the guarded documents and policy_iam.tf is deliberate: the two +// halves of the #1705 fix (the ceiling and the delegation that enforces it) are +// equally undermined by a Resource this test cannot read. +func TestPolicyDocumentsUseLiteralActionAndResourceLists(t *testing.T) { + files := append(guardedPolicyFiles(t), iamFile) + + scanned := 0 + for _, file := range files { + stmts := extractStatementBlocks(readPolicySource(t, file)) + if len(stmts) == 0 { + t.Errorf("%s: found zero Statement objects; the statement splitter in this test is broken and would otherwise pass vacuously", file) + continue + } + for _, stmt := range stmts { + scanned++ + for _, list := range []struct { + what string + pattern *regexp.Regexp + }{ + {"Action", actionAssignmentPattern}, + {"Resource", resourceAssignmentPattern}, + } { + if refs := unreadListElements(stmt, list.pattern); refs != "" { + t.Errorf("%s: statement %q has %s list elements that are not literal strings (%s). Every assertion in this file reads these lists by pulling the quoted strings out of them, so a referenced element is not read at all: the statement is measured as if it granted only its literal entries, and the assertion that pins it passes against a document narrower than the one AWS enforces. Write literals, or teach this file to resolve the reference before using one", file, statementSid(stmt), list.what, refs) + } + } + } + } + + if scanned == 0 { + t.Fatalf("scanned zero statements across %v; this guard would pass vacuously", files) + } +} From eb144f38cd4533ba2c625700a1d4dc995dc6e339 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sun, 16 Aug 2026 23:32:28 +0200 Subject: [PATCH 5/5] test(iac/aws): treat a quoted interpolation as an unread list element anyQuotedStringPattern strips "${local.x}" as an ordinary literal, so unreadListElements returned no residue for it and the element was accepted as parsed. Action lists survive that by count: the entry is counted but not parseable, so the parsed-vs-listed comparison fails. Resource lists have no count behind the check, so Resource = ["${local.x}"] reached statementResources, which handed back the raw ${local.x} text as though it were the granted ARN, and TestPolicyDocumentsUseLiteralActionAndResourceLists reported the document as literal. Match interpolated quoted elements separately and report them as unread, in the same refuse-what-cannot-be-resolved shape as the bare-reference case, rather than widening any pattern to accept them. Apply the same guard to the ArnNotEquals allowlist behind the iam:AttachRolePolicy Deny, which is read the same blind way: an element that is a reference is invisible to quotedStringPattern, so the set equality against the module-derived set compares only the literals and passes while the Deny exempts whatever the reference adds. Also fix two misspellings golangci-lint v2.10.1 flagged in comments. --- .../ci-cd-permissions/policy_guard_test.go | 34 +++++++++++++++++-- 1 file changed, 32 insertions(+), 2 deletions(-) diff --git a/terraform/environments/aws/ci-cd-permissions/policy_guard_test.go b/terraform/environments/aws/ci-cd-permissions/policy_guard_test.go index 30659fe19..37438e1a4 100644 --- a/terraform/environments/aws/ci-cd-permissions/policy_guard_test.go +++ b/terraform/environments/aws/ci-cd-permissions/policy_guard_test.go @@ -188,7 +188,7 @@ var actionAssignmentPattern = regexp.MustCompile(`(?m)^[ \t]*(?:Action|actions)\ // contributes nothing to TestBoundaryCoversWorkloadServices' derived set, so // the ceiling is never required to cover it and the role 403s at RUNTIME with // the apply green: the under-reporting direction, which is the failure mode -// this file is organised to avoid. +// this file is organized to avoid. var actionStringPattern = regexp.MustCompile(`"([A-Za-z0-9-]+:[A-Za-z0-9_*]+|\*)"`) // resourceAssignmentPattern matches a `Resource = [...]` or `Resource = "..."` @@ -247,7 +247,7 @@ var anyQuotedStringPattern = regexp.MustCompile(`"[^"]*"`) // until it accepts every form somebody might write is the losing half of that // trade: "*:*" and "iam*" are grants that must never appear in a ceiling, so the // fix is for the test to REFUSE them loudly, not to learn to parse them. -// Comparing the counts turns "the pattern did not recognise this entry" into a +// Comparing the counts turns "the pattern did not recognize this entry" into a // failure rather than a silent omission, which closes the class instead of the // two forms currently known. func countActionListStrings(stmt string) int { @@ -263,6 +263,13 @@ func countActionListStrings(stmt string) int { // not an element. var listStructurePattern = regexp.MustCompile(`[\[\],\s]`) +// interpolatedStringPattern matches a quoted list element that contains a +// Terraform interpolation, i.e. one whose value is decided at plan time and is +// therefore not the text this file reads out of the source. Matching it +// separately is what keeps anyQuotedStringPattern from stripping it as a +// literal: see unreadListElements. +var interpolatedStringPattern = regexp.MustCompile(`"[^"]*\$\{[^"]*"`) + // unreadListElements returns the elements of the values assignPattern captures // in stmt that are not quoted literals: local.x, var.y, a function call, // anything whose value this test cannot know. It returns "" when every element @@ -280,9 +287,22 @@ var listStructurePattern = regexp.MustCompile(`[\[\],\s]`) // one-element set every Resource assertion in this file expects, while the // grant it describes is whatever local.y adds. Both forms survive terraform fmt // and terraform validate. +// +// A quoted element holding an interpolation counts as unread for the same +// reason, and is the third form of the same hole rather than a fourth kind of +// element. `"${local.y}"` IS a quoted string, so anyQuotedStringPattern strips +// it and the residue is empty, but the text between the quotes is a reference +// whose value this test no more knows than it knows a bare local.y's. The two +// counts do not catch it either: it is one quoted string that parses to one +// action, so `Action = ["${local.y}"]` reads parsed == listed == 1. For Resource +// lists there is no count at all, so `Resource = ["${local.y}"]` reaches +// statementResources, which hands back the literal text `${local.y}` as if that +// were the ARN AWS will enforce, and every Resource equality in this file is +// then made against a string that is not a grant. func unreadListElements(stmt string, assignPattern *regexp.Regexp) string { var refs []string for _, m := range assignPattern.FindAllStringSubmatch(stmt, -1) { + refs = append(refs, interpolatedStringPattern.FindAllString(m[1], -1)...) if r := listStructurePattern.ReplaceAllString(anyQuotedStringPattern.ReplaceAllString(m[1], ""), ""); r != "" { refs = append(refs, r) } @@ -1146,6 +1166,16 @@ func TestDeployPolicyDeniesUnapprovedManagedPolicyAttachment(t *testing.T) { t.Fatalf("%s: statement %q has no %q list under %s; without it the Deny applies to every iam:AttachRolePolicy call and takes the deploy pipeline down", iamFile, statementSid(stmt), attachDenyConditionKey, attachDenyConditionOperator) } + // This list is read the same blind way an Action or Resource list is, and + // carries the same hole in the fail-open direction: an element expressed as + // a reference is invisible to quotedStringPattern, so the set equality below + // compares only the literals and passes while the Deny exempts whatever the + // reference adds. It is an ArnNotEquals allowlist on a Deny, so an entry this + // test cannot see is an extra managed policy attachable to any cudly-* role. + if refs := unreadListElements(op.body, valuePattern); refs != "" { + t.Fatalf("%s: statement %q has %q entries that are not literal strings (%s); this test cannot know what they resolve to, so they are absent from the set compared against terraform/modules below and the Deny exempts them silently. Write literal ARNs", iamFile, statementSid(stmt), attachDenyConditionKey, refs) + } + allowedMatches := quotedStringPattern.FindAllStringSubmatch(m[1], -1) allowed := make([]string, 0, len(allowedMatches)) for _, q := range allowedMatches {