Repository navigation
fix(ci): vend read-only plan credentials to app tf-plan - #162
Open
Alexanderamiri wants to merge 2 commits into
Open
Alexanderamiri wants to merge 2 commits into
Alexanderamiri wants to merge 2 commits into
Conversation
tf-plan runs on any branch, but the broker vended the write-capable
javabin-ci-team-{team} role, bypassing the LLM review and apply gate. Add
javabin-ci-plan-{team} (ReadOnlyAccess + state read, lock, plan upload)
and have the broker vend it for plan.
Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>
Terraform Plan🚧 Changes detected — Plan: 9 to add, 2 to change, 0 to destroy. Plan outputLLM ReviewRisk: 🟡 MEDIUM Plan creates 3 per-team CI plan IAM roles (cupcake, jzwatch, testteam) with policies and ReadOnlyAccess attachments, and updates the CI broker Lambda code and its IAM policy to allow assuming the new plan roles; nothing is destroyed.
|
ReadOnlyAccess and AllowAll cover every key in the state bucket and the
developer boundary exempts it, so the apps/{team}/* Allow was not a limit.
Add an explicit Deny on the platform prefixes and other teams' prefixes to
ci-plan (read) and ci-team (read/write). Stop the broker test from leaking
its boto3 stub into later tests.
Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>
Terraform Plan🚧 Changes detected — Plan: 9 to add, 5 to change, 0 to destroy. Plan outputLLM ReviewRisk: 🟡 MEDIUM Plan creates 9 CI plan IAM resources for three teams (cupcake, jzwatch, testteam), updates team-management policies and the CI broker role/Lambda, and destroys nothing.
|
This branch has not been deployed
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
The CI broker gives app
tf-planjobsjavabin-ci-team-{team}: AllowAll plus the developer boundary. That is the same write-capable role the apply gate hands out after LLM review. tf-plan runs on every branch and every PR, and the org default repo permission iswrite. So anyone who can push a branch to a team repo can change that team's AWS resources directly, with no review and no apply gate. This is finding H1 in the security audit.What
javabin-ci-plan-{team}roles (for_eachoverregistered_teams). They carry the developer boundary and ateamtag, and only the broker role can assume them.ReadOnlyAccessplus an inlineplan-backendpolicy:s3:GetObjectands3:ListBucketon the state bucket underapps/{team}/*. There is no Put or Delete, because plan never writes state.DenyForeignTerraformState: an explicit Deny ofs3:GetObject(Version)oninfra/*,org/*,state/*,env:/*and every other registered team'sapps/{team}/*. ReadOnlyAccess grantss3:Get*on everything and the boundary'sDenyXTeamexempts the state bucket, so the Allow alone was not a limit.dynamodb:LeadingKeyscondition asci_team.secretsmanager:GetSecretValueon{team}-*secrets, andkms:Decryptonly throughssmandsecretsmanager. These let plan refresh SecureString and secret-version resources, which ReadOnlyAccess cannot read.s3:PutObjectonplans/*in the plan-artifacts bucket, forupload-plan.sh.DenyWriteReviewVerdictsdeny onrisk.json,review.jsonandoverride.json.ci_team_allowgets the same Deny, also covering Put/Delete. Before this,ci-team-{team}could read the platform state (which holds the Google OAuth client secret) and overwrite any key in the bucket.ci_broker/handler.py:ROLE_MAP["plan"]becomesjavabin-ci-plan-.lambdas/main.tf: the broker may also assumejavabin-ci-plan-*.ci-team-*stays allowed for now, so a broker still running the old code keeps working during rollout.test_ci_broker.py:ci-plan-<team>;ci-deploy-<team>;ci-team-*;apply-gate.mdandreusable-workflows.md.The roles are named
ci-plan-*, notci-team-plan-*, so they cannot collide with a team whose name starts withplan-.How verified
terraform fmt -recursive -checkandterraform validatepass.python -m pytest terraform/lambda-src/tests scripts/testspasses (111 tests). Against the oldROLE_MAP, 2 of the 4 new test cases fail.aws_ssm_parameterdecrypts withaws/ssmthrough SSM.aws_iam_role.ci_plan, its policy attachment and its inline policy, one of each per registered team.ci_team_allow(in place, one per team).Review fixes
DenyForeignTerraformStatetoci_planandci_team(built fromlocal.foreign_state_keys). ANotResourceDeny was not used because it would also deny every other S3 object in the account (plan artifacts, app buckets). Gap: the legacy keyapps/platform-test-app/terraform.tfstateis not owned by a registered team and stays readable; it holds no secrets.test_ci_broker.pynow patchessys.modules,sys.pathand the environment only while importing the handler, so the boto3 stub no longer leaks into later tests.apply-gate.mdexplains that the Deny, not the Allow, limits state access.Rollout note
The Lambda code update and the role creation happen in the same apply. An app plan that starts in the seconds between the two could fail once with "Failed to assume role". Re-running the plan fixes it.
Follow-ups
ci-team-*from the broker'ssts:AssumeRoleand remove the broker trust onci_team.ci-deploy-{team}(ECR push and ECS update) is still vended for any ref. The broker could require the default branch fordeploy. I left that out because the moresleep CI work in flight touches the same flow.plan-backend.🤖 Generated with Claude Code