From b264f74f01f61478c25e377e77b1781e04569a71 Mon Sep 17 00:00:00 2001 From: Adam Moussa Date: Thu, 30 Jul 2026 18:03:40 -0400 Subject: [PATCH] fix(iam): correct two boundary-scoping defects found in review Post-implementation verification of the INFRA-186 prod/dev scoping found two functional defects that would have denied permissions the migrating stacks actually need. Neither is live today (prod/dev boundary usage is 0), but both would have surfaced as AccessDenied at first migration. - KMS: the ViaService list omitted ssm., while SSMParameterRead in the same policy grants ssm:GetParameter*. A SecureString read decrypts via the SSM service principal, so the boundary denied reads it also granted. - S3: payments-dashboard was classified read-only from the template's own permission-source comment, but that enumeration is incomplete -- the real stack grants s3:PutObject on BoaRawBucket (template.yaml:272, 1098-1099). Write is now allowed on seahaven-payments-boa-raw-* only; payroll-emails and payments-csv stay read-only, preserving the evidence-deletion protection. The seahaven-payments-* wildcard is replaced by the three literal bucket names, verified against payments-dashboard/template.yaml. Not changed: SES configuration-set/*. Review claimed dropping it rested on a false premise; verified live -- prod and dev both have ZERO configuration sets and member-baseline-stack.ts:44 excludes SES monitoring. The drop is correct. README: the 'substrate changes must edit both files' rule is now false for the boundary specifically, and said so uniformly. Corrected to distinguish the deliberately divergent boundary from the still-at-parity substrate resources. --- README.md | 21 +++++++--- .../deploy-substrate.template.yaml | 42 +++++++++++++++---- 2 files changed, 49 insertions(+), 14 deletions(-) diff --git a/README.md b/README.md index d8f168c..627f686 100644 --- a/README.md +++ b/README.md @@ -133,11 +133,22 @@ account-level deploy plumbing: only for an account that does not already have one — one provider per URL per account). -The template is a verbatim extraction of the substrate section of -`Sea-Haven-Industries/.github/oidc-deploy-roles.yaml` (see the provenance -header in the template — mgmt's copy remains source of truth for 328440206208 -until its stacks migrate out; substrate changes while both are live must edit -both files). Per-repo `githubdeploy-*` deploy roles are deliberately NOT part +The template began as a verbatim extraction of the substrate section of +`Sea-Haven-Industries/.github/oidc-deploy-roles.yaml`, which remains the source +of truth for mgmt (328440206208) until its stacks migrate out. + +**The two copies are no longer at parity, and the old "edit both files" rule no +longer applies uniformly.** Under INFRA-186, `seahaven-lambda-execution-boundary` +in *this* copy was scoped to per-workload prefixes for prod and dev (where +boundary usage was 0, so no live Lambda could break), while mgmt's copy keeps +the account-wide wildcards pending its own separately validated rollout across +26 live boundary-carrying roles. So: **the boundary resource is deliberately +divergent**; every *other* substrate resource (`github-cfn-execution-role`, +`seahaven-cfn-exec-iam-management`) is still expected to change in both files +together. The template's provenance header records which is which — read it +before assuming either parity or divergence. + +Per-repo `githubdeploy-*` deploy roles are deliberately NOT part of the substrate — they are provisioned per repo at migration/onboarding time so an account never carries trust relationships for repos that do not deploy to it. diff --git a/lib/deploy-substrate/deploy-substrate.template.yaml b/lib/deploy-substrate/deploy-substrate.template.yaml index e8a59d3..66bec29 100644 --- a/lib/deploy-substrate/deploy-substrate.template.yaml +++ b/lib/deploy-substrate/deploy-substrate.template.yaml @@ -474,13 +474,30 @@ Resources: - !Sub "arn:aws:s3:::meal-order-manager-*-${AWS::AccountId}" - !Sub "arn:aws:s3:::meal-order-manager-*-${AWS::AccountId}/*" - # ── S3 — read-only access (payments-dashboard) ────────────────────── - # payments-dashboard's enumerated need is s3:GetObject only, so a - # compromised payments function cannot delete payroll-email evidence. - # Its buckets carry the org-wide seahaven- prefix rather than a - # payments- one (seahaven-payments-csv-*, seahaven-payments-boa-raw-*, - # seahaven-payroll-emails-*), which is why two prefixes are needed here - # and why bucket names cannot be derived from stack names. + # ── S3 — payments-dashboard ───────────────────────────────────────── + # Split by direction, but note the source of truth: the template + # comment above enumerates "S3 GetObject" for payments-dashboard, and + # that enumeration is INCOMPLETE. Verified against the real stack + # 2026-07-30 — payments-dashboard/template.yaml grants s3:PutObject on + # BoaRawBucket (lines 272 and 1098-1099), so the fetchBoaTransactions + # path writes raw BoA payloads. A read-only grant here would deny that + # write at migration time. Write is therefore allowed on the boa-raw + # bucket ONLY; payroll-emails and payments-csv stay read-only, so a + # compromised payments function still cannot delete payroll evidence. + # Buckets carry the org-wide seahaven- prefix rather than a payments- + # one, which is why bucket names cannot be derived from stack names. + - Sid: S3PaymentsBoaRawWrite + Effect: Allow + Action: + - s3:GetObject + - s3:PutObject + - s3:GetObjectVersion + - s3:ListBucket + - s3:GetBucketLocation + Resource: + - !Sub "arn:aws:s3:::seahaven-payments-boa-raw-${AWS::AccountId}" + - !Sub "arn:aws:s3:::seahaven-payments-boa-raw-${AWS::AccountId}/*" + - Sid: S3WorkloadReadOnly Effect: Allow Action: @@ -489,8 +506,8 @@ Resources: - s3:ListBucket - s3:GetBucketLocation Resource: - - !Sub "arn:aws:s3:::seahaven-payments-*-${AWS::AccountId}" - - !Sub "arn:aws:s3:::seahaven-payments-*-${AWS::AccountId}/*" + - !Sub "arn:aws:s3:::seahaven-payments-csv-${AWS::AccountId}" + - !Sub "arn:aws:s3:::seahaven-payments-csv-${AWS::AccountId}/*" - !Sub "arn:aws:s3:::seahaven-payroll-emails-${AWS::AccountId}" - !Sub "arn:aws:s3:::seahaven-payroll-emails-${AWS::AccountId}/*" @@ -674,6 +691,13 @@ Resources: - logs.us-east-1.amazonaws.com - s3.us-east-1.amazonaws.com - secretsmanager.us-east-1.amazonaws.com + # REQUIRED for parity with SSMParameterRead above: a + # SecureString parameter decrypts via the SSM service + # principal, so omitting this denies reads that this same + # policy grants — a self-inconsistency caught by the + # 2026-07-30 review. Any ssm:GetParameter* grant in this + # boundary must keep this entry. + - ssm.us-east-1.amazonaws.com - sqs.us-east-1.amazonaws.com # ---------------------------------------------------------------------------