Repository navigation
fix(ci): review app plans in a platform Lambda instead of with team roles - #137
Merged
Merged
Conversation
- 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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
…oles 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) <noreply@anthropic.com>
Terraform Plan🚧 Changes detected — Plan: 76 to add, 52 to change, 1 to destroy. Plan outputLLM ReviewRisk: 🔴 HIGH Plan updates the developer permissions boundary (adds EC2 security group authorize/revoke actions and changes the ECS execution role exemption) and creates a large batch of per-team ECS execution IAM roles; the visible output is truncated, so the full plan has not been reviewed.
|
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) <noreply@anthropic.com>
Terraform Plan🚧 Changes detected — Plan: 76 to add, 52 to change, 1 to destroy. Plan outputLLM ReviewRisk: 🔴 HIGH Plan creates many per-team ECS execution roles and modifies the developer permissions boundary, which adds ec2:AuthorizeSecurityGroup*/RevokeSecurityGroup* permissions and changes the ECS execution role exemption; the output is truncated so the full plan (including any destroys and remaining changes) could not be reviewed.
|
Alexanderamiri
added a commit
that referenced
this pull request
Oct 23, 2026
…es (#134) Closes three of the isolation holes found while exploring app-to-app support. All of them matter before cupcake stores real credentials. Its sleepingpill credentials can read every talk submission. ## Why 1. **Cross-team secret access.** The shared `javabin-ecs-execution` role could read every `/javabin/apps/*` parameter, and every team was allowed to pass it. Any team could mount another team's secrets, or since #132 their Cognito client secrets, in its own task definition. 2. **Shared security group editable by teams.** `DenyShared` blocked `ec2:Delete*`/`Modify*` but not `ec2:AuthorizeSecurityGroupIngress`/`Revoke*`. The IAM policy simulator confirms `javabin-ci-team-testteam` may edit `javabin-ecs-tasks-sg`. Any team could open every app to every other app, or remove the ALB rule and take all apps offline. 3. **Broker and gate trusted the payload.** `ci-broker` and `apply-gate` took the repo from the request. The gate's `sign` action (approve a HIGH-risk plan) accepted 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. ## What changed - **Execution roles:** per-team `javabin-ecs-exec-{team}`, reading only `/javabin/apps/{team}/*`. They are tagged `team=shared`, so teams can't edit them. `platform-data` takes `team` and returns that role. The boundary's `DenyPassRole` only allows `javabin-ecs-exec-${PrincipalTag/team}`. The shared role loses its secrets policy. - **Boundary:** `DenyShared` now covers `ec2:AuthorizeSecurityGroup*`, `ec2:RevokeSecurityGroup*` and `iam:UpdateAssumeRolePolicy`. Without the last one, a team could make another team's execution role trust its own CI role. Size goes up about 110 bytes, to roughly 5,835 of 6,144. - **Broker and gate:** callers send a GitHub OIDC token (`scripts/github-oidc-token.sh`). The Lambdas verify it against GitHub's JWKS (`shared/github_oidc.py`, RS256 verified in plain Python, no dependencies or binaries) and take the repo from its signed `repository` claim. - `check` also requires the plan key to be under `plans/javaBin/<repo>/`. - `sign` requires a token from `approve-override.yml` in javaBin/platform, in the `override-approval` environment. The approver is the token's `actor`. - **approve-override.yml:** passes inputs through env instead of interpolating them into shell. Switching team roles to registry teams was split out into #135, because it destroys unused roles and plans HIGH. Until that merges, execution roles are created for every GitHub org team, like the CI roles today. ## Not in this PR - `service-rds` still allows ingress from the shared tasks SG, so team databases share a network path (password still required). Fixing that needs per-app SGs; recorded in CLAUDE.md. cupcake has no database. - Team roles can still write other repos' objects in the plan bucket. #137 denies writing `risk.json`/`review.json`/`override.json`. Overwriting another repo's `tfplan` only makes its apply fail the SHA256 check, so it's a nuisance, not an escalation. Prefix-scoping the bucket per repo is follow-up work. ## Checks - `python3 -m unittest discover -s terraform/lambda-src/tests`: 20 tests pass. The new ones sign real RS256 tokens: tampered payload, wrong audience/org/workflow/branch, the override-signing workflow and environment pin, non-object claims, and verification without any subprocess. - Reviewed by an independent agent. Its blockers are fixed in the last commit: the exec-role trust takeover, and the openssl dependency (the live runtime does have openssl, but verification no longer needs it). - Generator tests pass. - `terraform validate` on `terraform/platform`. - actionlint. ## Rollout - Scripts are read from `main`, so they send `id_token` from the moment this merges. The old Lambdas ignore it until the apply replaces them. The override script still sends `approved_by`, which the old gate needs. - The apply creates the exec roles before apps regenerate against the new `platform-data`. - platform-test-app has no running tasks, so dropping the shared role's secrets policy affects nothing live. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
# Conflicts: # docs/apply-gate.md # scripts/github-oidc-token.sh # scripts/invoke-override-sign.sh # terraform/lambda-src/shared/github_oidc.py # terraform/lambda-src/tests/test_github_oidc.py
Terraform Plan🚧 Changes detected — Plan: 76 to add, 52 to change, 1 to destroy. Plan outputLLM ReviewRisk: 🔴 HIGH Plan updates the developer permissions boundary (adding ec2 security group authorize/revoke and IAM UpdateAssumeRolePolicy permissions, and changing the ECS execution role exemption) and creates about 30+ per-team ECS execution roles, with the output truncated before the full plan is visible.
|
Alexanderamiri
added a commit
that referenced
this pull request
Oct 23, 2026
## Why Applying #137 and #138 failed (run 39978752602): `reading ZIP file (identity/builds/auth_claims.zip): no such file or directory`. The plan job uploads only `terraform/platform/lambdas/builds/` as the `lambda-zips` artifact. #138 added a Lambda in the `identity` module, whose zip is built at `identity/builds/`. A saved plan doesn't rebuild `archive_file` outputs, so the apply job needs every zip. ## What changed - Upload `${TF_ROOT}/*/builds/*.zip` (paths relative to `TF_ROOT`), and fail if nothing matches. - Download into `${TF_ROOT}/`, so each zip lands in its own module's `builds/`. ## Checks actionlint. The real check is this PR's own merge run, which must apply the pending #137/#138 changes (10 to add, 30 to change, MEDIUM). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #136. Stacked on #134 (it reuses that PR's GitHub OIDC verification). The base is
fix/team-isolation, so the diff shows only this change; merge #134 first and GitHub retargets this tomain.Why
tf-planranreview-plan.pywithjavabin-ci-team-<team>, and the developer boundary'sDenySecSvcsdeniesbedrock:Invoke*/Converse*. Every app review returned FAILED, so every app apply needed a board override. That's what cupcake and frosting hit./javabin/slack/*, whichDenyPlatSSMalso denies.risk.jsonitself, with team credentials, so it could mark its own plan LOW.What changed
Option 1 from the issue:
javabin-plan-review:javabin-plan-review), so the plan must be the repo's ownplan-output.txtwith the samescripts/review-plan.pyas platform CI, bundled into the zip; the Bedrock call is now injectablerisk.jsonandreview.json, and posts HIGH/FAILED to the override channel on pushes to mainjavabin-ci-app-brokervia OIDC, which may now invoke the Lambda, and callsscripts/invoke-plan-review.sh. The job no longer uploadsrisk.jsonor sends the Slack alert itself.s3:PutObject/DeleteObjectfor*/risk.json,*/review.jsonand*/override.jsonin the plan bucket.On the issue's second point: a review that can't run after retries is still FAILED and blocked. The PR comment now says the review could not run and to re-run the workflow, instead of looking like a risk verdict.
Checks
terraform validateand actionlint pass.After merge and apply, re-run frosting and cupcake. tf-apply should get a real verdict.
🤖 Generated with Claude Code