diff --git a/.security-review/suppressions.json b/.security-review/suppressions.json index 574fb0f7..e13ea036 100644 --- a/.security-review/suppressions.json +++ b/.security-review/suppressions.json @@ -12,11 +12,11 @@ }, { "id": "OSWE-IAC-SECRETS-LIST-01", - "title": "EC2 instance role can enumerate Secrets Manager secret NAMES/metadata account-wide via ListSecrets \"*\"", + "title": "EC2 instance role grants BatchGetSecretValue + ListSecrets on \"*\" (operation-level; secret-NAME enumeration account-wide)", "file": "infra/lib/constructs/instance-role.ts", "severity": "low", "status": "confirmed", - "suppression_justification": "ACCEPTED LOW residual, metadata-only. fetch-config.sh materializes the .env via `batch-get-secret-value --filters Key=name,Values=open-swe-/`, and the AWS API requires secretsmanager:ListSecrets for the filtered batch call. ListSecrets has resource type 'none' in the IAM reference and supports NO resource-level or name-condition scoping, so the `*` is structural, not a fixable scoping miss — it fits this role's stated exception for actions that genuinely lack resource-level scoping. Secret VALUES remain strictly prefix-scoped (GetSecretValue/BatchGetSecretValue on secret:open-swe-/*), so cross-env VALUE isolation is preserved; only NAMES/tags/descriptions are enumerable, within Sea Haven's own single-tenant account 328440206208. Confirmed by the T-bringup GPT-4.1 IAM cross-review (BLOCK: none) and the iac-iam detector (one low residual, no critical/high). Future hardening to eliminate the `*` entirely: switch fetch-config.sh to an explicit SecretIdList sourced from a non-sensitive SSM manifest param (no --filters -> no ListSecrets).", + "suppression_justification": "ACCEPTED LOW residual, metadata-only. fetch-config.sh materializes the .env via `batch-get-secret-value --filters Key=name,Values=open-swe-/`. With a name FILTER, both BatchGetSecretValue (a collection call) and ListSecrets are authorized by AWS against `*`, NOT a per-secret ARN — a prefix-scoped ARN AccessDenies the call (confirmed empirically on i-0af4e03e8bf70e6c3). So the two `*` grants are operation-level, not value-level. Secret VALUES remain strictly gated by the PREFIX-scoped GetSecretValue/DescribeSecret on secret:open-swe-/* (GetSecretValue is checked per-secret even within the batch), so cross-env VALUE isolation is preserved; only NAMES/tags/descriptions are enumerable, within Sea Haven's own single-tenant account 328440206208. Confirmed by GPT-4.1 IAM cross-review (BLOCK: none) and the iac-iam detector (one low residual, no critical/high). Future hardening to eliminate BOTH `*` grants: switch fetch-config.sh to an explicit `--secret-id-list` (no filter), which lets BatchGetSecretValue be prefix-scoped and needs no ListSecrets.", "owner": "adam@seahavenind.com", "added": "2026-06-26" } diff --git a/infra/lib/constructs/instance-role.ts b/infra/lib/constructs/instance-role.ts index 2dcfdbb4..9f3ea60a 100644 --- a/infra/lib/constructs/instance-role.ts +++ b/infra/lib/constructs/instance-role.ts @@ -60,34 +60,35 @@ export class InstanceRole extends Construct { }), ); - // Read secrets from Secrets Manager under open-swe-/*. Secret ARNs carry - // a random 6-char suffix, hence the trailing `*`. fetch-config.sh materializes - // the .env via `batch-get-secret-value` (one call instead of N), so the role - // needs BatchGetSecretValue (a DISTINCT action from GetSecretValue) in addition - // to the per-secret GetSecretValue/DescribeSecret. All three are scoped to the - // env prefix — the box can only read its own env's secret VALUES. + // VALUE access — Secrets Manager under open-swe-/*. Secret ARNs carry a + // 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 the batch call below. this.role.addToPolicy( new iam.PolicyStatement({ - sid: "ReadSecrets", - actions: [ - "secretsmanager:GetSecretValue", - "secretsmanager:BatchGetSecretValue", - "secretsmanager:DescribeSecret", - ], + sid: "ReadSecretValues", + actions: ["secretsmanager:GetSecretValue", "secretsmanager:DescribeSecret"], resources: [`arn:aws:secretsmanager:${REGION}:${ACCOUNT}:secret:${p}/*`], }), ); - // batch-get-secret-value with a name-prefix `--filters` additionally requires - // ListSecrets, which the AWS API does NOT support scoping by resource (it is a - // list-style action) — so it is `*`, matching this role's stated exception for - // actions that genuinely have no resource-level scoping. This grants only the - // ability to ENUMERATE secret names; reading a secret's VALUE still requires the - // prefix-scoped GetSecretValue above, so cross-env value isolation is preserved. + // OPERATION-level grants for fetch-config.sh's prefix-FILTERED batch read + // (`batch-get-secret-value --filters Key=name,Values=open-swe-/`). Both of + // these are collection operations that AWS authorizes against `*`, NOT a + // per-secret ARN: BatchGetSecretValue with a filter is a collection call (a + // prefix-scoped ARN does NOT satisfy it — it AccessDenies), and ListSecrets is a + // list action with no resource-level scoping at all. Neither returns or widens + // VALUE access: a secret's value is still only returned when the prefix-scoped + // GetSecretValue above allows it, so cross-env VALUE isolation is preserved. The + // residual is metadata-only (the box can ENUMERATE secret names account-wide). + // Future hardening to drop both `*` grants: switch fetch-config to an explicit + // `--secret-id-list` (no filter), which lets BatchGetSecretValue be prefix-scoped + // and needs no ListSecrets. Tracked as OSWE-IAC-SECRETS-LIST-01. this.role.addToPolicy( new iam.PolicyStatement({ - sid: "ListSecretsForBatchFilter", - actions: ["secretsmanager:ListSecrets"], + sid: "SecretsBatchListOps", + actions: ["secretsmanager:BatchGetSecretValue", "secretsmanager:ListSecrets"], resources: ["*"], }), );