Repository navigation
feat(auth): group claims, public and validate-only clients for app.yaml - #138
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>
Internal-pool tokens had no cognito:groups: federated users only bring an email, and team-provisioner only adds users who already signed in. So cupcake, which requires helter, would have refused everyone, and app.yaml auth.groups changed nothing. team-provisioner now records registry membership in a javabin-auth-memberships table, and a pre-token-generation V2 trigger (javabin-auth-claims) puts the user's groups into the ID and access token at every sign-in and refresh. When an app client lists allowed groups and the user is in none, the trigger refuses the token. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
- auth.client: public creates a client without a secret, for browser apps doing OIDC with PKCE (frosting); none requests no client and only injects issuer values, for APIs that verify another client's tokens (cupcake backend). - auth.groups is sent as the client's allowed groups. - auth.env exposes injected values under names the app already reads (NUXT_PUBLIC_OIDC_CLIENT_ID, OIDC_WELL_KNOWN_URL). - COGNITO_WELL_KNOWN_URL is injected; http://localhost callbacks are allowed for local development. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Terraform Plan🚧 Changes detected — Plan: 78 to add, 32 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 many per-team ECS execution roles, with no destructive actions visible in the truncated output.
|
…roups Review feedback on #138: - Removed or no-longer-cognito groups kept their member rows, so users kept those groups in tokens forever. A full group sync now prunes them (team-<name> and client rows are left alone). - A provisioner payload without member lists no longer overwrites the recorded membership with an empty list. - app-auth-sync refuses to record a client without its allowed groups when the table is not configured, instead of silently dropping the restriction. - Docs: validate-only APIs accept any pool client's token; changing auth.client replaces the client, so redeploy. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Terraform Plan🚧 Changes detected — Plan: 78 to add, 32 to change, 1 to destroy. Plan outputLLM ReviewRisk: 🟡 MEDIUM Plan updates the developer permissions boundary (adds EC2 security group authorize/revoke actions and changes an ECS execution role exclusion) and creates many per-team ECS execution roles, all with the org permissions boundary attached; no destruction of databases or public exposure is visible in the truncated output.
|
# Conflicts: # CLAUDE.md # 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: 78 to add, 32 to change, 1 to destroy. Plan outputLLM ReviewRisk: 🔴 HIGH Plan updates the developer permissions boundary (adds ec2 security group authorize/revoke and iam:UpdateAssumeRolePolicy to the allowed actions, and changes the ECS execution role exemption) and creates many new per-team ECS execution roles; the visible output is truncated, so the full plan should be checked before apply.
|
# Conflicts: # CLAUDE.md # docs/lambda-functions.md
Terraform Plan🚧 Changes detected — Plan: 82 to add, 57 to change, 1 to destroy. Plan outputLLM ReviewRisk: 🔴 HIGH Plan modifies the developer permission boundary (adds security group authorize/revoke and iam:UpdateAssumeRolePolicy permissions, and changes the ECS execution role exemption) and creates many new ECS execution IAM roles, with the visible portion of output truncated before all changes are reviewed.
|
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 was referenced Oct 23, 2026
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.
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
helterincognito: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, andauth.groupsdid nothing.auth: internalwould create a client it never uses.NUXT_PUBLIC_OIDC_CLIENT_ID,OIDC_WELL_KNOWN_URL.What changed
groups.yamlgroups withcognito: true, plusteam-<name>) in a newjavabin-auth-membershipstable.javabin-auth-claims(internal pool, ESSENTIALS tier), puts the user's groups intocognito:groupsof the ID and access token at every sign-in and refresh. Native Cognito groups are kept.client_typeconfidential/public; a type change replaces the client, since the secret setting is immutable.allowed_groupswritten to the table and removed with the client.http://localhostcallbacks allowed for local dev.client_type(nonerequests no client),allowed_groups, and awell_known_urloutput.auth.client: confidential|public|noneauth.groupsnow restricts sign-inauth.envaliases injected values to the app's own namesCOGNITO_WELL_KNOWN_URLis injectedCupcake's
app.yamlafter this:Checks
localhost.evil.com; and generator client types and aliases.terraform validateagainst the changed modules, as doesterraform/platform.Rollout notes
provision-groupsandprovision-teamsin javaBin/registry after apply. Until then tokens carry no registry groups, as today.aws_cognito_user_pool.internalin place (lambda_config). The GoogleSAML IdP, which was created outside Terraform, is a separate resource and is not touched.🤖 Generated with Claude Code