From 59852eff3415decd26d9e7219ef1af9e405bc3f9 Mon Sep 17 00:00:00 2001 From: Adam Moussa Date: Thu, 30 Jul 2026 18:17:16 -0400 Subject: [PATCH] fix(iam): restore four permissions the boundary would have denied at migration /sh-security-review (6 detectors + verifier) found four HIGH findings, all the same defect class: the template's permission-source comment block was used as the sanctioned scope source, but it is an incomplete and in places invented secondary record. Each was verified against the real stack template before fixing. None is live today (prod/dev boundary usage is 0); all four would have been AccessDenied at first migration, three of them SILENTLY. - SES configuration-set/seahaven-email-events restored. afterhours-shift-manager template.yaml:178-181 grants it with an in-repo comment stating the send is denied without it. An earlier revision dropped it after checking whether any config set exists in prod/dev today (none do) -- the wrong test. The right question is whether an enumerated stack's own IAM policy names it. - SES identity/seahavenind.com added. meal-order-manager's SenderEmail defaults to adam@seahavenind.com (template.yaml:20-22) and email_report sends with it. The prior 'unverified identity fails loudly anyway' argument holds only until the migration verifies the domain, which the migration procedure requires. - scheduler:Create/Delete/GetSchedule + iam:PassRole (scheduler.amazonaws.com only) added. afterhours template.yaml:110-120 needs both; the block omitted them entirely. Failure is silent -- app.py wraps create_schedule in a bare except, so the Slack command reports success and no schedule exists. - secret:afi-slack-webhook-* added. The block named 'afi-backup-monitor/slack-webhook-url', which does not exist; both afi secret ARNs are deploy parameters, so the real names live only in that repo's README:48-49 (afi-api-key, afi-slack-webhook). Also corrected, all comment-only: - The permission-source block itself, at each of the four points it was wrong, with the correction and its evidence recorded inline. - The false claim that SAM auto-names async DLQs (it does not -- all four payments queues are hand-written with explicit QueueNames). Replaced with the real invariant: any queue a boundary-carrying function sends to must be payments-* or the boundary widens in the same PR; a denied destination write is silent. - SIZE BUDGET: was 13 statements / 4,060 chars, actually 16 / 5,457 after these fixes. Headroom is 687 chars, roughly ONE more workload -- not the five the header claimed. Flagged per-workload boundaries as the realistic next move. Verified unchanged: logical id LambdaExecutionBoundary and ManagedPolicyName seahaven-lambda-execution-boundary, so all eight pinning conditions across both guardrail policies still resolve. --- .../deploy-substrate.template.yaml | 120 +++++++++++++++--- 1 file changed, 104 insertions(+), 16 deletions(-) diff --git a/lib/deploy-substrate/deploy-substrate.template.yaml b/lib/deploy-substrate/deploy-substrate.template.yaml index 66bec29..0a569f9 100644 --- a/lib/deploy-substrate/deploy-substrate.template.yaml +++ b/lib/deploy-substrate/deploy-substrate.template.yaml @@ -114,12 +114,20 @@ Description: >- # correctly left untouched. # # SIZE BUDGET: an attached managed policy document is capped at 6,144 characters -# (whitespace excluded). LambdaExecutionBoundary measures 4,060 characters across -# 13 statements as of 2026-07-30 (was 2,507 across 11 — TIGHTENING COSTS -# CHARACTERS). Measure before widening — -# len(json.dumps(doc,separators=(',',':'))) on the synthesized PolicyDocument with -# ${AWS::AccountId} resolved. A typical new workload costs ~450 characters, so -# roughly five more fit. CRITICAL: unlike the 2026-07-27 inline-limit incident, +# (whitespace excluded). LambdaExecutionBoundary measures 5,457 characters across +# 16 statements as of 2026-07-30 (was 2,507 across 11 before this scoping pass — +# TIGHTENING COSTS CHARACTERS, and the review corrections cost more). 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 once already inside a single branch). +# +# ⚠ HEADROOM IS 687 CHARACTERS — roughly ONE more workload at ~450 each, NOT the +# "five" an earlier revision of this header claimed. The next stack to migrate is +# likely to exhaust it. Read the note below before assuming there is room: the +# realistic next move is per-workload boundaries +# (seahaven-lambda-execution-boundary-), which is also the durable fix +# for the shared-ceiling residual documented in the SCOPING RULE. +# CRITICAL: unlike the 2026-07-27 inline-limit incident, # there is NO restructure available when this cap is reached — a role has exactly # ONE permissions boundary, so statements cannot be spilled into a second attached # managed policy. At the cap the only levers are prefix consolidation and dropping @@ -244,9 +252,18 @@ Resources: # afterhours-shift-manager (functions: afterhours-*, 6 live) # - DynamoDB CRUD (afterhours-shifts table) # - secretsmanager:GetSecretValue (afterhours-shift-manager/*) - # - ssm:GetParameter (/afterhours-shift-manager/*: channel-id, - # slack-bot-token, slack-signing-secret) - # - ses:SendEmail (SES identity) + # - ses:SendEmail on the identity AND on + # configuration-set/seahaven-email-events (template.yaml:178-181 — the + # send is DENIED without the config-set ARN when the identity has a + # default configuration set) + # - scheduler:CreateSchedule/DeleteSchedule/GetSchedule on + # schedule/default/holiday-* + iam:PassRole to scheduler.amazonaws.com + # (template.yaml:110-120). CORRECTED 2026-07-30: this block previously + # omitted both, and the omission is SILENT at runtime (bare except). + # - NO ssm. CORRECTED 2026-07-30: this block previously credited + # ssm:GetParameter to this stack; `grep -c 'ssm:' template.yaml` = 0. + # Its slack tokens come from Secrets Manager and the channel id from a + # CloudFormation parameter. # - lambda:InvokeFunction (ReleaseNotifyInvokeRole, # HolidaySchedulerExecutionRole — these two carry the boundary and need # ONLY this action) @@ -256,8 +273,12 @@ Resources: # # payments-dashboard (functions: payments-*) # - DynamoDB CRUD / Read (PaymentsDashboard table — legacy PascalCase) - # - S3 GetObject ONLY (seahaven-payments-csv-*, seahaven-payments-boa-raw-*, - # seahaven-payroll-emails-*) — no write intent enumerated + # - S3 GetObject on seahaven-payments-csv-* and seahaven-payroll-emails-*; + # GetObject + PutObject on seahaven-payments-boa-raw-* + # (template.yaml:272 and 1097-1099, fetchBoaTransactions raw archive). + # CORRECTED 2026-07-30: this block previously said "GetObject ONLY — + # no write intent enumerated", which was false and would have denied + # the raw-archive write at migration. # - secretsmanager:GetSecretValue (payments-dashboard/*) # - sqs Send/Receive/Delete etc. (payments-payroll-batch + DLQs) # - lambda:InvokeFunction (ExpenseReceiver -> ExpenseProcessor) @@ -282,8 +303,12 @@ Resources: # - no S3 / SQS / SSM / SES / KMS / VPC # # afi-backup-monitor (functions: afi-*) - # - secretsmanager:GetSecretValue (afi-backup-monitor/slack-webhook-url AND - # the bare, unprefixed afi-api-key — see the SecretsManager statement) + # - secretsmanager:GetSecretValue on TWO bare, unprefixed secrets: + # afi-api-key and afi-slack-webhook. CORRECTED 2026-07-30: this block + # previously named "afi-backup-monitor/slack-webhook-url", which does + # not exist. Both ARNs are deploy PARAMETERS in that stack + # (AfiApiKeySecretArn / SlackWebhookSecretArn), so no name is + # discoverable from its template — the names are in its README:48-49. # - CloudWatch Logs # - nothing else # @@ -542,6 +567,14 @@ Resources: - !Sub "arn:aws:secretsmanager:us-east-1:${AWS::AccountId}:secret:afterhours-shift-manager/*" - !Sub "arn:aws:secretsmanager:us-east-1:${AWS::AccountId}:secret:afi-backup-monitor/*" - !Sub "arn:aws:secretsmanager:us-east-1:${AWS::AccountId}:secret:afi-api-key-*" + # CORRECTION (2026-07-30 review): the permission-source block + # named this secret "afi-backup-monitor/slack-webhook-url", which + # does not exist. afi-backup-monitor/template.yaml takes both + # secret ARNs as deploy PARAMETERS, so no name is discoverable + # from the template; the real names are in that repo's README + # (afi-api-key and afi-slack-webhook, both bare/unprefixed). Same + # rename-at-migration obligation as afi-api-key applies. + - !Sub "arn:aws:secretsmanager:us-east-1:${AWS::AccountId}:secret:afi-slack-webhook-*" - !Sub "arn:aws:secretsmanager:us-east-1:${AWS::AccountId}:secret:front-integrations/*" - !Sub "arn:aws:secretsmanager:us-east-1:${AWS::AccountId}:secret:meal-order-manager/*" - !Sub "arn:aws:secretsmanager:us-east-1:${AWS::AccountId}:secret:payments-dashboard/*" @@ -579,9 +612,17 @@ Resources: # and workorder-shoc-emitter-rejected, where the wildcard was a silent # message-drain (data-loss) primitive against another tenant's pipeline. # Verified denied 2026-07-30 with iam simulate-custom-policy. - # A prefix rather than four literals is deliberate: SAM auto-names async - # DLQs from the function name, so a new payments-* function's DLQ is - # covered without a boundary edit. + # A prefix rather than four literals is deliberate, BUT the original + # justification for it was wrong and is corrected here: SAM does NOT + # auto-create or auto-name async DLQs — all four payments queues are + # hand-written AWS::SQS::Queue resources with explicit QueueNames, and + # OnFailure destinations take an explicit ARN. The real invariant is + # therefore a naming rule, not a framework behaviour: ANY queue a + # boundary-carrying function sends to — including async OnFailure + # destinations and DeadLetterQueue targets — must be named payments-*, + # or the boundary must be widened in the SAME PR. A denied destination + # write is SILENT: the async event is discarded with no caller to + # error, no Errors datapoint and no DLQ contents. # sqs:ListQueues is absent and must stay absent (authorised against "*"). - Sid: SQS Effect: Allow @@ -642,6 +683,22 @@ Resources: # A ses:FromAddress condition would be stronger but is not available: the # permission-source block names payroll@seahavenind.com, which is not a # verified identity in prod — writing that condition would invent a scope. + # CORRECTION (2026-07-30 review): an earlier revision DROPPED + # configuration-set/* on the reasoning that zero configuration sets + # exist in prod or dev today. That test was the wrong one. SES + # authorizes SendEmail against the CONFIGURATION-SET resource in + # addition to the identity whenever the identity has a default + # configuration set, and afterhours-shift-manager/template.yaml:178-181 + # grants exactly that ARN with an in-repo comment recording that + # omitting it DENIES the send. The correct question is not "does the + # resource exist in the target account yet" but "does an enumerated + # stack's own IAM policy name it". seahavenind.com is listed because + # meal-order-manager's SenderEmail parameter defaults to + # adam@seahavenind.com (meal-order-manager/template.yaml:20-22) and its + # email_report handler sends with that Source; the earlier "sending + # from an unverified identity fails loudly anyway" argument only holds + # until the migration verifies the domain, which the migration + # procedure itself requires. - Sid: SES Effect: Allow Action: @@ -650,6 +707,37 @@ Resources: Resource: - !Sub "arn:aws:ses:us-east-1:${AWS::AccountId}:identity/int.seahaven.com" - !Sub "arn:aws:ses:us-east-1:${AWS::AccountId}:identity/seahaven.com" + - !Sub "arn:aws:ses:us-east-1:${AWS::AccountId}:identity/seahavenind.com" + - !Sub "arn:aws:ses:us-east-1:${AWS::AccountId}:configuration-set/seahaven-email-events" + + # ── EventBridge Scheduler (afterhours-shift-manager holiday routing) ─ + # Omitted from the permission-source block entirely, which the block's + # "verified live" header did not catch. afterhours-shift-manager + # template.yaml:110-120 grants these three scheduler actions plus + # iam:PassRole on its holiday-scheduler execution role. Without them + # the failure is SILENT: src/slack-bot/app.py wraps create_schedule in + # a bare `except Exception`, so the Slack command returns success, the + # holiday record is written with no schedule, and no alarm fires. + - Sid: EventBridgeScheduler + Effect: Allow + Action: + - scheduler:CreateSchedule + - scheduler:DeleteSchedule + - scheduler:GetSchedule + Resource: + - !Sub "arn:aws:scheduler:us-east-1:${AWS::AccountId}:schedule/default/holiday-*" + + # PassRole is confined to the scheduler service principal, so this + # cannot be used to hand a role to Lambda or any other service. + - Sid: SchedulerPassRole + Effect: Allow + Action: + - iam:PassRole + Resource: + - !Sub "arn:aws:iam::${AWS::AccountId}:role/afterhours-shift-manager-*" + Condition: + StringEquals: + "iam:PassedToService": "scheduler.amazonaws.com" # ── KMS — key/* RETAINED, scoped by condition instead ─────────────── # KMS is the one high-value data plane that CANNOT be scoped by resource