Repository navigation
fix(ci): create team CI roles only for registry teams - #135
Merged
Merged
Conversation
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>
Terraform Plan🚧 Changes detected — Plan: 0 to add, 0 to change, 147 to destroy. Plan outputLLM ReviewRisk: 🔴 HIGH Plan destroys ci_deploy IAM roles (cake, commandchimps, developer, java-no, and likely more) because their for_each keys were removed from the configuration, which breaks CI deploy access for those teams and is not visible as an intentional, reviewed change.
|
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Terraform Plan🚧 Changes detected — Plan: 0 to add, 0 to change, 147 to destroy. Plan outputLLM ReviewRisk: 🔴 HIGH The plan destroys multiple per-team CI deploy IAM roles (for keys no longer in the for_each map, e.g. cake, commandchimps, developer, java-no, and likely more in the truncated output), which would break those teams' CI/CD pipelines; the output is truncated so the full scope cannot be confirmed.
|
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>
Terraform Plan🚧 Changes detected — Plan: 0 to add, 0 to change, 231 to destroy. Plan outputLLM ReviewRisk: 🔴 HIGH The plan destroys all 231 IAM resources (0 to add, 0 to change, 231 to destroy) for the team guardrail policies, CI deploy and CI team roles, ECS exec roles, and their inline/attached policies, as all of these team keys have been removed from the for_each maps; this is a near-total teardown of team IAM access and only part of the diff (lines 1–8134, 28 of 231 changes) was reviewed.
|
This was referenced Oct 24, 2026
Alexanderamiri
added a commit
that referenced
this pull request
Oct 24, 2026
…accounts (#148) ## What - `atlassian.py`: a group with `all_java_no_accounts: true` also gets every active, managed `@java.no` account. Those are exactly the Google Workspace users, because the Google user sync mirrors them into Atlassian. Unmanaged accounts on a java.no address (e.g. the old `drift@java.no` login) and inactive accounts are excluded. - `provision-atlassian.py` passes the flag through. - Still report-only (`atlassian_sync_apply` unchanged). ## Why Confluence access should cover everyone with a java.no Google account, not just `helter`. ## Verification - Unit tests: lambda-src 63 OK, scripts 31 OK. - Live report run with registry#39's updated YAML: `confluence-users` +47 / −118 (all 118 removals are personal accounts), no member missing an account, no java.no account removed. ## Merge order After this applies, merge javaBin/registry#39 (it uses the flag). Note: main's apply is currently blocked behind #135's HIGH-risk destroys and needs the board override; this change rides along. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Alexanderamiri
added a commit
that referenced
this pull request
Oct 24, 2026
…ole plan (#147) ## Why Reviews kept saying "Plan is truncated; the remaining changes ... could not be reviewed". Where the input was actually cut: | Reviews | Cut by | Real? | |---|---|---| | #134, #135 (14:14, 14:31), #137, #138, #139 on 2026-10-23, before #141 merged at 15:43 | the old 80k cut in `review-plan.py` | yes. These include the quoted "other roles, Lambdas, or networking" review (#135, 14:31) | | #135 on 2026-10-24 09:53 (run 40083680062) | #141's 400k cap: the plan was 830k chars, 231 destroys (destroy diffs print every attribute) | yes. The model saw 28 of 231 changes and correctly said so | | #145 (2026-10-24 09:25, 09:40), #146, app reviews (cupcake#258, frosting#249/#250) | none | no truncation claims | Ruled out: - (a) The PR comment's 60k cut: the review reads `plan-output.txt` itself. The cut was silent to humans, though, so the comment now says when it is cut. - (b) `run-plan.sh` writes the full `-no-color` output with no head/tail. - (c) platform-ci reviews `terraform/platform/plan-output.txt`, the full file. - (e) No reviews hit `max_tokens`. Converse is still called without inferenceConfig (no maxTokens set, on purpose), but a `stopReason: max_tokens` answer is now discarded instead of trusted. - (f) The Lambda reads the whole S3 object. ## What changed - `scripts/review-plan.py`: no default input cap. The whole plan goes to Haiku 5.5, which has a 1M-token context. `REVIEW_MAX_PLAN_CHARS` still turns on the existing notice + change-list mode if it is set. - The prompt now opens with `COMPLETE PLAN` (change count + the `Plan:` line) and says not to call the plan truncated. It also explains that Terraform hides parts of every plan by design: `# (N unchanged attributes/blocks/elements hidden)`, `(sensitive value)`, `(known after apply)` and partial `jsonencode` diffs are not truncation. - `stopReason == "max_tokens"` → returns None. Platform CI records FAILED; the Lambda retries. - `scripts/post-plan-comment.sh`: adds a notice when the plan is cut at 60k (GitHub's comment limit) and keeps the cut-off point configurable. - Docs: `docs/ci-workflow.md`. Trade-off: a plan larger than the model's context now fails the review (FAILED, override available) instead of being cut. ## Checks - `python3 -m unittest discover -s scripts/tests`: 38 OK. New tests: the complete-plan declaration, the elision markers, no default cap (2M-char plan), an 830k destroy plan sent whole, max_tokens discarded, and the comment cut notice. - `python3 -m unittest discover -s terraform/lambda-src/tests`: 63 OK. New tests: COMPLETE PLAN reaches Bedrock, and a max_tokens answer is retried. - `terraform fmt -check -recursive terraform`: OK. `shellcheck scripts/post-plan-comment.sh`: OK. No workflow changes. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Alexanderamiri
added a commit
that referenced
this pull request
Oct 24, 2026
Follow-up to #147, which merged before this review fix was pushed. ## Why With no ceiling, a plan beyond Haiku 5.5's 1M-token context fails the review with a Bedrock ValidationException, which blocks the apply until a board override. The review of #147 recommended a ceiling well above any real plan. ## What changed - `REVIEW_MAX_PLAN_CHARS` defaults to 2,000,000 chars, about 500–800k tokens and 2.4× the largest real plan (#135, 830k chars). Real plans are still sent whole. Only a pathological plan falls back to the change-list mode (every change listed, diff cut at a resource boundary, with a notice). - The prompt's `(known after apply)` explanation is corrected: the value is derived from other resources too, so judge the expression, not a value. - `post-plan-comment.sh` removes its temp file. ## Checks Both test suites pass, with new tests for real-size plans sent whole and the fallback above the ceiling. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Alexanderamiri
added a commit
that referenced
this pull request
Oct 24, 2026
#135 switched team sync to registry `teams/*.yaml`, but the old "GitHub org teams" row survived the merge next to the new one. One-line removal. 🤖 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.
Why
sync-registered-teams.pylists every GitHub org team, so applying #132 created AWS CI and deploy roles (javabin-ci-team-*,javabin-ci-deploy-*) for 21 org teams that are not app teams:owners,developer,java-no,team-*and others. Any repo in one of those GitHub teams could request those roles through the broker.What changed
The sync reads
teams/*.yamlfrom javaBin/registry, the documented source of truth: cupcake, jzwatch, testteam. The committed tfvars are updated to match.Expected plan: 148 to destroy, HIGH risk
21 teams × 7 role resources = 147, plus 1 that only appears if #134 hasn't merged yet. None belong to cupcake, jzwatch or testteam. The roles were created hours ago by #132 and have never been used.
The apply needs a board override through approve-override.yml. That is why this change is split out of #134.
🤖 Generated with Claude Code