From 757af91339aed0e9c1cf4ae6828779669e5a52a0 Mon Sep 17 00:00:00 2001 From: Alexanderamiri Date: Fri, 23 Oct 2026 16:09:40 +0200 Subject: [PATCH 1/7] fix(iam): per-team ECS execution roles and lock teams out of shared SGs - The shared javabin-ecs-execution role could read every /javabin/apps/* parameter and every team could pass it, so any team could mount another team's secrets in its own task definition. Each registered team now gets javabin-ecs-exec-{team}, reading only its own path; the developer boundary only lets a team pass its own. The shared role keeps image pull and logs only. - DenyShared did not cover ec2:AuthorizeSecurityGroup*/RevokeSecurityGroup*, so any team CI role could open every app to every other or remove the ALB rule on the shared ECS tasks SG (confirmed with the IAM simulator). Co-Authored-By: Claude Opus 5.5 (1M context) --- scripts/generate-modules.py | 1 + terraform/modules/platform-data/main.tf | 4 +- terraform/modules/platform-data/outputs.tf | 2 +- terraform/modules/platform-data/variables.tf | 5 ++ terraform/platform/iam/boundary.tf | 4 +- terraform/platform/iam/main.tf | 75 ++++++++++++++++---- 6 files changed, 73 insertions(+), 18 deletions(-) diff --git a/scripts/generate-modules.py b/scripts/generate-modules.py index da71a34..53f1694 100755 --- a/scripts/generate-modules.py +++ b/scripts/generate-modules.py @@ -271,6 +271,7 @@ def src(mod): blocks.append(module_block("platform", src("platform-data"), { "project": PROJECT, "domain": DOMAIN, + "team": team, })) # ECR (always) diff --git a/terraform/modules/platform-data/main.tf b/terraform/modules/platform-data/main.tf index 9228dc2..4be9be8 100644 --- a/terraform/modules/platform-data/main.tf +++ b/terraform/modules/platform-data/main.tf @@ -58,8 +58,10 @@ data "aws_ecs_cluster" "main" { cluster_name = "${var.project}-platform" } +# Per-team execution role (created by platform CI for every registered team); +# it can read only //apps//* secrets. data "aws_iam_role" "ecs_execution" { - name = "${var.project}-ecs-execution" + name = "${var.project}-ecs-exec-${var.team}" } data "aws_route53_zone" "main" { diff --git a/terraform/modules/platform-data/outputs.tf b/terraform/modules/platform-data/outputs.tf index 068bc7f..23ac52c 100644 --- a/terraform/modules/platform-data/outputs.tf +++ b/terraform/modules/platform-data/outputs.tf @@ -54,7 +54,7 @@ output "ecs_cluster_name" { } output "execution_role_arn" { - description = "ECS execution role ARN (shared across all services)" + description = "The team's ECS execution role ARN (reads only that team's secrets)" value = data.aws_iam_role.ecs_execution.arn } diff --git a/terraform/modules/platform-data/variables.tf b/terraform/modules/platform-data/variables.tf index 56e4f00..b96243d 100644 --- a/terraform/modules/platform-data/variables.tf +++ b/terraform/modules/platform-data/variables.tf @@ -15,3 +15,8 @@ variable "domain" { type = string default = "javazone.no" } + +variable "team" { + description = "Owning team — selects the team's ECS execution role" + type = string +} diff --git a/terraform/platform/iam/boundary.tf b/terraform/platform/iam/boundary.tf index 68c29c0..8ed9378 100644 --- a/terraform/platform/iam/boundary.tf +++ b/terraform/platform/iam/boundary.tf @@ -149,6 +149,8 @@ resource "aws_iam_policy" "developer_boundary" { "iam:DeleteRole", "iam:UpdateRole", "iam:DeleteRolePolicy", "iam:PutRolePolicy", "iam:AttachRolePolicy", "iam:DetachRolePolicy", "route53:DeleteHostedZone", "sns:DeleteTopic", "sns:SetTopicAttributes", + # Shared SGs (ECS tasks, ALB): a team rule here opens or cuts every app + "ec2:AuthorizeSecurityGroup*", "ec2:RevokeSecurityGroup*", ] Resource = "*" Condition = { StringEquals = { "aws:ResourceTag/team" = "shared" } } @@ -184,7 +186,7 @@ resource "aws_iam_policy" "developer_boundary" { Action = "iam:PassRole" NotResource = [ "arn:aws:iam::*:role/$${aws:PrincipalTag/team}-*", - "arn:aws:iam::*:role/${var.project}-ecs-execution", + "arn:aws:iam::*:role/${var.project}-ecs-exec-$${aws:PrincipalTag/team}", ] }, { diff --git a/terraform/platform/iam/main.tf b/terraform/platform/iam/main.tf index 2e8d90b..f98fc12 100644 --- a/terraform/platform/iam/main.tf +++ b/terraform/platform/iam/main.tf @@ -522,7 +522,7 @@ resource "aws_iam_role_policy" "ci_deploy_ecs" { Effect = "Allow" Action = ["iam:PassRole"] Resource = [ - aws_iam_role.ecs_execution.arn, + aws_iam_role.ecs_exec_team[each.key].arn, "arn:aws:iam::${var.aws_account_id}:role/${each.key}-*", ] } @@ -754,6 +754,8 @@ resource "aws_iam_role_policy" "ci_apply_gate" { # Individual task roles are created per-app via the service-role module. ################################################################################ +# Deprecated shared execution role. Kept (image pull + logs only, no secrets) +# until no task definition references it; teams can no longer pass it. resource "aws_iam_role" "ecs_execution" { name = "${var.project}-ecs-execution" @@ -781,25 +783,68 @@ resource "aws_iam_role_policy_attachment" "ecs_execution_base" { policy_arn = "arn:aws:iam::aws:policy/service-role/AmazonECSTaskExecutionRolePolicy" } -resource "aws_iam_role_policy" "ecs_execution_secrets" { - name = "secrets-read" - role = aws_iam_role.ecs_execution.id +################################################################################ +# Per-team ECS execution roles — javabin-ecs-exec-{team} +# +# The execution role pulls the image and resolves the task's `secrets` at +# launch. One shared role that can read every //apps/* parameter let +# any team mount another team's secrets in its own task definition. Each team +# now gets a role that reads only its own path, and the developer boundary's +# DenyPassRole only lets a team pass its own role. +# +# Tagged team=shared so DenyShared/DenyTagStrip stop teams from editing them. +################################################################################ - # //apps///... holds service-secret params and the - # Cognito client credentials written by app-auth-sync. The execution role - # fetches them at task launch via the ECS `secrets` block. - policy = jsonencode({ +resource "aws_iam_role" "ecs_exec_team" { + for_each = toset(var.registered_teams) + + name = "${var.project}-ecs-exec-${each.key}" + # The org boundary denies creating any role without one + permissions_boundary = local.org_boundary_arn + + assume_role_policy = jsonencode({ Version = "2012-10-17" Statement = [ { - Sid = "SSMAppSecrets" Effect = "Allow" - Action = [ - "ssm:GetParameters", - ] - Resource = [ - "arn:aws:ssm:${var.region}:${var.aws_account_id}:parameter/${var.project}/apps/*", - ] + Principal = { + Service = "ecs-tasks.amazonaws.com" + } + Action = "sts:AssumeRole" + } + ] + }) + + tags = { + Name = "${var.project}-ecs-exec-${each.key}" + team = "shared" + for-team = each.key + } +} + +resource "aws_iam_role_policy_attachment" "ecs_exec_team_base" { + for_each = toset(var.registered_teams) + + role = aws_iam_role.ecs_exec_team[each.key].name + policy_arn = "arn:aws:iam::aws:policy/service-role/AmazonECSTaskExecutionRolePolicy" +} + +resource "aws_iam_role_policy" "ecs_exec_team_secrets" { + for_each = toset(var.registered_teams) + + name = "team-secrets-read" + role = aws_iam_role.ecs_exec_team[each.key].id + + # Service-secret params and Cognito client credentials (app-auth-sync) + # under the team's own path only. + policy = jsonencode({ + Version = "2012-10-17" + Statement = [ + { + Sid = "SSMTeamSecrets" + Effect = "Allow" + Action = ["ssm:GetParameters"] + Resource = "arn:aws:ssm:${var.region}:${var.aws_account_id}:parameter/${var.project}/apps/${each.key}/*" } ] }) From 84527012793ee045b78f907cd6b5e0cc65fd448f Mon Sep 17 00:00:00 2001 From: Alexanderamiri Date: Fri, 23 Oct 2026 16:09:40 +0200 Subject: [PATCH 2/7] fix(ci): identify callers of ci-broker and apply-gate by GitHub OIDC Both Lambdas took the repo from the request payload, and the gate's sign action (approve a HIGH-risk plan) was open to any caller. Code running in an app's platform job (a Terraform external data source, say) could claim another repo's team credentials or sign its own override. Callers now send a GitHub OIDC token; the Lambdas verify it against GitHub's JWKS (RS256 via openssl, no new dependencies) and read the repo from its signed claims. check also requires the plan to be the repo's own, and sign requires a token from approve-override.yml in javaBin/platform in the override-approval environment, recording the token's actor. The override workflow passes inputs via env instead of interpolating them. Co-Authored-By: Claude Opus 5.5 (1M context) --- .github/workflows/approve-override.yml | 27 ++-- docs/apply-gate.md | 22 ++- scripts/github-oidc-token.sh | 16 ++ scripts/invoke-apply-gate.sh | 5 +- scripts/invoke-ci-broker.sh | 7 +- scripts/invoke-override-sign.sh | 26 +++ terraform/lambda-src/apply_gate/handler.py | 35 +++- terraform/lambda-src/ci_broker/handler.py | 14 +- terraform/lambda-src/shared/github_oidc.py | 150 ++++++++++++++++++ .../lambda-src/tests/test_github_oidc.py | 105 ++++++++++++ terraform/platform/lambdas/main.tf | 8 + 11 files changed, 392 insertions(+), 23 deletions(-) create mode 100644 scripts/github-oidc-token.sh create mode 100644 scripts/invoke-override-sign.sh create mode 100644 terraform/lambda-src/shared/github_oidc.py create mode 100644 terraform/lambda-src/tests/test_github_oidc.py diff --git a/.github/workflows/approve-override.yml b/.github/workflows/approve-override.yml index 998f3fd..bca1177 100644 --- a/.github/workflows/approve-override.yml +++ b/.github/workflows/approve-override.yml @@ -48,25 +48,26 @@ jobs: role-to-assume: arn:aws:iam::${{ env.AWS_ACCOUNT_ID }}:role/javabin-ci-override-approver aws-region: ${{ env.AWS_REGION }} + # The gate verifies this job's GitHub OIDC token (workflow, repo and + # override-approval environment) and records its actor as approver. + # Inputs go through env, never into the script text. - name: Sign override via gate Lambda - run: | - aws lambda invoke \ - --function-name javabin-apply-gate \ - --payload "$(jq -n \ - --arg action sign \ - --arg plan_key "${{ inputs.plan_key }}" \ - --arg approved_by "${{ github.actor }}" \ - --arg reason "${{ inputs.reason }}" \ - '{action: $action, plan_key: $plan_key, approved_by: $approved_by, reason: $reason}')" \ - --cli-binary-format raw-in-base64-out \ - /dev/stdout + env: + PLAN_KEY: ${{ inputs.plan_key }} + REASON: ${{ inputs.reason }} + run: sh scripts/invoke-override-sign.sh - name: Notify Slack env: SSM_WEBHOOK_PARAM: /javabin/slack/platform-override-alerts-webhook - run: sh scripts/notify-slack.sh "Risk Override Approved" "*Repo:* ${{ inputs.repo }}\n*Plan:* \`${{ inputs.plan_key }}\`\n*By:* ${{ github.actor }}\n*Reason:* ${{ inputs.reason }}" + REPO: ${{ inputs.repo }} + PLAN_KEY: ${{ inputs.plan_key }} + REASON: ${{ inputs.reason }} + run: sh scripts/notify-slack.sh "Risk Override Approved" "*Repo:* ${REPO}\n*Plan:* \`${PLAN_KEY}\`\n*By:* ${GITHUB_ACTOR}\n*Reason:* ${REASON}" - name: Retrigger failed apply env: GH_TOKEN: ${{ github.token }} - run: gh run rerun ${{ inputs.run_id }} --repo ${{ inputs.repo }} --failed + RUN_ID: ${{ inputs.run_id }} + REPO: ${{ inputs.repo }} + run: gh run rerun "$RUN_ID" --repo "$REPO" --failed diff --git a/docs/apply-gate.md b/docs/apply-gate.md index 5c0996f..e014c27 100644 --- a/docs/apply-gate.md +++ b/docs/apply-gate.md @@ -20,7 +20,10 @@ tf-plan → uploads plan + plan-output.txt to S3 plan-review → LLM reviews plan → uploads risk.json to S3 tf-apply → assumes javabin-ci-apply-gate (lightweight OIDC role) → downloads plan from S3, verifies SHA256 - → invokes gate Lambda: check(plan_key, repo_name) + → invokes gate Lambda: check(plan_key, team, id_token) + → Lambda verifies the GitHub OIDC token (aud javabin-apply-gate, + a javaBin/platform workflow on main) and takes the repo from its + signed `repository` claim; plan_key must be under plans/javaBin// → Lambda reads risk.json from S3 → LOW/MEDIUM: assumes app CI role via STS, returns temp credentials → HIGH: checks for override.json, verifies HMAC signature @@ -29,6 +32,23 @@ tf-apply → assumes javabin-ci-apply-gate (lightweight OIDC role) → tf-apply uses temp credentials for terraform init + apply ``` +## Caller identity + +The gate and broker roles can be assumed from any javaBin repo's platform +workflow job, and code inside that job (e.g. a Terraform `external` data +source) can invoke the Lambdas directly. So nothing identity-related is taken +from the payload: + +- `check` — repo comes from the job's GitHub OIDC token, the team must be the + app.yaml team the repo belongs to, and the plan must be the repo's own. +- `sign` — only a token from `approve-override.yml` in javaBin/platform, + running in the board-reviewed `override-approval` environment, is accepted. + The approver is the token's `actor`. Without this, any app's tf-apply job + could sign its own HIGH-risk override. + +Tokens are minted with `scripts/github-oidc-token.sh ` and verified +in `terraform/lambda-src/shared/github_oidc.py` (RS256 against GitHub's JWKS). + ## IAM Roles | Role | Purpose | Permissions | diff --git a/scripts/github-oidc-token.sh b/scripts/github-oidc-token.sh new file mode 100644 index 0000000..dde09b4 --- /dev/null +++ b/scripts/github-oidc-token.sh @@ -0,0 +1,16 @@ +#!/bin/sh +# Print a GitHub Actions OIDC token for the given audience. +# +# Usage: github-oidc-token.sh +# +# The job needs `permissions: id-token: write`. The platform Lambdas +# (ci-broker, apply-gate) verify the token and read the repo from its signed +# claims, so a job can only ever act as its own repo. + +set -e + +AUDIENCE="${1:?Usage: github-oidc-token.sh }" +: "${ACTIONS_ID_TOKEN_REQUEST_URL:?id-token: write permission is required}" + +curl -sSf -H "Authorization: bearer ${ACTIONS_ID_TOKEN_REQUEST_TOKEN}" \ + "${ACTIONS_ID_TOKEN_REQUEST_URL}&audience=${AUDIENCE}" | jq -r '.value' diff --git a/scripts/invoke-apply-gate.sh b/scripts/invoke-apply-gate.sh index 528acbf..6cc9473 100644 --- a/scripts/invoke-apply-gate.sh +++ b/scripts/invoke-apply-gate.sh @@ -26,13 +26,16 @@ echo "Requesting apply credentials from gate Lambda..." # Build session name from GitHub context for alert attribution GATE_SESSION="${GITHUB_ACTOR:-unknown}-$(echo "${GITHUB_SHA:-00000000}" | cut -c1-8)-${GITHUB_RUN_ID:-0}" +ID_TOKEN=$(sh "$(dirname "$0")/github-oidc-token.sh" "${PROJECT:-javabin}-apply-gate") + PAYLOAD=$(jq -n \ --arg action "check" \ --arg plan_key "$PLAN_KEY" \ --arg repo_name "$REPO_NAME" \ --arg session_name "$GATE_SESSION" \ --arg team "$TEAM" \ - '{action: $action, plan_key: $plan_key, repo_name: $repo_name, session_name: $session_name, team: $team}') + --arg id_token "$ID_TOKEN" \ + '{action: $action, plan_key: $plan_key, repo_name: $repo_name, session_name: $session_name, team: $team, id_token: $id_token}') RESPONSE=$(aws lambda invoke \ --function-name "$LAMBDA_NAME" \ diff --git a/scripts/invoke-ci-broker.sh b/scripts/invoke-ci-broker.sh index 0829ac7..45e76e1 100755 --- a/scripts/invoke-ci-broker.sh +++ b/scripts/invoke-ci-broker.sh @@ -4,7 +4,8 @@ # Usage: invoke-ci-broker.sh # action: "plan" or "deploy" # -# Env: GITHUB_REPOSITORY (org/repo) +# Env: GITHUB_REPOSITORY (org/repo, informational — the broker takes the +# repo from the job's OIDC token; needs id-token: write) # APP_TEAM (optional) — team from app.yaml; the broker verifies the repo # belongs to it instead of picking the repo's first GitHub team # Output: Exports AWS_ACCESS_KEY_ID, AWS_SECRET_ACCESS_KEY, AWS_SESSION_TOKEN @@ -14,13 +15,15 @@ set -e ACTION="${1:?Usage: invoke-ci-broker.sh }" REPO="${GITHUB_REPOSITORY##*/}" +ID_TOKEN=$(sh "$(dirname "$0")/github-oidc-token.sh" "${PROJECT:-javabin}-ci-broker") echo "Requesting ${ACTION} credentials for ${REPO}..." RESPONSE=$(aws lambda invoke \ --function-name "${PROJECT:-javabin}-ci-broker" \ --payload "$(jq -nc --arg repo "$REPO" --arg action "$ACTION" --arg session_name "${SESSION_NAME:-}" --arg team "${APP_TEAM:-}" \ - '{repo: $repo, action: $action, session_name: $session_name, team: $team}')" \ + --arg id_token "$ID_TOKEN" \ + '{repo: $repo, action: $action, session_name: $session_name, team: $team, id_token: $id_token}')" \ --cli-binary-format raw-in-base64-out \ /tmp/broker-response.json 2>&1) diff --git a/scripts/invoke-override-sign.sh b/scripts/invoke-override-sign.sh new file mode 100644 index 0000000..0bbd8a8 --- /dev/null +++ b/scripts/invoke-override-sign.sh @@ -0,0 +1,26 @@ +#!/bin/sh +# Ask the apply-gate Lambda to sign a risk override for a plan. +# +# Usage: invoke-override-sign.sh +# Env: PLAN_KEY, REASON; runs in approve-override.yml (the gate checks the +# OIDC token comes from that workflow and the override-approval +# environment, and records the token's actor as approver). + +set -e + +DIR=$(dirname "$0") +ID_TOKEN=$(sh "$DIR/github-oidc-token.sh" "${PROJECT:-javabin}-apply-gate") + +aws lambda invoke \ + --function-name "${PROJECT:-javabin}-apply-gate" \ + --payload "$(jq -nc --arg plan_key "$PLAN_KEY" --arg reason "$REASON" --arg id_token "$ID_TOKEN" \ + '{action: "sign", plan_key: $plan_key, reason: $reason, id_token: $id_token}')" \ + --cli-binary-format raw-in-base64-out \ + /tmp/override-response.json >/dev/null + +BODY=$(jq -r '.body' /tmp/override-response.json) +if [ "$(echo "$BODY" | jq -r '.signed // false')" != "true" ]; then + echo "ERROR: override not signed: $(echo "$BODY" | jq -r '.error // "unknown error"')" + exit 1 +fi +echo "Override signed for $PLAN_KEY" diff --git a/terraform/lambda-src/apply_gate/handler.py b/terraform/lambda-src/apply_gate/handler.py index 487e7b1..dfd5624 100644 --- a/terraform/lambda-src/apply_gate/handler.py +++ b/terraform/lambda-src/apply_gate/handler.py @@ -19,6 +19,9 @@ import boto3 from shared.github import resolve_team +from shared.github_oidc import TokenError, require_workflow +from shared.github_oidc import repo_name as oidc_repo_name +from shared.github_oidc import verify as verify_oidc logger = logging.getLogger(__name__) logger.setLevel(logging.INFO) @@ -32,6 +35,8 @@ ) PLAN_BUCKET = os.environ.get("PLAN_BUCKET", "") PROJECT = os.environ.get("PROJECT", "javabin") +GITHUB_ORG = os.environ.get("GITHUB_ORG", "javaBin") +OIDC_AUDIENCE = os.environ.get("OIDC_AUDIENCE", f"{PROJECT}-apply-gate") CREDENTIAL_DURATION = int(os.environ.get("CREDENTIAL_DURATION", "900")) # 15 min # Cache signing key across invocations @@ -87,8 +92,17 @@ def action_check(event): Output: {approved, risk_level, credentials?, reason?} """ plan_key = event["plan_key"] - repo_name = event["repo_name"] team = event.get("team") or None + # Repo from the signed GitHub OIDC token, not the payload (see ci_broker) + try: + repo_name = oidc_repo_name(verify_oidc(event.get("id_token"), OIDC_AUDIENCE)) + except TokenError as e: + logger.warning("Rejected gate call: %s", e) + return {"approved": False, "reason": f"Invalid GitHub OIDC token: {e}"} + # Plans are stored under plans///...; never apply another + # repo's (possibly LOW-risk or overridden) plan. + if not plan_key.startswith(f"plans/{GITHUB_ORG}/{repo_name}/"): + return {"approved": False, "reason": f"Plan {plan_key} does not belong to {repo_name}"} session_name = event.get("session_name") prefix = _plan_prefix(plan_key) @@ -153,11 +167,26 @@ def action_check(event): def action_sign(event): """Create a signed override token and write it to S3. - Input: {plan_key, approved_by, reason} + Input: {plan_key, reason, id_token} Output: {signed: true, plan_key} + + Only the approve-override workflow in javaBin/platform, running behind + the board-reviewed override-approval environment, may sign. Without this + check any app's tf-apply job could invoke the gate and approve its own + HIGH-risk plan. The approver is the token's actor, not a payload field. """ plan_key = event["plan_key"] - approved_by = event["approved_by"] + try: + claims = require_workflow( + verify_oidc(event.get("id_token"), OIDC_AUDIENCE), + "approve-override.yml", + repository=f"{GITHUB_ORG}/platform", + environment="override-approval", + ) + except TokenError as e: + logger.warning("Rejected override signing: %s", e) + return {"signed": False, "error": f"Invalid GitHub OIDC token: {e}"} + approved_by = claims["actor"] reason = event.get("reason", "") prefix = _plan_prefix(plan_key) diff --git a/terraform/lambda-src/ci_broker/handler.py b/terraform/lambda-src/ci_broker/handler.py index af56074..20e211b 100644 --- a/terraform/lambda-src/ci_broker/handler.py +++ b/terraform/lambda-src/ci_broker/handler.py @@ -16,6 +16,7 @@ import boto3 from shared.github import resolve_team +from shared.github_oidc import TokenError, repo_name, verify logger = logging.getLogger(__name__) logger.setLevel(logging.INFO) @@ -57,13 +58,20 @@ def _assume_role(role_arn, session_name, duration): } +OIDC_AUDIENCE = os.environ.get("OIDC_AUDIENCE", f"{PROJECT}-ci-broker") + + def handler(event, context): - repo = event.get("repo", "") action = event.get("action", "plan") requested_team = event.get("team") or None - if not repo: - return {"error": "repo is required", "approved": False} + # The repo comes from a GitHub OIDC token, never from the payload: code + # running inside an app job can invoke this Lambda and name any repo. + try: + repo = repo_name(verify(event.get("id_token"), OIDC_AUDIENCE)) + except TokenError as e: + logger.warning("Rejected broker call: %s", e) + return {"error": f"Invalid GitHub OIDC token: {e}", "approved": False} if action not in ROLE_MAP: return {"error": f"Invalid action: {action}. Must be plan or deploy", "approved": False} diff --git a/terraform/lambda-src/shared/github_oidc.py b/terraform/lambda-src/shared/github_oidc.py new file mode 100644 index 0000000..d53dda7 --- /dev/null +++ b/terraform/lambda-src/shared/github_oidc.py @@ -0,0 +1,150 @@ +"""Verify GitHub Actions OIDC tokens — shared by ci_broker and apply_gate. + +The broker and gate roles are assumable from any javaBin repo running a +platform workflow, and code running inside that job (e.g. a Terraform +`external` data source during tf-plan) can call the Lambdas too. So the repo +must not come from the request payload: the caller sends a GitHub OIDC token +minted for this audience, and the repo is read from its signed `repository` +claim, which the job cannot forge. + +RS256 is verified with the openssl CLI (in the Lambda runtime), so no +third-party packages are needed. +""" + +import base64 +import json +import os +import subprocess +import tempfile +import time +import urllib.request + +ISSUER = "https://token.actions.githubusercontent.com" +JWKS_URL = f"{ISSUER}/.well-known/jwks" +GITHUB_ORG = os.environ.get("GITHUB_ORG", "javaBin") +PLATFORM_WORKFLOW_PREFIX = f"{GITHUB_ORG}/platform/.github/workflows/" +CLOCK_SKEW = 60 + +_jwks_cache = {"keys": None, "fetched_at": 0} + + +class TokenError(ValueError): + """The token is missing, malformed, expired, or not from a platform workflow.""" + + +def _b64url_decode(s): + return base64.urlsafe_b64decode(s + "=" * (-len(s) % 4)) + + +def _der_len(n): + if n < 0x80: + return bytes([n]) + b = n.to_bytes((n.bit_length() + 7) // 8, "big") + return bytes([0x80 | len(b)]) + b + + +def _der_int(b): + if b[0] & 0x80: + b = b"\x00" + b + return b"\x02" + _der_len(len(b)) + b + + +def _der_seq(*parts): + body = b"".join(parts) + return b"\x30" + _der_len(len(body)) + body + + +def jwk_to_pem(jwk): + """Encode an RSA JWK (n, e) as a SubjectPublicKeyInfo PEM.""" + rsa_key = _der_seq(_der_int(_b64url_decode(jwk["n"])), _der_int(_b64url_decode(jwk["e"]))) + algorithm = _der_seq(b"\x06\x09\x2a\x86\x48\x86\xf7\x0d\x01\x01\x01", b"\x05\x00") # rsaEncryption, NULL + bit_string = b"\x03" + _der_len(len(rsa_key) + 1) + b"\x00" + rsa_key + der = _der_seq(algorithm, bit_string) + b64 = base64.b64encode(der).decode() + lines = [b64[i:i + 64] for i in range(0, len(b64), 64)] + return "-----BEGIN PUBLIC KEY-----\n" + "\n".join(lines) + "\n-----END PUBLIC KEY-----\n" + + +def _jwks(force=False): + if force or not _jwks_cache["keys"] or time.time() - _jwks_cache["fetched_at"] > 3600: + with urllib.request.urlopen(JWKS_URL, timeout=10) as resp: + _jwks_cache["keys"] = {k["kid"]: k for k in json.loads(resp.read())["keys"]} + _jwks_cache["fetched_at"] = time.time() + return _jwks_cache["keys"] + + +def _verify_rs256(signing_input, signature, jwk): + with tempfile.NamedTemporaryFile("w", suffix=".pem", delete=False) as k, \ + tempfile.NamedTemporaryFile("wb", suffix=".sig", delete=False) as sig: + k.write(jwk_to_pem(jwk)) + sig.write(signature) + try: + r = subprocess.run( + ["openssl", "dgst", "-sha256", "-verify", k.name, "-signature", sig.name], + input=signing_input, capture_output=True, + ) + return r.returncode == 0 + finally: + os.unlink(k.name) + os.unlink(sig.name) + + +def verify(token, audience, now=None, keys=None): + """Return the verified claims, or raise TokenError. + + Accepts tokens from any javaBin/platform workflow on main; callers that + need a specific workflow use require_workflow() on the result. + """ + if not token or token.count(".") != 2: + raise TokenError("missing or malformed GitHub OIDC token") + header_b64, payload_b64, sig_b64 = token.split(".") + try: + header = json.loads(_b64url_decode(header_b64)) + claims = json.loads(_b64url_decode(payload_b64)) + signature = _b64url_decode(sig_b64) + except ValueError as e: + raise TokenError(f"undecodable token: {e}") + + if header.get("alg") != "RS256": + raise TokenError(f"unsupported alg {header.get('alg')!r}") + jwks = keys if keys is not None else _jwks() + jwk = jwks.get(header.get("kid")) + if jwk is None and keys is None: + jwk = _jwks(force=True).get(header.get("kid")) # key rotation + if jwk is None: + raise TokenError("unknown signing key") + if not _verify_rs256(f"{header_b64}.{payload_b64}".encode(), signature, jwk): + raise TokenError("bad signature") + + now = now if now is not None else time.time() + if claims.get("iss") != ISSUER: + raise TokenError("wrong issuer") + aud = claims.get("aud") + if audience not in (aud if isinstance(aud, list) else [aud]): + raise TokenError("wrong audience") + if claims.get("exp", 0) + CLOCK_SKEW < now or claims.get("nbf", 0) - CLOCK_SKEW > now: + raise TokenError("token expired or not yet valid") + if claims.get("repository_owner") != GITHUB_ORG: + raise TokenError("token is not from the javaBin org") + if not str(claims.get("job_workflow_ref", "")).startswith(PLATFORM_WORKFLOW_PREFIX) or \ + not str(claims.get("job_workflow_ref", "")).endswith("@refs/heads/main"): + raise TokenError("token is not from a javaBin/platform workflow on main") + return claims + + +def require_workflow(claims, workflow, repository=None, environment=None): + """Raise TokenError unless the token came from this platform workflow + file (e.g. "approve-override.yml"), optionally in a given repo and + GitHub environment.""" + if claims.get("job_workflow_ref") != f"{PLATFORM_WORKFLOW_PREFIX}{workflow}@refs/heads/main": + raise TokenError(f"token is not from {workflow}") + if repository and claims.get("repository") != repository: + raise TokenError(f"token is not from {repository}") + if environment and claims.get("environment") != environment: + raise TokenError(f"token is not from the {environment} environment") + return claims + + +def repo_name(claims): + """Repo name (without org) from verified claims.""" + return claims["repository"].split("/", 1)[1] diff --git a/terraform/lambda-src/tests/test_github_oidc.py b/terraform/lambda-src/tests/test_github_oidc.py new file mode 100644 index 0000000..7658c67 --- /dev/null +++ b/terraform/lambda-src/tests/test_github_oidc.py @@ -0,0 +1,105 @@ +"""Tests for shared.github_oidc — run: python3 -m unittest discover terraform/lambda-src/tests + +Signs tokens with a throwaway RSA key via the openssl CLI, so the real +verification path (JWK -> PEM -> openssl dgst -verify) is exercised. +""" + +import base64 +import json +import os +import shutil +import subprocess +import sys +import tempfile +import unittest + +sys.path.insert(0, os.path.join(os.path.dirname(__file__), "..")) +from shared import github_oidc as oidc # noqa: E402 + +NOW = 1_800_000_000 +AUD = "javabin-ci-broker" + + +def b64url(b): + return base64.urlsafe_b64encode(b).rstrip(b"=").decode() + + +@unittest.skipUnless(shutil.which("openssl"), "openssl not installed") +class GithubOidcTest(unittest.TestCase): + @classmethod + def setUpClass(cls): + cls.dir = tempfile.mkdtemp() + cls.key = os.path.join(cls.dir, "key.pem") + subprocess.run(["openssl", "genrsa", "-out", cls.key, "2048"], check=True, capture_output=True) + mod = subprocess.run(["openssl", "rsa", "-in", cls.key, "-noout", "-modulus"], + check=True, capture_output=True, text=True).stdout.strip().split("=")[1] + cls.jwks = {"k1": {"kid": "k1", "kty": "RSA", "e": b64url((65537).to_bytes(3, "big")), + "n": b64url(bytes.fromhex(mod))}} + + @classmethod + def tearDownClass(cls): + shutil.rmtree(cls.dir) + + def token(self, **overrides): + claims = { + "iss": oidc.ISSUER, "aud": AUD, "exp": NOW + 300, "nbf": NOW - 10, + "repository": "javaBin/cupcake", "repository_owner": "javaBin", "actor": "someone", + "job_workflow_ref": "javaBin/platform/.github/workflows/tf-plan.yml@refs/heads/main", + } + claims.update(overrides) + signing_input = f"{b64url(json.dumps({'alg': 'RS256', 'kid': 'k1'}).encode())}.{b64url(json.dumps(claims).encode())}" + sig = subprocess.run(["openssl", "dgst", "-sha256", "-sign", self.key], + input=signing_input.encode(), check=True, capture_output=True).stdout + return f"{signing_input}.{b64url(sig)}" + + def verify(self, token, audience=AUD): + return oidc.verify(token, audience, now=NOW, keys=self.jwks) + + def assertRejected(self, token, fragment, audience=AUD): + with self.assertRaises(oidc.TokenError) as cm: + self.verify(token, audience) + self.assertIn(fragment, str(cm.exception)) + + def test_valid_token_yields_repo_from_signed_claims(self): + self.assertEqual(oidc.repo_name(self.verify(self.token())), "cupcake") + + def test_tampered_payload_fails_signature(self): + head, _, sig = self.token().split(".") + forged = b64url(json.dumps({"iss": oidc.ISSUER, "aud": AUD, "exp": NOW + 300, + "repository": "javaBin/moresleep", "repository_owner": "javaBin", + "job_workflow_ref": "javaBin/platform/.github/workflows/tf-plan.yml@refs/heads/main"}).encode()) + self.assertRejected(f"{head}.{forged}.{sig}", "bad signature") + + def test_wrong_audience_expired_and_foreign_org_are_rejected(self): + self.assertRejected(self.token(), "wrong audience", audience="javabin-apply-gate") + self.assertRejected(self.token(exp=NOW - 120), "expired") + self.assertRejected(self.token(repository_owner="evil", repository="evil/cupcake"), "javaBin org") + + def test_token_from_app_repo_workflow_is_rejected(self): + # A token minted by the app's own workflow, not a platform reusable workflow + self.assertRejected(self.token(job_workflow_ref="javaBin/cupcake/.github/workflows/deploy.yaml@refs/heads/main"), + "javaBin/platform workflow") + self.assertRejected(self.token(job_workflow_ref="javaBin/platform/.github/workflows/tf-plan.yml@refs/heads/evil"), + "javaBin/platform workflow") + + def test_missing_or_garbage_token(self): + self.assertRejected(None, "missing") + self.assertRejected("a.b", "missing") + + def test_require_workflow_for_override_signing(self): + ok = self.verify(self.token(repository="javaBin/platform", environment="override-approval", + job_workflow_ref="javaBin/platform/.github/workflows/approve-override.yml@refs/heads/main")) + oidc.require_workflow(ok, "approve-override.yml", "javaBin/platform", "override-approval") + + tf_apply = self.verify(self.token(job_workflow_ref="javaBin/platform/.github/workflows/tf-apply.yml@refs/heads/main")) + with self.assertRaises(oidc.TokenError): + oidc.require_workflow(tf_apply, "approve-override.yml", "javaBin/platform", "override-approval") + + no_env = self.verify(self.token(repository="javaBin/platform", + job_workflow_ref="javaBin/platform/.github/workflows/approve-override.yml@refs/heads/main")) + with self.assertRaises(oidc.TokenError): + oidc.require_workflow(no_env, "approve-override.yml", "javaBin/platform", "override-approval") + + +if __name__ == "__main__": + unittest.main() diff --git a/terraform/platform/lambdas/main.tf b/terraform/platform/lambdas/main.tf index e6c8471..5a02b73 100644 --- a/terraform/platform/lambdas/main.tf +++ b/terraform/platform/lambdas/main.tf @@ -1383,6 +1383,10 @@ data "archive_file" "apply_gate" { content = file("${local.lambda_src_path}/shared/github.py") filename = "shared/github.py" } + source { + content = file("${local.lambda_src_path}/shared/github_oidc.py") + filename = "shared/github_oidc.py" + } } resource "aws_iam_role" "apply_gate" { @@ -1487,6 +1491,10 @@ data "archive_file" "ci_broker" { content = file("${local.lambda_src_path}/shared/github.py") filename = "shared/github.py" } + source { + content = file("${local.lambda_src_path}/shared/github_oidc.py") + filename = "shared/github_oidc.py" + } } resource "aws_iam_role" "ci_broker" { From c9f16de0593e9049c667cf92a1aa88ec4dccd67a Mon Sep 17 00:00:00 2001 From: Alexanderamiri Date: Fri, 23 Oct 2026 16:09:40 +0200 Subject: [PATCH 3/7] fix(ci): create team CI roles only for registry teams sync-registered-teams listed every GitHub org team, so non-app teams like owners and developer got AWS CI and deploy roles. Use teams/*.yaml in javaBin/registry, the documented source of truth. Co-Authored-By: Claude Opus 5.5 (1M context) --- CLAUDE.md | 8 +++--- scripts/sync-registered-teams.py | 26 ++++++++++++------- .../platform/registered-teams.auto.tfvars | 4 ++- 3 files changed, 25 insertions(+), 13 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index ae9cf2b..f9260c6 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -168,7 +168,7 @@ terraform/state/ | `password-set` | Self-service password-set for new hero accounts (Function URL) | | `budget-enforcer` | Scales ECS services to zero when team exceeds 200% budget | | `resource-tagger` | EventBridge-triggered, auto-tags created-by + commit on new resources | -| `ci-broker` | Validates repo ∈ app.yaml team, vends short-lived team role credentials | +| `ci-broker` | Verifies the job's GitHub OIDC token (repo from signed claims), checks repo ∈ app.yaml team, vends short-lived team role credentials | | `app-auth-sync` | Creates/deletes Cognito app clients requested by app.yaml `auth:` (SSM request → client + credentials) | ### Scripts @@ -178,7 +178,8 @@ terraform/state/ | `scripts/generate-modules.py` | CI: validates app.yaml, generates fmt-clean module blocks into terraform/ (tests: `scripts/tests/`) | | `scripts/registry.py` | Module sources + boilerplate templates for generate-modules.py | | `scripts/resolve-tool-version.sh` | CI: resolve node/java version from the app repo's version files | -| `scripts/sync-registered-teams.py` | CI: GitHub org teams → `registered-teams.auto.tfvars` (needs App token) | +| `scripts/sync-registered-teams.py` | CI: registry `teams/*.yaml` → `registered-teams.auto.tfvars` (needs App token) | +| `scripts/github-oidc-token.sh` | CI: mint a GitHub OIDC token for the broker / apply gate | | `scripts/provision-teams.py` | CI: fetch team YAMLs from registry, invoke team-provisioner Lambda | | `scripts/provision-groups.py` | CI: resolve members.yaml + access.yaml, invoke team-provisioner Lambda | | `scripts/sync-members.py` | CI: sync approved heroes from Google Sheets into members.yaml | @@ -202,6 +203,7 @@ terraform/state/ | SSM params (platform) | `/javabin/{namespace}/{name}` | | SSM params (app) | `/javabin/apps/{team}/{service}/{name}` | | ECS cluster | `javabin-platform` | +| ECS execution roles | `javabin-ecs-exec-{team}` (platform-owned, reads only `/javabin/apps/{team}/*`) | | ECR repos | `{team}-{service}` | | SNS topics | `javabin-{alerts,security,budget-enforcement}` | | Log groups | `/ecs/{team}/{service}` | @@ -287,7 +289,7 @@ The SA JSON key is at `/javabin/platform/google-admin-sa`, the impersonation tar ### Known Issues - **ECS deploy stabilization**: platform-test-app task registers but service fails health check - **Cognito**: pools deployed; internal pool's `GoogleSAML` IdP was created outside Terraform -- **CI broker trusts the payload repo name**: code running inside an app job (e.g. a Terraform `external` data source in tf-plan) can mint a new OIDC token for the broker role and name another repo. Fix: verify a GitHub OIDC token in the Lambda instead of a payload field +- **App databases share one network path**: service-rds allows ingress from the shared ECS tasks SG, so every team's tasks can reach every team's DB port (password still required). Fix: per-app security groups - **Team provisioner Lambda**: All sync functions working (Google/GitHub/Budget/Cognito/Identity Center). Password-set flow deployed. - **`registered_app_repos` manually managed**: Being replaced with team-scoped IAM roles (repo→team resolved via GitHub API at runtime) - **Cost allocation tags pending activation**: `repo`, `created-by`, `commit` tags need activation in Billing console (requires billing data to appear first) diff --git a/scripts/sync-registered-teams.py b/scripts/sync-registered-teams.py index d1c608f..713dc6d 100644 --- a/scripts/sync-registered-teams.py +++ b/scripts/sync-registered-teams.py @@ -1,19 +1,22 @@ #!/usr/bin/env python3 -"""Sync registered teams from GitHub org teams. +"""Sync registered teams from the registry. -Queries the GitHub API for all teams in the javaBin org and writes -registered-teams.auto.tfvars. This drives team-scoped IAM role creation. +Lists teams/*.yaml in javaBin/registry and writes +registered-teams.auto.tfvars. This drives team-scoped IAM role creation +(CI roles, ECS execution roles). -Source of truth: GitHub teams (created by team-provisioner from registry). +Source of truth: the registry. GitHub org teams also include non-app teams +(owners, developer, ...) that must not get AWS CI roles. Usage: sync-registered-teams.py -Requires: GITHUB_TOKEN env var (with org team read access). +Requires: GITHUB_TOKEN env var (read access to javaBin/registry). """ import json import logging import os +import re import sys import urllib.request @@ -45,10 +48,15 @@ def github_get(path, token): return results +REGISTRY_REPO = os.environ.get("REGISTRY_REPO", "registry") +TEAM_NAME = re.compile(r"^[a-z]{1,12}$") + + def get_teams(token): - """Get all org teams, excluding platform-internal ones.""" - teams = github_get(f"/orgs/{GITHUB_ORG}/teams", token) - slugs = sorted(t["slug"] for t in teams if t["slug"] not in EXCLUDED_TEAMS) + """Registry team names (teams/.yaml), excluding platform-internal ones.""" + files = github_get(f"/repos/{GITHUB_ORG}/{REGISTRY_REPO}/contents/teams", token) + names = [f["name"][:-5] for f in files if f["type"] == "file" and f["name"].endswith(".yaml")] + slugs = sorted(n for n in names if TEAM_NAME.match(n) and n not in EXCLUDED_TEAMS) logger.info("Found %d teams (excluding %s)", len(slugs), ", ".join(EXCLUDED_TEAMS)) for slug in slugs: logger.info(" %s", slug) @@ -59,7 +67,7 @@ def write_tfvars(teams, output_path): """Write the registered-teams.auto.tfvars file.""" lines = [ "# GENERATED by sync-registered-teams.py — do not edit manually", - "# Source of truth: GitHub teams in javaBin org", + "# Source of truth: teams/*.yaml in javaBin/registry", "", "registered_teams = [", ] diff --git a/terraform/platform/registered-teams.auto.tfvars b/terraform/platform/registered-teams.auto.tfvars index bcc0487..9ce27b9 100644 --- a/terraform/platform/registered-teams.auto.tfvars +++ b/terraform/platform/registered-teams.auto.tfvars @@ -1,6 +1,8 @@ # GENERATED by sync-registered-teams.py — do not edit manually -# Source of truth: GitHub teams in javaBin org +# Source of truth: teams/*.yaml in javaBin/registry registered_teams = [ + "cupcake", + "jzwatch", "testteam", ] From 1fbc270dc6fb510a78aefe00b83360ecf7ec25e9 Mon Sep 17 00:00:00 2001 From: Alexanderamiri Date: Fri, 23 Oct 2026 16:12:54 +0200 Subject: [PATCH 4/7] revert: keep syncing team roles from GitHub org teams in this PR Moved to its own PR: switching to registry teams destroys the roles of 21 non-app org teams (148 resources), which plans HIGH and needs a board override. The security fixes here should not wait on that. Co-Authored-By: Claude Opus 5.5 (1M context) --- CLAUDE.md | 8 +++--- scripts/sync-registered-teams.py | 26 +++++++------------ .../platform/registered-teams.auto.tfvars | 4 +-- 3 files changed, 13 insertions(+), 25 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index f9260c6..ae9cf2b 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -168,7 +168,7 @@ terraform/state/ | `password-set` | Self-service password-set for new hero accounts (Function URL) | | `budget-enforcer` | Scales ECS services to zero when team exceeds 200% budget | | `resource-tagger` | EventBridge-triggered, auto-tags created-by + commit on new resources | -| `ci-broker` | Verifies the job's GitHub OIDC token (repo from signed claims), checks repo ∈ app.yaml team, vends short-lived team role credentials | +| `ci-broker` | Validates repo ∈ app.yaml team, vends short-lived team role credentials | | `app-auth-sync` | Creates/deletes Cognito app clients requested by app.yaml `auth:` (SSM request → client + credentials) | ### Scripts @@ -178,8 +178,7 @@ terraform/state/ | `scripts/generate-modules.py` | CI: validates app.yaml, generates fmt-clean module blocks into terraform/ (tests: `scripts/tests/`) | | `scripts/registry.py` | Module sources + boilerplate templates for generate-modules.py | | `scripts/resolve-tool-version.sh` | CI: resolve node/java version from the app repo's version files | -| `scripts/sync-registered-teams.py` | CI: registry `teams/*.yaml` → `registered-teams.auto.tfvars` (needs App token) | -| `scripts/github-oidc-token.sh` | CI: mint a GitHub OIDC token for the broker / apply gate | +| `scripts/sync-registered-teams.py` | CI: GitHub org teams → `registered-teams.auto.tfvars` (needs App token) | | `scripts/provision-teams.py` | CI: fetch team YAMLs from registry, invoke team-provisioner Lambda | | `scripts/provision-groups.py` | CI: resolve members.yaml + access.yaml, invoke team-provisioner Lambda | | `scripts/sync-members.py` | CI: sync approved heroes from Google Sheets into members.yaml | @@ -203,7 +202,6 @@ terraform/state/ | SSM params (platform) | `/javabin/{namespace}/{name}` | | SSM params (app) | `/javabin/apps/{team}/{service}/{name}` | | ECS cluster | `javabin-platform` | -| ECS execution roles | `javabin-ecs-exec-{team}` (platform-owned, reads only `/javabin/apps/{team}/*`) | | ECR repos | `{team}-{service}` | | SNS topics | `javabin-{alerts,security,budget-enforcement}` | | Log groups | `/ecs/{team}/{service}` | @@ -289,7 +287,7 @@ The SA JSON key is at `/javabin/platform/google-admin-sa`, the impersonation tar ### Known Issues - **ECS deploy stabilization**: platform-test-app task registers but service fails health check - **Cognito**: pools deployed; internal pool's `GoogleSAML` IdP was created outside Terraform -- **App databases share one network path**: service-rds allows ingress from the shared ECS tasks SG, so every team's tasks can reach every team's DB port (password still required). Fix: per-app security groups +- **CI broker trusts the payload repo name**: code running inside an app job (e.g. a Terraform `external` data source in tf-plan) can mint a new OIDC token for the broker role and name another repo. Fix: verify a GitHub OIDC token in the Lambda instead of a payload field - **Team provisioner Lambda**: All sync functions working (Google/GitHub/Budget/Cognito/Identity Center). Password-set flow deployed. - **`registered_app_repos` manually managed**: Being replaced with team-scoped IAM roles (repo→team resolved via GitHub API at runtime) - **Cost allocation tags pending activation**: `repo`, `created-by`, `commit` tags need activation in Billing console (requires billing data to appear first) diff --git a/scripts/sync-registered-teams.py b/scripts/sync-registered-teams.py index 713dc6d..d1c608f 100644 --- a/scripts/sync-registered-teams.py +++ b/scripts/sync-registered-teams.py @@ -1,22 +1,19 @@ #!/usr/bin/env python3 -"""Sync registered teams from the registry. +"""Sync registered teams from GitHub org teams. -Lists teams/*.yaml in javaBin/registry and writes -registered-teams.auto.tfvars. This drives team-scoped IAM role creation -(CI roles, ECS execution roles). +Queries the GitHub API for all teams in the javaBin org and writes +registered-teams.auto.tfvars. This drives team-scoped IAM role creation. -Source of truth: the registry. GitHub org teams also include non-app teams -(owners, developer, ...) that must not get AWS CI roles. +Source of truth: GitHub teams (created by team-provisioner from registry). Usage: sync-registered-teams.py -Requires: GITHUB_TOKEN env var (read access to javaBin/registry). +Requires: GITHUB_TOKEN env var (with org team read access). """ import json import logging import os -import re import sys import urllib.request @@ -48,15 +45,10 @@ def github_get(path, token): return results -REGISTRY_REPO = os.environ.get("REGISTRY_REPO", "registry") -TEAM_NAME = re.compile(r"^[a-z]{1,12}$") - - def get_teams(token): - """Registry team names (teams/.yaml), excluding platform-internal ones.""" - files = github_get(f"/repos/{GITHUB_ORG}/{REGISTRY_REPO}/contents/teams", token) - names = [f["name"][:-5] for f in files if f["type"] == "file" and f["name"].endswith(".yaml")] - slugs = sorted(n for n in names if TEAM_NAME.match(n) and n not in EXCLUDED_TEAMS) + """Get all org teams, excluding platform-internal ones.""" + teams = github_get(f"/orgs/{GITHUB_ORG}/teams", token) + slugs = sorted(t["slug"] for t in teams if t["slug"] not in EXCLUDED_TEAMS) logger.info("Found %d teams (excluding %s)", len(slugs), ", ".join(EXCLUDED_TEAMS)) for slug in slugs: logger.info(" %s", slug) @@ -67,7 +59,7 @@ def write_tfvars(teams, output_path): """Write the registered-teams.auto.tfvars file.""" lines = [ "# GENERATED by sync-registered-teams.py — do not edit manually", - "# Source of truth: teams/*.yaml in javaBin/registry", + "# Source of truth: GitHub teams in javaBin org", "", "registered_teams = [", ] diff --git a/terraform/platform/registered-teams.auto.tfvars b/terraform/platform/registered-teams.auto.tfvars index 9ce27b9..bcc0487 100644 --- a/terraform/platform/registered-teams.auto.tfvars +++ b/terraform/platform/registered-teams.auto.tfvars @@ -1,8 +1,6 @@ # GENERATED by sync-registered-teams.py — do not edit manually -# Source of truth: teams/*.yaml in javaBin/registry +# Source of truth: GitHub teams in javaBin org registered_teams = [ - "cupcake", - "jzwatch", "testteam", ] From 7c0cc785681cefbea27e2e83680ba31c9840b51e Mon Sep 17 00:00:00 2001 From: Alexanderamiri Date: Fri, 23 Oct 2026 16:30:14 +0200 Subject: [PATCH 5/7] docs: restore CLAUDE.md notes for this PR's changes The previous revert also undid the CLAUDE.md entries for the execution roles, the OIDC token script and the broker fix. Co-Authored-By: Claude Opus 5.5 (1M context) --- CLAUDE.md | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index ae9cf2b..3d1a8c8 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -168,7 +168,7 @@ terraform/state/ | `password-set` | Self-service password-set for new hero accounts (Function URL) | | `budget-enforcer` | Scales ECS services to zero when team exceeds 200% budget | | `resource-tagger` | EventBridge-triggered, auto-tags created-by + commit on new resources | -| `ci-broker` | Validates repo ∈ app.yaml team, vends short-lived team role credentials | +| `ci-broker` | Verifies the job's GitHub OIDC token (repo from signed claims), checks repo ∈ app.yaml team, vends short-lived team role credentials | | `app-auth-sync` | Creates/deletes Cognito app clients requested by app.yaml `auth:` (SSM request → client + credentials) | ### Scripts @@ -179,6 +179,7 @@ terraform/state/ | `scripts/registry.py` | Module sources + boilerplate templates for generate-modules.py | | `scripts/resolve-tool-version.sh` | CI: resolve node/java version from the app repo's version files | | `scripts/sync-registered-teams.py` | CI: GitHub org teams → `registered-teams.auto.tfvars` (needs App token) | +| `scripts/github-oidc-token.sh` | CI: mint a GitHub OIDC token for the broker / apply gate | | `scripts/provision-teams.py` | CI: fetch team YAMLs from registry, invoke team-provisioner Lambda | | `scripts/provision-groups.py` | CI: resolve members.yaml + access.yaml, invoke team-provisioner Lambda | | `scripts/sync-members.py` | CI: sync approved heroes from Google Sheets into members.yaml | @@ -202,6 +203,7 @@ terraform/state/ | SSM params (platform) | `/javabin/{namespace}/{name}` | | SSM params (app) | `/javabin/apps/{team}/{service}/{name}` | | ECS cluster | `javabin-platform` | +| ECS execution roles | `javabin-ecs-exec-{team}` (platform-owned, reads only `/javabin/apps/{team}/*`) | | ECR repos | `{team}-{service}` | | SNS topics | `javabin-{alerts,security,budget-enforcement}` | | Log groups | `/ecs/{team}/{service}` | @@ -287,7 +289,7 @@ The SA JSON key is at `/javabin/platform/google-admin-sa`, the impersonation tar ### Known Issues - **ECS deploy stabilization**: platform-test-app task registers but service fails health check - **Cognito**: pools deployed; internal pool's `GoogleSAML` IdP was created outside Terraform -- **CI broker trusts the payload repo name**: code running inside an app job (e.g. a Terraform `external` data source in tf-plan) can mint a new OIDC token for the broker role and name another repo. Fix: verify a GitHub OIDC token in the Lambda instead of a payload field +- **App databases share one network path**: service-rds allows ingress from the shared ECS tasks SG, so every team's tasks can reach every team's DB port (password still required). Fix: per-app security groups - **Team provisioner Lambda**: All sync functions working (Google/GitHub/Budget/Cognito/Identity Center). Password-set flow deployed. - **`registered_app_repos` manually managed**: Being replaced with team-scoped IAM roles (repo→team resolved via GitHub API at runtime) - **Cost allocation tags pending activation**: `repo`, `created-by`, `commit` tags need activation in Billing console (requires billing data to appear first) From 7d2540fc63e56b18e26b36a9b46408d0f4c882c3 Mon Sep 17 00:00:00 2001 From: Alexanderamiri Date: Fri, 23 Oct 2026 16:41:10 +0200 Subject: [PATCH 6/7] fix(ci): review app plans in a platform Lambda instead of with team roles App tf-plan ran the LLM review with the team CI role, which the developer boundary denies Bedrock, so every app review FAILED and every apply needed a board override (#136). The HIGH-risk Slack alert read /javabin/slack/*, also denied, and the job wrote risk.json itself with team credentials. tf-plan now invokes javabin-plan-review through the broker OIDC role. The Lambda verifies the job's GitHub OIDC token, reviews the repo's own plan text with the same review-plan.py as platform CI (retrying transient errors), writes risk.json/review.json and sends the Slack alert. Team roles are denied writing risk.json, review.json and override.json, so a plan job cannot set its own verdict. Fixes #136 Co-Authored-By: Claude Opus 5.5 (1M context) --- .github/workflows/tf-plan.yml | 28 ++-- CLAUDE.md | 6 +- docs/lambda-functions.md | 13 ++ docs/reusable-workflows.md | 4 +- scripts/invoke-plan-review.sh | 39 ++++++ scripts/review-plan.py | 46 ++++--- terraform/lambda-src/plan_review/handler.py | 110 +++++++++++++++ .../lambda-src/tests/test_plan_review.py | 102 ++++++++++++++ terraform/platform/iam/main.tf | 16 ++- terraform/platform/iam/variables.tf | 5 + terraform/platform/lambdas/main.tf | 130 ++++++++++++++++++ terraform/platform/lambdas/outputs.tf | 5 + terraform/platform/main.tf | 19 +-- 13 files changed, 476 insertions(+), 47 deletions(-) create mode 100644 scripts/invoke-plan-review.sh create mode 100644 terraform/lambda-src/plan_review/handler.py create mode 100644 terraform/lambda-src/tests/test_plan_review.py diff --git a/.github/workflows/tf-plan.yml b/.github/workflows/tf-plan.yml index fad29b5..326fa81 100644 --- a/.github/workflows/tf-plan.yml +++ b/.github/workflows/tf-plan.yml @@ -126,21 +126,25 @@ jobs: run: .platform/scripts/upload-plan.sh "${{ inputs.tf_root }}" # ---------------------------------------------------------------- - # LLM Plan Review (runs before PR comment so both are combined) + # LLM plan review — done by the javabin-plan-review Lambda with + # platform credentials (team roles have no Bedrock access, and only + # the Lambda may write risk.json). Re-assume the broker OIDC role, + # which may invoke it. # ---------------------------------------------------------------- + - name: Assume broker role for plan review + if: steps.plan.outputs.has_changes == 'true' + uses: aws-actions/configure-aws-credentials@v6 + with: + role-to-assume: arn:aws:iam::${{ inputs.aws_account_id }}:role/javabin-ci-app-broker + aws-region: ${{ inputs.aws_region }} + role-session-name: ${{ env.SESSION_NAME }} + - name: Run LLM review id: review if: steps.plan.outputs.has_changes == 'true' env: - REVIEW_RESULT_PATH: review-result.json - run: sh .platform/scripts/extract-review-risk.sh .platform/scripts/review-plan.py "${{ inputs.tf_root }}/plan-output.txt" - - - name: Upload risk assessment to S3 - if: steps.plan.outputs.has_changes == 'true' && steps.review.outputs.risk_level != '' - run: | - PLAN_DIR=$(dirname "${{ steps.upload.outputs.plan_key }}") - echo '{"level":"${{ steps.review.outputs.risk_level }}","reviewed_at":"'"$(date -u +%Y-%m-%dT%H:%M:%SZ)"'"}' | \ - aws s3 cp - "s3://${PLAN_BUCKET}/${PLAN_DIR}/risk.json" --content-type application/json + NOTIFY: ${{ github.event_name == 'push' && github.ref == 'refs/heads/main' }} + run: sh .platform/scripts/invoke-plan-review.sh "${{ steps.upload.outputs.plan_key }}" - name: Post plan + review to PR if: github.event_name == 'pull_request' @@ -148,7 +152,3 @@ jobs: GH_TOKEN: ${{ github.token }} PR_NUMBER: ${{ github.event.pull_request.number }} run: sh .platform/scripts/post-plan-comment.sh "${{ inputs.tf_root }}/plan-output.txt" "${{ steps.plan.outputs.has_changes }}" - - - name: Alert Slack on blocked risk - if: github.event_name == 'push' && github.ref == 'refs/heads/main' && (steps.review.outputs.risk_level == 'HIGH' || steps.review.outputs.risk_level == 'FAILED') - run: sh .platform/scripts/notify-high-risk.sh /javabin/slack/platform-override-alerts-webhook "https://github.com/javaBin/platform/actions/workflows/approve-override.yml" diff --git a/CLAUDE.md b/CLAUDE.md index 3d1a8c8..93e74d1 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -96,7 +96,7 @@ terraform/platform/ iam/ GitHub OIDC, CI roles, permission boundary compute/ ECS cluster, ECR base config monitoring/ SNS, EventBridge, Config, GuardDuty, Security Hub - lambdas/ slack-alert, cost-report, daily-cost-check, compliance-reporter, resource-tagger, budget-enforcer, override-cleanup, team-provisioner, apply-gate, securityhub-summary, password-set, ci-broker, app-auth-sync + lambdas/ slack-alert, cost-report, daily-cost-check, compliance-reporter, resource-tagger, budget-enforcer, override-cleanup, team-provisioner, apply-gate, securityhub-summary, password-set, ci-broker, app-auth-sync, plan-review identity/ Cognito user pools (internal + external). Identity Center is in terraform/org/ ``` @@ -170,6 +170,7 @@ terraform/state/ | `resource-tagger` | EventBridge-triggered, auto-tags created-by + commit on new resources | | `ci-broker` | Verifies the job's GitHub OIDC token (repo from signed claims), checks repo ∈ app.yaml team, vends short-lived team role credentials | | `app-auth-sync` | Creates/deletes Cognito app clients requested by app.yaml `auth:` (SSM request → client + credentials) | +| `plan-review` | LLM review of app Terraform plans with platform credentials; writes `risk.json` (invoked by app tf-plan) | ### Scripts | Script | What | @@ -183,7 +184,8 @@ terraform/state/ | `scripts/provision-teams.py` | CI: fetch team YAMLs from registry, invoke team-provisioner Lambda | | `scripts/provision-groups.py` | CI: resolve members.yaml + access.yaml, invoke team-provisioner Lambda | | `scripts/sync-members.py` | CI: sync approved heroes from Google Sheets into members.yaml | -| `scripts/review-plan.py` | CI: LLM plan review via Bedrock | +| `scripts/review-plan.py` | CI: LLM plan review via Bedrock (platform CI; also bundled into the plan-review Lambda) | +| `scripts/invoke-plan-review.sh` | CI: app tf-plan → plan-review Lambda | | `scripts/notify-slack.py` | CI: generic Slack webhook notification | | `scripts/invoke-apply-gate.sh` | CI: invoke gate Lambda for apply credentials | | `scripts/run-plan.sh` | CI: terraform plan with exit code handling | diff --git a/docs/lambda-functions.md b/docs/lambda-functions.md index 4a38c85..3e7abf5 100644 --- a/docs/lambda-functions.md +++ b/docs/lambda-functions.md @@ -116,6 +116,19 @@ Idempotent: skips resources that already have a `created-by` tag (preserves orig **Manual run:** `aws lambda invoke --function-name javabin-app-auth-sync --payload '{"action":"reconcile"}' --cli-binary-format raw-in-base64-out /dev/stdout` +## plan-review + +**Trigger:** Invoked by app `tf-plan` (via the `javabin-ci-app-broker` OIDC role) after the plan is uploaded +**Purpose:** LLM risk review of app Terraform plans with platform credentials. Team CI roles have no Bedrock access (developer boundary), and any code in a plan job runs with them. + +**Flow:** Verify the job's GitHub OIDC token (aud `javabin-plan-review`) → plan must be under `plans/javaBin//` → read `plan-output.txt` → review with Bedrock using `scripts/review-plan.py` (bundled as `review_plan.py`), up to 3 attempts with backoff → write `risk.json` (read by apply-gate) and `review.json` → on HIGH/FAILED for pushes to main, post to the override Slack channel → return risk + markdown for the PR comment. + +Team roles are denied writing `risk.json`, `review.json` and `override.json` in the plan bucket, so a plan job cannot set its own verdict. + +| SSM Parameter | Use | +|---------------|-----| +| `/javabin/slack/platform-override-alerts-webhook` | HIGH/FAILED alerts | + ## Shared Module: pricing `terraform/lambda-src/shared/pricing.py` — provides `estimate_cost()` for enriching alerts with cost impact. Uses AWS Pricing API (us-east-1 endpoint). Caches pricing data across Lambda invocations. diff --git a/docs/reusable-workflows.md b/docs/reusable-workflows.md index 434ad93..4a3f435 100644 --- a/docs/reusable-workflows.md +++ b/docs/reusable-workflows.md @@ -103,9 +103,9 @@ Docker BuildKit build + push to ECR repo `{team}-{app_name}`. Uses GitHub Action ### tf-plan.yml -Plans the committed `terraform/` at the branch head (after expand), uploads the plan to S3 with its SHA256, runs the LLM review (`scripts/review-plan.py`, Bedrock Claude Haiku 5.5) and posts plan + review to the PR. +Plans the committed `terraform/` at the branch head (after expand), uploads the plan to S3 with its SHA256, has the `javabin-plan-review` Lambda review it (Bedrock Claude Haiku 5.5, platform credentials; it writes `risk.json`) and posts plan + review to the PR. A review that cannot run after retries is `FAILED`: re-run the workflow, or request an override if it persists. -**AWS role:** `javabin-ci-app-broker` (OIDC) → broker Lambda → `javabin-ci-team-{team}` +**AWS role:** `javabin-ci-app-broker` (OIDC) → broker Lambda → `javabin-ci-team-{team}`; the review step uses the broker OIDC role to invoke `javabin-plan-review` **Outputs:** `has_changes`, `plan_key`, `plan_sha256`, `risk_level`, `planned_sha` ### tf-apply.yml diff --git a/scripts/invoke-plan-review.sh b/scripts/invoke-plan-review.sh new file mode 100644 index 0000000..6b5b5b6 --- /dev/null +++ b/scripts/invoke-plan-review.sh @@ -0,0 +1,39 @@ +#!/bin/sh +# Ask the plan-review Lambda to review an uploaded app plan. +# +# Usage: invoke-plan-review.sh +# +# Runs with the javabin-ci-app-broker OIDC role (team roles have no Bedrock +# access). The Lambda verifies this job's GitHub OIDC token, reviews the +# plan text next to , and writes risk.json itself. +# +# Env: NOTIFY=true to post HIGH/FAILED to the override Slack channel. +# Output: review-output.txt (markdown for the PR comment), +# risk_level= in GITHUB_OUTPUT. + +set -e + +PLAN_KEY="${1:?Usage: invoke-plan-review.sh }" +ID_TOKEN=$(sh "$(dirname "$0")/github-oidc-token.sh" "${PROJECT:-javabin}-plan-review") + +aws lambda invoke \ + --function-name "${PROJECT:-javabin}-plan-review" \ + --cli-read-timeout 310 \ + --payload "$(jq -nc --arg plan_key "$PLAN_KEY" --arg id_token "$ID_TOKEN" --argjson notify "${NOTIFY:-false}" \ + '{plan_key: $plan_key, id_token: $id_token, notify: $notify}')" \ + --cli-binary-format raw-in-base64-out \ + /tmp/plan-review.json >/dev/null + +ERROR=$(jq -r '.error // .errorMessage // empty' /tmp/plan-review.json) +if [ -n "$ERROR" ]; then + echo "::error::Plan review failed: $ERROR" + RISK=FAILED + printf '**Risk: ⚪ FAILED**\n\nPlan review could not run: %s\n' "$ERROR" > review-output.txt +else + RISK=$(jq -r '.risk' /tmp/plan-review.json) + jq -r '.markdown' /tmp/plan-review.json > review-output.txt +fi + +cat review-output.txt +echo "risk_level=${RISK}" >> "${GITHUB_OUTPUT:-/dev/null}" +echo "Plan review risk: ${RISK}" diff --git a/scripts/review-plan.py b/scripts/review-plan.py index cacf13e..94f73fd 100755 --- a/scripts/review-plan.py +++ b/scripts/review-plan.py @@ -6,6 +6,9 @@ Uses AWS CLI for Bedrock Converse (no boto3 version dependency). Writes structured JSON to REVIEW_RESULT_PATH (default: review-result.json). + +Also bundled into the javabin-plan-review Lambda (app plans), which passes +its own boto3-based `converse` to review_plan(). """ import json @@ -67,8 +70,28 @@ CATEGORY_EMOJI = {"security": "\U0001f512", "cost": "\U0001f4b0", "destruction": "\U0001f4a5", "routine": "\u2705"} -def review_plan(plan_text): - """Send plan to Bedrock via AWS CLI for LLM review, return structured dict.""" +def cli_converse(messages, tool_config): + """Bedrock Converse via the AWS CLI. Returns the response dict or None.""" + result = subprocess.run( + [ + "aws", "bedrock-runtime", "converse", + "--model-id", MODEL_ID, + "--messages", json.dumps(messages), + "--tool-config", json.dumps(tool_config), + "--region", REGION, + ], + capture_output=True, + text=True, + timeout=120, + ) + if result.returncode != 0: + print(f"AWS CLI error: {result.stderr}", file=sys.stderr) + return None + return json.loads(result.stdout) + + +def review_plan(plan_text, converse=cli_converse): + """Send plan to Bedrock for LLM review, return structured dict (or None).""" if len(plan_text) > 80000: plan_text = plan_text[:80000] + "\n\n... (truncated)" @@ -92,25 +115,10 @@ def review_plan(plan_text): messages = [{"role": "user", "content": [{"text": prompt}]}] - result = subprocess.run( - [ - "aws", "bedrock-runtime", "converse", - "--model-id", MODEL_ID, - "--messages", json.dumps(messages), - "--tool-config", json.dumps(REVIEW_TOOL_CONFIG), - "--region", REGION, - ], - capture_output=True, - text=True, - timeout=120, - ) - - if result.returncode != 0: - print(f"AWS CLI error: {result.stderr}", file=sys.stderr) + response = converse(messages, REVIEW_TOOL_CONFIG) + if not response: return None - response = json.loads(result.stdout) - for content in response.get("output", {}).get("message", {}).get("content", []): if "toolUse" in content and content["toolUse"]["name"] == "submit_review": return content["toolUse"]["input"] diff --git a/terraform/lambda-src/plan_review/handler.py b/terraform/lambda-src/plan_review/handler.py new file mode 100644 index 0000000..682d436 --- /dev/null +++ b/terraform/lambda-src/plan_review/handler.py @@ -0,0 +1,110 @@ +"""Plan review — LLM risk review of an app's Terraform plan, on platform credentials. + +tf-plan runs with the team's CI role, which the developer boundary keeps away +from Bedrock (and must: any code in a plan job runs with those credentials). +So app plans are reviewed here instead: + + 1. Verify the caller's GitHub OIDC token; the repo comes from its signed + claims, and the plan must be that repo's own (plans/javaBin//...). + 2. Read plan-output.txt next to the plan in the plan bucket. + 3. Review it with Bedrock (review_plan.py, shared with platform CI), + retrying transient failures. + 4. Write risk.json (read by the apply gate) and review.json. Team roles + cannot write these objects, so a job cannot mark its own plan LOW. + 5. For HIGH or FAILED on main, post the override request to Slack. + +Input: {plan_key, id_token, notify?} +Output: {risk, markdown} or {error} +""" + +import json +import logging +import os +import time + +import boto3 + +import review_plan +from shared.github_oidc import TokenError, repo_name, verify +from shared.slack import get_webhook_url, post_to_slack + +logger = logging.getLogger(__name__) +logger.setLevel(logging.INFO) + +PROJECT = os.environ.get("PROJECT", "javabin") +GITHUB_ORG = os.environ.get("GITHUB_ORG", "javaBin") +PLAN_BUCKET = os.environ["PLAN_BUCKET"] +OIDC_AUDIENCE = os.environ.get("OIDC_AUDIENCE", f"{PROJECT}-plan-review") +SLACK_PARAM = os.environ.get("SLACK_WEBHOOK_PARAM", f"/{PROJECT}/slack/platform-override-alerts-webhook") +OVERRIDE_URL = f"https://github.com/{GITHUB_ORG}/platform/actions/workflows/approve-override.yml" +ATTEMPTS = int(os.environ.get("REVIEW_ATTEMPTS", "3")) + +s3 = boto3.client("s3") +ssm = boto3.client("ssm") +bedrock = boto3.client("bedrock-runtime") + + +def converse(messages, tool_config): + return bedrock.converse(modelId=review_plan.MODEL_ID, messages=messages, toolConfig=tool_config) + + +def review_with_retries(plan_text): + for attempt in range(1, ATTEMPTS + 1): + try: + review = review_plan.review_plan(plan_text, converse=converse) + if review: + return review + logger.warning("Review attempt %d returned no structured output", attempt) + except Exception as e: # throttling, timeouts, model errors + logger.warning("Review attempt %d failed: %s", attempt, e) + if attempt < ATTEMPTS: + time.sleep(2 ** attempt) + return None + + +def put_json(key, body): + s3.put_object(Bucket=PLAN_BUCKET, Key=key, Body=json.dumps(body).encode(), ContentType="application/json") + + +def notify(repo, plan_key, risk, review): + summary = review.get("summary", "") if review else "The LLM review could not run; re-run the workflow or request an override." + text = (f"*Apply blocked — {risk} risk*\n*Repo:* {GITHUB_ORG}/{repo}\n*Plan:* `{plan_key}`\n{summary}\n" + f"<{OVERRIDE_URL}|Approve override>") + try: + post_to_slack(get_webhook_url(ssm, SLACK_PARAM), {"text": text}) + except Exception as e: + logger.error("Slack notification failed: %s", e) + + +def handler(event, context): + try: + repo = repo_name(verify(event.get("id_token"), OIDC_AUDIENCE)) + except TokenError as e: + logger.warning("Rejected review call: %s", e) + return {"error": f"Invalid GitHub OIDC token: {e}"} + + plan_key = event.get("plan_key", "") + prefix = f"plans/{GITHUB_ORG}/{repo}/" + if not plan_key.startswith(prefix) or not plan_key.endswith("/tfplan") or ".." in plan_key: + return {"error": f"Plan {plan_key!r} does not belong to {repo}"} + plan_dir = plan_key.rsplit("/", 1)[0] + + plan_text = s3.get_object(Bucket=PLAN_BUCKET, Key=f"{plan_dir}/plan-output.txt")["Body"].read().decode() + if not plan_text.strip(): + review = {"risk": "LOW", "summary": "Empty plan", "findings": []} + else: + review = review_with_retries(plan_text) + + risk = review["risk"] if review else "FAILED" + reviewed_at = time.strftime("%Y-%m-%dT%H:%M:%SZ", time.gmtime()) + put_json(f"{plan_dir}/risk.json", {"level": risk, "reviewed_at": reviewed_at, "reviewer": f"{PROJECT}-plan-review"}) + put_json(f"{plan_dir}/review.json", review or {"risk": "FAILED"}) + logger.info("Reviewed %s: %s", plan_key, risk) + + if risk in ("HIGH", "FAILED") and event.get("notify"): + notify(repo, plan_key, risk, review) + + markdown = review_plan.format_review(review) if review else ( + "**Risk: ⚪ FAILED**\n\nThe LLM review could not run after " + f"{ATTEMPTS} attempts. Re-run the workflow; if it keeps failing, request an override.") + return {"risk": risk, "markdown": markdown} diff --git a/terraform/lambda-src/tests/test_plan_review.py b/terraform/lambda-src/tests/test_plan_review.py new file mode 100644 index 0000000..bbc4248 --- /dev/null +++ b/terraform/lambda-src/tests/test_plan_review.py @@ -0,0 +1,102 @@ +"""Tests for plan_review — run: python3 -m unittest discover terraform/lambda-src/tests""" + +import importlib.util +import io +import json +import os +import sys +import types +import unittest +from unittest import mock + +HERE = os.path.dirname(__file__) +SRC = os.path.join(HERE, "..") + + +class FakeS3: + def __init__(self, objects): + self.objects = dict(objects) + + def get_object(self, Bucket, Key): + return {"Body": io.BytesIO(self.objects[Key].encode())} + + def put_object(self, Bucket, Key, Body, ContentType): + self.objects[Key] = Body.decode() + + +class FakeBedrock: + def __init__(self, responses): + self.responses = list(responses) + self.calls = 0 + + def converse(self, **kw): + self.calls += 1 + r = self.responses.pop(0) + if isinstance(r, Exception): + raise r + return r + + +def tool_response(risk): + return {"output": {"message": {"content": [{"toolUse": {"name": "submit_review", "input": { + "risk": risk, "summary": "Adds an ECS service", "findings": [{"category": "routine", "detail": "ok"}]}}}]}}} + + +PLAN = "plans/javaBin/frosting/123/tfplan" + + +def load(s3, bedrock, repo="frosting"): + os.environ["PLAN_BUCKET"] = "bucket" + os.environ["REVIEW_ATTEMPTS"] = "3" + boto3 = types.ModuleType("boto3") + boto3.client = lambda svc: {"s3": s3, "bedrock-runtime": bedrock, "ssm": mock.MagicMock()}[svc] + sys.modules["boto3"] = boto3 + rp = importlib.util.spec_from_file_location("review_plan", os.path.join(SRC, "..", "..", "scripts", "review-plan.py")) + review_plan = importlib.util.module_from_spec(rp) + rp.loader.exec_module(review_plan) + sys.modules["review_plan"] = review_plan + sys.path.insert(0, SRC) + spec = importlib.util.spec_from_file_location("plan_review_handler", os.path.join(SRC, "plan_review", "handler.py")) + mod = importlib.util.module_from_spec(spec) + spec.loader.exec_module(mod) + mod.verify = lambda token, aud: {"repository": f"javaBin/{repo}"} if token == "good" else (_ for _ in ()).throw(mod.TokenError("bad")) + mod.time.sleep = lambda s: None + return mod + + +class PlanReviewTest(unittest.TestCase): + def setUp(self): + self.s3 = FakeS3({"plans/javaBin/frosting/123/plan-output.txt": "Plan: 15 to add, 0 to change, 0 to destroy."}) + + def test_review_writes_risk_and_returns_markdown(self): + h = load(self.s3, FakeBedrock([tool_response("LOW")])) + result = h.handler({"plan_key": PLAN, "id_token": "good"}, None) + self.assertEqual(result["risk"], "LOW") + self.assertIn("Adds an ECS service", result["markdown"]) + self.assertEqual(json.loads(self.s3.objects["plans/javaBin/frosting/123/risk.json"])["level"], "LOW") + + def test_transient_failures_are_retried(self): + bedrock = FakeBedrock([RuntimeError("ThrottlingException"), tool_response("MEDIUM")]) + h = load(self.s3, bedrock) + self.assertEqual(h.handler({"plan_key": PLAN, "id_token": "good"}, None)["risk"], "MEDIUM") + self.assertEqual(bedrock.calls, 2) + + def test_persistent_failure_is_failed_not_high(self): + h = load(self.s3, FakeBedrock([RuntimeError("boom")] * 3)) + result = h.handler({"plan_key": PLAN, "id_token": "good"}, None) + self.assertEqual(result["risk"], "FAILED") + self.assertIn("could not run", result["markdown"]) + self.assertEqual(json.loads(self.s3.objects["plans/javaBin/frosting/123/risk.json"])["level"], "FAILED") + + def test_another_repos_plan_is_refused(self): + h = load(self.s3, FakeBedrock([]), repo="cupcake") + self.assertIn("does not belong", h.handler({"plan_key": PLAN, "id_token": "good"}, None)["error"]) + self.assertNotIn("plans/javaBin/frosting/123/risk.json", self.s3.objects) + + def test_invalid_token_is_refused(self): + h = load(self.s3, FakeBedrock([])) + self.assertIn("Invalid GitHub OIDC token", h.handler({"plan_key": PLAN, "id_token": "forged"}, None)["error"]) + + +if __name__ == "__main__": + unittest.main() diff --git a/terraform/platform/iam/main.tf b/terraform/platform/iam/main.tf index f98fc12..e35ae8f 100644 --- a/terraform/platform/iam/main.tf +++ b/terraform/platform/iam/main.tf @@ -306,12 +306,14 @@ resource "aws_iam_role_policy" "ci_app_broker" { name = "invoke-ci-broker-only" role = aws_iam_role.ci_app_broker.id + # The broker vends team credentials; plan-review reviews app plans with + # platform credentials (team roles have no Bedrock access). policy = jsonencode({ Version = "2012-10-17" Statement = [{ Effect = "Allow" Action = "lambda:InvokeFunction" - Resource = var.ci_broker_function_arn + Resource = [var.ci_broker_function_arn, var.plan_review_function_arn] }] }) } @@ -378,6 +380,18 @@ resource "aws_iam_role_policy" "ci_team_allow" { Action = "*" Resource = "*" }, + { + # Review verdicts and overrides come only from the plan-review and + # apply-gate Lambdas; a plan job must not mark its own plan LOW. + Sid = "DenyWriteReviewVerdicts" + Effect = "Deny" + Action = ["s3:PutObject", "s3:DeleteObject"] + Resource = [ + "arn:aws:s3:::${var.project}-ci-plan-artifacts-${var.aws_account_id}/*/risk.json", + "arn:aws:s3:::${var.project}-ci-plan-artifacts-${var.aws_account_id}/*/review.json", + "arn:aws:s3:::${var.project}-ci-plan-artifacts-${var.aws_account_id}/*/override.json", + ] + }, { # Terraform state: S3 scoped to team's key prefix. Sid = "AllowTerraformBackend" diff --git a/terraform/platform/iam/variables.tf b/terraform/platform/iam/variables.tf index 95c0ca0..3e07fa3 100644 --- a/terraform/platform/iam/variables.tf +++ b/terraform/platform/iam/variables.tf @@ -39,3 +39,8 @@ variable "ci_broker_function_arn" { description = "ARN of the CI broker Lambda function (for OIDC broker role invoke permission)" type = string } + +variable "plan_review_function_arn" { + description = "ARN of the plan review Lambda (broker role may invoke it)" + type = string +} diff --git a/terraform/platform/lambdas/main.tf b/terraform/platform/lambdas/main.tf index 5a02b73..216c6c7 100644 --- a/terraform/platform/lambdas/main.tf +++ b/terraform/platform/lambdas/main.tf @@ -1727,3 +1727,133 @@ resource "aws_lambda_permission" "app_auth_sync_reconcile" { principal = "events.amazonaws.com" source_arn = aws_cloudwatch_event_rule.app_auth_sync_reconcile.arn } + +################################################################################ +# Plan Review — LLM risk review of app Terraform plans (javabin-plan-review) +# +# App tf-plan jobs run with team CI roles, which the developer boundary keeps +# away from Bedrock. tf-plan invokes this Lambda (via the broker OIDC role) +# with its GitHub OIDC token; the Lambda reviews the plan text in the plan +# bucket and writes risk.json, which team roles cannot write themselves. +################################################################################ + +locals { + plan_bucket = "${var.project}-ci-plan-artifacts-${var.aws_account_id}" +} + +data "archive_file" "plan_review" { + type = "zip" + output_path = "${path.module}/builds/plan_review.zip" + output_file_mode = "0644" + + source { + content = file("${local.lambda_src_path}/plan_review/handler.py") + filename = "handler.py" + } + # Same prompt and schema as platform CI's own review + source { + content = file("${path.module}/../../../scripts/review-plan.py") + filename = "review_plan.py" + } + source { + content = file("${local.lambda_src_path}/shared/__init__.py") + filename = "shared/__init__.py" + } + source { + content = file("${local.lambda_src_path}/shared/github_oidc.py") + filename = "shared/github_oidc.py" + } + source { + content = file("${local.lambda_src_path}/shared/slack.py") + filename = "shared/slack.py" + } +} + +resource "aws_iam_role" "plan_review" { + name = "${var.project}-plan-review" + # The org boundary denies creating any role without one + permissions_boundary = var.org_boundary_arn + + assume_role_policy = jsonencode({ + Version = "2012-10-17" + Statement = [{ + Effect = "Allow" + Principal = { Service = "lambda.amazonaws.com" } + Action = "sts:AssumeRole" + }] + }) +} + +resource "aws_iam_role_policy" "plan_review" { + name = "${var.project}-plan-review" + role = aws_iam_role.plan_review.id + + policy = jsonencode({ + Version = "2012-10-17" + Statement = [ + { + Sid = "ReadPlanText" + Effect = "Allow" + Action = "s3:GetObject" + Resource = "arn:aws:s3:::${local.plan_bucket}/plans/*/plan-output.txt" + }, + { + Sid = "WriteReview" + Effect = "Allow" + Action = "s3:PutObject" + Resource = [ + "arn:aws:s3:::${local.plan_bucket}/plans/*/risk.json", + "arn:aws:s3:::${local.plan_bucket}/plans/*/review.json", + ] + }, + { + Sid = "BedrockInferenceProfile" + Effect = "Allow" + Action = ["bedrock:InvokeModel", "bedrock:Converse"] + Resource = "arn:aws:bedrock:${var.region}:${var.aws_account_id}:inference-profile/eu.anthropic.*" + }, + { + # Cross-region inference profiles route to EU foundation models; + # the condition only allows them via our inference profiles. + Sid = "BedrockFoundationModels" + Effect = "Allow" + Action = ["bedrock:InvokeModel", "bedrock:Converse"] + Resource = "arn:aws:bedrock:eu-*::foundation-model/anthropic.*" + Condition = { + StringLike = { + "bedrock:InferenceProfileArn" = "arn:aws:bedrock:${var.region}:${var.aws_account_id}:inference-profile/eu.anthropic.*" + } + } + }, + { + Sid = "SlackWebhook" + Effect = "Allow" + Action = "ssm:GetParameter" + Resource = "arn:aws:ssm:${var.region}:${var.aws_account_id}:parameter/${var.project}/slack/platform-override-alerts-webhook" + }, + ] + }) +} + +resource "aws_iam_role_policy_attachment" "plan_review_logs" { + role = aws_iam_role.plan_review.name + policy_arn = "arn:aws:iam::aws:policy/service-role/AWSLambdaBasicExecutionRole" +} + +resource "aws_lambda_function" "plan_review" { + function_name = "${var.project}-plan-review" + role = aws_iam_role.plan_review.arn + handler = "handler.handler" + runtime = "python3.12" + timeout = 300 + memory_size = 256 + filename = data.archive_file.plan_review.output_path + source_code_hash = data.archive_file.plan_review.output_base64sha256 + + environment { + variables = { + PROJECT = var.project + PLAN_BUCKET = local.plan_bucket + } + } +} diff --git a/terraform/platform/lambdas/outputs.tf b/terraform/platform/lambdas/outputs.tf index 58273aa..18b7c2f 100644 --- a/terraform/platform/lambdas/outputs.tf +++ b/terraform/platform/lambdas/outputs.tf @@ -67,3 +67,8 @@ output "budget_enforcement_topic_arn" { description = "ARN of the budget-enforcement SNS topic" value = aws_sns_topic.budget_enforcement.arn } + +output "plan_review_function_arn" { + description = "ARN of the plan review Lambda (invoked by app tf-plan via the broker OIDC role)" + value = aws_lambda_function.plan_review.arn +} diff --git a/terraform/platform/main.tf b/terraform/platform/main.tf index e0112f2..5e72cf1 100644 --- a/terraform/platform/main.tf +++ b/terraform/platform/main.tf @@ -31,15 +31,16 @@ module "ingress" { } module "iam" { - source = "./iam" - project = var.project - region = var.region - aws_account_id = var.aws_account_id - registered_teams = var.registered_teams - github_org = var.github_org - apply_gate_role_arn = module.lambdas.apply_gate_role_arn - ci_broker_role_arn = module.lambdas.ci_broker_role_arn - ci_broker_function_arn = module.lambdas.ci_broker_function_arn + source = "./iam" + project = var.project + region = var.region + aws_account_id = var.aws_account_id + registered_teams = var.registered_teams + github_org = var.github_org + apply_gate_role_arn = module.lambdas.apply_gate_role_arn + ci_broker_role_arn = module.lambdas.ci_broker_role_arn + ci_broker_function_arn = module.lambdas.ci_broker_function_arn + plan_review_function_arn = module.lambdas.plan_review_function_arn } module "compute" { From 1f41e78364ee52e21070e62d568192bd32b9189e Mon Sep 17 00:00:00 2001 From: Alexanderamiri Date: Fri, 23 Oct 2026 17:07:35 +0200 Subject: [PATCH 7/7] fix(ci): plan review always leaves a verdict, even on timeout Review feedback on #137: - boto3's default retries could spend the whole Lambda timeout on one slow Bedrock call, leaving no risk.json, and the gate then refuses even a board override. Bound each call (90s, no SDK retries), stop retrying when time runs short, and write risk.json as FAILED before reviewing. - A failed invoke now yields a FAILED review and PR comment instead of failing the job. - Docs: the plan text is uploaded by the job, so the review catches mistakes rather than forged plans. Co-Authored-By: Claude Opus 5.5 (1M context) --- docs/lambda-functions.md | 2 +- scripts/invoke-plan-review.sh | 7 ++++-- terraform/lambda-src/plan_review/handler.py | 22 +++++++++++++++---- .../lambda-src/tests/test_plan_review.py | 19 +++++++++++++++- 4 files changed, 42 insertions(+), 8 deletions(-) diff --git a/docs/lambda-functions.md b/docs/lambda-functions.md index 3e7abf5..17be8af 100644 --- a/docs/lambda-functions.md +++ b/docs/lambda-functions.md @@ -123,7 +123,7 @@ Idempotent: skips resources that already have a `created-by` tag (preserves orig **Flow:** Verify the job's GitHub OIDC token (aud `javabin-plan-review`) → plan must be under `plans/javaBin//` → read `plan-output.txt` → review with Bedrock using `scripts/review-plan.py` (bundled as `review_plan.py`), up to 3 attempts with backoff → write `risk.json` (read by apply-gate) and `review.json` → on HIGH/FAILED for pushes to main, post to the override Slack channel → return risk + markdown for the PR comment. -Team roles are denied writing `risk.json`, `review.json` and `override.json` in the plan bucket, so a plan job cannot set its own verdict. +Team roles are denied writing `risk.json`, `review.json` and `override.json` in the plan bucket. `risk.json` is written as FAILED before the review starts, so a crash or timeout still leaves a verdict the gate (and a board override) can act on. The plan text is uploaded by the plan job itself, so the review catches mistakes; it is not a control against code in the job forging its own plan text. | SSM Parameter | Use | |---------------|-----| diff --git a/scripts/invoke-plan-review.sh b/scripts/invoke-plan-review.sh index 6b5b5b6..36efdc2 100644 --- a/scripts/invoke-plan-review.sh +++ b/scripts/invoke-plan-review.sh @@ -16,15 +16,18 @@ set -e PLAN_KEY="${1:?Usage: invoke-plan-review.sh }" ID_TOKEN=$(sh "$(dirname "$0")/github-oidc-token.sh" "${PROJECT:-javabin}-plan-review") +# A failed invoke is a FAILED review, not a failed job: the PR still gets a +# comment and the gate still blocks. +echo '{}' > /tmp/plan-review.json aws lambda invoke \ --function-name "${PROJECT:-javabin}-plan-review" \ --cli-read-timeout 310 \ --payload "$(jq -nc --arg plan_key "$PLAN_KEY" --arg id_token "$ID_TOKEN" --argjson notify "${NOTIFY:-false}" \ '{plan_key: $plan_key, id_token: $id_token, notify: $notify}')" \ --cli-binary-format raw-in-base64-out \ - /tmp/plan-review.json >/dev/null + /tmp/plan-review.json >/dev/null || echo '{"error": "could not invoke the plan-review Lambda"}' > /tmp/plan-review.json -ERROR=$(jq -r '.error // .errorMessage // empty' /tmp/plan-review.json) +ERROR=$(jq -r '.error // .errorMessage // (if .risk then empty else "empty response" end)' /tmp/plan-review.json) if [ -n "$ERROR" ]; then echo "::error::Plan review failed: $ERROR" RISK=FAILED diff --git a/terraform/lambda-src/plan_review/handler.py b/terraform/lambda-src/plan_review/handler.py index 682d436..4844e16 100644 --- a/terraform/lambda-src/plan_review/handler.py +++ b/terraform/lambda-src/plan_review/handler.py @@ -10,7 +10,9 @@ 3. Review it with Bedrock (review_plan.py, shared with platform CI), retrying transient failures. 4. Write risk.json (read by the apply gate) and review.json. Team roles - cannot write these objects, so a job cannot mark its own plan LOW. + cannot write these objects. (The plan text itself is uploaded by the + plan job, so the review guards against mistakes, not against code in + the job forging its own plan text.) 5. For HIGH or FAILED on main, post the override request to Slack. Input: {plan_key, id_token, notify?} @@ -23,6 +25,7 @@ import time import boto3 +from botocore.config import Config import review_plan from shared.github_oidc import TokenError, repo_name, verify @@ -41,15 +44,22 @@ s3 = boto3.client("s3") ssm = boto3.client("ssm") -bedrock = boto3.client("bedrock-runtime") +# Bounded per call, so retries fit inside the Lambda timeout; the handler +# does its own retrying. +bedrock = boto3.client("bedrock-runtime", config=Config(read_timeout=90, connect_timeout=10, + retries={"max_attempts": 1, "mode": "standard"})) +CALL_BUDGET_MS = 100_000 def converse(messages, tool_config): return bedrock.converse(modelId=review_plan.MODEL_ID, messages=messages, toolConfig=tool_config) -def review_with_retries(plan_text): +def review_with_retries(plan_text, context=None): for attempt in range(1, ATTEMPTS + 1): + if context and context.get_remaining_time_in_millis() < CALL_BUDGET_MS: + logger.warning("Stopping after %d attempts: not enough time left for another", attempt - 1) + return None try: review = review_plan.review_plan(plan_text, converse=converse) if review: @@ -89,11 +99,15 @@ def handler(event, context): return {"error": f"Plan {plan_key!r} does not belong to {repo}"} plan_dir = plan_key.rsplit("/", 1)[0] + # Record FAILED first: if anything below crashes or times out, the gate + # still finds a verdict (and a board override can unblock it). + put_json(f"{plan_dir}/risk.json", {"level": "FAILED", "reviewed_at": None, "reviewer": f"{PROJECT}-plan-review"}) + plan_text = s3.get_object(Bucket=PLAN_BUCKET, Key=f"{plan_dir}/plan-output.txt")["Body"].read().decode() if not plan_text.strip(): review = {"risk": "LOW", "summary": "Empty plan", "findings": []} else: - review = review_with_retries(plan_text) + review = review_with_retries(plan_text, context) risk = review["risk"] if review else "FAILED" reviewed_at = time.strftime("%Y-%m-%dT%H:%M:%SZ", time.gmtime()) diff --git a/terraform/lambda-src/tests/test_plan_review.py b/terraform/lambda-src/tests/test_plan_review.py index bbc4248..7757c92 100644 --- a/terraform/lambda-src/tests/test_plan_review.py +++ b/terraform/lambda-src/tests/test_plan_review.py @@ -49,8 +49,11 @@ def load(s3, bedrock, repo="frosting"): os.environ["PLAN_BUCKET"] = "bucket" os.environ["REVIEW_ATTEMPTS"] = "3" boto3 = types.ModuleType("boto3") - boto3.client = lambda svc: {"s3": s3, "bedrock-runtime": bedrock, "ssm": mock.MagicMock()}[svc] + boto3.client = lambda svc, **kw: {"s3": s3, "bedrock-runtime": bedrock, "ssm": mock.MagicMock()}[svc] sys.modules["boto3"] = boto3 + botocore = types.ModuleType("botocore"); config = types.ModuleType("botocore.config") + config.Config = lambda **kw: None + sys.modules["botocore"] = botocore; sys.modules["botocore.config"] = config rp = importlib.util.spec_from_file_location("review_plan", os.path.join(SRC, "..", "..", "scripts", "review-plan.py")) review_plan = importlib.util.module_from_spec(rp) rp.loader.exec_module(review_plan) @@ -88,6 +91,20 @@ def test_persistent_failure_is_failed_not_high(self): self.assertIn("could not run", result["markdown"]) self.assertEqual(json.loads(self.s3.objects["plans/javaBin/frosting/123/risk.json"])["level"], "FAILED") + def test_failed_is_recorded_before_review_so_a_crash_leaves_a_verdict(self): + s3 = FakeS3({}) # plan text missing: get_object raises + h = load(s3, FakeBedrock([])) + with self.assertRaises(KeyError): + h.handler({"plan_key": PLAN, "id_token": "good"}, None) + self.assertEqual(json.loads(s3.objects["plans/javaBin/frosting/123/risk.json"])["level"], "FAILED") + + def test_stops_retrying_when_time_runs_out(self): + bedrock = FakeBedrock([RuntimeError("slow")] * 3) + h = load(self.s3, bedrock) + ctx = types.SimpleNamespace(get_remaining_time_in_millis=lambda: 50_000) + self.assertEqual(h.handler({"plan_key": PLAN, "id_token": "good"}, ctx)["risk"], "FAILED") + self.assertEqual(bedrock.calls, 0) + def test_another_repos_plan_is_refused(self): h = load(self.s3, FakeBedrock([]), repo="cupcake") self.assertIn("does not belong", h.handler({"plan_key": PLAN, "id_token": "good"}, None)["error"])