fix: BatchGetSecretValue must be granted on * (corrects #48, fixes dev crash-loop) (#50)
Some checks are pending
Infra CD / Infra CI (pre-deploy) (push) Waiting to run
Infra CD / Deploy open-swe-dev (push) Blocked by required conditions
Infra CD / Deploy open-swe-prod (push) Blocked by required conditions
CI / Lint (push) Waiting to run
CI / Format check (push) Waiting to run
CI / Unit tests (push) Waiting to run
CI / Playwright E2E (push) Waiting to run
Build & publish app artifacts / Publish + deploy (dev) (push) Waiting to run
Build & publish app artifacts / Publish + deploy (prod) (push) Waiting to run

#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.
This commit is contained in:
Adam Moussa 2026-06-28 22:08:02 -04:00 • committed by GitHub
parent 9acf071ae4
commit 3c69dd9de6
No known key found for this signature in database
GPG key ID: B5690EEEBB952194

View file

@ -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