From 0033ab4b34d2fe88767045bd70e85cb63ab8344e Mon Sep 17 00:00:00 2001 From: Alexanderamiri Date: Sat, 24 Oct 2026 15:38:16 +0200 Subject: [PATCH 1/2] fix(ci): vend read-only plan credentials to app tf-plan 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) --- docs/apply-gate.md | 9 +- docs/reusable-workflows.md | 4 +- terraform/lambda-src/ci_broker/handler.py | 9 +- terraform/lambda-src/tests/test_ci_broker.py | 67 ++++++++++ terraform/platform/iam/main.tf | 131 ++++++++++++++++++- terraform/platform/lambdas/main.tf | 2 + 6 files changed, 216 insertions(+), 6 deletions(-) create mode 100644 terraform/lambda-src/tests/test_ci_broker.py diff --git a/docs/apply-gate.md b/docs/apply-gate.md index e60200b..f1c9f31 100644 --- a/docs/apply-gate.md +++ b/docs/apply-gate.md @@ -49,13 +49,20 @@ from the payload: Tokens are minted with `scripts/github-oidc-token.sh ` and verified in `terraform/lambda-src/shared/github_oidc.py` (RS256 against GitHub's JWKS). +Plan credentials are read-only. The broker vends `javabin-ci-plan-{team}` +(ReadOnlyAccess, read on the team's state, the state lock, and PutObject on +`plans/*`) to tf-plan, which runs on any branch. The only way to get the +write-capable `javabin-ci-team-{team}` is through this gate, for a reviewed +plan. + ## IAM Roles | Role | Purpose | Permissions | |------|---------|-------------| | `javabin-ci-apply-gate` | OIDC role for tf-apply workflow | `lambda:InvokeFunction` on gate Lambda + `s3:GetObject` on plan bucket | | `javabin-apply-gate` | Lambda execution role | `ssm:GetParameter` (signing key) + `s3:Get/PutObject` (plan bucket) + `sts:AssumeRole` (app CI roles) | -| `javabin-ci-app-{repo}` | App infrastructure role | Trusted by OIDC (plan/review) AND by gate Lambda role (apply) | +| `javabin-ci-plan-{team}` | Read-only plan role | Vended by the CI broker for tf-plan: ReadOnlyAccess + state read + lock + plan upload | +| `javabin-ci-team-{team}` | Team infrastructure role (write) | Vended by the gate Lambda for a reviewed plan (apply) | ## Credential Lifetime diff --git a/docs/reusable-workflows.md b/docs/reusable-workflows.md index e682ee6..cb37ad5 100644 --- a/docs/reusable-workflows.md +++ b/docs/reusable-workflows.md @@ -29,7 +29,7 @@ The pipeline detects your repo contents and runs the right jobs. No repository o | Prerequisite | Why | How | |---|---|---| -| Team exists in `javaBin/registry` (`teams/.yaml`) | Creates the GitHub team; platform CI then creates the team's CI roles `javabin-ci-team-` and `javabin-ci-deploy-` | `javabin register-team`, merge the PR | +| Team exists in `javaBin/registry` (`teams/.yaml`) | Creates the GitHub team; platform CI then creates the team's CI roles `javabin-ci-plan-`, `javabin-ci-team-` and `javabin-ci-deploy-` | `javabin register-team`, merge the PR | | Repo belongs to the GitHub team named in `app.yaml` `team:` | The CI broker verifies repo → team before vending team credentials | `gh api -X PUT orgs/javaBin/teams//repos/javaBin/ -f permission=push` | | `app.yaml` at repo root | Declares the infrastructure | See [app-yaml-reference.md](app-yaml-reference.md) | @@ -105,7 +105,7 @@ Docker BuildKit build + push to ECR repo `{team}-{app_name}`. Uses GitHub Action Plans the committed `terraform/` at the branch head (after expand), uploads the plan to S3 with its SHA256, has the `javabin-plan-review` Lambda review it (Bedrock Claude Haiku 5.5, platform credentials; it writes `risk.json`) and posts plan + review to the PR. A review that cannot run after retries is `FAILED`: re-run the workflow, or request an override if it persists. -**AWS role:** `javabin-ci-app-broker` (OIDC) → broker Lambda → `javabin-ci-team-{team}`; the review step uses the broker OIDC role to invoke `javabin-plan-review` +**AWS role:** `javabin-ci-app-broker` (OIDC) → broker Lambda → `javabin-ci-plan-{team}` (read-only: refresh, state read, state lock, plan upload; tf-plan runs on any branch, so it cannot change resources); the review step uses the broker OIDC role to invoke `javabin-plan-review` **Outputs:** `has_changes`, `plan_key`, `plan_sha256`, `risk_level`, `planned_sha` ### tf-apply.yml diff --git a/terraform/lambda-src/ci_broker/handler.py b/terraform/lambda-src/ci_broker/handler.py index 20e211b..2e1c2ef 100644 --- a/terraform/lambda-src/ci_broker/handler.py +++ b/terraform/lambda-src/ci_broker/handler.py @@ -6,7 +6,10 @@ then receive team-scoped credentials if the repo belongs to a registered team. Actions: - - plan: Returns credentials for javabin-ci-team-{team} (Terraform plan/apply) + - plan: Returns credentials for javabin-ci-plan-{team} (read-only Terraform + plan: refresh, state lock, plan upload). Write access comes only + from the apply gate, which vends javabin-ci-team-{team} for a + reviewed plan. - deploy: Returns credentials for javabin-ci-deploy-{team} (Docker build/ECS deploy) """ @@ -31,7 +34,9 @@ DEPLOY_DURATION = 900 # 15 minutes for deploy ROLE_MAP = { - "plan": f"{PROJECT}-ci-team-", + # Read-only: any branch of a team repo can run tf-plan, so plan + # credentials must not be able to change the team's resources. + "plan": f"{PROJECT}-ci-plan-", "deploy": f"{PROJECT}-ci-deploy-", } diff --git a/terraform/lambda-src/tests/test_ci_broker.py b/terraform/lambda-src/tests/test_ci_broker.py new file mode 100644 index 0000000..148a9f2 --- /dev/null +++ b/terraform/lambda-src/tests/test_ci_broker.py @@ -0,0 +1,67 @@ +"""Tests for ci_broker — run: python3 -m unittest discover terraform/lambda-src/tests""" + +import importlib.util +import os +import sys +import types +import unittest +from unittest import mock + +SRC = os.path.join(os.path.dirname(__file__), "..") + + +class FakeSTS: + def __init__(self): + self.calls = [] + + def assume_role(self, RoleArn, RoleSessionName, DurationSeconds): + self.calls.append({"RoleArn": RoleArn, "DurationSeconds": DurationSeconds}) + return {"Credentials": {"AccessKeyId": "a", "SecretAccessKey": "s", "SessionToken": "t"}} + + +def load(sts): + os.environ["AWS_ACCOUNT_ID"] = "123456789012" + os.environ["PROJECT"] = "javabin" + boto3 = types.ModuleType("boto3") + boto3.client = lambda svc, **kw: sts if svc == "sts" else mock.MagicMock() + sys.modules["boto3"] = boto3 + sys.path.insert(0, SRC) + spec = importlib.util.spec_from_file_location("ci_broker_handler", os.path.join(SRC, "ci_broker", "handler.py")) + mod = importlib.util.module_from_spec(spec) + spec.loader.exec_module(mod) + mod.verify = lambda token, aud: ({"repository": "javaBin/cupcake"} if token == "good" + else (_ for _ in ()).throw(mod.TokenError("bad"))) + mod.resolve_team = lambda repo, requested: "cupcake" + return mod + + +class CiBrokerTest(unittest.TestCase): + def setUp(self): + self.sts = FakeSTS() + self.broker = load(self.sts) + + def call(self, action): + return self.broker.handler({"action": action, "id_token": "good", "team": "cupcake"}, None) + + def test_plan_vends_the_read_only_plan_role_not_the_team_role(self): + result = self.call("plan") + self.assertTrue(result["approved"]) + self.assertEqual(self.sts.calls[0]["RoleArn"], "arn:aws:iam::123456789012:role/javabin-ci-plan-cupcake") + + def test_deploy_still_vends_the_deploy_role(self): + self.assertTrue(self.call("deploy")["approved"]) + self.assertEqual(self.sts.calls[0]["RoleArn"], "arn:aws:iam::123456789012:role/javabin-ci-deploy-cupcake") + + def test_no_action_ever_vends_the_write_capable_team_role(self): + for action in self.broker.ROLE_MAP: + self.call(action) + self.assertFalse(any("ci-team-" in c["RoleArn"] for c in self.sts.calls)) + + def test_bad_token_gets_no_credentials(self): + result = self.broker.handler({"action": "plan", "id_token": "forged"}, None) + self.assertFalse(result["approved"]) + self.assertEqual(self.sts.calls, []) + + +if __name__ == "__main__": + unittest.main() diff --git a/terraform/platform/iam/main.tf b/terraform/platform/iam/main.tf index de8eb0d..4cc9a61 100644 --- a/terraform/platform/iam/main.tf +++ b/terraform/platform/iam/main.tf @@ -324,9 +324,11 @@ resource "aws_iam_role_policy" "ci_app_broker" { } ################################################################################ -# 2. javabin-ci-team-{team} — Per-team Terraform role (plan + apply) +# 2. javabin-ci-team-{team} — Per-team Terraform role (apply) # # Trust: ONLY the CI broker Lambda and apply-gate Lambda can assume these roles. +# The broker vends javabin-ci-plan-{team} for plan (2b); the broker trust here +# stays so a broker Lambda still on the old code keeps working during rollout. # No direct OIDC trust — the broker validates repo→team membership via GitHub # API before issuing credentials. This is the hard enforcement point. # @@ -432,6 +434,133 @@ resource "aws_iam_role_policy" "ci_team_allow" { }) } +################################################################################ +# 2b. javabin-ci-plan-{team} — Per-team read-only Terraform plan role +# +# Trust: ONLY the CI broker Lambda. The broker vends this role for tf-plan, +# which runs on any branch and on pull requests. Write access to the team's +# resources comes only from javabin-ci-team-{team}, which the apply gate +# vends for a reviewed plan. +# +# Permissions: ReadOnlyAccess (refresh + data sources), the team's state +# (read), the state lock, and the plan upload. Named ci-plan-*, not +# ci-team-plan-*, so it cannot collide with a team called "plan-...". +################################################################################ + +resource "aws_iam_role" "ci_plan" { + for_each = toset(var.registered_teams) + + name = "${var.project}-ci-plan-${each.key}" + permissions_boundary = aws_iam_policy.developer_boundary.arn + + assume_role_policy = jsonencode({ + Version = "2012-10-17" + Statement = [ + { + Sid = "AllowViaCIBroker" + Effect = "Allow" + Principal = { + AWS = var.ci_broker_role_arn + } + Action = "sts:AssumeRole" + }, + ] + }) + + tags = { + Name = "${var.project}-ci-plan-${each.key}" + team = each.key + } +} + +resource "aws_iam_role_policy_attachment" "ci_plan_readonly" { + for_each = toset(var.registered_teams) + + role = aws_iam_role.ci_plan[each.key].name + policy_arn = "arn:aws:iam::aws:policy/ReadOnlyAccess" +} + +resource "aws_iam_role_policy" "ci_plan" { + for_each = toset(var.registered_teams) + + name = "plan-backend" + role = aws_iam_role.ci_plan[each.key].id + + policy = jsonencode({ + Version = "2012-10-17" + Statement = [ + { + # Terraform state: read-only, team's key prefix. Plan never writes state. + Sid = "ReadTerraformState" + Effect = "Allow" + Action = ["s3:GetObject", "s3:ListBucket"] + Resource = [ + "arn:aws:s3:::${var.project}-terraform-state-${var.aws_account_id}", + "arn:aws:s3:::${var.project}-terraform-state-${var.aws_account_id}/apps/${each.key}/*", + ] + }, + { + # Plan takes the state lock. Same scoping as ci_team. + Sid = "AllowTerraformLocking" + Effect = "Allow" + Action = [ + "dynamodb:GetItem", + "dynamodb:PutItem", + "dynamodb:DeleteItem", + ] + Resource = "arn:aws:dynamodb:${var.region}:${var.aws_account_id}:table/${var.project}-terraform-app-locks" + Condition = { + "ForAllValues:StringLike" = { + "dynamodb:LeadingKeys" = "${var.project}-terraform-state-${var.aws_account_id}/apps/${each.key}/*" + } + } + }, + { + # Refreshing aws_secretsmanager_secret_version reads the value; + # ReadOnlyAccess does not include it. Team-prefixed secrets only. + Sid = "RefreshTeamSecrets" + Effect = "Allow" + Action = "secretsmanager:GetSecretValue" + Resource = "arn:aws:secretsmanager:${var.region}:${var.aws_account_id}:secret:${each.key}-*" + }, + { + # SecureString / secret refresh decrypts through the owning service. + Sid = "DecryptViaParameterServices" + Effect = "Allow" + Action = "kms:Decrypt" + Resource = "*" # Key ARNs are not known here; scoped by kms:ViaService + Condition = { + StringEquals = { + "kms:ViaService" = [ + "ssm.${var.region}.amazonaws.com", + "secretsmanager.${var.region}.amazonaws.com", + ] + } + } + }, + { + # upload-plan.sh stores tfplan + plan-output.txt for the apply gate. + Sid = "UploadPlan" + Effect = "Allow" + Action = "s3:PutObject" + Resource = "arn:aws:s3:::${var.project}-ci-plan-artifacts-${var.aws_account_id}/plans/*" + }, + { + # Review verdicts and overrides come only from the plan-review and + # apply-gate Lambdas; a plan job must not mark its own plan LOW. + Sid = "DenyWriteReviewVerdicts" + Effect = "Deny" + Action = ["s3:PutObject", "s3:DeleteObject"] + Resource = [ + "arn:aws:s3:::${var.project}-ci-plan-artifacts-${var.aws_account_id}/*/risk.json", + "arn:aws:s3:::${var.project}-ci-plan-artifacts-${var.aws_account_id}/*/review.json", + "arn:aws:s3:::${var.project}-ci-plan-artifacts-${var.aws_account_id}/*/override.json", + ] + }, + ] + }) +} + ################################################################################ # 3. javabin-ci-deploy-{team} — Per-team deploy role (build + push + deploy) # diff --git a/terraform/platform/lambdas/main.tf b/terraform/platform/lambdas/main.tf index 4102476..7c3e2ba 100644 --- a/terraform/platform/lambdas/main.tf +++ b/terraform/platform/lambdas/main.tf @@ -1536,6 +1536,8 @@ resource "aws_iam_role_policy" "ci_broker" { Effect = "Allow" Action = "sts:AssumeRole" Resource = [ + "arn:aws:iam::${var.aws_account_id}:role/${var.project}-ci-plan-*", + # Kept while the broker Lambda rolls over to ci-plan-* (one apply). "arn:aws:iam::${var.aws_account_id}:role/${var.project}-ci-team-*", "arn:aws:iam::${var.aws_account_id}:role/${var.project}-ci-deploy-*", ] From dab858f027fac0d5c3e58604f6b76cfadc575ce9 Mon Sep 17 00:00:00 2001 From: Alexanderamiri Date: Sat, 24 Oct 2026 15:59:23 +0200 Subject: [PATCH 2/2] fix(iam): deny team CI roles access to other teams' and platform state 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) --- docs/apply-gate.md | 13 ++++++-- terraform/lambda-src/tests/test_ci_broker.py | 21 ++++++++---- terraform/platform/iam/main.tf | 35 +++++++++++++++++++- 3 files changed, 59 insertions(+), 10 deletions(-) diff --git a/docs/apply-gate.md b/docs/apply-gate.md index f1c9f31..e282d71 100644 --- a/docs/apply-gate.md +++ b/docs/apply-gate.md @@ -55,14 +55,23 @@ Plan credentials are read-only. The broker vends `javabin-ci-plan-{team}` write-capable `javabin-ci-team-{team}` is through this gate, for a reviewed plan. +State access is limited by an explicit Deny, not by the Allow. `ReadOnlyAccess` +(plan) and `AllowAll` (team) cover every key in the state bucket, and the +developer boundary's `DenyXTeam` exempts that bucket, because S3 objects carry +no team tag. Both roles therefore carry `DenyForeignTerraformState`, which +denies the platform's state (`infra/`, `org/`, `state/`, `env:/`) and every +other registered team's `apps/{team}/` prefix. The list is built from +`registered_teams`, so a new top-level prefix in the bucket must be added to +`local.foreign_state_keys` in `terraform/platform/iam/main.tf`. + ## IAM Roles | Role | Purpose | Permissions | |------|---------|-------------| | `javabin-ci-apply-gate` | OIDC role for tf-apply workflow | `lambda:InvokeFunction` on gate Lambda + `s3:GetObject` on plan bucket | | `javabin-apply-gate` | Lambda execution role | `ssm:GetParameter` (signing key) + `s3:Get/PutObject` (plan bucket) + `sts:AssumeRole` (app CI roles) | -| `javabin-ci-plan-{team}` | Read-only plan role | Vended by the CI broker for tf-plan: ReadOnlyAccess + state read + lock + plan upload | -| `javabin-ci-team-{team}` | Team infrastructure role (write) | Vended by the gate Lambda for a reviewed plan (apply) | +| `javabin-ci-plan-{team}` | Read-only plan role | Vended by the CI broker for tf-plan: ReadOnlyAccess + own state read (other keys denied) + lock + plan upload | +| `javabin-ci-team-{team}` | Team infrastructure role (write) | Vended by the gate Lambda for a reviewed plan (apply); other teams' and platform state denied | ## Credential Lifetime diff --git a/terraform/lambda-src/tests/test_ci_broker.py b/terraform/lambda-src/tests/test_ci_broker.py index 148a9f2..a6f0c12 100644 --- a/terraform/lambda-src/tests/test_ci_broker.py +++ b/terraform/lambda-src/tests/test_ci_broker.py @@ -20,15 +20,22 @@ def assume_role(self, RoleArn, RoleSessionName, DurationSeconds): def load(sts): - os.environ["AWS_ACCOUNT_ID"] = "123456789012" - os.environ["PROJECT"] = "javabin" + """Import the handler against a stub boto3 without leaking it. + + The handler reads env and builds its STS client at import time, so the + patches only need to hold during exec_module. patch.dict restores + sys.modules afterwards, so later tests that import boto3 (or shared.*) + get the real modules regardless of test order. + """ boto3 = types.ModuleType("boto3") boto3.client = lambda svc, **kw: sts if svc == "sts" else mock.MagicMock() - sys.modules["boto3"] = boto3 - sys.path.insert(0, SRC) - spec = importlib.util.spec_from_file_location("ci_broker_handler", os.path.join(SRC, "ci_broker", "handler.py")) - mod = importlib.util.module_from_spec(spec) - spec.loader.exec_module(mod) + env = {"AWS_ACCOUNT_ID": "123456789012", "PROJECT": "javabin"} + with mock.patch.dict(os.environ, env), \ + mock.patch.dict(sys.modules, {"boto3": boto3}), \ + mock.patch.object(sys, "path", [SRC] + sys.path): + spec = importlib.util.spec_from_file_location("ci_broker_handler", os.path.join(SRC, "ci_broker", "handler.py")) + mod = importlib.util.module_from_spec(spec) + spec.loader.exec_module(mod) mod.verify = lambda token, aud: ({"repository": "javaBin/cupcake"} if token == "good" else (_ for _ in ()).throw(mod.TokenError("bad"))) mod.resolve_team = lambda repo, requested: "cupcake" diff --git a/terraform/platform/iam/main.tf b/terraform/platform/iam/main.tf index 4cc9a61..273c313 100644 --- a/terraform/platform/iam/main.tf +++ b/terraform/platform/iam/main.tf @@ -336,6 +336,24 @@ resource "aws_iam_role_policy" "ci_app_broker" { # tagged with the correct team name. ################################################################################ +locals { + state_bucket_arn = "arn:aws:s3:::${var.project}-terraform-state-${var.aws_account_id}" + + # State keys a team's CI roles must not touch: the platform's own state + # (infra/ holds the Google OAuth client secret) and every other team's + # apps/{team}/ prefix. A Deny with NotResource would also hit every other + # S3 object in the account, so the forbidden prefixes are listed instead. + # The boundary's DenyXTeam exempts the state bucket, and AllowAll / + # ReadOnlyAccess cover every key, so without this Deny a team role could + # read (ci_plan) or overwrite (ci_team) any state in the bucket. + foreign_state_keys = { + for team in var.registered_teams : team => concat( + [for prefix in ["infra", "org", "state", "env:"] : "${local.state_bucket_arn}/${prefix}/*"], + [for other in var.registered_teams : "${local.state_bucket_arn}/apps/${other}/*" if other != team], + ) + } +} + resource "aws_iam_role" "ci_team" { for_each = toset(var.registered_teams) @@ -414,6 +432,12 @@ resource "aws_iam_role_policy" "ci_team_allow" { "arn:aws:s3:::${var.project}-terraform-state-${var.aws_account_id}/apps/${each.key}/*", ] }, + { + Sid = "DenyForeignTerraformState" + Effect = "Deny" + Action = ["s3:GetObject", "s3:GetObjectVersion", "s3:PutObject", "s3:DeleteObject", "s3:DeleteObjectVersion"] + Resource = local.foreign_state_keys[each.key] + }, { # Terraform locking: DynamoDB scoped to team's state paths. Sid = "AllowTerraformLocking" @@ -490,7 +514,8 @@ resource "aws_iam_role_policy" "ci_plan" { Version = "2012-10-17" Statement = [ { - # Terraform state: read-only, team's key prefix. Plan never writes state. + # Terraform state: read-only. Plan never writes state. The Deny + # below is what keeps plan off other keys. Sid = "ReadTerraformState" Effect = "Allow" Action = ["s3:GetObject", "s3:ListBucket"] @@ -499,6 +524,14 @@ resource "aws_iam_role_policy" "ci_plan" { "arn:aws:s3:::${var.project}-terraform-state-${var.aws_account_id}/apps/${each.key}/*", ] }, + { + # ReadOnlyAccess grants s3:Get* on every key, so the Allow above does + # not limit reads by itself. See local.foreign_state_keys. + Sid = "DenyForeignTerraformState" + Effect = "Deny" + Action = ["s3:GetObject", "s3:GetObjectVersion"] + Resource = local.foreign_state_keys[each.key] + }, { # Plan takes the state lock. Same scoping as ci_team. Sid = "AllowTerraformLocking"