diff --git a/deploy/seahaven/fetch-config.sh b/deploy/seahaven/fetch-config.sh index e029ed5b..bf82cad1 100755 --- a/deploy/seahaven/fetch-config.sh +++ b/deploy/seahaven/fetch-config.sh @@ -158,34 +158,64 @@ while IFS=$'\t' read -r nb vb; do done < <(jq -r '.Parameters[] | (.Name|@base64) + "\t" + (.Value|@base64)' <<<"$ssm_json") log "loaded ${ssm_count} config param(s) from SSM" -# 2) Secrets Manager (sensitive values). batch-get-secret-value filters by name -# prefix; one secret per env var, SecretString = the value. The AWS CLI does NOT -# auto-paginate this operation (unlike list/get-parameters-by-path), and a single -# response caps well under the full set (~10 items/page) — so we MUST follow -# NextToken ourselves or secrets on later pages are silently dropped (they then -# surface as FAIL-FAST "missing required var"). `--no-cli-pager` only disables the -# OUTPUT pager, not API pagination. Each page's records are base64(namevalue). +# 2) Secrets Manager (sensitive values). We request secrets by EXPLICIT id +# (`batch-get-secret-value --secret-id-list ...`) rather than a name-prefix +# `--filters` collection scan (OSWE-IAC-SECRETS-LIST-01). Two wins: +# (a) Least privilege — an explicit id list lets the instance role scope +# BatchGetSecretValue to the per-secret ARN prefix and DROP the account-wide +# `secretsmanager:ListSecrets` grant that a filtered scan unavoidably forces +# (ListSecrets has no resource-level scoping). A filtered batch call also only +# authorizes against `*`; an id-list call authorizes per-secret ARN. +# (b) Deterministic set — the value set is the fixed SECRET_VARS shells created by +# the CDK ConfigStore, so we no longer depend on a list scan returning every +# page. Value-less shells and ids with no current value come back in the +# response `.Errors[]` (ResourceNotFound), never in `.SecretValues[]`, so a +# genuinely-missing REQUIRED secret is still caught by the FAIL-FAST check +# below — an absent optional secret is simply skipped. +# `--secret-id-list` is capped at 20 ids per call, so we chunk it. `--no-cli-pager` +# disables only the OUTPUT pager. Each record is base64(namevalue). +# +# SOURCE OF TRUTH for this list: infra/lib/constructs/config-store.ts `SECRET_VARS`. +# Keep the two in lockstep — a new secret shell created there must be added here or it +# will never be fetched into the .env. +SECRET_VARS=( + ANTHROPIC_API_KEY CORRIDOR_API_TOKEN CORRIDOR_MCP_TOKEN CORRIDOR_TOKEN + DASHBOARD_JWT_SECRET DAYTONA_API_KEY EXA_API_KEY FIREWORKS_API_KEY + GITHUB_APP_CLIENT_SECRET GITHUB_APP_PRIVATE_KEY GITHUB_PAT GITHUB_WEBHOOK_SECRET + GOOGLE_API_KEY GROQ_API_KEY JUDGE_ANTHROPIC_API_KEY LANGSMITH_API_KEY + LANGSMITH_API_KEY_PROD LANGCHAIN_API_KEY LINEAR_API_KEY LINEAR_WEBHOOK_SECRET + OPENAI_API_KEY RUNLOOP_API_KEY SLACK_BOT_TOKEN SLACK_CLIENT_SECRET + SLACK_SIGNING_SECRET TOKEN_ENCRYPTION_KEY USER_ID_API_KEY_MAP X_SERVICE_AUTH_JWT_SECRET +) + batch_get_secrets_tsv() { - local token="" page - while :; do - if [ -n "$token" ]; then - page="$(aws secretsmanager batch-get-secret-value \ - --filters "Key=name,Values=${SECRET_PREFIX}" \ - --region "$REGION" --no-cli-pager --output json --next-token "$token")" - else - page="$(aws secretsmanager batch-get-secret-value \ - --filters "Key=name,Values=${SECRET_PREFIX}" \ - --region "$REGION" --no-cli-pager --output json)" - fi + local -a ids=() + local v + for v in "${SECRET_VARS[@]}"; do ids+=("${SECRET_PREFIX}${v}"); done + + local i page + local -a chunk + for ((i = 0; i < ${#ids[@]}; i += 20)); do + chunk=("${ids[@]:i:20}") + # Capture the response into a variable FIRST so a non-zero `aws` exit (throttle, + # AccessDenied, KMS DecryptionFailure) aborts under set -e instead of being + # silently swallowed — then we'd FAIL-FAST below as "missing secret" with a wrong + # root cause. (Value-less / absent shells come back in .Errors[], not .SecretValues[].) + page="$(aws secretsmanager batch-get-secret-value \ + --secret-id-list "${chunk[@]}" \ + --region "$REGION" --no-cli-pager --output json)" printf '%s' "$page" \ | jq -r '.SecretValues[] | select(.SecretString != null) | (.Name|@base64) + "\t" + (.SecretString|@base64)' - token="$(printf '%s' "$page" | jq -r '.NextToken // empty')" - [ -n "$token" ] || break done } log "reading secrets under ${SECRET_PREFIX} ..." secret_count=0 +# Capture into a variable (NOT `done < <(...)` process substitution) so a non-zero +# exit from batch_get_secrets_tsv propagates under set -e — process substitution hides +# the producer's exit status from the parent shell, which would let a failed AWS call +# fall through to a misleading "missing required var" FAIL-FAST. Mirrors the SSM read. +secrets_tsv="$(batch_get_secrets_tsv)" while IFS=$'\t' read -r nb vb; do [ -n "$nb" ] || continue name="$(printf '%s' "$nb" | b64d)" @@ -198,7 +228,7 @@ while IFS=$'\t' read -r nb vb; do [ -n "$key" ] || continue accept_var "$key" "$value" "Secrets" secret_count=$((secret_count + 1)) -done < <(batch_get_secrets_tsv) +done <<<"$secrets_tsv" log "loaded ${secret_count} secret(s) from Secrets Manager" # --- Sea Haven DEFAULT_REPO_OWNER guard -------------------------------------- diff --git a/infra/lib/constructs/instance-role.ts b/infra/lib/constructs/instance-role.ts index 9f3ea60a..2d75a2e0 100644 --- a/infra/lib/constructs/instance-role.ts +++ b/infra/lib/constructs/instance-role.ts @@ -64,35 +64,28 @@ 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 the batch call below. + // 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-/* secret values and + // nothing else — it can no longer enumerate secret names account-wide. this.role.addToPolicy( new iam.PolicyStatement({ sid: "ReadSecretValues", - actions: ["secretsmanager:GetSecretValue", "secretsmanager:DescribeSecret"], + actions: [ + "secretsmanager:GetSecretValue", + "secretsmanager:BatchGetSecretValue", + "secretsmanager:DescribeSecret", + ], resources: [`arn:aws:secretsmanager:${REGION}:${ACCOUNT}:secret:${p}/*`], }), ); - // 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: "SecretsBatchListOps", - actions: ["secretsmanager:BatchGetSecretValue", "secretsmanager:ListSecrets"], - 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