Repository navigation
fix(iam): give per-team SSO sessions guardrails with their team written in - #145
Merged
Merged
Conversation
…en in
Identity Center sessions carry team='' (CloudTrail AssumeRoleWithSAML
principalTags, every session): the ABAC attribute ${path:enterprise.team}
is never set on users. The per-team permission sets attach the developer
boundary, whose DenyXTeam compares resource tags with that empty tag, so
developers were denied every team-scoped resource: their own secrets
(javabin secret list/set), logs and ECS services.
Platform Terraform now renders javabin-team-{team}-guardrails, the
developer boundary with the team name substituted, self-protected like the
boundary. team-provisioner attaches it to javabin-team-{team} instead of
the shared boundary, denies writes to platform-managed parameters and to
parameters that don't exist yet, and re-provisions the permission set so
existing sessions pick it up.
Fixes #144
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Terraform Plan🚧 Changes detected — Plan: 24 to add, 3 to change, 0 to destroy. Plan outputLLM ReviewRisk: 🟡 MEDIUM Plan adds 22 team guardrail IAM policies (javabin-team-*-guardrails), updates the developer boundary policy to also cover the new guardrail policies, and updates the team provisioner Lambda and its IAM role policy; 0 destroys.
|
…e deny Review feedback on #145: - Blocker: anyone with the guardrails (or a team CI role) could create javabin-team-<new>-guardrails with Allow * before a team is registered; team-provisioner attaches it by name. DenyBndMod now also denies iam:CreatePolicy/TagPolicy/UntagPolicy on those names, and the provisioner only attaches a policy tagged managed-by=terraform from javaBin/platform. - PutParameter has no aws:ResourceTag condition key, so the 'deny creating parameters' rule would have denied every secret write. Use ssm:Overwrite instead (creation requires Overwrite unset or false). - When a team's guardrails don't exist yet (new team, or platform), leave the permission set untouched and report pending instead of detaching the boundary and failing provisioning. - Provisioning status errors no longer escape. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Terraform Plan🚧 Changes detected — Plan: 24 to add, 3 to change, 0 to destroy. Plan outputLLM ReviewRisk: 🟡 MEDIUM Plan adds 24 team guardrail IAM policies (one per team), updates the developer boundary policy to let developers create and tag policies, extends the team-provisioner role's SSO and IAM permissions, and updates the provisioner Lambda code; nothing is destroyed.
|
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.
Fixes #144.
Why
principalTags: {team: ""}onAssumeRoleWithSAML, forjavabin-team-cupcake,javabin-team-jzwatchandjavabin-adminalike.${path:enterprise.team}, and nothing ever sets that attribute on users.javabin-developer-boundaryas a policy, and itsDenyXTeamcompares each resource'steamtag with that empty principal tag. So developers are denied everything team-scoped: their own app secrets (javabin secret list/set),/ecs/<team>/*logs, and their ECS services.What changed
terraform/platform/iam/boundary.tf:javabin-team-{team}-guardrailsper registered team: the same rules with${aws:PrincipalTag/team}replaced by the team name.team=shared, andDenyBndModnow also coversjavabin-team-*-guardrails, so team CI roles can't rewrite them.javabin-team-{team}:javabin-team-{team}-guardrailsand detaches the shared boundary.cognito-client-*,auth,db-master-password).terraform apply.Detach…,ProvisionPermissionSet,DescribePermissionSetProvisioningStatus, andiam:GetPolicyon the guardrail policies.The ABAC
javabin-developerpermission set interraform/orgis still unable to reach team resources. That's documented rather than fixed here, because the per-team sets are what people use. #139'sterraform/orgchange targets that set, so it doesn't need applying for secrets self-service.After merge
javabin-team-cupcake, thenjavabin secret list.Checks
terraform validatepasses.terraform consoleconfirms the substitution renders the team name in both the conditions and the ARNs.Guard overrides
aws route53 change-resource-record-sets --hosted-zone-id Z09335963LMV0Z5QB9L45 --change-batch file:///tmp/cupcake-cname-delete.json— deleted the oldcupcake.javazone.noCNAME (→d2cxhqskuuhit7.cloudfront.net) to cut frosting over to the platform, on the user's explicit instruction. Not part of this PR's code; listed because it ran from this session.🤖 Generated with Claude Code