From 08a1d41b05682b0e086e1b5da582bc0d44859441 Mon Sep 17 00:00:00 2001 From: Adam Moussa Date: Fri, 31 Jul 2026 13:44:21 -0400 Subject: [PATCH] docs(iam): resolve confirmed review findings from both INFRA-186 gates Cross-family round 1 plus the /sh-security-review verifier confirmed 11 findings on the floor reduction, all documentation defects; no policy statement changes. The one HIGH: the Terraform migration checklist never widened the boundary, so a Lambda-bearing Terraform migration would deploy green and lose every data-plane call at first invoke. Checklist step 2 now carries the widening requirement, step 3 verifies deployed boundary content, and the terraform-substrate header no longer reads as 'Terraform path unaffected'. Also corrected: Description is a REPLACEMENT property (a Description edit wedges the custom-named policy and CFN's remedy is the forbidden rename), the sanctioned-source contradiction, the false AWSLambdaVPCAccessExecutionRole parity claim, the KMS log-group category error, stale size numbers (691/5,453), the same-PR widening contradiction, per-workload residue text, a LoggingConfig silent-log-loss note, the us-east-1 region pin rationale, and ENI DoS deferral now tracked as INFRA-200. --- README.md | 14 ++- .../deploy-substrate.template.yaml | 102 ++++++++++++------ .../terraform-substrate.template.yaml | 10 +- 3 files changed, 92 insertions(+), 34 deletions(-) diff --git a/README.md b/README.md index abf1f9c..9aaf5f9 100644 --- a/README.md +++ b/README.md @@ -272,14 +272,24 @@ split this substrate exists to enforce. `organization:seahaven:project:seahaven-:workspace::run_phase:plan` (or `:apply`). Exact `StringEquals` only — never `StringLike`, never a wildcarded `run_phase` (a speculative PR plan must never hold write - credentials). IAM roles = mandatory GPT-4.1 cross-review + + credentials). **If the stack creates Lambda execution roles, this same PR + must also widen `seahaven-lambda-execution-boundary`** per the WIDENING + PATH in `lib/deploy-substrate/deploy-substrate.template.yaml`: the + guardrail forces every Terraform-created role to carry that boundary, and + it is a fleet-wide floor with zero data-plane permissions until widened — + an unwidened migration deploys green, then every data-plane call is denied + at first invoke and async/DLQ writes are discarded silently. IAM roles and + boundary widenings = mandatory cross-family review + `/sh-security-review` on the diff. 3. After deploy, verify: both roles exist; `hcptf-` lists `seahaven-hcptf-iam-management` in `list-attached-role-policies`; trust subs match the live org/project/workspace names byte-for-byte; simulate the apply role against a `hcptf-*` ARN (expect `explicitDeny` from `DenySelfMutation`) and against a normal stack role name (expect - `allowed`). + `allowed`); and if step 2 widened the boundary, confirm the deployed + default version carries the stack's data-plane statements + (`aws iam get-policy-version`) — role verification alone never checks + boundary content. 4. Set **workspace-level** variables `TFC_AWS_PLAN_ROLE_ARN` + `TFC_AWS_APPLY_ROLE_ARN` (category env) to the verified role ARNs, plus `TFC_AWS_PROVIDER_AUTH=true`. Never project-scoped variable sets — the diff --git a/lib/deploy-substrate/deploy-substrate.template.yaml b/lib/deploy-substrate/deploy-substrate.template.yaml index a7a756b..f966b9d 100644 --- a/lib/deploy-substrate/deploy-substrate.template.yaml +++ b/lib/deploy-substrate/deploy-substrate.template.yaml @@ -40,8 +40,11 @@ Description: >- # + #98 Phase B); DenySelfMutation and the widened policy/seahaven-* # DenyBoundaryPolicyEdit scope were then ported back here, so the two # copies' GUARDRAIL statement sets were reconciled as of that date. -# SamCfnIamManagementPolicy and SamCfnExecutionRole remain byte-identical -# across the two files and MUST still be changed together; the only delta +# SamCfnIamManagementPolicy and SamCfnExecutionRole remain at parity on +# their IAM STATEMENT SETS across the two files and MUST still be changed +# together. Parity covers statements, not surrounding comments — a comment +# may diverge where it describes boundary content, which now differs +# between the files. The only functional delta # between them is the DependsOn line above, which is ordering, not # permission. LambdaExecutionBoundary is NO LONGER byte-identical — see # DELIBERATE DIVERGENCE below. @@ -118,13 +121,13 @@ Description: >- # correctly left untouched. # # SIZE BUDGET: an attached managed policy document is capped at 6,144 characters -# (whitespace excluded). LambdaExecutionBoundary measures 703 characters across -# 4 statements as of 2026-07-30 — the fleet-wide floor only. Measure before +# (whitespace excluded). LambdaExecutionBoundary measures 691 characters across +# 4 statements as of 2026-07-31 — the fleet-wide floor only. Measure before # widening — len(json.dumps(doc,separators=(',',':'))) on the synthesized # PolicyDocument with ${AWS::AccountId} resolved, and UPDATE THESE TWO NUMBERS in # the same edit (they went stale twice inside this branch alone). # -# Headroom is 5,441 characters, roughly TWELVE workloads at ~450 each. That is a +# Headroom is 5,453 characters, roughly TWELVE workloads at ~450 each. That is a # deliberate outcome, not luck: an earlier revision of this branch pre-loaded # per-workload prefixes for all five mgmt SAM stacks and reached 5,457 characters # with 687 left — about one workload of room — before any stack had actually @@ -214,14 +217,27 @@ Resources: # Ec2Eni name runtime-created resources or use actions AWS authorises against # "*" regardless of the ARN supplied. Do not "tighten" those. # + # Every "verified " annotation in this file is a POINT-IN-TIME + # observation, not live state. Re-validate (usage counts, log-group CMK state, + # per-stack permission sources) before citing one as justification for a + # future change. + # # WIDENING PATH — read this before migrating a stack into prod or dev. # The boundary is never widened by the person who hits the AccessDenied. It is # widened by the migrating stack's owner, in THIS repo, BEFORE the workload's # first deploy into the target account: # 0. PRECONDITION — check the managed-policy VERSION budget BEFORE merging: # max 5 versions, both - # accounts are on v1 today. Every widening — and every Description-only - # edit — burns one. Delete the oldest non-default version if at 5. + # accounts are on v1 today. Every widening (PolicyDocument edit) burns + # one. Description, ManagedPolicyName and Path are REPLACEMENT + # properties per the CFN resource reference — CloudFormation cannot + # replace a custom-named policy, so a Description-only edit FAILS the + # stack update, and the error's suggested remedy (rename) is exactly + # the forbidden rename in the COUPLING note above. Never edit those + # three properties. Delete the oldest non-default version if at 5: + # aws iam list-policy-versions --policy-arn \ + # arn:aws:iam:::policy/seahaven-lambda-execution-boundary + # aws iam delete-policy-version --version-id v ... # 1. Derive the workload's needs from ITS OWN TEMPLATE — open the stack's # template.yaml and read the actual IAM policy statements. The # permission-source block below is a STARTING POINT, NOT THE AUTHORITY: @@ -234,8 +250,11 @@ Resources: # SILENT classes specifically: a denied SQS destination/DLQ write discards # the async event with no error and no alarm; a denied scheduler call may # sit behind a bare except; a denied KMS decrypt for env-var encryption - # fails at cold-start INIT; and any CMK-encrypted resource needs the - # matching kms:ViaService principal, not just the kms action. + # fails at cold-start INIT; any CMK-encrypted resource needs the + # matching kms:ViaService principal, not just the kms action; and a + # function using LoggingConfig with a custom log-group name outside + # /aws/lambda* silently loses ALL logs — add a scoped logs statement + # for the custom group or keep the default group name. # 3. Measure. See SIZE BUDGET in the header. There is NO escape hatch. # 4. Both review gates run and neither discharges the other: the GPT-4.1 # cross-family review against the real diff, and /sh-security-review @@ -250,7 +269,7 @@ Resources: # production failure. # # CONSIDERED AND REJECTED: a Deny statement reserving the seahaven-* namespace. - # With the Allow set now enumerated per workload it is fully redundant (verified + # With the Allow set reduced to the fleet-wide floor it is fully redundant (verified # 2026-07-30: seahaven-prod-config-* and seahaven-prod-vpc-flow-logs-* are # already denied by the Allow set alone), and a Deny inside a BOUNDARY is the # hardest failure mode in the estate to debug — it beats every Allow in every @@ -260,12 +279,13 @@ Resources: # NOTE ON WHAT THESE PREFIXES ARE. All five stacks below currently live in the # MANAGEMENT account and none of their resources exists in seahaven-prod or # seahaven-dev yet. These are MIGRATION-CANDIDATE prefixes for the accounts this - # template deploys to, not an inventory of what is deployed there. They are the - # sanctioned scope source because they are the enumerated permission sources of - # the stacks this boundary exists to cap. + # template deploys to, not an inventory of what is deployed there. They are a + # SECONDARY RECORD and a starting point for widening PRs — the authority is + # each stack's own template (WIDENING PATH step 1). No Resource pattern in + # the floor above derives from this block. # - # Permission sources per stack (verified live 2026-07-30; this block is the - # sanctioned source for every Resource pattern above — keep it accurate): + # Permission sources per stack (verified live 2026-07-30; starting point + # only — verify every entry against the owning repo before use): # # afterhours-shift-manager (functions: afterhours-*, 6 live) # - DynamoDB CRUD (afterhours-shifts table) @@ -378,6 +398,13 @@ Resources: # /aws/lambda/* group (log poisoning). No read action is granted here, so # this is not an exfiltration path. Tighten only once every # boundary-carrying function is confirmed to set an explicit FunctionName. + # REGION IS PINNED TO us-east-1 DELIBERATELY: every Sea Haven workload + # deploys to us-east-1, and this template itself only ever deploys there. + # ${AWS::Region} would resolve to the identical string, so it would + # document nothing. A future stack in another region carries this + # boundary but CANNOT write its logs (the silent class above) — so a + # cross-region migration MUST add region-scoped statements in its + # widening PR, same as any other data-plane need. - Sid: CloudWatchLogsWrite Effect: Allow Action: @@ -415,8 +442,10 @@ Resources: Resource: "*" # ── VPC / ENI management (UNSCOPABLE, kept "*" deliberately) ──────── - # Matches AWSLambdaVPCAccessExecutionRole exactly, and for the same - # reasons. The three ec2:Describe* actions do not support resource-level + # Derived from AWSLambdaVPCAccessExecutionRole, NOT an exact match: + # DescribeSecurityGroups and DescribeVpcs exceed that managed policy + # (kept for CFN/SAM VpcConfig validation; read-only). The four + # ec2:Describe* actions do not support resource-level # permissions AT ALL — an ARN in Resource is ignored and the call is # authorised against "*" — so narrowing them is cosmetic. The ENI in # CreateNetworkInterface / DeleteNetworkInterface is created by the @@ -425,16 +454,20 @@ Resources: # AssignPrivateIpAddresses / UnassignPrivateIpAddresses are for EFA and # secondary IPs — not part of the Lambda ENI lifecycle — omitted. # - # KNOWN OPEN ITEM (pre-existing, NOT introduced by INFRA-186): + # KNOWN OPEN ITEM (pre-existing, NOT introduced by INFRA-186; tracked + # as INFRA-200): # ec2:DeleteNetworkInterface on "*" lets a bounded Lambda delete any ENI # in the account, including NAT / VPC-endpoint / RDS ENIs — a # denial-of-service primitive inherited from the AWS managed policy. The - # durable fix is a Condition on ec2:Subnet / ec2:Vpc naming the VPC the - # Lambda fleet attaches to. That VPC does not exist in seahaven-prod or - # seahaven-dev today (payments-dashboard's 10.20.0.0/16 VPC is in mgmt), - # so writing the condition now would encode an mgmt resource into a - # prod/dev template. Whoever brings the VPC across in payments-dashboard's - # migration PR adds the condition in the same PR. + # durable fix is a Condition on ec2:Subnet / ec2:Vpc naming THE SET OF + # VPCs that boundary-carrying Lambdas attach to — not a single VPC id; + # the list must be extended whenever a workload introduces a new VPC + # (tag-based conditions are the alternative if the set churns). No such + # VPC exists in seahaven-prod or seahaven-dev today (payments-dashboard's + # 10.20.0.0/16 VPC is in mgmt), so writing the condition now would encode + # an mgmt resource into a prod/dev template. Whoever brings the first VPC + # across in payments-dashboard's migration PR adds the condition in the + # same PR. - Sid: Ec2Eni Effect: Allow Action: @@ -478,8 +511,11 @@ Resources: # # CONSEQUENCE FOR EVERY MIGRATION PR (mandatory, see WIDENING PATH in # the header): a stack landing in prod or dev MUST add its own - # data-plane statements here, derived from ITS OWN template, in the - # same PR that deploys it. Without them its Lambdas get AccessDenied + # data-plane statements here, derived from ITS OWN template, in its + # own PR to THIS repo, deployed to UPDATE_COMPLETE before the + # workload's first deploy from its own repo (the workload PR and the + # widening PR cannot be the same PR — they live in different repos). + # Without them its Lambdas get AccessDenied # at first invoke. The permission-source block above is the starting # point, NOT the authority — verify every entry against the stack. # @@ -489,8 +525,13 @@ Resources: # justifications; they are also the SILENT-failure classes, which is # why they belong in the floor rather than in per-workload widenings. # - # KMS is absent deliberately: both accounts have ZERO CMK-encrypted - # log groups today (verified 2026-07-30). A workload bringing a + # KMS is absent deliberately: zero boundary-carrying roles exist, so + # nothing bounded decrypts anything yet. (Log-group CMKs are the one + # KMS case that never hits an execution role — CloudWatch Logs + # decrypts via the key policy's grant to the Logs service principal — + # so log-group CMK state is not the gate here. seahaven-prod already + # hosts the seahaven-dynamodb-cmk table key; the first migrating + # stack touching that table must cover it.) A workload bringing a # CMK-encrypted resource adds a KMS statement with the matching # kms:ViaService principal in its own migration PR — a missing # ViaService entry denies, and for env-var encryption it fails at @@ -999,8 +1040,9 @@ Resources: # conditioned on iam:PermissionsBoundary StringEquals the # seahaven-lambda-execution-boundary ARN. That condition means # any role this execution role creates must have the boundary - # applied, so it can never exceed what the boundary allows - # (which is scoped to the services the five stacks actually use). + # applied, so it can never exceed what the boundary allows (in + # THIS file the fleet-wide floor plus per-migration widenings; + # in mgmt's copy the five SAM stacks' service wildcards). # # iam:PassRole is also included here so CloudFormation can pass # the auto-generated Lambda execution role to the Lambda service. diff --git a/lib/terraform-substrate/terraform-substrate.template.yaml b/lib/terraform-substrate/terraform-substrate.template.yaml index 538f6da..4875cd7 100644 --- a/lib/terraform-substrate/terraform-substrate.template.yaml +++ b/lib/terraform-substrate/terraform-substrate.template.yaml @@ -65,8 +65,14 @@ Description: >- # deploy-substrate stack instead. The coupling is by NAME: if the boundary # policy is ever renamed or replaced, every Condition below (and the # deploy-substrate copy) must change in the same piece of work. INFRA-186 -# (per-workload boundary scoping) changes the boundary's CONTENT, not its ARN, -# and does not touch this file. +# (boundary reduced to a fleet-wide floor; per-workload boundaries are +# INFRA-187) changed the boundary's CONTENT, not its ARN, so this file is +# textually untouched — but the Terraform path IS affected: the Conditions +# below FORCE every role a Terraform apply creates onto that boundary, and +# the floor carries zero data-plane permissions. A migrating stack that +# creates Lambda execution roles must widen the boundary per the WIDENING +# PATH in lib/deploy-substrate/deploy-substrate.template.yaml, deployed +# before its first apply (README migration checklist step 2). # # SIZE BUDGET: an attached managed policy document is capped at 6,144 # characters (whitespace excluded). The statement set below is ~2.5 KB.