From 3c69dd9de6c89dc3c7f5b10e06b76c2c2d52ce6d Mon Sep 17 00:00:00 2001
From: Adam Moussa <166072409+amoussa1229@users.noreply.github.com>
Date: Sun, 28 Jun 2026 22:08:02 -0400
Subject: [PATCH] fix: BatchGetSecretValue must be granted on * (corrects #48,
 fixes dev crash-loop) (#50)

#48 (OSWE-IAC-SECRETS-LIST-01) scoped secretsmanager:BatchGetSecretValue
to the open-swe-<env>/* ARN on the theory that an explicit --secret-id-list
batch authorizes per-secret. That is FALSE: BatchGetSecretValue is a
collection action AWS authorizes against the account (*), regardless of
--filters vs --secret-id-list. A prefix-scoped grant AccessDenies the whole
call. The dev box passed right after #48 only because the prior broad grant
had not finished propagating; once it lapsed, fetch-config got AccessDenied
-> loaded 0 secrets -> FAIL-FAST -> open-swe.service crash-loop. Verified on
the live dev box (i-0af4e03e8bf70e6c3): the exact call returned
'not authorized to perform: secretsmanager:BatchGetSecretValue'; restoring
the * grant recovered it.

Move BatchGetSecretValue back to Resource:* (its own statement); keep
GetSecretValue + DescribeSecret prefix-scoped (those gate VALUE access, so
cross-env isolation holds). The surviving win from #48: --secret-id-list
needs no name filter, so ListSecrets stays dropped -> no account-wide name
enumeration. fetch-config.sh is unchanged (--secret-id-list is correct).

The /sh-security-review finding OSWE-IAC-IAM-01 called this out and was
wrongly refuted; the reference_secretsmanager_batch_get memory was wrong.
---
 infra/lib/constructs/instance-role.ts | 37 ++++++++++++++++-----------
 1 file changed, 22 insertions(+), 15 deletions(-)

diff --git a/infra/lib/constructs/instance-role.ts b/infra/lib/constructs/instance-role.ts
index 2d75a2e0..2acbba39 100644
--- a/infra/lib/constructs/instance-role.ts
+++ b/infra/lib/constructs/instance-role.ts
@@ -64,28 +64,35 @@ export class InstanceRole extends Construct {
     // random 6-char suffix, hence the trailing `*`. This is the statement that
     // actually gates which secret VALUES the box can read: prefix-scoped, so the
     // dev box can never read prod secret values (and vice versa). GetSecretValue is
-    // checked per-secret even when the value is returned via BatchGetSecretValue.
-    //
-    // BatchGetSecretValue is included here (prefix-scoped, NOT `*`) because
-    // fetch-config.sh now fetches by EXPLICIT `--secret-id-list` rather than a name
-    // `--filters` scan (OSWE-IAC-SECRETS-LIST-01). An id-list batch call authorizes
-    // against each referenced secret's ARN, so the prefix ARN satisfies it — and we
-    // no longer need the account-wide `secretsmanager:ListSecrets` grant that a
-    // filtered collection scan unavoidably forced (ListSecrets has no resource-level
-    // scoping). Net effect: the box can read open-swe-<env>/* secret values and
-    // nothing else — it can no longer enumerate secret names account-wide.
+    // checked per-secret even when the value is returned via the batch call below.
     this.role.addToPolicy(
       new iam.PolicyStatement({
         sid: "ReadSecretValues",
-        actions: [
-          "secretsmanager:GetSecretValue",
-          "secretsmanager:BatchGetSecretValue",
-          "secretsmanager:DescribeSecret",
-        ],
+        actions: ["secretsmanager:GetSecretValue", "secretsmanager:DescribeSecret"],
         resources: [`arn:aws:secretsmanager:${REGION}:${ACCOUNT}:secret:${p}/*`],
       }),
     );
 
+    // BatchGetSecretValue MUST be granted on `*` — it is a collection action that
+    // AWS authorizes against the account, NOT the per-secret ARN, REGARDLESS of
+    // whether the caller uses `--filters` or `--secret-id-list`. A prefix-scoped
+    // BatchGetSecretValue AccessDenies the whole call ("no identity-based policy
+    // allows the secretsmanager:BatchGetSecretValue action") — VERIFIED on the live
+    // dev box 2026-06-29 (the OSWE-IAC-SECRETS-LIST-01 attempt to prefix-scope it
+    // crash-looped the box once the prior broad grant's eventual-consistency lapsed).
+    // This `*` does NOT widen VALUE access: a secret value is only returned when the
+    // prefix-scoped GetSecretValue above also allows it, so cross-env value isolation
+    // holds. The win that DID survive: fetch-config uses `--secret-id-list` (explicit
+    // names, no name filter), so `secretsmanager:ListSecrets` is NOT needed and is
+    // intentionally omitted — the box cannot enumerate secret names account-wide.
+    this.role.addToPolicy(
+      new iam.PolicyStatement({
+        sid: "BatchGetSecretValues",
+        actions: ["secretsmanager:BatchGetSecretValue"],
+        resources: ["*"],
+      }),
+    );
+
     // NOTE (T11): SSM SecureString + Secrets Manager here are assumed to use the
     // AWS-managed keys (alias/aws/ssm, alias/aws/secretsmanager) for which the
     // service grants Decrypt implicitly — so NO kms:Decrypt is granted. If T11