Repository navigation
fix(iam): close cross-team secret, security group and CI-identity holes - #134
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>
Terraform Plan🚧 Changes detected — Plan: 9 to add, 6 to change, 148 to destroy. Plan outputLLM ReviewRisk: 🔴 HIGH Plan destroys multiple ci_deploy team IAM roles (cake, commandchimps, developer, java-no, and likely more truncated) and modifies the shared developer permissions boundary, which is high-impact for CI access across teams.
|
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>
Terraform Plan🚧 Changes detected — Plan: 72 to add, 27 to change, 1 to destroy. Plan outputLLM ReviewRisk: 🟡 MEDIUM Plan updates the developer permissions boundary (adds EC2 security group authorize/revoke actions and changes the ECS execution role exception), and creates per-team ECS execution roles, all carrying the org permissions boundary; no destruction or database changes are visible in the captured portion.
|
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>
Terraform Plan🚧 Changes detected — Plan: 72 to add, 27 to change, 1 to destroy. Plan outputLLM ReviewRisk: 🔴 HIGH The plan updates the developer permissions boundary and creates many per-team ECS execution IAM roles, with the visible portion showing security-relevant changes: the boundary now allows EC2 security group ingress/egress rule changes, and the ECS execution role path is being changed. The output is truncated, so the full scope is unknown.
|
This was referenced Oct 23, 2026
Review feedback on #134: - Blocker: teams could rewrite the trust policy of shared roles (iam:UpdateAssumeRolePolicy was not in DenyShared), so team A could make javabin-ecs-exec-B trust itself and read team B's secrets. Denied. - RS256 verification no longer shells out to openssl: modular exponentiation plus an exact PKCS#1 v1.5 comparison, no temp files. Non-object JWT parts are a TokenError; unknown-kid JWKS refetches are limited to once a minute. - invoke-override-sign.sh still sends approved_by, which the currently deployed gate requires, so overrides keep working during rollout. - github-oidc-token.sh fails clearly when no token is returned; the gate no longer logs the id_token. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Terraform Plan🚧 Changes detected — Plan: 72 to add, 27 to change, 1 to destroy. Plan outputLLM ReviewRisk: 🔴 HIGH Plan modifies the developer permissions boundary (adds EC2 security group authorize/revoke and iam:UpdateAssumeRolePolicy to allowed actions, and swaps the ECS execution role exemption) and creates ~20+ per-team ECS execution roles, with the visible portion showing no destruction, but the boundary loosening is a security-relevant change that needs careful review.
|
Alexanderamiri
added a commit
that referenced
this pull request
Oct 23, 2026
…oles (#137) 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 to `main`. ## Why - **Every review fails.** App `tf-plan` ran `review-plan.py` with `javabin-ci-team-<team>`, and the developer boundary's `DenySecSvcs` denies `bedrock:Invoke*`/`Converse*`. Every app review returned FAILED, so every app apply needed a board override. That's what cupcake and frosting hit. - **The Slack alert can't work either.** The HIGH-risk alert read `/javabin/slack/*`, which `DenyPlatSSM` also denies. - **A plan job could set its own verdict.** The job wrote `risk.json` itself, with team credentials, so it could mark its own plan LOW. ## What changed Option 1 from the issue: - **New Lambda `javabin-plan-review`:** - verifies the job's GitHub OIDC token (aud `javabin-plan-review`), so the plan must be the repo's own - reviews `plan-output.txt` with the same `scripts/review-plan.py` as platform CI, bundled into the zip; the Bedrock call is now injectable - retries transient errors up to 3 times with backoff - writes `risk.json` and `review.json`, and posts HIGH/FAILED to the override channel on pushes to main - returns the markdown for the PR comment - **tf-plan:** re-assumes `javabin-ci-app-broker` via OIDC, which may now invoke the Lambda, and calls `scripts/invoke-plan-review.sh`. The job no longer uploads `risk.json` or sends the Slack alert itself. - **Team roles:** an explicit Deny on `s3:PutObject`/`DeleteObject` for `*/risk.json`, `*/review.json` and `*/override.json` in the plan bucket. - **Bedrock stays out of team roles.** The developer boundary is unchanged. 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 - Lambda tests (23) pass. The new ones cover LOW/MEDIUM verdict written, retry then success, persistent failure → FAILED, another repo's plan refused, bad token refused. - `terraform validate` and actionlint pass. After merge and apply, re-run frosting and cupcake. tf-apply should get a real verdict. 🤖 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 23, 2026
…ml (#138) **Stacked on #134** (per-team execution roles). Merge #134 first and GitHub retargets this to `main`. What cupcake/frosting need for login to work. ## Why - **Groups never reach tokens.** cupcake's backend requires `helter` in `cognito:groups`. Federated (GoogleSAML) users arrive with only an email, and team-provisioner only adds users who already signed in. The internal pool has 0 users and empty groups, so every login would get a 401, and `auth.groups` did nothing. - **The frontend needs a public client.** frosting does OIDC from the browser (oidc-client-ts with PKCE). The platform only made confidential clients with a secret, which a browser can't use. - **The backend needs no client.** cupcake's backend only verifies tokens. `auth: internal` would create a client it never uses. - **The apps read their own env var names:** `NUXT_PUBLIC_OIDC_CLIENT_ID`, `OIDC_WELL_KNOWN_URL`. ## What changed - **Group claims:** - team-provisioner records registry membership (`groups.yaml` groups with `cognito: true`, plus `team-<name>`) in a new `javabin-auth-memberships` table. - A new pre-token-generation V2 trigger, `javabin-auth-claims` (internal pool, ESSENTIALS tier), puts the user's groups into `cognito:groups` of the ID and access token at every sign-in and refresh. Native Cognito groups are kept. - It refuses the token when the client has allowed groups and the user is in none. - **app-auth-sync:** - `client_type` confidential/public; a type change replaces the client, since the secret setting is immutable. - `allowed_groups` written to the table and removed with the client. - `http://localhost` callbacks allowed for local dev. - **cognito-app-client module:** `client_type` (`none` requests no client), `allowed_groups`, and a `well_known_url` output. - **app.yaml:** - `auth.client: confidential|public|none` - `auth.groups` now restricts sign-in - `auth.env` aliases injected values to the app's own names - `COGNITO_WELL_KNOWN_URL` is injected - **Docs:** app-yaml-reference (client types, groups, aliases), lambda-functions, reusable-modules, CLAUDE.md. Cupcake's `app.yaml` after this: ```yaml # frosting auth: pool: internal client: public groups: [helter] callback_urls: [https://cupcake.javazone.no/, https://cupcake.javazone.no/auth/callback, http://localhost:3000/] env: {COGNITO_CLIENT_ID: NUXT_PUBLIC_OIDC_CLIENT_ID, COGNITO_ISSUER_URL: NUXT_PUBLIC_OIDC_AUTHORITY} # cupcake backend auth: pool: internal client: none env: {COGNITO_WELL_KNOWN_URL: OIDC_WELL_KNOWN_URL} ``` ## Checks - Generator tests (19) and Lambda tests (35) pass. New ones cover the auth-claims group union, deny, and caching; public clients, type switching, allowed groups, and localhost-vs-`localhost.evil.com`; and generator client types and aliases. - Generated configs for both cupcake apps pass `terraform validate` against the changed modules, as does `terraform/platform`. ## Rollout notes - Memberships fill on the next registry provisioning run. Re-run `provision-groups` and `provision-teams` in javaBin/registry after apply. Until then tokens carry no registry groups, as today. - Adding the trigger modifies `aws_cognito_user_pool.internal` in place (`lambda_config`). The GoogleSAML IdP, which was created outside Terraform, is a separate resource and is not touched. 🤖 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
## Why `sync-registered-teams.py` lists 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/*.yaml` from 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](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>
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.
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
javabin-ecs-executionrole could read every/javabin/apps/*parameter, and every team was allowed to pass it. Any team could mount another team's secrets, or since fix(ci): make app.yaml deploys work end to end without app secrets #132 their Cognito client secrets, in its own task definition.DenySharedblockedec2:Delete*/Modify*but notec2:AuthorizeSecurityGroupIngress/Revoke*. The IAM policy simulator confirmsjavabin-ci-team-testteammay editjavabin-ecs-tasks-sg. Any team could open every app to every other app, or remove the ALB rule and take all apps offline.ci-brokerandapply-gatetook the repo from the request. The gate'ssignaction (approve a HIGH-risk plan) accepted any caller. Code running in an app's platform job (a Terraformexternaldata source, say) could claim another repo's team credentials or sign its own override.What changed
javabin-ecs-exec-{team}, reading only/javabin/apps/{team}/*. They are taggedteam=shared, so teams can't edit them.platform-datatakesteamand returns that role. The boundary'sDenyPassRoleonly allowsjavabin-ecs-exec-${PrincipalTag/team}. The shared role loses its secrets policy.DenySharednow coversec2:AuthorizeSecurityGroup*,ec2:RevokeSecurityGroup*andiam: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.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 signedrepositoryclaim.checkalso requires the plan key to be underplans/javaBin/<repo>/.signrequires a token fromapprove-override.ymlin javaBin/platform, in theoverride-approvalenvironment. The approver is the token'sactor.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-rdsstill 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.risk.json/review.json/override.json. Overwriting another repo'stfplanonly 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.terraform validateonterraform/platform.Rollout
main, so they sendid_tokenfrom the moment this merges. The old Lambdas ignore it until the apply replaces them. The override script still sendsapproved_by, which the old gate needs.platform-data.🤖 Generated with Claude Code