diff --git a/cloudformation/stacks/CUDly/template.yaml b/cloudformation/stacks/CUDly/template.yaml index 229bd9a60..8da66879f 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, 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. + 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..af8a8dfd2 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**, 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 +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..a7c026ac2 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 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, +# 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..274200610 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 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 = [] +} + # ============================================== # Additional Configuration # ============================================== 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 new file mode 100644 index 000000000..7a9f9994e --- /dev/null +++ b/terraform/modules/compute/aws/cross_account_sts_guard_test.go @@ -0,0 +1,266 @@ +// 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. +// +// 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" + "path/filepath" + "regexp" + "strings" + "testing" +) + +// 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"), +} + +// 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, 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 +// 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, "|") + `)`) +) + +// 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 "" +} + +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) + } + }) + } +} 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