From 65c08e2c3b4fe4a3d1247ea668cf37c0a54189a4 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Thu, 20 Aug 2026 08:44:49 +0200 Subject: [PATCH 1/2] sec(iac/aws): pin cross-account sts:AssumeRole to declared account IDs The hub's Lambda and Fargate roles, and the shipped CloudFormation hub, granted sts:AssumeRole on arn:aws:iam::*:role/CUDly* -- any CUDly-prefixed role in any AWS account in the partition. The role-name prefix narrows WHICH role but never WHOSE account, and the customer-side template defaults the role name to literally "CUDly", so the pattern matched every linked account by construction and every un-linked one as well. The sts:ExternalId StringLike "*" condition on the two Terraform sites could not cover for that: it is a presence check, and the role ARN and external ID are read off the same CloudAccount record, so a mis-selected account supplies both consistently. IAM therefore authorised the whole namespace and the only thing standing between an account-selection bug and an irreversible purchase in the wrong account was the application layer. The CloudFormation copy carried no Condition at all. All three sites now carry a StringEquals aws:ResourceAccount condition holding the account IDs the operator declared, so an undeclared account is an AccessDenied rather than a successful purchase. The ExternalId presence check is retained where it existed and added to CloudFormation for parity. Empty means no reach, never all accounts: the environment derives enable_cross_account_sts from the list, CloudFormation drops the statement via Fn::If, and a module consumer that enables the grant with an empty list fails at plan time on a precondition rather than rendering an unconstrained policy. The permissions boundary needs no change -- a boundary caps by intersection, so narrowing an identity policy can never exceed the existing CrossAccountAssumeRoleCeiling. Guarded by terraform/modules/compute/aws/cross_account_sts_guard_test.go, which discovers policy sites by walking terraform/ and cloudformation/stacks/ rather than opening a list of three paths, and fails loudly if it inspects nothing or misses a known site. Proven against 17 mutations, each asserted on its specific failure message: dropping the condition at any of the three sites, weakening StringEquals to StringLike or ForAllValues:StringEquals, dropping the ExternalId check, hardcoding the account list, moving the condition into a decoy locals block, hiding a grant in an inline_policy block, in another module, or beside the permissions boundary, leaving the conditions behind as // comments, and emptying the walk itself. Two of the cases are inverse: a service-principal trust policy and an ARN glob containing /* must leave the guard green, because a guard that fires on correct code gets deleted rather than fixed. BREAKING: multi-account deployments must declare their accounts. Terraform: cross_account_target_account_ids. CloudFormation: CrossAccountTargetAccountIds. Without it the grant is not created and cross-account collection fails with AccessDenied. The three checked-in github-*.tfvars are set to [] explicitly so the removal is reviewed rather than silent. For bastion-mode accounts the BASTION's account ID is the one to list: only the first hop runs on the deployment's own identity. Closes #1636 --- cloudformation/stacks/CUDly/template.yaml | 59 +- docs/DEPLOYMENT.md | 25 + terraform/environments/aws/compute.tf | 15 +- terraform/environments/aws/dev.tfvars.example | 12 + terraform/environments/aws/github-dev.tfvars | 16 + terraform/environments/aws/github-prod.tfvars | 16 + .../environments/aws/github-staging.tfvars | 16 + terraform/environments/aws/variables.tf | 6 + .../aws/cross_account_sts_guard_test.go | 520 ++++++++++++++++++ terraform/modules/compute/aws/fargate/main.tf | 23 +- .../modules/compute/aws/fargate/variables.tf | 11 + terraform/modules/compute/aws/lambda/main.tf | 40 +- .../modules/compute/aws/lambda/variables.tf | 11 + 13 files changed, 744 insertions(+), 26 deletions(-) create mode 100644 terraform/modules/compute/aws/cross_account_sts_guard_test.go diff --git a/cloudformation/stacks/CUDly/template.yaml b/cloudformation/stacks/CUDly/template.yaml index 229bd9a60..fd5f19495 100644 --- a/cloudformation/stacks/CUDly/template.yaml +++ b/cloudformation/stacks/CUDly/template.yaml @@ -140,6 +140,22 @@ Parameters: Default: "rate(1 day)" Description: How often to check for new recommendations + CrossAccountTargetAccountIds: + Type: CommaDelimitedList + Default: "" + AllowedPattern: "^$|^[0-9]{12}$" + Description: >- + Comma-separated AWS account IDs this stack itself calls sts:AssumeRole + against, for multi-account plan execution. Empty (the default) creates no + cross-account grant at all. There is no "any account" value: the previous + unconditional grant let the hub assume any CUDly* role in any AWS + account, so a mis-selected account produced a successful purchase in the + wrong account rather than an AccessDenied (#1636). For bastion-mode + accounts list the bastion's account, not the target's — only the first + hop runs on this stack's identity. UPGRADING AN EXISTING STACK WITHOUT + SETTING THIS REMOVES THE GRANT, and multi-account collection then fails + with AccessDenied until the accounts are listed. + Conditions: DeployDashboard: Fn::Equals: @@ -163,6 +179,17 @@ Conditions: - Ref: CpuArchitecture - arm64 + # Joining the list back to a string is how a CommaDelimitedList is tested for + # emptiness: the Default of "" arrives as a one-element list holding "", which + # is not equal to an empty list and cannot be compared to one. + HasCrossAccountTargets: + Fn::Not: + - Fn::Equals: + - Fn::Join: + - "" + - Ref: CrossAccountTargetAccountIds + - "" + Resources: # ============================================================================= # DynamoDB Tables @@ -480,12 +507,32 @@ Resources: - organizations:DescribeOrganization Resource: "*" - # Cross-account role assumption for multi-account plans - - Sid: CrossAccountAssumeRole - Effect: Allow - Action: - - sts:AssumeRole - Resource: "arn:aws:iam::*:role/CUDly*" + # Cross-account role assumption for multi-account plans. + # + # aws:ResourceAccount pins the grant to the declared accounts. The + # Resource pattern narrows which role but never whose account: + # arn:aws:iam::*:role/CUDly* matches a CUDly role in any AWS account + # on earth, so before #1636 a mis-selected account produced a + # successful AssumeRole instead of an AccessDenied. sts:ExternalId + # StringLike "*" requires the field to be present and non-empty, at + # parity with the Terraform modules; per-account values are checked + # in internal/credentials/resolver.go. + # + # Dropped entirely rather than widened when no accounts are declared. + - Fn::If: + - HasCrossAccountTargets + - Sid: CrossAccountAssumeRole + Effect: Allow + Action: + - sts:AssumeRole + Resource: "arn:aws:iam::*:role/CUDly*" + Condition: + StringEquals: + aws:ResourceAccount: + Ref: CrossAccountTargetAccountIds + StringLike: + sts:ExternalId: "*" + - Ref: AWS::NoValue # DynamoDB access - Sid: DynamoDBAccess diff --git a/docs/DEPLOYMENT.md b/docs/DEPLOYMENT.md index 1a040c85f..eff52f87f 100644 --- a/docs/DEPLOYMENT.md +++ b/docs/DEPLOYMENT.md @@ -605,6 +605,31 @@ aws rds describe-db-proxies --db-proxy-name cudly-dev-proxy aws rds describe-db-clusters --db-cluster-identifier cudly-dev-postgres ``` +### Multi-Account Cross-Account Access + +Declare every AWS account the deployment will call `sts:AssumeRole` against. Nothing is reachable +cross-account until you do. + +| Deployment shape | Setting | +| ---------------- | ------- | +| Terraform (`terraform/environments/aws`) | `cross_account_target_account_ids = ["111111111111", ...]` in your tfvars | +| CloudFormation (`cloudformation/stacks/CUDly`) | `CrossAccountTargetAccountIds` stack parameter, comma-separated | + +Both render an `aws:ResourceAccount` condition onto the grant. An account that is not listed is +denied by IAM, not merely by the app's account selection. Leaving the setting empty creates **no +cross-account grant at all** — that is the intended fail-closed default, not an oversight. + +For accounts using `bastion` auth mode, list the **bastion's** account ID rather than the target's. +Only the first hop runs on the deployment's own identity; the bastion assumes into the target on its +own identity policy, which this setting does not govern. + +> **Upgrading an existing multi-account deployment**: the grant used to be scoped by role name only +> (`arn:aws:iam::*:role/CUDly*`), which matched a CUDly role in *every* AWS account rather than in +> yours (#1636). List your linked accounts **in the same change that picks up this version**. On +> Terraform the apply removes `aws_iam_role_policy.cross_account_sts` and exits 0; on CloudFormation +> the stack update drops the statement. Either way the first symptom otherwise is a runtime +> `AccessDenied` during collection, not a failed deploy. + ### Multi-Account Credential Encryption Multi-account support requires an AES-256-GCM encryption key for stored cloud account credentials. Terraform creates the key secret automatically (see `specs/multi-account-execution/iac.md`). diff --git a/terraform/environments/aws/compute.tf b/terraform/environments/aws/compute.tf index a32bf9593..99cc1d951 100644 --- a/terraform/environments/aws/compute.tf +++ b/terraform/environments/aws/compute.tf @@ -99,9 +99,12 @@ module "compute_lambda" { ) # Multi-account IAM capabilities. cross_account_role_name_prefix scopes the - # Lambda role's sts:AssumeRole IAM grant to role names starting with the - # prefix — defence-in-depth on top of the app-layer ExternalId check. - enable_cross_account_sts = true + # Lambda role's sts:AssumeRole grant to role names starting with the prefix; + # cross_account_target_account_ids scopes it to the accounts the operator + # declared. The prefix alone never constrained the account (#1636), so the + # grant is derived from the account list: no accounts declared, no grant. + enable_cross_account_sts = length(var.cross_account_target_account_ids) > 0 + cross_account_target_account_ids = var.cross_account_target_account_ids cross_account_role_name_prefix = "CUDly" enable_org_discovery = true credential_encryption_key_secret_arn = module.secrets.credential_encryption_key_secret_arn @@ -220,9 +223,11 @@ module "compute_fargate" { # Multi-account IAM capabilities — kept at parity with the Lambda branch. # cross_account_role_name_prefix scopes the task role's sts:AssumeRole grant - # to role names starting with the prefix; ExternalId validation still + # to role names starting with the prefix; cross_account_target_account_ids + # scopes it to the declared accounts (#1636). ExternalId validation still # happens at the app layer (credentials/resolver.go). - enable_cross_account_sts = true + enable_cross_account_sts = length(var.cross_account_target_account_ids) > 0 + cross_account_target_account_ids = var.cross_account_target_account_ids cross_account_role_name_prefix = "CUDly" enable_org_discovery = true credential_encryption_key_secret_arn = module.secrets.credential_encryption_key_secret_arn diff --git a/terraform/environments/aws/dev.tfvars.example b/terraform/environments/aws/dev.tfvars.example index 7f0128abe..ea3b37562 100644 --- a/terraform/environments/aws/dev.tfvars.example +++ b/terraform/environments/aws/dev.tfvars.example @@ -83,6 +83,18 @@ credential_encryption_key = "REPLACE_WITH_64_CHAR_HEX_STRING" # Maximum number of cloud accounts to process in parallel during plan fan-out. max_account_parallelism = 10 +# AWS account IDs this deployment itself calls sts:AssumeRole against. +# Listing an account here is what authorises it at IAM; onboarding it is still +# a separate step in the app. Leave empty and no cross-account sts:AssumeRole +# grant is created at all — the grant used to be scoped by role name only, +# which matched a CUDly role in every AWS account rather than in yours (#1636). +# Add every linked account before switching a single-account deployment to +# multi-account, or collection from the new account fails with AccessDenied. +# For bastion-mode accounts list the BASTION's account ID, not the target's: +# only the first hop runs on this deployment's identity, the second runs on +# the bastion role's own policy. +# cross_account_target_account_ids = ["111111111111", "222222222222"] + # ============================================== # Frontend (CloudFront + S3) # ============================================== diff --git a/terraform/environments/aws/github-dev.tfvars b/terraform/environments/aws/github-dev.tfvars index 10b05b07f..c6ae7b485 100644 --- a/terraform/environments/aws/github-dev.tfvars +++ b/terraform/environments/aws/github-dev.tfvars @@ -82,6 +82,22 @@ create_subdomain_zone = false enable_scheduled_tasks = true recommendation_schedule = "rate(1 day)" +# ============================================== +# Multi-Account Cross-Account Access +# ============================================== + +# Accounts this deployment may call sts:AssumeRole against. Empty means the +# cross-account grant is not created at all. +# +# These environments have never declared a linked account in Terraform, so [] is +# the honest value. It is written out rather than left to the default because +# the first apply after #1636 DESTROYS aws_iam_role_policy.cross_account_sts: +# the grant used to be unconditional, scoped by role name only, and matched a +# CUDly* role in every AWS account rather than in ours. If any of these +# environments has had a linked account added through the app, list it here in +# the same change or its collection starts failing with AccessDenied. +cross_account_target_account_ids = [] + # ============================================== # Variables provided by GitHub Actions: # TF_VAR_admin_email = ${{ secrets.ADMIN_EMAIL }} diff --git a/terraform/environments/aws/github-prod.tfvars b/terraform/environments/aws/github-prod.tfvars index 447d5082d..d19ff7d2f 100644 --- a/terraform/environments/aws/github-prod.tfvars +++ b/terraform/environments/aws/github-prod.tfvars @@ -81,6 +81,22 @@ create_subdomain_zone = false enable_scheduled_tasks = true recommendation_schedule = "rate(1 day)" +# ============================================== +# Multi-Account Cross-Account Access +# ============================================== + +# Accounts this deployment may call sts:AssumeRole against. Empty means the +# cross-account grant is not created at all. +# +# These environments have never declared a linked account in Terraform, so [] is +# the honest value. It is written out rather than left to the default because +# the first apply after #1636 DESTROYS aws_iam_role_policy.cross_account_sts: +# the grant used to be unconditional, scoped by role name only, and matched a +# CUDly* role in every AWS account rather than in ours. If any of these +# environments has had a linked account added through the app, list it here in +# the same change or its collection starts failing with AccessDenied. +cross_account_target_account_ids = [] + # ============================================== # Variables provided by GitHub Actions: # TF_VAR_admin_email = ${{ secrets.ADMIN_EMAIL }} diff --git a/terraform/environments/aws/github-staging.tfvars b/terraform/environments/aws/github-staging.tfvars index 86068d31d..ee816cd41 100644 --- a/terraform/environments/aws/github-staging.tfvars +++ b/terraform/environments/aws/github-staging.tfvars @@ -80,6 +80,22 @@ create_subdomain_zone = false enable_scheduled_tasks = true recommendation_schedule = "rate(1 day)" +# ============================================== +# Multi-Account Cross-Account Access +# ============================================== + +# Accounts this deployment may call sts:AssumeRole against. Empty means the +# cross-account grant is not created at all. +# +# These environments have never declared a linked account in Terraform, so [] is +# the honest value. It is written out rather than left to the default because +# the first apply after #1636 DESTROYS aws_iam_role_policy.cross_account_sts: +# the grant used to be unconditional, scoped by role name only, and matched a +# CUDly* role in every AWS account rather than in ours. If any of these +# environments has had a linked account added through the app, list it here in +# the same change or its collection starts failing with AccessDenied. +cross_account_target_account_ids = [] + # ============================================== # Variables provided by GitHub Actions: # TF_VAR_admin_email = ${{ secrets.ADMIN_EMAIL }} diff --git a/terraform/environments/aws/variables.tf b/terraform/environments/aws/variables.tf index fa4f857a1..b2e3d2e88 100644 --- a/terraform/environments/aws/variables.tf +++ b/terraform/environments/aws/variables.tf @@ -329,6 +329,12 @@ variable "max_account_parallelism" { default = 10 } +variable "cross_account_target_account_ids" { + description = "AWS account IDs this deployment itself calls sts:AssumeRole against, for multi-account plan execution. Empty (the default) grants no cross-account sts:AssumeRole at all — the compute module's grant is not created. Listing an account here is what authorises it at IAM; it does not onboard it, which still happens in the app. For bastion-mode accounts list the bastion's account, not the target's: only the first hop runs on this deployment's identity. Deliberately not defaulted to a wildcard: see #1636." + type = list(string) + default = [] +} + # ============================================== # Additional Configuration # ============================================== diff --git a/terraform/modules/compute/aws/cross_account_sts_guard_test.go b/terraform/modules/compute/aws/cross_account_sts_guard_test.go new file mode 100644 index 000000000..9cd897d8e --- /dev/null +++ b/terraform/modules/compute/aws/cross_account_sts_guard_test.go @@ -0,0 +1,520 @@ +// Guard for issue #1636: the hub's cross-account sts:AssumeRole grant must be +// pinned to account IDs the operator declared, not to a role-name prefix alone. +// +// arn:aws:iam::*:role/CUDly* narrows WHICH role but never WHOSE account, and +// the customer-side templates default the role name to literally "CUDly", so +// the pattern matched every onboarded account by construction and every +// un-onboarded one as well. A mis-selected CloudAccount row therefore produced +// a successful AssumeRole rather than an AccessDenied, and the first sign of +// the defect was an irreversible purchase in the wrong account. The +// sts:ExternalId StringLike "*" condition cannot cover for this: the role ARN +// and the external ID are read off the same record, so a wrong selection +// supplies both consistently. +// +// The grant lives in three independently maintained copies (two Terraform +// modules and the shipped CloudFormation hub), which is why this guard +// DISCOVERS them by walking the tree instead of opening a list of three paths. +// A list of paths is the same defect one level up: a fourth copy added later is +// invisible to a sweep that only opens the files someone remembered to name. +package aws_test + +import ( + "fmt" + "os" + "path/filepath" + "regexp" + "sort" + "strings" + "testing" +) + +// grantRoots are walked recursively for policy sources. Relative to this +// package directory, terraform/modules/compute/aws — so these are all of +// terraform/ and all of cloudformation/stacks/. +// +// Deliberately the whole of terraform/ rather than this module: an +// unconditioned copy of the grant added to, say, modules/database/aws is +// exactly as dangerous as one added here, and a walk rooted at the module +// would call the tree clean without opening it. That is the same "only the +// files someone remembered" defect this guard exists to catch. +// +// iac/federation/** is outside these roots: it defines the TARGET account's +// trust policy, where the hub is the principal being trusted rather than the +// caller. An aws:ResourceAccount condition is meaningless there. Trust +// policies that DO fall inside the roots (cloudformation/stacks/ +// CUDly-CrossAccount, the service-principal trusts in modules/networking and +// modules/database) are excluded by classification, not by path. +// Repo-relative, resolved against repoRoot, so a failure message names a path +// the reader can open rather than a chain of "..". +var grantRoots = []string{ + "terraform", + filepath.Join("cloudformation", "stacks"), +} + +// repoRoot is four levels up from this package (aws -> compute -> modules -> +// terraform -> root). +func repoRoot(t *testing.T) string { + t.Helper() + + root, err := filepath.Abs(filepath.Join("..", "..", "..", "..")) + if err != nil { + t.Fatalf("resolving repo root: %v", err) + } + return root +} + +// skippedGrantFiles hold an sts:AssumeRole that is not a grant. policy_boundary.tf +// defines the permissions BOUNDARY: a ceiling capping what a role may be granted +// rather than granting anything, so pinning it to account IDs would cap the +// modules below their own grant and 403 at runtime. It has its own suite +// (ci-cd-permissions/policy_guard_test.go, TestBoundaryMatchesCrossAccountRolePrefix). +// +// By file rather than by directory: skipping all of ci-cd-permissions would +// exempt every other policy in it, and an unconditioned grant dropped beside the +// boundary is exactly as dangerous as one dropped anywhere else. +// Repo-relative PATHS, not basenames. A basename match exempts every file with +// that name anywhere in the walk -- an earlier version matched the directory +// name `ci-cd-permissions` and so skipped all three of +// environments/{aws,azure,gcp}, plus any future directory that happened to +// share the name. The sibling suite would not have caught an unconditioned +// grant dropped in there either: policy_guard_test.go asserts on the +// CrossAccountAssumeRoleCeiling Sid, not on how many statements exist. +var skippedGrantFiles = []string{ + filepath.Join("terraform", "environments", "aws", "ci-cd-permissions", "policy_boundary.tf"), +} + +var grantExtensions = map[string]bool{".tf": true, ".yaml": true, ".yml": true} + +// knownGrantSites must all be found by the walk. Not the guard's input (the +// walk is), but its floor: if a refactor moves or renames a file, the walk +// silently inspects two sites instead of three and still reports "no +// violations", which is how a sweep passes by looking at nothing. +var knownGrantSites = []string{ + filepath.Join("terraform", "modules", "compute", "aws", "lambda", "main.tf"), + filepath.Join("terraform", "modules", "compute", "aws", "fargate", "main.tf"), + filepath.Join("cloudformation", "stacks", "CUDly", "template.yaml"), +} + +// assumeRoleActionPattern matches sts:AssumeRole where it is an ACTION VALUE +// rather than prose. Every variable description in these modules names the +// action in running text, and a guard that fires on those constrains what may +// be written about the grant instead of what the grant does. The three +// alternatives are the only three forms the sources use: a quoted HCL string, +// a YAML list item, and a YAML scalar after Action:. +var assumeRoleActionPattern = regexp.MustCompile(`(?m)"sts:AssumeRole"|^\s*-\s+sts:AssumeRole\s*$|Action:\s+sts:AssumeRole\s*$`) + +// trustMarkers and identityMarkers classify an sts:AssumeRole action by +// whichever marker most recently precedes it. A trust policy names the +// principal allowed to assume the role it is attached to; an identity policy +// names the roles its holder may assume. Only the second kind can carry an +// aws:ResourceAccount condition, because only there is the role the resource. +// +// Regexes rather than substrings, and this is not decoration: "PolicyDocument" +// is a SUBSTRING of "AssumeRolePolicyDocument", so a substring rule reads every +// YAML trust policy as an identity policy and demands a condition that cannot +// exist there. TestGrantClassifierSeparatesTrustFromIdentityPolicies caught +// exactly that. The identity form therefore requires a non-letter before the +// key; RE2 has no lookbehind, so it is written as an alternation. +var ( + trustMarkers = []*regexp.Regexp{ + regexp.MustCompile(`assume_role_policy\s*=`), + regexp.MustCompile(`AssumeRolePolicyDocument\s*:`), + } + identityMarkers = []*regexp.Regexp{ + regexp.MustCompile(`resource "aws_iam_role_policy"`), + regexp.MustCompile(`resource "aws_iam_policy"`), + // An identity policy does not have to be its own resource. + // inline_policy was invisible to an earlier version of this list: a + // grant written that way classified as trust, because the nearest + // preceding marker was the assume_role_policy above it in the same + // aws_iam_role block. + regexp.MustCompile(`inline_policy\s*\{`), + regexp.MustCompile(`(?:^|[^A-Za-z])PolicyDocument\s*:`), + regexp.MustCompile(`AWS::IAM::ManagedPolicy`), + } +) + +// KNOWN GAP, stated rather than papered over: `data "aws_iam_policy_document"` +// is not an identity marker, so an identity grant written that way is skipped +// unless some other identity marker happens to precede it in the same file. +// The marker was briefly on the list above and had to come off, because the +// same block is ALSO the canonical way to write a trust policy: +// +// data "aws_iam_policy_document" "assume_role" { +// statement { +// actions = ["sts:AssumeRole"] +// principals { type = "Service" ... } +// } +// } +// +// Telling the two apart means finding whether a principals block exists inside +// the same statement, which most-recent-marker-wins cannot do: the principals +// block follows the actions line rather than preceding it. Doing it properly +// needs a real HCL parse, and a guard that demanded aws:ResourceAccount on +// every service-principal trust policy in the repo would be removed within a +// week. Leaving it off costs a missed grant written that way; leaving it on +// cost false failures on correct files. + +// accountConditionKey pins the grant to declared accounts; externalIDCondition +// requires sts:ExternalId to be present at all. Both must survive: they close +// different gaps and neither substitutes for the other. +const ( + accountConditionKey = "aws:ResourceAccount" + externalIDCondition = "sts:ExternalId" +) + +// accountSourceRefs are the config inputs an aws:ResourceAccount value must +// come from. A literal list of account IDs in the policy would be equally safe +// at IAM; it is rejected because it drifts, and the drift is silent in the +// direction that matters -- an account removed from the deployment's config +// stays reachable by a policy nobody re-read. +var accountSourceRefs = []string{"cross_account_target_account_ids", "CrossAccountTargetAccountIds"} + +// Every check below matches the condition where it is ASSIGNED a value, not +// merely where the key appears. Both halves of that are load-bearing, and every +// one of them was established by mutating this tree rather than reasoned about: +// +// - Prose satisfies a bare containment check. The block comments in +// lambda/main.tf explain what aws:ResourceAccount and sts:ExternalId do, so +// deleting the actual conditions left the words behind and the guard stayed +// green. Hence stripCommentLines below. +// - Comment stripping alone is not enough. The precondition's error_message +// names cross_account_target_account_ids in running text, and an +// error_message is code, not a comment, so hardcoding the account list +// still read as "sourced from config". Hence the assignment anchors. +// - The KEY being pinned to the right value says nothing about the OPERATOR +// it is pinned under. Swapping StringEquals for StringLike passed an +// earlier version of this guard, and under StringLike a value like "1111*" +// matches most of AWS. Hence accountConditionOperator. +var ( + // Requires StringEquals, or ForAnyValue:StringEquals, to be the operator + // immediately enclosing aws:ResourceAccount. Nothing may intervene but + // whitespace and the opening brace. + // + // The leading [^:\w] is what rejects ForAllValues:StringEquals, which looks + // like a harmless spelling and is not: for a single-valued key such as + // aws:ResourceAccount, ForAllValues evaluates TRUE when the key is absent + // from the request, so it turns the pin into a no-op. RE2 has no lookbehind, + // so "not preceded by a colon" is written as a character class. + accountConditionOperator = regexp.MustCompile( + `(?:^|[^:\w])(?:ForAnyValue:)?StringEquals"?\s*[:=]\s*\{?\s*"?aws:ResourceAccount`) + externalIDAssigned = regexp.MustCompile(`sts:ExternalId"?\s*[:=]`) + accountSourcedFromConfig = regexp.MustCompile( + `aws:ResourceAccount"?\s*[:=]\s*(?:Ref:\s*)?(?:var\.)?(?:` + + strings.Join(accountSourceRefs, "|") + `)`) + // `#` for both languages, `//` because HCL has it too: an earlier version + // stripped only `#`, so deleting both real conditions and leaving them + // behind as `//` comments passed. + // + // Whole-line only, and there is deliberately NO /* */ pass. One was added + // and removed: `(?s)/\*.*?\*/` reads the `/*` in an ARN glob such as + // "arn:aws:s3:::bucket/*" as an opening delimiter and eats everything up to + // the next `*/` in an unrelated string. In an IAM policy file that silently + // swallowed the real conditions and left the guard reporting clean. A + // comment stripper that can blind the guard is worse than the HCL block + // comments it was meant to catch, of which the policy sources have none. + commentLinePattern = regexp.MustCompile(`(?m)^[ \t]*(?:#|//).*$`) +) + +// stripCommentLines blanks whole-line `#` and `//` comments. Whole-line only: a +// `#` or `//` inside a quoted string is content in both HCL and YAML, and +// removing it would corrupt the very values these assertions read. +func stripCommentLines(content string) string { + return commentLinePattern.ReplaceAllString(content, "") +} + +// conditionProximityWindow bounds how far after the sts:AssumeRole action the +// conditions are looked for. Every site puts Condition within ~250 bytes of +// Action, so the window is generous. +// +// It exists because the assertions used to run against the whole file, and a +// condition written ANYWHERE in it counted: moving the StringEquals block into +// a decoy `locals` block left the real grant unconditioned with the guard +// green. This is a heuristic, not a parse. The honest alternative is an HCL +// parser and a YAML parser for a check whose whole value is being cheap enough +// to live in the module's own test package. It assumes Condition follows +// Action, which is true at all three sites and is the conventional ordering. +const conditionProximityWindow = 800 + +// identityGrantRegion returns the slice of code running from the file's first +// identity-policy sts:AssumeRole action to conditionProximityWindow bytes later. +// Callers have already asserted there is exactly one such action. +func identityGrantRegion(code string) string { + for _, loc := range assumeRoleActionPattern.FindAllStringIndex(code, -1) { + if isTrustGrant(code, loc[0]) { + continue + } + end := min(loc[0]+conditionProximityWindow, len(code)) + return code[loc[0]:end] + } + return "" +} + +// identityGrantSites walks grantRoots and returns every file holding at least +// one identity-policy sts:AssumeRole grant, keyed by path. The stored value is +// the file with whole-line comments removed, so no assertion downstream can be +// satisfied by a comment that merely describes the grant. +func identityGrantSites(t *testing.T) map[string]string { + t.Helper() + + root := repoRoot(t) + sites := map[string]string{} + inspected := 0 + for _, rel := range grantRoots { + walkPolicyFiles(t, root, filepath.Join(root, rel), func(path, content string) { + inspected++ + code := stripCommentLines(content) + if countIdentityGrants(code) > 0 { + sites[path] = code + } + }) + } + if inspected == 0 { + t.Fatalf("walked %v and opened zero policy files; every assertion below would pass by inspecting nothing", grantRoots) + } + return sites +} + +// walkPolicyFiles calls visit for every file under dir with a policy-source +// extension, passing the path relative to root. Discovered via filepath.WalkDir +// rather than a glob so a grant added at any depth is reached. +func walkPolicyFiles(t *testing.T, root, dir string, visit func(path, content string)) { + t.Helper() + + err := filepath.WalkDir(dir, func(path string, d os.DirEntry, err error) error { + if err != nil { + return err + } + if d.IsDir() || !grantExtensions[strings.ToLower(filepath.Ext(path))] { + return nil + } + rel, relErr := filepath.Rel(root, path) + if relErr != nil { + return relErr + } + for _, skip := range skippedGrantFiles { + if rel == skip { + return nil + } + } + data, readErr := os.ReadFile(path) + if readErr != nil { + return readErr + } + visit(rel, string(data)) + return nil + }) + if err != nil { + t.Fatalf("walking %s: %v", dir, err) + } +} + +// countIdentityGrants counts sts:AssumeRole actions that classify as +// identity-policy grants. +func countIdentityGrants(content string) int { + count := 0 + for _, loc := range assumeRoleActionPattern.FindAllStringIndex(content, -1) { + if !isTrustGrant(content, loc[0]) { + count++ + } + } + return count +} + +// isTrustGrant reports whether the action at idx sits in a trust policy, +// decided by which marker class most recently precedes it. +// +// With NO identity marker preceding it, the answer is trust. That default is +// the safe direction and it is load-bearing: a standalone file holding only a +// service-principal trust policy has no marker of either class, and the +// opposite default made the guard demand aws:ResourceAccount on a correct file. +// A guard that fires on correct code gets deleted; the cost of this default is +// a grant written in a form the identity markers do not recognise, which is the +// gap already stated above. +func isTrustGrant(content string, idx int) bool { + before := content[:idx] + identity := lastMarkerIndex(before, identityMarkers) + return identity == -1 || lastMarkerIndex(before, trustMarkers) > identity +} + +func lastMarkerIndex(content string, markers []*regexp.Regexp) int { + last := -1 + for _, marker := range markers { + all := marker.FindAllStringIndex(content, -1) + if len(all) == 0 { + continue + } + if i := all[len(all)-1][0]; i > last { + last = i + } + } + return last +} + +func TestCrossAccountAssumeRoleIsPinnedToDeclaredAccounts(t *testing.T) { + sites := identityGrantSites(t) + + for _, want := range knownGrantSites { + if _, ok := sites[want]; !ok { + t.Fatalf("%s grants no identity-policy sts:AssumeRole, but it is one of the three sites this guard exists to cover. Either the grant moved (point knownGrantSites at its new home) or it was deleted; a walk that no longer reaches it reports a clean result for a file it never opened. Found: %v", want, sortedKeys(sites)) + } + } + + for path, code := range sites { + t.Run(path, func(t *testing.T) { + assertGrantPinned(t, path, code) + }) + } +} + +// assertGrantPinned takes code with comments already stripped by +// identityGrantSites. +func assertGrantPinned(t *testing.T, path, code string) { + t.Helper() + + // Exactly one, because identityGrantRegion below inspects the FIRST + // identity grant in the file and nothing else. This bounds the number of + // sts:AssumeRole actions; it does not by itself prove the conditions found + // belong to that statement -- that is what the region scoping does. + if n := countIdentityGrants(code); n != 1 { + t.Fatalf("%s holds %d identity-policy sts:AssumeRole grants; only the first is inspected, so the others would go unchecked. Split the file or teach the guard to read statements", path, n) + } + // Scoped to the grant's own region, not the whole file: a condition written + // elsewhere in the document must not stand in for the one on this statement. + region := identityGrantRegion(code) + + if !accountConditionOperator.MatchString(region) { + t.Fatalf("%s grants sts:AssumeRole with no StringEquals %s condition on the statement itself. The Resource pattern is not a restriction on the account (see TestAssumeRoleResourcePatternDoesNotRestrictTheAccount), so without this condition the grant reaches a CUDly* role in ANY AWS account and a mis-selected account produces a purchase instead of an AccessDenied (#1636). The operator has to be StringEquals: StringLike would let \"1111*\" match most of AWS, and ForAllValues:StringEquals is TRUE when the key is absent", path, accountConditionKey) + } + if !accountSourcedFromConfig.MatchString(region) { + t.Errorf("%s sets %s but not from %v. A hardcoded list drifts from the accounts the deployment actually declares, and the drift is silent in the direction that matters: an account added to the app but not to the policy fails, an account removed from the app stays reachable", path, accountConditionKey, accountSourceRefs) + } + if !externalIDAssigned.MatchString(region) { + t.Errorf("%s dropped the %s condition. It closes a different gap from %s -- an app-layer bug that omits the external ID entirely -- and the two do not substitute for each other", path, externalIDCondition, accountConditionKey) + } +} + +// There is deliberately no "reject a wildcard account ID" assertion here. +// Under StringEquals a "*" is the literal string "*", which no account ID is, +// so a wildcard denies everything rather than allowing everything -- it fails +// closed, which is not the direction this guard defends. Format is already +// enforced where an operator can actually get it wrong: the variable +// validation blocks in {lambda,fargate}/variables.tf and the AllowedPattern on +// the CloudFormation parameter. Asserting it a third time here bought a +// fragile regex protecting an unreachable bypass. + +// TestAssumeRoleResourcePatternDoesNotRestrictTheAccount pins WHY the +// condition above is load-bearing, by evaluating the Resource pattern the +// grant still uses against an account that was never declared. If a later +// change pins the account in the ARN itself this test fails, which is the +// signal to revisit the condition rather than to keep both. +func TestAssumeRoleResourcePatternDoesNotRestrictTheAccount(t *testing.T) { + const ( + pattern = "arn:aws:iam::*:role/CUDly*" + undeclaredID = "999999999999" + ) + undeclared := fmt.Sprintf("arn:aws:iam::%s:role/CUDly", undeclaredID) + + matcher := arnPatternMatcher(t, pattern) + if !matcher.MatchString(undeclared) { + t.Fatalf("%q no longer matches %q. The Resource now constrains the account on its own; re-derive what the %s condition is still buying before leaving both in place", pattern, undeclared, accountConditionKey) + } + + // The same pattern is what both Terraform modules render, so the reader is + // not taking the constant above on trust. + // Selected by extension rather than by slice position: the Terraform sites + // are the ones that render this exact interpolated string, and a [:2] slice + // would turn a reordering of knownGrantSites into a false failure. + const rendered = `"arn:aws:iam::*:role/${var.cross_account_role_name_prefix}*"` + root := repoRoot(t) + checked := 0 + for _, rel := range knownGrantSites { + if filepath.Ext(rel) != ".tf" { + continue + } + checked++ + if !strings.Contains(readFile(t, filepath.Join(root, rel)), rendered) { + t.Errorf("%s no longer renders the account-wildcard Resource this test evaluates; update the pattern constant so the demonstration keeps matching the deployed policy", rel) + } + } + if checked == 0 { + t.Fatalf("no .tf entry in knownGrantSites (%v), so the demonstration above was cross-checked against nothing", knownGrantSites) + } +} + +// arnPatternMatcher compiles an IAM resource pattern into the regexp IAM +// evaluates it as: `*` matches any sequence, `?` any single character, and +// neither stops at a `/`, which is why a path segment can carry a name prefix. +func arnPatternMatcher(t *testing.T, pattern string) *regexp.Regexp { + t.Helper() + + var b strings.Builder + b.WriteString("^") + for _, r := range pattern { + switch r { + case '*': + b.WriteString(".*") + case '?': + b.WriteString(".") + default: + b.WriteString(regexp.QuoteMeta(string(r))) + } + } + b.WriteString("$") + + re, err := regexp.Compile(b.String()) + if err != nil { + t.Fatalf("compiling ARN pattern %q: %v", pattern, err) + } + return re +} + +// TestGrantClassifierSeparatesTrustFromIdentityPolicies fixtures the rule the +// walk depends on. Without it, a classifier that called everything a trust +// policy would make every assertion above vacuous while still reporting green. +func TestGrantClassifierSeparatesTrustFromIdentityPolicies(t *testing.T) { + cases := []struct { + name string + content string + wantCount int + }{ + {"hcl trust policy", "resource \"aws_iam_role\" \"x\" {\n assume_role_policy = jsonencode({\n Action = \"sts:AssumeRole\"\n })\n}\n", 0}, + {"hcl identity policy", "resource \"aws_iam_role_policy\" \"x\" {\n policy = jsonencode({\n Action = [\"sts:AssumeRole\"]\n })\n}\n", 1}, + {"yaml trust policy", " AssumeRolePolicyDocument:\n Statement:\n - Action: sts:AssumeRole\n", 0}, + {"yaml identity policy", " Policies:\n - PolicyDocument:\n Statement:\n - Action:\n - sts:AssumeRole\n", 1}, + {"trust then identity in one file", "resource \"aws_iam_role\" \"x\" {\n assume_role_policy = \"sts:AssumeRole\"\n}\nresource \"aws_iam_role_policy\" \"y\" {\n policy = \"sts:AssumeRole\"\n}\n", 1}, + // The canonical service-principal trust policy, in a file of its own so + // no marker of either class precedes the action. Must classify as trust: + // an aws:ResourceAccount condition is meaningless on it, and demanding + // one is a false failure on correct code. + {"standalone policy document with no marker", "data \"aws_iam_policy_document\" \"assume_role\" {\n statement {\n actions = [\"sts:AssumeRole\"]\n principals {\n type = \"Service\"\n identifiers = [\"lambda.amazonaws.com\"]\n }\n }\n}\n", 0}, + {"identity policy still wins over a preceding trust one", "resource \"aws_iam_role\" \"x\" {\n assume_role_policy = \"y\"\n}\nresource \"aws_iam_role_policy\" \"z\" {\n policy = jsonencode({ Action = [\"sts:AssumeRole\"] })\n}\n", 1}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + if got := countIdentityGrants(tc.content); got != tc.wantCount { + t.Errorf("countIdentityGrants() = %d, want %d", got, tc.wantCount) + } + }) + } +} + +func readFile(t *testing.T, path string) string { + t.Helper() + + data, err := os.ReadFile(path) + if err != nil { + t.Fatalf("reading %s: %v", path, err) + } + return string(data) +} + +func sortedKeys(m map[string]string) []string { + out := make([]string, 0, len(m)) + for k := range m { + out = append(out, k) + } + sort.Strings(out) + return out +} diff --git a/terraform/modules/compute/aws/fargate/main.tf b/terraform/modules/compute/aws/fargate/main.tf index 14d2c1fc4..31b6362ae 100644 --- a/terraform/modules/compute/aws/fargate/main.tf +++ b/terraform/modules/compute/aws/fargate/main.tf @@ -240,17 +240,27 @@ resource "aws_iam_role_policy" "ses_access" { }) } -# Cross-account role assumption for multi-account plan execution. Scoped by -# var.cross_account_role_name_prefix (default "CUDly"). ExternalId is also -# enforced at the app layer in the credentials resolver; the IAM StringLike -# "*" condition here is defence-in-depth that requires a non-empty ExternalId -# to be supplied on every AssumeRole call. Mirrors the Lambda module. +# Cross-account role assumption for multi-account plan execution. Mirrors the +# Lambda module, including the two conditions and the precondition; see the +# block comment on aws_iam_role_policy.cross_account_sts in +# modules/compute/aws/lambda/main.tf for why each one is there. +# +# In short: aws:ResourceAccount pins the grant to the declared account IDs, +# because the Resource pattern narrows which role but never whose account +# (#1636); sts:ExternalId StringLike "*" only requires the field to be present. resource "aws_iam_role_policy" "cross_account_sts" { count = var.enable_cross_account_sts ? 1 : 0 name = "cross-account-sts" role = aws_iam_role.task.id + lifecycle { + precondition { + condition = length(var.cross_account_target_account_ids) > 0 + error_message = "enable_cross_account_sts is true but cross_account_target_account_ids is empty. List the AWS account IDs this deployment may assume a role in, or set enable_cross_account_sts = false. There is no 'any account' setting: that was the #1636 grant." + } + } + policy = jsonencode({ Version = "2012-10-17" Statement = [ @@ -259,6 +269,9 @@ resource "aws_iam_role_policy" "cross_account_sts" { Action = ["sts:AssumeRole"] Resource = "arn:aws:iam::*:role/${var.cross_account_role_name_prefix}*" Condition = { + StringEquals = { + "aws:ResourceAccount" = var.cross_account_target_account_ids + } StringLike = { "sts:ExternalId" = "*" } diff --git a/terraform/modules/compute/aws/fargate/variables.tf b/terraform/modules/compute/aws/fargate/variables.tf index f5d1ff897..48d11c706 100644 --- a/terraform/modules/compute/aws/fargate/variables.tf +++ b/terraform/modules/compute/aws/fargate/variables.tf @@ -305,6 +305,17 @@ variable "cross_account_role_name_prefix" { default = "CUDly" } +variable "cross_account_target_account_ids" { + description = "AWS account IDs the task role itself calls sts:AssumeRole against. Rendered into an aws:ResourceAccount condition on the grant, so an account not listed here is denied by IAM rather than by application-layer account selection (#1636). For role_arn accounts this is the target account. For bastion accounts it is the BASTION's account, not the target's: the task assumes into the bastion, and the bastion assumes into the target on its own identity policy, which this condition does not govern (internal/credentials/resolver.go:242-247). Must be non-empty when enable_cross_account_sts is true (enforced by a precondition in main.tf); an empty list means no cross-account reach, never all accounts." + type = list(string) + default = [] + + validation { + condition = alltrue([for id in var.cross_account_target_account_ids : can(regex("^[0-9]{12}$", id))]) + error_message = "cross_account_target_account_ids must contain bare 12-digit AWS account IDs. A wildcard is not an account ID: the condition uses StringEquals, under which '*' is the literal string '*' and matches nothing, so it denies every cross-account call rather than allowing them. There is no 'any account' value." + } +} + variable "enable_org_discovery" { description = "Grant the task role organizations:ListAccounts / DescribeOrganization so CUDly can enumerate accounts in an AWS Organizations management / delegated account." type = bool diff --git a/terraform/modules/compute/aws/lambda/main.tf b/terraform/modules/compute/aws/lambda/main.tf index b2ae5e504..133b6e60e 100644 --- a/terraform/modules/compute/aws/lambda/main.tf +++ b/terraform/modules/compute/aws/lambda/main.tf @@ -398,23 +398,40 @@ resource "aws_iam_role_policy" "ri_exchange" { # Cross-account role assumption for multi-account plan execution. # -# Scoped by var.cross_account_role_name_prefix (default "CUDly") so the -# Lambda can only assume roles whose names start with that prefix. The -# shipped federation templates (iac/federation/aws-*) create roles matching -# this prefix. ExternalId validation also happens at the application layer -# (resolver.go); this IAM condition is defence-in-depth so a single app- -# layer bug cannot pivot into arbitrary roles without a non-empty ExternalId. +# Two independent conditions, closing two different gaps: # -# The StringLike "*" condition requires that sts:ExternalId is present and -# non-empty in every AssumeRole call. Per-account ExternalId values are -# validated at the application layer; IAM here enforces that the field is -# present at all, closing the gap where an app-layer bug could omit it. +# aws:ResourceAccount pins the grant to the account IDs the operator +# declared in var.cross_account_target_account_ids. The Resource pattern +# alone is not a restriction: arn:aws:iam::*:role/CUDly* matches +# arn:aws:iam::999999999999:role/CUDly in an account nobody onboarded, so +# before #1636 a mis-selected CloudAccount row produced a successful +# AssumeRole instead of an AccessDenied, and the first sign of the defect +# was an irreversible purchase in the wrong account. The role-name prefix +# still narrows *which* role, it just never narrowed *whose*. +# +# sts:ExternalId StringLike "*" requires the field to be present and +# non-empty on every call. Per-account values are checked at the app layer +# (internal/credentials/resolver.go); IAM here only closes the gap where an +# app-layer bug omits the field entirely. It cannot detect a wrong-account +# selection, because the ARN and the external ID are read off the same +# record, so a mis-selected account supplies both consistently. +# +# An empty account list means no cross-account reach at all, not reach into +# every account: the precondition below fails the plan rather than rendering +# a policy whose only account constraint is absent. resource "aws_iam_role_policy" "cross_account_sts" { count = var.enable_cross_account_sts ? 1 : 0 name_prefix = "${var.stack_name}-cross-account-sts-" role = aws_iam_role.lambda.id + lifecycle { + precondition { + condition = length(var.cross_account_target_account_ids) > 0 + error_message = "enable_cross_account_sts is true but cross_account_target_account_ids is empty. List the AWS account IDs this deployment may assume a role in, or set enable_cross_account_sts = false. There is no 'any account' setting: that was the #1636 grant." + } + } + policy = jsonencode({ Version = "2012-10-17" Statement = [ @@ -423,6 +440,9 @@ resource "aws_iam_role_policy" "cross_account_sts" { Action = ["sts:AssumeRole"] Resource = "arn:aws:iam::*:role/${var.cross_account_role_name_prefix}*" Condition = { + StringEquals = { + "aws:ResourceAccount" = var.cross_account_target_account_ids + } StringLike = { "sts:ExternalId" = "*" } diff --git a/terraform/modules/compute/aws/lambda/variables.tf b/terraform/modules/compute/aws/lambda/variables.tf index b6527e750..d0d1d5de2 100644 --- a/terraform/modules/compute/aws/lambda/variables.tf +++ b/terraform/modules/compute/aws/lambda/variables.tf @@ -273,6 +273,17 @@ variable "cross_account_role_name_prefix" { default = "CUDly" } +variable "cross_account_target_account_ids" { + description = "AWS account IDs the Lambda itself calls sts:AssumeRole against. Rendered into an aws:ResourceAccount condition on the grant, so an account not listed here is denied by IAM rather than by application-layer account selection (#1636). For role_arn accounts this is the target account. For bastion accounts it is the BASTION's account, not the target's: the Lambda assumes into the bastion, and the bastion assumes into the target on its own identity policy, which this condition does not govern (internal/credentials/resolver.go:242-247). Must be non-empty when enable_cross_account_sts is true (enforced by a precondition in main.tf); an empty list means no cross-account reach, never all accounts." + type = list(string) + default = [] + + validation { + condition = alltrue([for id in var.cross_account_target_account_ids : can(regex("^[0-9]{12}$", id))]) + error_message = "cross_account_target_account_ids must contain bare 12-digit AWS account IDs. A wildcard is not an account ID: the condition uses StringEquals, under which '*' is the literal string '*' and matches nothing, so it denies every cross-account call rather than allowing them. There is no 'any account' value." + } +} + variable "enable_org_discovery" { description = "Allow Lambda to call AWS Organizations ListAccounts for member account discovery" type = bool From 9a5a8fbaf75e0d877cd10dccf489470884dc234e Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Thu, 20 Aug 2026 09:33:54 +0200 Subject: [PATCH 2/2] fix(iac/aws): split the #1636 guard, drop UK spellings and em-dashes CI's golangci-lint (misspell, locale US) failed on `recognise` in the guard test. Fixed, along with two `authorises` in the Terraform descriptions and the five em-dashes the changeset introduced. The guard test had grown to 521 lines against the project's 500-line rule, so the discovery and trust/identity classification move to cross_account_sts_discovery_test.go and the assertions stay put. Same package, no behaviour change: 262 and 268 lines. Mutation harness re-run and retargeted at the new file, since a mutation result stops being evidence once the tree underneath it moves. 18/18 still bite. --- cloudformation/stacks/CUDly/template.yaml | 4 +- docs/DEPLOYMENT.md | 2 +- terraform/environments/aws/dev.tfvars.example | 4 +- terraform/environments/aws/variables.tf | 2 +- .../aws/cross_account_sts_discovery_test.go | 268 ++++++++++++++++++ .../aws/cross_account_sts_guard_test.go | 264 +---------------- 6 files changed, 279 insertions(+), 265 deletions(-) create mode 100644 terraform/modules/compute/aws/cross_account_sts_discovery_test.go diff --git a/cloudformation/stacks/CUDly/template.yaml b/cloudformation/stacks/CUDly/template.yaml index fd5f19495..8da66879f 100644 --- a/cloudformation/stacks/CUDly/template.yaml +++ b/cloudformation/stacks/CUDly/template.yaml @@ -151,8 +151,8 @@ Parameters: unconditional grant let the hub assume any CUDly* role in any AWS account, so a mis-selected account produced a successful purchase in the wrong account rather than an AccessDenied (#1636). For bastion-mode - accounts list the bastion's account, not the target's — only the first - hop runs on this stack's identity. UPGRADING AN EXISTING STACK WITHOUT + accounts list the bastion's account, not the target's, because only the + first hop runs on this stack's identity. UPGRADING AN EXISTING STACK WITHOUT SETTING THIS REMOVES THE GRANT, and multi-account collection then fails with AccessDenied until the accounts are listed. diff --git a/docs/DEPLOYMENT.md b/docs/DEPLOYMENT.md index eff52f87f..af8a8dfd2 100644 --- a/docs/DEPLOYMENT.md +++ b/docs/DEPLOYMENT.md @@ -617,7 +617,7 @@ cross-account until you do. Both render an `aws:ResourceAccount` condition onto the grant. An account that is not listed is denied by IAM, not merely by the app's account selection. Leaving the setting empty creates **no -cross-account grant at all** — that is the intended fail-closed default, not an oversight. +cross-account grant at all**, which is the intended fail-closed default rather than an oversight. For accounts using `bastion` auth mode, list the **bastion's** account ID rather than the target's. Only the first hop runs on the deployment's own identity; the bastion assumes into the target on its diff --git a/terraform/environments/aws/dev.tfvars.example b/terraform/environments/aws/dev.tfvars.example index ea3b37562..a7c026ac2 100644 --- a/terraform/environments/aws/dev.tfvars.example +++ b/terraform/environments/aws/dev.tfvars.example @@ -84,9 +84,9 @@ credential_encryption_key = "REPLACE_WITH_64_CHAR_HEX_STRING" max_account_parallelism = 10 # AWS account IDs this deployment itself calls sts:AssumeRole against. -# Listing an account here is what authorises it at IAM; onboarding it is still +# Listing an account here is what authorizes it at IAM; onboarding it is still # a separate step in the app. Leave empty and no cross-account sts:AssumeRole -# grant is created at all — the grant used to be scoped by role name only, +# grant is created at all. The grant used to be scoped by role name only, # which matched a CUDly role in every AWS account rather than in yours (#1636). # Add every linked account before switching a single-account deployment to # multi-account, or collection from the new account fails with AccessDenied. diff --git a/terraform/environments/aws/variables.tf b/terraform/environments/aws/variables.tf index b2e3d2e88..274200610 100644 --- a/terraform/environments/aws/variables.tf +++ b/terraform/environments/aws/variables.tf @@ -330,7 +330,7 @@ variable "max_account_parallelism" { } variable "cross_account_target_account_ids" { - description = "AWS account IDs this deployment itself calls sts:AssumeRole against, for multi-account plan execution. Empty (the default) grants no cross-account sts:AssumeRole at all — the compute module's grant is not created. Listing an account here is what authorises it at IAM; it does not onboard it, which still happens in the app. For bastion-mode accounts list the bastion's account, not the target's: only the first hop runs on this deployment's identity. Deliberately not defaulted to a wildcard: see #1636." + description = "AWS account IDs this deployment itself calls sts:AssumeRole against, for multi-account plan execution. Empty (the default) grants no cross-account sts:AssumeRole at all: the compute module's grant is not created. Listing an account here is what authorizes it at IAM; it does not onboard it, which still happens in the app. For bastion-mode accounts list the bastion's account, not the target's, because only the first hop runs on this deployment's identity. Deliberately not defaulted to a wildcard: see #1636." type = list(string) default = [] } diff --git a/terraform/modules/compute/aws/cross_account_sts_discovery_test.go b/terraform/modules/compute/aws/cross_account_sts_discovery_test.go new file mode 100644 index 000000000..65f68b7bf --- /dev/null +++ b/terraform/modules/compute/aws/cross_account_sts_discovery_test.go @@ -0,0 +1,268 @@ +// Discovery and classification for the #1636 cross-account sts:AssumeRole +// guard: how the policy sites are found, and how a grant is told apart from a +// trust policy. The assertions these feed live in +// cross_account_sts_guard_test.go, which is also where the guard's purpose is +// written down. +package aws_test + +import ( + "os" + "path/filepath" + "regexp" + "sort" + "strings" + "testing" +) + +// grantRoots are walked recursively for policy sources. Repo-relative, resolved +// against repoRoot, so a failure message names a path the reader can open +// rather than a chain of "..". +// +// Deliberately the whole of terraform/ rather than just this module: an +// unconditioned copy of the grant added to, say, modules/database/aws is +// exactly as dangerous as one added here, and a walk rooted at the module would +// call the tree clean without opening it. That is the same "only the files +// someone remembered" defect this guard exists to catch. +// +// iac/federation/** is outside these roots: it defines the TARGET account's +// trust policy, where the hub is the principal being trusted rather than the +// caller. An aws:ResourceAccount condition is meaningless there. Trust policies +// that DO fall inside the roots (cloudformation/stacks/CUDly-CrossAccount, the +// service-principal trusts in modules/networking and modules/database) are +// excluded by classification, not by path. +var grantRoots = []string{ + "terraform", + filepath.Join("cloudformation", "stacks"), +} + +// repoRoot is four levels up from this package (aws -> compute -> modules -> +// terraform -> root). +func repoRoot(t *testing.T) string { + t.Helper() + + root, err := filepath.Abs(filepath.Join("..", "..", "..", "..")) + if err != nil { + t.Fatalf("resolving repo root: %v", err) + } + return root +} + +// skippedGrantFiles hold an sts:AssumeRole that is not a grant. +// policy_boundary.tf defines the permissions BOUNDARY: a ceiling capping what a +// role may be granted rather than granting anything, so pinning it to account +// IDs would cap the modules below their own grant and 403 at runtime. It has +// its own suite (ci-cd-permissions/policy_guard_test.go, +// TestBoundaryMatchesCrossAccountRolePrefix). +// +// Repo-relative PATHS, not basenames. A basename match exempts every file with +// that name anywhere in the walk, and an earlier version matched the DIRECTORY +// name `ci-cd-permissions`, so it skipped all three of +// environments/{aws,azure,gcp} plus any future directory sharing the name. The +// sibling suite would not have caught an unconditioned grant dropped in there +// either: policy_guard_test.go asserts on the CrossAccountAssumeRoleCeiling +// Sid, not on how many statements exist. +var skippedGrantFiles = []string{ + filepath.Join("terraform", "environments", "aws", "ci-cd-permissions", "policy_boundary.tf"), +} + +var grantExtensions = map[string]bool{".tf": true, ".yaml": true, ".yml": true} + +// assumeRoleActionPattern matches sts:AssumeRole where it is an ACTION VALUE +// rather than prose. Every variable description in these modules names the +// action in running text, and a guard that fires on those constrains what may +// be written about the grant instead of what the grant does. The three +// alternatives are the only three forms the sources use: a quoted HCL string, +// a YAML list item, and a YAML scalar after Action:. +var assumeRoleActionPattern = regexp.MustCompile(`(?m)"sts:AssumeRole"|^\s*-\s+sts:AssumeRole\s*$|Action:\s+sts:AssumeRole\s*$`) + +// trustMarkers and identityMarkers classify an sts:AssumeRole action by +// whichever marker most recently precedes it. A trust policy names the +// principal allowed to assume the role it is attached to; an identity policy +// names the roles its holder may assume. Only the second kind can carry an +// aws:ResourceAccount condition, because only there is the role the resource. +// +// Regexes rather than substrings, and this is not decoration: "PolicyDocument" +// is a SUBSTRING of "AssumeRolePolicyDocument", so a substring rule reads every +// YAML trust policy as an identity policy and demands a condition that cannot +// exist there. TestGrantClassifierSeparatesTrustFromIdentityPolicies caught +// exactly that. The identity form therefore requires a non-letter before the +// key; RE2 has no lookbehind, so it is written as an alternation. +var ( + trustMarkers = []*regexp.Regexp{ + regexp.MustCompile(`assume_role_policy\s*=`), + regexp.MustCompile(`AssumeRolePolicyDocument\s*:`), + } + identityMarkers = []*regexp.Regexp{ + regexp.MustCompile(`resource "aws_iam_role_policy"`), + regexp.MustCompile(`resource "aws_iam_policy"`), + // An identity policy does not have to be its own resource. + // inline_policy was invisible to an earlier version of this list: a + // grant written that way classified as trust, because the nearest + // preceding marker was the assume_role_policy above it in the same + // aws_iam_role block. + regexp.MustCompile(`inline_policy\s*\{`), + regexp.MustCompile(`(?:^|[^A-Za-z])PolicyDocument\s*:`), + regexp.MustCompile(`AWS::IAM::ManagedPolicy`), + } +) + +// KNOWN GAP, stated rather than papered over: `data "aws_iam_policy_document"` +// is not an identity marker, so an identity grant written that way is skipped +// unless some other identity marker happens to precede it in the same file. +// The marker was briefly on the list above and had to come off, because the +// same block is ALSO the canonical way to write a trust policy: +// +// data "aws_iam_policy_document" "assume_role" { +// statement { +// actions = ["sts:AssumeRole"] +// principals { type = "Service" ... } +// } +// } +// +// Telling the two apart means finding whether a principals block exists inside +// the same statement, which most-recent-marker-wins cannot do: the principals +// block follows the actions line rather than preceding it. Doing it properly +// needs a real HCL parse, and a guard that demanded aws:ResourceAccount on +// every service-principal trust policy in the repo would be removed within a +// week. Leaving it off costs a missed grant written that way; leaving it on +// cost false failures on correct files. + +// `#` for both languages, `//` because HCL has it too: an earlier version +// stripped only `#`, so deleting both real conditions and leaving them behind +// as `//` comments passed. +// +// Whole-line only, and there is deliberately NO /* */ pass. One was added and +// removed: `(?s)/\*.*?\*/` reads the `/*` in an ARN glob such as +// "arn:aws:s3:::bucket/*" as an opening delimiter and eats everything up to the +// next `*/` in an unrelated string. In an IAM policy file that silently +// swallowed the real conditions and left the guard reporting clean. A comment +// stripper that can blind the guard is worse than the HCL block comments it was +// meant to catch, of which the policy sources have none. +var commentLinePattern = regexp.MustCompile(`(?m)^[ \t]*(?:#|//).*$`) + +// stripCommentLines blanks whole-line `#` and `//` comments. Whole-line only: a +// `#` or `//` inside a quoted string is content in both HCL and YAML, and +// removing it would corrupt the very values these assertions read. +func stripCommentLines(content string) string { + return commentLinePattern.ReplaceAllString(content, "") +} + +// identityGrantSites walks grantRoots and returns every file holding at least +// one identity-policy sts:AssumeRole grant, keyed by path. The stored value is +// the file with whole-line comments removed, so no assertion downstream can be +// satisfied by a comment that merely describes the grant. +func identityGrantSites(t *testing.T) map[string]string { + t.Helper() + + root := repoRoot(t) + sites := map[string]string{} + inspected := 0 + for _, rel := range grantRoots { + walkPolicyFiles(t, root, filepath.Join(root, rel), func(path, content string) { + inspected++ + code := stripCommentLines(content) + if countIdentityGrants(code) > 0 { + sites[path] = code + } + }) + } + if inspected == 0 { + t.Fatalf("walked %v and opened zero policy files; every assertion below would pass by inspecting nothing", grantRoots) + } + return sites +} + +// walkPolicyFiles calls visit for every file under dir with a policy-source +// extension, passing the path relative to root. Discovered via filepath.WalkDir +// rather than a glob so a grant added at any depth is reached. +func walkPolicyFiles(t *testing.T, root, dir string, visit func(path, content string)) { + t.Helper() + + err := filepath.WalkDir(dir, func(path string, d os.DirEntry, err error) error { + if err != nil { + return err + } + if d.IsDir() || !grantExtensions[strings.ToLower(filepath.Ext(path))] { + return nil + } + rel, relErr := filepath.Rel(root, path) + if relErr != nil { + return relErr + } + for _, skip := range skippedGrantFiles { + if rel == skip { + return nil + } + } + data, readErr := os.ReadFile(path) + if readErr != nil { + return readErr + } + visit(rel, string(data)) + return nil + }) + if err != nil { + t.Fatalf("walking %s: %v", dir, err) + } +} + +// countIdentityGrants counts sts:AssumeRole actions that classify as +// identity-policy grants. +func countIdentityGrants(content string) int { + count := 0 + for _, loc := range assumeRoleActionPattern.FindAllStringIndex(content, -1) { + if !isTrustGrant(content, loc[0]) { + count++ + } + } + return count +} + +// isTrustGrant reports whether the action at idx sits in a trust policy, +// decided by which marker class most recently precedes it. +// +// With NO identity marker preceding it, the answer is trust. That default is +// the safe direction and it is load-bearing: a standalone file holding only a +// service-principal trust policy has no marker of either class, and the +// opposite default made the guard demand aws:ResourceAccount on a correct file. +// A guard that fires on correct code gets deleted; the cost of this default is +// a grant written in a form the identity markers do not recognize, which is the +// gap stated above. +func isTrustGrant(content string, idx int) bool { + before := content[:idx] + identity := lastMarkerIndex(before, identityMarkers) + return identity == -1 || lastMarkerIndex(before, trustMarkers) > identity +} + +func lastMarkerIndex(content string, markers []*regexp.Regexp) int { + last := -1 + for _, marker := range markers { + all := marker.FindAllStringIndex(content, -1) + if len(all) == 0 { + continue + } + if i := all[len(all)-1][0]; i > last { + last = i + } + } + return last +} + +func readFile(t *testing.T, path string) string { + t.Helper() + + data, err := os.ReadFile(path) + if err != nil { + t.Fatalf("reading %s: %v", path, err) + } + return string(data) +} + +func sortedKeys(m map[string]string) []string { + out := make([]string, 0, len(m)) + for k := range m { + out = append(out, k) + } + sort.Strings(out) + return out +} diff --git a/terraform/modules/compute/aws/cross_account_sts_guard_test.go b/terraform/modules/compute/aws/cross_account_sts_guard_test.go index 9cd897d8e..7a9f9994e 100644 --- a/terraform/modules/compute/aws/cross_account_sts_guard_test.go +++ b/terraform/modules/compute/aws/cross_account_sts_guard_test.go @@ -16,75 +16,20 @@ // DISCOVERS them by walking the tree instead of opening a list of three paths. // A list of paths is the same defect one level up: a fourth copy added later is // invisible to a sweep that only opens the files someone remembered to name. +// +// The walking and the trust/identity classification live in +// cross_account_sts_discovery_test.go; this file holds what is asserted about +// what they find. package aws_test import ( "fmt" - "os" "path/filepath" "regexp" - "sort" "strings" "testing" ) -// grantRoots are walked recursively for policy sources. Relative to this -// package directory, terraform/modules/compute/aws — so these are all of -// terraform/ and all of cloudformation/stacks/. -// -// Deliberately the whole of terraform/ rather than this module: an -// unconditioned copy of the grant added to, say, modules/database/aws is -// exactly as dangerous as one added here, and a walk rooted at the module -// would call the tree clean without opening it. That is the same "only the -// files someone remembered" defect this guard exists to catch. -// -// iac/federation/** is outside these roots: it defines the TARGET account's -// trust policy, where the hub is the principal being trusted rather than the -// caller. An aws:ResourceAccount condition is meaningless there. Trust -// policies that DO fall inside the roots (cloudformation/stacks/ -// CUDly-CrossAccount, the service-principal trusts in modules/networking and -// modules/database) are excluded by classification, not by path. -// Repo-relative, resolved against repoRoot, so a failure message names a path -// the reader can open rather than a chain of "..". -var grantRoots = []string{ - "terraform", - filepath.Join("cloudformation", "stacks"), -} - -// repoRoot is four levels up from this package (aws -> compute -> modules -> -// terraform -> root). -func repoRoot(t *testing.T) string { - t.Helper() - - root, err := filepath.Abs(filepath.Join("..", "..", "..", "..")) - if err != nil { - t.Fatalf("resolving repo root: %v", err) - } - return root -} - -// skippedGrantFiles hold an sts:AssumeRole that is not a grant. policy_boundary.tf -// defines the permissions BOUNDARY: a ceiling capping what a role may be granted -// rather than granting anything, so pinning it to account IDs would cap the -// modules below their own grant and 403 at runtime. It has its own suite -// (ci-cd-permissions/policy_guard_test.go, TestBoundaryMatchesCrossAccountRolePrefix). -// -// By file rather than by directory: skipping all of ci-cd-permissions would -// exempt every other policy in it, and an unconditioned grant dropped beside the -// boundary is exactly as dangerous as one dropped anywhere else. -// Repo-relative PATHS, not basenames. A basename match exempts every file with -// that name anywhere in the walk -- an earlier version matched the directory -// name `ci-cd-permissions` and so skipped all three of -// environments/{aws,azure,gcp}, plus any future directory that happened to -// share the name. The sibling suite would not have caught an unconditioned -// grant dropped in there either: policy_guard_test.go asserts on the -// CrossAccountAssumeRoleCeiling Sid, not on how many statements exist. -var skippedGrantFiles = []string{ - filepath.Join("terraform", "environments", "aws", "ci-cd-permissions", "policy_boundary.tf"), -} - -var grantExtensions = map[string]bool{".tf": true, ".yaml": true, ".yml": true} - // knownGrantSites must all be found by the walk. Not the guard's input (the // walk is), but its floor: if a refactor moves or renames a file, the walk // silently inspects two sites instead of three and still reports "no @@ -95,66 +40,6 @@ var knownGrantSites = []string{ filepath.Join("cloudformation", "stacks", "CUDly", "template.yaml"), } -// assumeRoleActionPattern matches sts:AssumeRole where it is an ACTION VALUE -// rather than prose. Every variable description in these modules names the -// action in running text, and a guard that fires on those constrains what may -// be written about the grant instead of what the grant does. The three -// alternatives are the only three forms the sources use: a quoted HCL string, -// a YAML list item, and a YAML scalar after Action:. -var assumeRoleActionPattern = regexp.MustCompile(`(?m)"sts:AssumeRole"|^\s*-\s+sts:AssumeRole\s*$|Action:\s+sts:AssumeRole\s*$`) - -// trustMarkers and identityMarkers classify an sts:AssumeRole action by -// whichever marker most recently precedes it. A trust policy names the -// principal allowed to assume the role it is attached to; an identity policy -// names the roles its holder may assume. Only the second kind can carry an -// aws:ResourceAccount condition, because only there is the role the resource. -// -// Regexes rather than substrings, and this is not decoration: "PolicyDocument" -// is a SUBSTRING of "AssumeRolePolicyDocument", so a substring rule reads every -// YAML trust policy as an identity policy and demands a condition that cannot -// exist there. TestGrantClassifierSeparatesTrustFromIdentityPolicies caught -// exactly that. The identity form therefore requires a non-letter before the -// key; RE2 has no lookbehind, so it is written as an alternation. -var ( - trustMarkers = []*regexp.Regexp{ - regexp.MustCompile(`assume_role_policy\s*=`), - regexp.MustCompile(`AssumeRolePolicyDocument\s*:`), - } - identityMarkers = []*regexp.Regexp{ - regexp.MustCompile(`resource "aws_iam_role_policy"`), - regexp.MustCompile(`resource "aws_iam_policy"`), - // An identity policy does not have to be its own resource. - // inline_policy was invisible to an earlier version of this list: a - // grant written that way classified as trust, because the nearest - // preceding marker was the assume_role_policy above it in the same - // aws_iam_role block. - regexp.MustCompile(`inline_policy\s*\{`), - regexp.MustCompile(`(?:^|[^A-Za-z])PolicyDocument\s*:`), - regexp.MustCompile(`AWS::IAM::ManagedPolicy`), - } -) - -// KNOWN GAP, stated rather than papered over: `data "aws_iam_policy_document"` -// is not an identity marker, so an identity grant written that way is skipped -// unless some other identity marker happens to precede it in the same file. -// The marker was briefly on the list above and had to come off, because the -// same block is ALSO the canonical way to write a trust policy: -// -// data "aws_iam_policy_document" "assume_role" { -// statement { -// actions = ["sts:AssumeRole"] -// principals { type = "Service" ... } -// } -// } -// -// Telling the two apart means finding whether a principals block exists inside -// the same statement, which most-recent-marker-wins cannot do: the principals -// block follows the actions line rather than preceding it. Doing it properly -// needs a real HCL parse, and a guard that demanded aws:ResourceAccount on -// every service-principal trust policy in the repo would be removed within a -// week. Leaving it off costs a missed grant written that way; leaving it on -// cost false failures on correct files. - // accountConditionKey pins the grant to declared accounts; externalIDCondition // requires sts:ExternalId to be present at all. Both must survive: they close // different gaps and neither substitutes for the other. @@ -177,7 +62,7 @@ var accountSourceRefs = []string{"cross_account_target_account_ids", "CrossAccou // - Prose satisfies a bare containment check. The block comments in // lambda/main.tf explain what aws:ResourceAccount and sts:ExternalId do, so // deleting the actual conditions left the words behind and the guard stayed -// green. Hence stripCommentLines below. +// green. Hence stripCommentLines, in the discovery file. // - Comment stripping alone is not enough. The precondition's error_message // names cross_account_target_account_ids in running text, and an // error_message is code, not a comment, so hardcoding the account list @@ -202,27 +87,8 @@ var ( accountSourcedFromConfig = regexp.MustCompile( `aws:ResourceAccount"?\s*[:=]\s*(?:Ref:\s*)?(?:var\.)?(?:` + strings.Join(accountSourceRefs, "|") + `)`) - // `#` for both languages, `//` because HCL has it too: an earlier version - // stripped only `#`, so deleting both real conditions and leaving them - // behind as `//` comments passed. - // - // Whole-line only, and there is deliberately NO /* */ pass. One was added - // and removed: `(?s)/\*.*?\*/` reads the `/*` in an ARN glob such as - // "arn:aws:s3:::bucket/*" as an opening delimiter and eats everything up to - // the next `*/` in an unrelated string. In an IAM policy file that silently - // swallowed the real conditions and left the guard reporting clean. A - // comment stripper that can blind the guard is worse than the HCL block - // comments it was meant to catch, of which the policy sources have none. - commentLinePattern = regexp.MustCompile(`(?m)^[ \t]*(?:#|//).*$`) ) -// stripCommentLines blanks whole-line `#` and `//` comments. Whole-line only: a -// `#` or `//` inside a quoted string is content in both HCL and YAML, and -// removing it would corrupt the very values these assertions read. -func stripCommentLines(content string) string { - return commentLinePattern.ReplaceAllString(content, "") -} - // conditionProximityWindow bounds how far after the sts:AssumeRole action the // conditions are looked for. Every site puts Condition within ~250 bytes of // Action, so the window is generous. @@ -250,107 +116,6 @@ func identityGrantRegion(code string) string { return "" } -// identityGrantSites walks grantRoots and returns every file holding at least -// one identity-policy sts:AssumeRole grant, keyed by path. The stored value is -// the file with whole-line comments removed, so no assertion downstream can be -// satisfied by a comment that merely describes the grant. -func identityGrantSites(t *testing.T) map[string]string { - t.Helper() - - root := repoRoot(t) - sites := map[string]string{} - inspected := 0 - for _, rel := range grantRoots { - walkPolicyFiles(t, root, filepath.Join(root, rel), func(path, content string) { - inspected++ - code := stripCommentLines(content) - if countIdentityGrants(code) > 0 { - sites[path] = code - } - }) - } - if inspected == 0 { - t.Fatalf("walked %v and opened zero policy files; every assertion below would pass by inspecting nothing", grantRoots) - } - return sites -} - -// walkPolicyFiles calls visit for every file under dir with a policy-source -// extension, passing the path relative to root. Discovered via filepath.WalkDir -// rather than a glob so a grant added at any depth is reached. -func walkPolicyFiles(t *testing.T, root, dir string, visit func(path, content string)) { - t.Helper() - - err := filepath.WalkDir(dir, func(path string, d os.DirEntry, err error) error { - if err != nil { - return err - } - if d.IsDir() || !grantExtensions[strings.ToLower(filepath.Ext(path))] { - return nil - } - rel, relErr := filepath.Rel(root, path) - if relErr != nil { - return relErr - } - for _, skip := range skippedGrantFiles { - if rel == skip { - return nil - } - } - data, readErr := os.ReadFile(path) - if readErr != nil { - return readErr - } - visit(rel, string(data)) - return nil - }) - if err != nil { - t.Fatalf("walking %s: %v", dir, err) - } -} - -// countIdentityGrants counts sts:AssumeRole actions that classify as -// identity-policy grants. -func countIdentityGrants(content string) int { - count := 0 - for _, loc := range assumeRoleActionPattern.FindAllStringIndex(content, -1) { - if !isTrustGrant(content, loc[0]) { - count++ - } - } - return count -} - -// isTrustGrant reports whether the action at idx sits in a trust policy, -// decided by which marker class most recently precedes it. -// -// With NO identity marker preceding it, the answer is trust. That default is -// the safe direction and it is load-bearing: a standalone file holding only a -// service-principal trust policy has no marker of either class, and the -// opposite default made the guard demand aws:ResourceAccount on a correct file. -// A guard that fires on correct code gets deleted; the cost of this default is -// a grant written in a form the identity markers do not recognise, which is the -// gap already stated above. -func isTrustGrant(content string, idx int) bool { - before := content[:idx] - identity := lastMarkerIndex(before, identityMarkers) - return identity == -1 || lastMarkerIndex(before, trustMarkers) > identity -} - -func lastMarkerIndex(content string, markers []*regexp.Regexp) int { - last := -1 - for _, marker := range markers { - all := marker.FindAllStringIndex(content, -1) - if len(all) == 0 { - continue - } - if i := all[len(all)-1][0]; i > last { - last = i - } - } - return last -} - func TestCrossAccountAssumeRoleIsPinnedToDeclaredAccounts(t *testing.T) { sites := identityGrantSites(t) @@ -499,22 +264,3 @@ func TestGrantClassifierSeparatesTrustFromIdentityPolicies(t *testing.T) { }) } } - -func readFile(t *testing.T, path string) string { - t.Helper() - - data, err := os.ReadFile(path) - if err != nil { - t.Fatalf("reading %s: %v", path, err) - } - return string(data) -} - -func sortedKeys(m map[string]string) []string { - out := make([]string, 0, len(m)) - for k := range m { - out = append(out, k) - } - sort.Strings(out) - return out -}