From 757af91339aed0e9c1cf4ae6828779669e5a52a0 Mon Sep 17 00:00:00 2001 From: Alexanderamiri Date: Fri, 23 Oct 2026 16:09:40 +0200 Subject: [PATCH 1/8] fix(iam): per-team ECS execution roles and lock teams out of shared SGs - 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) --- scripts/generate-modules.py | 1 + terraform/modules/platform-data/main.tf | 4 +- terraform/modules/platform-data/outputs.tf | 2 +- terraform/modules/platform-data/variables.tf | 5 ++ terraform/platform/iam/boundary.tf | 4 +- terraform/platform/iam/main.tf | 75 ++++++++++++++++---- 6 files changed, 73 insertions(+), 18 deletions(-) diff --git a/scripts/generate-modules.py b/scripts/generate-modules.py index da71a34..53f1694 100755 --- a/scripts/generate-modules.py +++ b/scripts/generate-modules.py @@ -271,6 +271,7 @@ def src(mod): blocks.append(module_block("platform", src("platform-data"), { "project": PROJECT, "domain": DOMAIN, + "team": team, })) # ECR (always) diff --git a/terraform/modules/platform-data/main.tf b/terraform/modules/platform-data/main.tf index 9228dc2..4be9be8 100644 --- a/terraform/modules/platform-data/main.tf +++ b/terraform/modules/platform-data/main.tf @@ -58,8 +58,10 @@ data "aws_ecs_cluster" "main" { cluster_name = "${var.project}-platform" } +# Per-team execution role (created by platform CI for every registered team); +# it can read only //apps//* secrets. data "aws_iam_role" "ecs_execution" { - name = "${var.project}-ecs-execution" + name = "${var.project}-ecs-exec-${var.team}" } data "aws_route53_zone" "main" { diff --git a/terraform/modules/platform-data/outputs.tf b/terraform/modules/platform-data/outputs.tf index 068bc7f..23ac52c 100644 --- a/terraform/modules/platform-data/outputs.tf +++ b/terraform/modules/platform-data/outputs.tf @@ -54,7 +54,7 @@ output "ecs_cluster_name" { } output "execution_role_arn" { - description = "ECS execution role ARN (shared across all services)" + description = "The team's ECS execution role ARN (reads only that team's secrets)" value = data.aws_iam_role.ecs_execution.arn } diff --git a/terraform/modules/platform-data/variables.tf b/terraform/modules/platform-data/variables.tf index 56e4f00..b96243d 100644 --- a/terraform/modules/platform-data/variables.tf +++ b/terraform/modules/platform-data/variables.tf @@ -15,3 +15,8 @@ variable "domain" { type = string default = "javazone.no" } + +variable "team" { + description = "Owning team — selects the team's ECS execution role" + type = string +} diff --git a/terraform/platform/iam/boundary.tf b/terraform/platform/iam/boundary.tf index 68c29c0..8ed9378 100644 --- a/terraform/platform/iam/boundary.tf +++ b/terraform/platform/iam/boundary.tf @@ -149,6 +149,8 @@ resource "aws_iam_policy" "developer_boundary" { "iam:DeleteRole", "iam:UpdateRole", "iam:DeleteRolePolicy", "iam:PutRolePolicy", "iam:AttachRolePolicy", "iam:DetachRolePolicy", "route53:DeleteHostedZone", "sns:DeleteTopic", "sns:SetTopicAttributes", + # Shared SGs (ECS tasks, ALB): a team rule here opens or cuts every app + "ec2:AuthorizeSecurityGroup*", "ec2:RevokeSecurityGroup*", ] Resource = "*" Condition = { StringEquals = { "aws:ResourceTag/team" = "shared" } } @@ -184,7 +186,7 @@ resource "aws_iam_policy" "developer_boundary" { Action = "iam:PassRole" NotResource = [ "arn:aws:iam::*:role/$${aws:PrincipalTag/team}-*", - "arn:aws:iam::*:role/${var.project}-ecs-execution", + "arn:aws:iam::*:role/${var.project}-ecs-exec-$${aws:PrincipalTag/team}", ] }, { diff --git a/terraform/platform/iam/main.tf b/terraform/platform/iam/main.tf index 2e8d90b..f98fc12 100644 --- a/terraform/platform/iam/main.tf +++ b/terraform/platform/iam/main.tf @@ -522,7 +522,7 @@ resource "aws_iam_role_policy" "ci_deploy_ecs" { Effect = "Allow" Action = ["iam:PassRole"] Resource = [ - aws_iam_role.ecs_execution.arn, + aws_iam_role.ecs_exec_team[each.key].arn, "arn:aws:iam::${var.aws_account_id}:role/${each.key}-*", ] } @@ -754,6 +754,8 @@ resource "aws_iam_role_policy" "ci_apply_gate" { # Individual task roles are created per-app via the service-role module. ################################################################################ +# Deprecated shared execution role. Kept (image pull + logs only, no secrets) +# until no task definition references it; teams can no longer pass it. resource "aws_iam_role" "ecs_execution" { name = "${var.project}-ecs-execution" @@ -781,25 +783,68 @@ resource "aws_iam_role_policy_attachment" "ecs_execution_base" { policy_arn = "arn:aws:iam::aws:policy/service-role/AmazonECSTaskExecutionRolePolicy" } -resource "aws_iam_role_policy" "ecs_execution_secrets" { - name = "secrets-read" - role = aws_iam_role.ecs_execution.id +################################################################################ +# Per-team ECS execution roles — javabin-ecs-exec-{team} +# +# The execution role pulls the image and resolves the task's `secrets` at +# launch. One shared role that can read every //apps/* parameter let +# any team mount another team's secrets in its own task definition. Each team +# now gets a role that reads only its own path, and the developer boundary's +# DenyPassRole only lets a team pass its own role. +# +# Tagged team=shared so DenyShared/DenyTagStrip stop teams from editing them. +################################################################################ - # //apps///... holds service-secret params and the - # Cognito client credentials written by app-auth-sync. The execution role - # fetches them at task launch via the ECS `secrets` block. - policy = jsonencode({ +resource "aws_iam_role" "ecs_exec_team" { + for_each = toset(var.registered_teams) + + name = "${var.project}-ecs-exec-${each.key}" + # The org boundary denies creating any role without one + permissions_boundary = local.org_boundary_arn + + assume_role_policy = jsonencode({ Version = "2012-10-17" Statement = [ { - Sid = "SSMAppSecrets" Effect = "Allow" - Action = [ - "ssm:GetParameters", - ] - Resource = [ - "arn:aws:ssm:${var.region}:${var.aws_account_id}:parameter/${var.project}/apps/*", - ] + Principal = { + Service = "ecs-tasks.amazonaws.com" + } + Action = "sts:AssumeRole" + } + ] + }) + + tags = { + Name = "${var.project}-ecs-exec-${each.key}" + team = "shared" + for-team = each.key + } +} + +resource "aws_iam_role_policy_attachment" "ecs_exec_team_base" { + for_each = toset(var.registered_teams) + + role = aws_iam_role.ecs_exec_team[each.key].name + policy_arn = "arn:aws:iam::aws:policy/service-role/AmazonECSTaskExecutionRolePolicy" +} + +resource "aws_iam_role_policy" "ecs_exec_team_secrets" { + for_each = toset(var.registered_teams) + + name = "team-secrets-read" + role = aws_iam_role.ecs_exec_team[each.key].id + + # Service-secret params and Cognito client credentials (app-auth-sync) + # under the team's own path only. + policy = jsonencode({ + Version = "2012-10-17" + Statement = [ + { + Sid = "SSMTeamSecrets" + Effect = "Allow" + Action = ["ssm:GetParameters"] + Resource = "arn:aws:ssm:${var.region}:${var.aws_account_id}:parameter/${var.project}/apps/${each.key}/*" } ] }) From 84527012793ee045b78f907cd6b5e0cc65fd448f Mon Sep 17 00:00:00 2001 From: Alexanderamiri Date: Fri, 23 Oct 2026 16:09:40 +0200 Subject: [PATCH 2/8] fix(ci): identify callers of ci-broker and apply-gate by GitHub OIDC 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) --- .github/workflows/approve-override.yml | 27 ++-- docs/apply-gate.md | 22 ++- scripts/github-oidc-token.sh | 16 ++ scripts/invoke-apply-gate.sh | 5 +- scripts/invoke-ci-broker.sh | 7 +- scripts/invoke-override-sign.sh | 26 +++ terraform/lambda-src/apply_gate/handler.py | 35 +++- terraform/lambda-src/ci_broker/handler.py | 14 +- terraform/lambda-src/shared/github_oidc.py | 150 ++++++++++++++++++ .../lambda-src/tests/test_github_oidc.py | 105 ++++++++++++ terraform/platform/lambdas/main.tf | 8 + 11 files changed, 392 insertions(+), 23 deletions(-) create mode 100644 scripts/github-oidc-token.sh create mode 100644 scripts/invoke-override-sign.sh create mode 100644 terraform/lambda-src/shared/github_oidc.py create mode 100644 terraform/lambda-src/tests/test_github_oidc.py diff --git a/.github/workflows/approve-override.yml b/.github/workflows/approve-override.yml index 998f3fd..bca1177 100644 --- a/.github/workflows/approve-override.yml +++ b/.github/workflows/approve-override.yml @@ -48,25 +48,26 @@ jobs: role-to-assume: arn:aws:iam::${{ env.AWS_ACCOUNT_ID }}:role/javabin-ci-override-approver aws-region: ${{ env.AWS_REGION }} + # The gate verifies this job's GitHub OIDC token (workflow, repo and + # override-approval environment) and records its actor as approver. + # Inputs go through env, never into the script text. - name: Sign override via gate Lambda - run: | - aws lambda invoke \ - --function-name javabin-apply-gate \ - --payload "$(jq -n \ - --arg action sign \ - --arg plan_key "${{ inputs.plan_key }}" \ - --arg approved_by "${{ github.actor }}" \ - --arg reason "${{ inputs.reason }}" \ - '{action: $action, plan_key: $plan_key, approved_by: $approved_by, reason: $reason}')" \ - --cli-binary-format raw-in-base64-out \ - /dev/stdout + env: + PLAN_KEY: ${{ inputs.plan_key }} + REASON: ${{ inputs.reason }} + run: sh scripts/invoke-override-sign.sh - name: Notify Slack env: SSM_WEBHOOK_PARAM: /javabin/slack/platform-override-alerts-webhook - run: sh scripts/notify-slack.sh "Risk Override Approved" "*Repo:* ${{ inputs.repo }}\n*Plan:* \`${{ inputs.plan_key }}\`\n*By:* ${{ github.actor }}\n*Reason:* ${{ inputs.reason }}" + REPO: ${{ inputs.repo }} + PLAN_KEY: ${{ inputs.plan_key }} + REASON: ${{ inputs.reason }} + run: sh scripts/notify-slack.sh "Risk Override Approved" "*Repo:* ${REPO}\n*Plan:* \`${PLAN_KEY}\`\n*By:* ${GITHUB_ACTOR}\n*Reason:* ${REASON}" - name: Retrigger failed apply env: GH_TOKEN: ${{ github.token }} - run: gh run rerun ${{ inputs.run_id }} --repo ${{ inputs.repo }} --failed + RUN_ID: ${{ inputs.run_id }} + REPO: ${{ inputs.repo }} + run: gh run rerun "$RUN_ID" --repo "$REPO" --failed diff --git a/docs/apply-gate.md b/docs/apply-gate.md index 5c0996f..e014c27 100644 --- a/docs/apply-gate.md +++ b/docs/apply-gate.md @@ -20,7 +20,10 @@ tf-plan → uploads plan + plan-output.txt to S3 plan-review → LLM reviews plan → uploads risk.json to S3 tf-apply → assumes javabin-ci-apply-gate (lightweight OIDC role) → downloads plan from S3, verifies SHA256 - → invokes gate Lambda: check(plan_key, repo_name) + → invokes gate Lambda: check(plan_key, team, id_token) + → Lambda verifies the GitHub OIDC token (aud javabin-apply-gate, + a javaBin/platform workflow on main) and takes the repo from its + signed `repository` claim; plan_key must be under plans/javaBin// → Lambda reads risk.json from S3 → LOW/MEDIUM: assumes app CI role via STS, returns temp credentials → HIGH: checks for override.json, verifies HMAC signature @@ -29,6 +32,23 @@ tf-apply → assumes javabin-ci-apply-gate (lightweight OIDC role) → tf-apply uses temp credentials for terraform init + apply ``` +## Caller identity + +The gate and broker roles can be assumed from any javaBin repo's platform +workflow job, and code inside that job (e.g. a Terraform `external` data +source) can invoke the Lambdas directly. So nothing identity-related is taken +from the payload: + +- `check` — repo comes from the job's GitHub OIDC token, the team must be the + app.yaml team the repo belongs to, and the plan must be the repo's own. +- `sign` — only a token from `approve-override.yml` in javaBin/platform, + running in the board-reviewed `override-approval` environment, is accepted. + The approver is the token's `actor`. Without this, any app's tf-apply job + could sign its own HIGH-risk override. + +Tokens are minted with `scripts/github-oidc-token.sh ` and verified +in `terraform/lambda-src/shared/github_oidc.py` (RS256 against GitHub's JWKS). + ## IAM Roles | Role | Purpose | Permissions | diff --git a/scripts/github-oidc-token.sh b/scripts/github-oidc-token.sh new file mode 100644 index 0000000..dde09b4 --- /dev/null +++ b/scripts/github-oidc-token.sh @@ -0,0 +1,16 @@ +#!/bin/sh +# Print a GitHub Actions OIDC token for the given audience. +# +# Usage: github-oidc-token.sh +# +# The job needs `permissions: id-token: write`. The platform Lambdas +# (ci-broker, apply-gate) verify the token and read the repo from its signed +# claims, so a job can only ever act as its own repo. + +set -e + +AUDIENCE="${1:?Usage: github-oidc-token.sh }" +: "${ACTIONS_ID_TOKEN_REQUEST_URL:?id-token: write permission is required}" + +curl -sSf -H "Authorization: bearer ${ACTIONS_ID_TOKEN_REQUEST_TOKEN}" \ + "${ACTIONS_ID_TOKEN_REQUEST_URL}&audience=${AUDIENCE}" | jq -r '.value' diff --git a/scripts/invoke-apply-gate.sh b/scripts/invoke-apply-gate.sh index 528acbf..6cc9473 100644 --- a/scripts/invoke-apply-gate.sh +++ b/scripts/invoke-apply-gate.sh @@ -26,13 +26,16 @@ echo "Requesting apply credentials from gate Lambda..." # Build session name from GitHub context for alert attribution GATE_SESSION="${GITHUB_ACTOR:-unknown}-$(echo "${GITHUB_SHA:-00000000}" | cut -c1-8)-${GITHUB_RUN_ID:-0}" +ID_TOKEN=$(sh "$(dirname "$0")/github-oidc-token.sh" "${PROJECT:-javabin}-apply-gate") + PAYLOAD=$(jq -n \ --arg action "check" \ --arg plan_key "$PLAN_KEY" \ --arg repo_name "$REPO_NAME" \ --arg session_name "$GATE_SESSION" \ --arg team "$TEAM" \ - '{action: $action, plan_key: $plan_key, repo_name: $repo_name, session_name: $session_name, team: $team}') + --arg id_token "$ID_TOKEN" \ + '{action: $action, plan_key: $plan_key, repo_name: $repo_name, session_name: $session_name, team: $team, id_token: $id_token}') RESPONSE=$(aws lambda invoke \ --function-name "$LAMBDA_NAME" \ diff --git a/scripts/invoke-ci-broker.sh b/scripts/invoke-ci-broker.sh index 0829ac7..45e76e1 100755 --- a/scripts/invoke-ci-broker.sh +++ b/scripts/invoke-ci-broker.sh @@ -4,7 +4,8 @@ # Usage: invoke-ci-broker.sh # action: "plan" or "deploy" # -# Env: GITHUB_REPOSITORY (org/repo) +# Env: GITHUB_REPOSITORY (org/repo, informational — the broker takes the +# repo from the job's OIDC token; needs id-token: write) # APP_TEAM (optional) — team from app.yaml; the broker verifies the repo # belongs to it instead of picking the repo's first GitHub team # Output: Exports AWS_ACCESS_KEY_ID, AWS_SECRET_ACCESS_KEY, AWS_SESSION_TOKEN @@ -14,13 +15,15 @@ set -e ACTION="${1:?Usage: invoke-ci-broker.sh }" REPO="${GITHUB_REPOSITORY##*/}" +ID_TOKEN=$(sh "$(dirname "$0")/github-oidc-token.sh" "${PROJECT:-javabin}-ci-broker") echo "Requesting ${ACTION} credentials for ${REPO}..." RESPONSE=$(aws lambda invoke \ --function-name "${PROJECT:-javabin}-ci-broker" \ --payload "$(jq -nc --arg repo "$REPO" --arg action "$ACTION" --arg session_name "${SESSION_NAME:-}" --arg team "${APP_TEAM:-}" \ - '{repo: $repo, action: $action, session_name: $session_name, team: $team}')" \ + --arg id_token "$ID_TOKEN" \ + '{repo: $repo, action: $action, session_name: $session_name, team: $team, id_token: $id_token}')" \ --cli-binary-format raw-in-base64-out \ /tmp/broker-response.json 2>&1) diff --git a/scripts/invoke-override-sign.sh b/scripts/invoke-override-sign.sh new file mode 100644 index 0000000..0bbd8a8 --- /dev/null +++ b/scripts/invoke-override-sign.sh @@ -0,0 +1,26 @@ +#!/bin/sh +# Ask the apply-gate Lambda to sign a risk override for a plan. +# +# Usage: invoke-override-sign.sh +# Env: PLAN_KEY, REASON; runs in approve-override.yml (the gate checks the +# OIDC token comes from that workflow and the override-approval +# environment, and records the token's actor as approver). + +set -e + +DIR=$(dirname "$0") +ID_TOKEN=$(sh "$DIR/github-oidc-token.sh" "${PROJECT:-javabin}-apply-gate") + +aws lambda invoke \ + --function-name "${PROJECT:-javabin}-apply-gate" \ + --payload "$(jq -nc --arg plan_key "$PLAN_KEY" --arg reason "$REASON" --arg id_token "$ID_TOKEN" \ + '{action: "sign", plan_key: $plan_key, reason: $reason, id_token: $id_token}')" \ + --cli-binary-format raw-in-base64-out \ + /tmp/override-response.json >/dev/null + +BODY=$(jq -r '.body' /tmp/override-response.json) +if [ "$(echo "$BODY" | jq -r '.signed // false')" != "true" ]; then + echo "ERROR: override not signed: $(echo "$BODY" | jq -r '.error // "unknown error"')" + exit 1 +fi +echo "Override signed for $PLAN_KEY" diff --git a/terraform/lambda-src/apply_gate/handler.py b/terraform/lambda-src/apply_gate/handler.py index 487e7b1..dfd5624 100644 --- a/terraform/lambda-src/apply_gate/handler.py +++ b/terraform/lambda-src/apply_gate/handler.py @@ -19,6 +19,9 @@ import boto3 from shared.github import resolve_team +from shared.github_oidc import TokenError, require_workflow +from shared.github_oidc import repo_name as oidc_repo_name +from shared.github_oidc import verify as verify_oidc logger = logging.getLogger(__name__) logger.setLevel(logging.INFO) @@ -32,6 +35,8 @@ ) PLAN_BUCKET = os.environ.get("PLAN_BUCKET", "") PROJECT = os.environ.get("PROJECT", "javabin") +GITHUB_ORG = os.environ.get("GITHUB_ORG", "javaBin") +OIDC_AUDIENCE = os.environ.get("OIDC_AUDIENCE", f"{PROJECT}-apply-gate") CREDENTIAL_DURATION = int(os.environ.get("CREDENTIAL_DURATION", "900")) # 15 min # Cache signing key across invocations @@ -87,8 +92,17 @@ def action_check(event): Output: {approved, risk_level, credentials?, reason?} """ plan_key = event["plan_key"] - repo_name = event["repo_name"] team = event.get("team") or None + # Repo from the signed GitHub OIDC token, not the payload (see ci_broker) + try: + repo_name = oidc_repo_name(verify_oidc(event.get("id_token"), OIDC_AUDIENCE)) + except TokenError as e: + logger.warning("Rejected gate call: %s", e) + return {"approved": False, "reason": f"Invalid GitHub OIDC token: {e}"} + # Plans are stored under plans///...; never apply another + # repo's (possibly LOW-risk or overridden) plan. + if not plan_key.startswith(f"plans/{GITHUB_ORG}/{repo_name}/"): + return {"approved": False, "reason": f"Plan {plan_key} does not belong to {repo_name}"} session_name = event.get("session_name") prefix = _plan_prefix(plan_key) @@ -153,11 +167,26 @@ def action_check(event): def action_sign(event): """Create a signed override token and write it to S3. - Input: {plan_key, approved_by, reason} + Input: {plan_key, reason, id_token} Output: {signed: true, plan_key} + + Only the approve-override workflow in javaBin/platform, running behind + the board-reviewed override-approval environment, may sign. Without this + check any app's tf-apply job could invoke the gate and approve its own + HIGH-risk plan. The approver is the token's actor, not a payload field. """ plan_key = event["plan_key"] - approved_by = event["approved_by"] + try: + claims = require_workflow( + verify_oidc(event.get("id_token"), OIDC_AUDIENCE), + "approve-override.yml", + repository=f"{GITHUB_ORG}/platform", + environment="override-approval", + ) + except TokenError as e: + logger.warning("Rejected override signing: %s", e) + return {"signed": False, "error": f"Invalid GitHub OIDC token: {e}"} + approved_by = claims["actor"] reason = event.get("reason", "") prefix = _plan_prefix(plan_key) diff --git a/terraform/lambda-src/ci_broker/handler.py b/terraform/lambda-src/ci_broker/handler.py index af56074..20e211b 100644 --- a/terraform/lambda-src/ci_broker/handler.py +++ b/terraform/lambda-src/ci_broker/handler.py @@ -16,6 +16,7 @@ import boto3 from shared.github import resolve_team +from shared.github_oidc import TokenError, repo_name, verify logger = logging.getLogger(__name__) logger.setLevel(logging.INFO) @@ -57,13 +58,20 @@ def _assume_role(role_arn, session_name, duration): } +OIDC_AUDIENCE = os.environ.get("OIDC_AUDIENCE", f"{PROJECT}-ci-broker") + + def handler(event, context): - repo = event.get("repo", "") action = event.get("action", "plan") requested_team = event.get("team") or None - if not repo: - return {"error": "repo is required", "approved": False} + # The repo comes from a GitHub OIDC token, never from the payload: code + # running inside an app job can invoke this Lambda and name any repo. + try: + repo = repo_name(verify(event.get("id_token"), OIDC_AUDIENCE)) + except TokenError as e: + logger.warning("Rejected broker call: %s", e) + return {"error": f"Invalid GitHub OIDC token: {e}", "approved": False} if action not in ROLE_MAP: return {"error": f"Invalid action: {action}. Must be plan or deploy", "approved": False} diff --git a/terraform/lambda-src/shared/github_oidc.py b/terraform/lambda-src/shared/github_oidc.py new file mode 100644 index 0000000..d53dda7 --- /dev/null +++ b/terraform/lambda-src/shared/github_oidc.py @@ -0,0 +1,150 @@ +"""Verify GitHub Actions OIDC tokens — shared by ci_broker and apply_gate. + +The broker and gate roles are assumable from any javaBin repo running a +platform workflow, and code running inside that job (e.g. a Terraform +`external` data source during tf-plan) can call the Lambdas too. So the repo +must not come from the request payload: the caller sends a GitHub OIDC token +minted for this audience, and the repo is read from its signed `repository` +claim, which the job cannot forge. + +RS256 is verified with the openssl CLI (in the Lambda runtime), so no +third-party packages are needed. +""" + +import base64 +import json +import os +import subprocess +import tempfile +import time +import urllib.request + +ISSUER = "https://token.actions.githubusercontent.com" +JWKS_URL = f"{ISSUER}/.well-known/jwks" +GITHUB_ORG = os.environ.get("GITHUB_ORG", "javaBin") +PLATFORM_WORKFLOW_PREFIX = f"{GITHUB_ORG}/platform/.github/workflows/" +CLOCK_SKEW = 60 + +_jwks_cache = {"keys": None, "fetched_at": 0} + + +class TokenError(ValueError): + """The token is missing, malformed, expired, or not from a platform workflow.""" + + +def _b64url_decode(s): + return base64.urlsafe_b64decode(s + "=" * (-len(s) % 4)) + + +def _der_len(n): + if n < 0x80: + return bytes([n]) + b = n.to_bytes((n.bit_length() + 7) // 8, "big") + return bytes([0x80 | len(b)]) + b + + +def _der_int(b): + if b[0] & 0x80: + b = b"\x00" + b + return b"\x02" + _der_len(len(b)) + b + + +def _der_seq(*parts): + body = b"".join(parts) + return b"\x30" + _der_len(len(body)) + body + + +def jwk_to_pem(jwk): + """Encode an RSA JWK (n, e) as a SubjectPublicKeyInfo PEM.""" + rsa_key = _der_seq(_der_int(_b64url_decode(jwk["n"])), _der_int(_b64url_decode(jwk["e"]))) + algorithm = _der_seq(b"\x06\x09\x2a\x86\x48\x86\xf7\x0d\x01\x01\x01", b"\x05\x00") # rsaEncryption, NULL + bit_string = b"\x03" + _der_len(len(rsa_key) + 1) + b"\x00" + rsa_key + der = _der_seq(algorithm, bit_string) + b64 = base64.b64encode(der).decode() + lines = [b64[i:i + 64] for i in range(0, len(b64), 64)] + return "-----BEGIN PUBLIC KEY-----\n" + "\n".join(lines) + "\n-----END PUBLIC KEY-----\n" + + +def _jwks(force=False): + if force or not _jwks_cache["keys"] or time.time() - _jwks_cache["fetched_at"] > 3600: + with urllib.request.urlopen(JWKS_URL, timeout=10) as resp: + _jwks_cache["keys"] = {k["kid"]: k for k in json.loads(resp.read())["keys"]} + _jwks_cache["fetched_at"] = time.time() + return _jwks_cache["keys"] + + +def _verify_rs256(signing_input, signature, jwk): + with tempfile.NamedTemporaryFile("w", suffix=".pem", delete=False) as k, \ + tempfile.NamedTemporaryFile("wb", suffix=".sig", delete=False) as sig: + k.write(jwk_to_pem(jwk)) + sig.write(signature) + try: + r = subprocess.run( + ["openssl", "dgst", "-sha256", "-verify", k.name, "-signature", sig.name], + input=signing_input, capture_output=True, + ) + return r.returncode == 0 + finally: + os.unlink(k.name) + os.unlink(sig.name) + + +def verify(token, audience, now=None, keys=None): + """Return the verified claims, or raise TokenError. + + Accepts tokens from any javaBin/platform workflow on main; callers that + need a specific workflow use require_workflow() on the result. + """ + if not token or token.count(".") != 2: + raise TokenError("missing or malformed GitHub OIDC token") + header_b64, payload_b64, sig_b64 = token.split(".") + try: + header = json.loads(_b64url_decode(header_b64)) + claims = json.loads(_b64url_decode(payload_b64)) + signature = _b64url_decode(sig_b64) + except ValueError as e: + raise TokenError(f"undecodable token: {e}") + + if header.get("alg") != "RS256": + raise TokenError(f"unsupported alg {header.get('alg')!r}") + jwks = keys if keys is not None else _jwks() + jwk = jwks.get(header.get("kid")) + if jwk is None and keys is None: + jwk = _jwks(force=True).get(header.get("kid")) # key rotation + if jwk is None: + raise TokenError("unknown signing key") + if not _verify_rs256(f"{header_b64}.{payload_b64}".encode(), signature, jwk): + raise TokenError("bad signature") + + now = now if now is not None else time.time() + if claims.get("iss") != ISSUER: + raise TokenError("wrong issuer") + aud = claims.get("aud") + if audience not in (aud if isinstance(aud, list) else [aud]): + raise TokenError("wrong audience") + if claims.get("exp", 0) + CLOCK_SKEW < now or claims.get("nbf", 0) - CLOCK_SKEW > now: + raise TokenError("token expired or not yet valid") + if claims.get("repository_owner") != GITHUB_ORG: + raise TokenError("token is not from the javaBin org") + if not str(claims.get("job_workflow_ref", "")).startswith(PLATFORM_WORKFLOW_PREFIX) or \ + not str(claims.get("job_workflow_ref", "")).endswith("@refs/heads/main"): + raise TokenError("token is not from a javaBin/platform workflow on main") + return claims + + +def require_workflow(claims, workflow, repository=None, environment=None): + """Raise TokenError unless the token came from this platform workflow + file (e.g. "approve-override.yml"), optionally in a given repo and + GitHub environment.""" + if claims.get("job_workflow_ref") != f"{PLATFORM_WORKFLOW_PREFIX}{workflow}@refs/heads/main": + raise TokenError(f"token is not from {workflow}") + if repository and claims.get("repository") != repository: + raise TokenError(f"token is not from {repository}") + if environment and claims.get("environment") != environment: + raise TokenError(f"token is not from the {environment} environment") + return claims + + +def repo_name(claims): + """Repo name (without org) from verified claims.""" + return claims["repository"].split("/", 1)[1] diff --git a/terraform/lambda-src/tests/test_github_oidc.py b/terraform/lambda-src/tests/test_github_oidc.py new file mode 100644 index 0000000..7658c67 --- /dev/null +++ b/terraform/lambda-src/tests/test_github_oidc.py @@ -0,0 +1,105 @@ +"""Tests for shared.github_oidc — run: python3 -m unittest discover terraform/lambda-src/tests + +Signs tokens with a throwaway RSA key via the openssl CLI, so the real +verification path (JWK -> PEM -> openssl dgst -verify) is exercised. +""" + +import base64 +import json +import os +import shutil +import subprocess +import sys +import tempfile +import unittest + +sys.path.insert(0, os.path.join(os.path.dirname(__file__), "..")) +from shared import github_oidc as oidc # noqa: E402 + +NOW = 1_800_000_000 +AUD = "javabin-ci-broker" + + +def b64url(b): + return base64.urlsafe_b64encode(b).rstrip(b"=").decode() + + +@unittest.skipUnless(shutil.which("openssl"), "openssl not installed") +class GithubOidcTest(unittest.TestCase): + @classmethod + def setUpClass(cls): + cls.dir = tempfile.mkdtemp() + cls.key = os.path.join(cls.dir, "key.pem") + subprocess.run(["openssl", "genrsa", "-out", cls.key, "2048"], check=True, capture_output=True) + mod = subprocess.run(["openssl", "rsa", "-in", cls.key, "-noout", "-modulus"], + check=True, capture_output=True, text=True).stdout.strip().split("=")[1] + cls.jwks = {"k1": {"kid": "k1", "kty": "RSA", "e": b64url((65537).to_bytes(3, "big")), + "n": b64url(bytes.fromhex(mod))}} + + @classmethod + def tearDownClass(cls): + shutil.rmtree(cls.dir) + + def token(self, **overrides): + claims = { + "iss": oidc.ISSUER, "aud": AUD, "exp": NOW + 300, "nbf": NOW - 10, + "repository": "javaBin/cupcake", "repository_owner": "javaBin", "actor": "someone", + "job_workflow_ref": "javaBin/platform/.github/workflows/tf-plan.yml@refs/heads/main", + } + claims.update(overrides) + signing_input = f"{b64url(json.dumps({'alg': 'RS256', 'kid': 'k1'}).encode())}.{b64url(json.dumps(claims).encode())}" + sig = subprocess.run(["openssl", "dgst", "-sha256", "-sign", self.key], + input=signing_input.encode(), check=True, capture_output=True).stdout + return f"{signing_input}.{b64url(sig)}" + + def verify(self, token, audience=AUD): + return oidc.verify(token, audience, now=NOW, keys=self.jwks) + + def assertRejected(self, token, fragment, audience=AUD): + with self.assertRaises(oidc.TokenError) as cm: + self.verify(token, audience) + self.assertIn(fragment, str(cm.exception)) + + def test_valid_token_yields_repo_from_signed_claims(self): + self.assertEqual(oidc.repo_name(self.verify(self.token())), "cupcake") + + def test_tampered_payload_fails_signature(self): + head, _, sig = self.token().split(".") + forged = b64url(json.dumps({"iss": oidc.ISSUER, "aud": AUD, "exp": NOW + 300, + "repository": "javaBin/moresleep", "repository_owner": "javaBin", + "job_workflow_ref": "javaBin/platform/.github/workflows/tf-plan.yml@refs/heads/main"}).encode()) + self.assertRejected(f"{head}.{forged}.{sig}", "bad signature") + + def test_wrong_audience_expired_and_foreign_org_are_rejected(self): + self.assertRejected(self.token(), "wrong audience", audience="javabin-apply-gate") + self.assertRejected(self.token(exp=NOW - 120), "expired") + self.assertRejected(self.token(repository_owner="evil", repository="evil/cupcake"), "javaBin org") + + def test_token_from_app_repo_workflow_is_rejected(self): + # A token minted by the app's own workflow, not a platform reusable workflow + self.assertRejected(self.token(job_workflow_ref="javaBin/cupcake/.github/workflows/deploy.yaml@refs/heads/main"), + "javaBin/platform workflow") + self.assertRejected(self.token(job_workflow_ref="javaBin/platform/.github/workflows/tf-plan.yml@refs/heads/evil"), + "javaBin/platform workflow") + + def test_missing_or_garbage_token(self): + self.assertRejected(None, "missing") + self.assertRejected("a.b", "missing") + + def test_require_workflow_for_override_signing(self): + ok = self.verify(self.token(repository="javaBin/platform", environment="override-approval", + job_workflow_ref="javaBin/platform/.github/workflows/approve-override.yml@refs/heads/main")) + oidc.require_workflow(ok, "approve-override.yml", "javaBin/platform", "override-approval") + + tf_apply = self.verify(self.token(job_workflow_ref="javaBin/platform/.github/workflows/tf-apply.yml@refs/heads/main")) + with self.assertRaises(oidc.TokenError): + oidc.require_workflow(tf_apply, "approve-override.yml", "javaBin/platform", "override-approval") + + no_env = self.verify(self.token(repository="javaBin/platform", + job_workflow_ref="javaBin/platform/.github/workflows/approve-override.yml@refs/heads/main")) + with self.assertRaises(oidc.TokenError): + oidc.require_workflow(no_env, "approve-override.yml", "javaBin/platform", "override-approval") + + +if __name__ == "__main__": + unittest.main() diff --git a/terraform/platform/lambdas/main.tf b/terraform/platform/lambdas/main.tf index e6c8471..5a02b73 100644 --- a/terraform/platform/lambdas/main.tf +++ b/terraform/platform/lambdas/main.tf @@ -1383,6 +1383,10 @@ data "archive_file" "apply_gate" { content = file("${local.lambda_src_path}/shared/github.py") filename = "shared/github.py" } + source { + content = file("${local.lambda_src_path}/shared/github_oidc.py") + filename = "shared/github_oidc.py" + } } resource "aws_iam_role" "apply_gate" { @@ -1487,6 +1491,10 @@ data "archive_file" "ci_broker" { content = file("${local.lambda_src_path}/shared/github.py") filename = "shared/github.py" } + source { + content = file("${local.lambda_src_path}/shared/github_oidc.py") + filename = "shared/github_oidc.py" + } } resource "aws_iam_role" "ci_broker" { From c9f16de0593e9049c667cf92a1aa88ec4dccd67a Mon Sep 17 00:00:00 2001 From: Alexanderamiri Date: Fri, 23 Oct 2026 16:09:40 +0200 Subject: [PATCH 3/8] fix(ci): create team CI roles only for registry teams 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) --- CLAUDE.md | 8 +++--- scripts/sync-registered-teams.py | 26 ++++++++++++------- .../platform/registered-teams.auto.tfvars | 4 ++- 3 files changed, 25 insertions(+), 13 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index ae9cf2b..f9260c6 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -168,7 +168,7 @@ terraform/state/ | `password-set` | Self-service password-set for new hero accounts (Function URL) | | `budget-enforcer` | Scales ECS services to zero when team exceeds 200% budget | | `resource-tagger` | EventBridge-triggered, auto-tags created-by + commit on new resources | -| `ci-broker` | Validates repo ∈ app.yaml team, vends short-lived team role credentials | +| `ci-broker` | Verifies the job's GitHub OIDC token (repo from signed claims), checks repo ∈ app.yaml team, vends short-lived team role credentials | | `app-auth-sync` | Creates/deletes Cognito app clients requested by app.yaml `auth:` (SSM request → client + credentials) | ### Scripts @@ -178,7 +178,8 @@ terraform/state/ | `scripts/generate-modules.py` | CI: validates app.yaml, generates fmt-clean module blocks into terraform/ (tests: `scripts/tests/`) | | `scripts/registry.py` | Module sources + boilerplate templates for generate-modules.py | | `scripts/resolve-tool-version.sh` | CI: resolve node/java version from the app repo's version files | -| `scripts/sync-registered-teams.py` | CI: GitHub org teams → `registered-teams.auto.tfvars` (needs App token) | +| `scripts/sync-registered-teams.py` | CI: registry `teams/*.yaml` → `registered-teams.auto.tfvars` (needs App token) | +| `scripts/github-oidc-token.sh` | CI: mint a GitHub OIDC token for the broker / apply gate | | `scripts/provision-teams.py` | CI: fetch team YAMLs from registry, invoke team-provisioner Lambda | | `scripts/provision-groups.py` | CI: resolve members.yaml + access.yaml, invoke team-provisioner Lambda | | `scripts/sync-members.py` | CI: sync approved heroes from Google Sheets into members.yaml | @@ -202,6 +203,7 @@ terraform/state/ | SSM params (platform) | `/javabin/{namespace}/{name}` | | SSM params (app) | `/javabin/apps/{team}/{service}/{name}` | | ECS cluster | `javabin-platform` | +| ECS execution roles | `javabin-ecs-exec-{team}` (platform-owned, reads only `/javabin/apps/{team}/*`) | | ECR repos | `{team}-{service}` | | SNS topics | `javabin-{alerts,security,budget-enforcement}` | | Log groups | `/ecs/{team}/{service}` | @@ -287,7 +289,7 @@ The SA JSON key is at `/javabin/platform/google-admin-sa`, the impersonation tar ### Known Issues - **ECS deploy stabilization**: platform-test-app task registers but service fails health check - **Cognito**: pools deployed; internal pool's `GoogleSAML` IdP was created outside Terraform -- **CI broker trusts the payload repo name**: code running inside an app job (e.g. a Terraform `external` data source in tf-plan) can mint a new OIDC token for the broker role and name another repo. Fix: verify a GitHub OIDC token in the Lambda instead of a payload field +- **App databases share one network path**: service-rds allows ingress from the shared ECS tasks SG, so every team's tasks can reach every team's DB port (password still required). Fix: per-app security groups - **Team provisioner Lambda**: All sync functions working (Google/GitHub/Budget/Cognito/Identity Center). Password-set flow deployed. - **`registered_app_repos` manually managed**: Being replaced with team-scoped IAM roles (repo→team resolved via GitHub API at runtime) - **Cost allocation tags pending activation**: `repo`, `created-by`, `commit` tags need activation in Billing console (requires billing data to appear first) diff --git a/scripts/sync-registered-teams.py b/scripts/sync-registered-teams.py index d1c608f..713dc6d 100644 --- a/scripts/sync-registered-teams.py +++ b/scripts/sync-registered-teams.py @@ -1,19 +1,22 @@ #!/usr/bin/env python3 -"""Sync registered teams from GitHub org teams. +"""Sync registered teams from the registry. -Queries the GitHub API for all teams in the javaBin org and writes -registered-teams.auto.tfvars. This drives team-scoped IAM role creation. +Lists teams/*.yaml in javaBin/registry and writes +registered-teams.auto.tfvars. This drives team-scoped IAM role creation +(CI roles, ECS execution roles). -Source of truth: GitHub teams (created by team-provisioner from registry). +Source of truth: the registry. GitHub org teams also include non-app teams +(owners, developer, ...) that must not get AWS CI roles. Usage: sync-registered-teams.py -Requires: GITHUB_TOKEN env var (with org team read access). +Requires: GITHUB_TOKEN env var (read access to javaBin/registry). """ import json import logging import os +import re import sys import urllib.request @@ -45,10 +48,15 @@ def github_get(path, token): return results +REGISTRY_REPO = os.environ.get("REGISTRY_REPO", "registry") +TEAM_NAME = re.compile(r"^[a-z]{1,12}$") + + def get_teams(token): - """Get all org teams, excluding platform-internal ones.""" - teams = github_get(f"/orgs/{GITHUB_ORG}/teams", token) - slugs = sorted(t["slug"] for t in teams if t["slug"] not in EXCLUDED_TEAMS) + """Registry team names (teams/.yaml), excluding platform-internal ones.""" + files = github_get(f"/repos/{GITHUB_ORG}/{REGISTRY_REPO}/contents/teams", token) + names = [f["name"][:-5] for f in files if f["type"] == "file" and f["name"].endswith(".yaml")] + slugs = sorted(n for n in names if TEAM_NAME.match(n) and n not in EXCLUDED_TEAMS) logger.info("Found %d teams (excluding %s)", len(slugs), ", ".join(EXCLUDED_TEAMS)) for slug in slugs: logger.info(" %s", slug) @@ -59,7 +67,7 @@ def write_tfvars(teams, output_path): """Write the registered-teams.auto.tfvars file.""" lines = [ "# GENERATED by sync-registered-teams.py — do not edit manually", - "# Source of truth: GitHub teams in javaBin org", + "# Source of truth: teams/*.yaml in javaBin/registry", "", "registered_teams = [", ] diff --git a/terraform/platform/registered-teams.auto.tfvars b/terraform/platform/registered-teams.auto.tfvars index bcc0487..9ce27b9 100644 --- a/terraform/platform/registered-teams.auto.tfvars +++ b/terraform/platform/registered-teams.auto.tfvars @@ -1,6 +1,8 @@ # GENERATED by sync-registered-teams.py — do not edit manually -# Source of truth: GitHub teams in javaBin org +# Source of truth: teams/*.yaml in javaBin/registry registered_teams = [ + "cupcake", + "jzwatch", "testteam", ] From 1fbc270dc6fb510a78aefe00b83360ecf7ec25e9 Mon Sep 17 00:00:00 2001 From: Alexanderamiri Date: Fri, 23 Oct 2026 16:12:54 +0200 Subject: [PATCH 4/8] revert: keep syncing team roles from GitHub org teams in this PR 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) --- CLAUDE.md | 8 +++--- scripts/sync-registered-teams.py | 26 +++++++------------ .../platform/registered-teams.auto.tfvars | 4 +-- 3 files changed, 13 insertions(+), 25 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index f9260c6..ae9cf2b 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -168,7 +168,7 @@ terraform/state/ | `password-set` | Self-service password-set for new hero accounts (Function URL) | | `budget-enforcer` | Scales ECS services to zero when team exceeds 200% budget | | `resource-tagger` | EventBridge-triggered, auto-tags created-by + commit on new resources | -| `ci-broker` | Verifies the job's GitHub OIDC token (repo from signed claims), checks repo ∈ app.yaml team, vends short-lived team role credentials | +| `ci-broker` | Validates repo ∈ app.yaml team, vends short-lived team role credentials | | `app-auth-sync` | Creates/deletes Cognito app clients requested by app.yaml `auth:` (SSM request → client + credentials) | ### Scripts @@ -178,8 +178,7 @@ terraform/state/ | `scripts/generate-modules.py` | CI: validates app.yaml, generates fmt-clean module blocks into terraform/ (tests: `scripts/tests/`) | | `scripts/registry.py` | Module sources + boilerplate templates for generate-modules.py | | `scripts/resolve-tool-version.sh` | CI: resolve node/java version from the app repo's version files | -| `scripts/sync-registered-teams.py` | CI: registry `teams/*.yaml` → `registered-teams.auto.tfvars` (needs App token) | -| `scripts/github-oidc-token.sh` | CI: mint a GitHub OIDC token for the broker / apply gate | +| `scripts/sync-registered-teams.py` | CI: GitHub org teams → `registered-teams.auto.tfvars` (needs App token) | | `scripts/provision-teams.py` | CI: fetch team YAMLs from registry, invoke team-provisioner Lambda | | `scripts/provision-groups.py` | CI: resolve members.yaml + access.yaml, invoke team-provisioner Lambda | | `scripts/sync-members.py` | CI: sync approved heroes from Google Sheets into members.yaml | @@ -203,7 +202,6 @@ terraform/state/ | SSM params (platform) | `/javabin/{namespace}/{name}` | | SSM params (app) | `/javabin/apps/{team}/{service}/{name}` | | ECS cluster | `javabin-platform` | -| ECS execution roles | `javabin-ecs-exec-{team}` (platform-owned, reads only `/javabin/apps/{team}/*`) | | ECR repos | `{team}-{service}` | | SNS topics | `javabin-{alerts,security,budget-enforcement}` | | Log groups | `/ecs/{team}/{service}` | @@ -289,7 +287,7 @@ The SA JSON key is at `/javabin/platform/google-admin-sa`, the impersonation tar ### Known Issues - **ECS deploy stabilization**: platform-test-app task registers but service fails health check - **Cognito**: pools deployed; internal pool's `GoogleSAML` IdP was created outside Terraform -- **App databases share one network path**: service-rds allows ingress from the shared ECS tasks SG, so every team's tasks can reach every team's DB port (password still required). Fix: per-app security groups +- **CI broker trusts the payload repo name**: code running inside an app job (e.g. a Terraform `external` data source in tf-plan) can mint a new OIDC token for the broker role and name another repo. Fix: verify a GitHub OIDC token in the Lambda instead of a payload field - **Team provisioner Lambda**: All sync functions working (Google/GitHub/Budget/Cognito/Identity Center). Password-set flow deployed. - **`registered_app_repos` manually managed**: Being replaced with team-scoped IAM roles (repo→team resolved via GitHub API at runtime) - **Cost allocation tags pending activation**: `repo`, `created-by`, `commit` tags need activation in Billing console (requires billing data to appear first) diff --git a/scripts/sync-registered-teams.py b/scripts/sync-registered-teams.py index 713dc6d..d1c608f 100644 --- a/scripts/sync-registered-teams.py +++ b/scripts/sync-registered-teams.py @@ -1,22 +1,19 @@ #!/usr/bin/env python3 -"""Sync registered teams from the registry. +"""Sync registered teams from GitHub org teams. -Lists teams/*.yaml in javaBin/registry and writes -registered-teams.auto.tfvars. This drives team-scoped IAM role creation -(CI roles, ECS execution roles). +Queries the GitHub API for all teams in the javaBin org and writes +registered-teams.auto.tfvars. This drives team-scoped IAM role creation. -Source of truth: the registry. GitHub org teams also include non-app teams -(owners, developer, ...) that must not get AWS CI roles. +Source of truth: GitHub teams (created by team-provisioner from registry). Usage: sync-registered-teams.py -Requires: GITHUB_TOKEN env var (read access to javaBin/registry). +Requires: GITHUB_TOKEN env var (with org team read access). """ import json import logging import os -import re import sys import urllib.request @@ -48,15 +45,10 @@ def github_get(path, token): return results -REGISTRY_REPO = os.environ.get("REGISTRY_REPO", "registry") -TEAM_NAME = re.compile(r"^[a-z]{1,12}$") - - def get_teams(token): - """Registry team names (teams/.yaml), excluding platform-internal ones.""" - files = github_get(f"/repos/{GITHUB_ORG}/{REGISTRY_REPO}/contents/teams", token) - names = [f["name"][:-5] for f in files if f["type"] == "file" and f["name"].endswith(".yaml")] - slugs = sorted(n for n in names if TEAM_NAME.match(n) and n not in EXCLUDED_TEAMS) + """Get all org teams, excluding platform-internal ones.""" + teams = github_get(f"/orgs/{GITHUB_ORG}/teams", token) + slugs = sorted(t["slug"] for t in teams if t["slug"] not in EXCLUDED_TEAMS) logger.info("Found %d teams (excluding %s)", len(slugs), ", ".join(EXCLUDED_TEAMS)) for slug in slugs: logger.info(" %s", slug) @@ -67,7 +59,7 @@ def write_tfvars(teams, output_path): """Write the registered-teams.auto.tfvars file.""" lines = [ "# GENERATED by sync-registered-teams.py — do not edit manually", - "# Source of truth: teams/*.yaml in javaBin/registry", + "# Source of truth: GitHub teams in javaBin org", "", "registered_teams = [", ] diff --git a/terraform/platform/registered-teams.auto.tfvars b/terraform/platform/registered-teams.auto.tfvars index 9ce27b9..bcc0487 100644 --- a/terraform/platform/registered-teams.auto.tfvars +++ b/terraform/platform/registered-teams.auto.tfvars @@ -1,8 +1,6 @@ # GENERATED by sync-registered-teams.py — do not edit manually -# Source of truth: teams/*.yaml in javaBin/registry +# Source of truth: GitHub teams in javaBin org registered_teams = [ - "cupcake", - "jzwatch", "testteam", ] From 7c0cc785681cefbea27e2e83680ba31c9840b51e Mon Sep 17 00:00:00 2001 From: Alexanderamiri Date: Fri, 23 Oct 2026 16:30:14 +0200 Subject: [PATCH 5/8] docs: restore CLAUDE.md notes for this PR's changes 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) --- CLAUDE.md | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index ae9cf2b..3d1a8c8 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -168,7 +168,7 @@ terraform/state/ | `password-set` | Self-service password-set for new hero accounts (Function URL) | | `budget-enforcer` | Scales ECS services to zero when team exceeds 200% budget | | `resource-tagger` | EventBridge-triggered, auto-tags created-by + commit on new resources | -| `ci-broker` | Validates repo ∈ app.yaml team, vends short-lived team role credentials | +| `ci-broker` | Verifies the job's GitHub OIDC token (repo from signed claims), checks repo ∈ app.yaml team, vends short-lived team role credentials | | `app-auth-sync` | Creates/deletes Cognito app clients requested by app.yaml `auth:` (SSM request → client + credentials) | ### Scripts @@ -179,6 +179,7 @@ terraform/state/ | `scripts/registry.py` | Module sources + boilerplate templates for generate-modules.py | | `scripts/resolve-tool-version.sh` | CI: resolve node/java version from the app repo's version files | | `scripts/sync-registered-teams.py` | CI: GitHub org teams → `registered-teams.auto.tfvars` (needs App token) | +| `scripts/github-oidc-token.sh` | CI: mint a GitHub OIDC token for the broker / apply gate | | `scripts/provision-teams.py` | CI: fetch team YAMLs from registry, invoke team-provisioner Lambda | | `scripts/provision-groups.py` | CI: resolve members.yaml + access.yaml, invoke team-provisioner Lambda | | `scripts/sync-members.py` | CI: sync approved heroes from Google Sheets into members.yaml | @@ -202,6 +203,7 @@ terraform/state/ | SSM params (platform) | `/javabin/{namespace}/{name}` | | SSM params (app) | `/javabin/apps/{team}/{service}/{name}` | | ECS cluster | `javabin-platform` | +| ECS execution roles | `javabin-ecs-exec-{team}` (platform-owned, reads only `/javabin/apps/{team}/*`) | | ECR repos | `{team}-{service}` | | SNS topics | `javabin-{alerts,security,budget-enforcement}` | | Log groups | `/ecs/{team}/{service}` | @@ -287,7 +289,7 @@ The SA JSON key is at `/javabin/platform/google-admin-sa`, the impersonation tar ### Known Issues - **ECS deploy stabilization**: platform-test-app task registers but service fails health check - **Cognito**: pools deployed; internal pool's `GoogleSAML` IdP was created outside Terraform -- **CI broker trusts the payload repo name**: code running inside an app job (e.g. a Terraform `external` data source in tf-plan) can mint a new OIDC token for the broker role and name another repo. Fix: verify a GitHub OIDC token in the Lambda instead of a payload field +- **App databases share one network path**: service-rds allows ingress from the shared ECS tasks SG, so every team's tasks can reach every team's DB port (password still required). Fix: per-app security groups - **Team provisioner Lambda**: All sync functions working (Google/GitHub/Budget/Cognito/Identity Center). Password-set flow deployed. - **`registered_app_repos` manually managed**: Being replaced with team-scoped IAM roles (repo→team resolved via GitHub API at runtime) - **Cost allocation tags pending activation**: `repo`, `created-by`, `commit` tags need activation in Billing console (requires billing data to appear first) From 41142511c0abe4ffb2f071c6040a8f72ddfa04cf Mon Sep 17 00:00:00 2001 From: Alexanderamiri Date: Fri, 23 Oct 2026 16:29:00 +0200 Subject: [PATCH 6/8] feat(auth): put registry groups into tokens and enforce app groups 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) --- terraform/lambda-src/auth_claims/handler.py | 91 ++++++++++++++ .../lambda-src/team_provisioner/handler.py | 30 ++++- .../lambda-src/tests/test_auth_claims.py | 88 ++++++++++++++ terraform/platform/identity/main.tf | 111 ++++++++++++++++++ terraform/platform/identity/outputs.tf | 10 ++ terraform/platform/identity/variables.tf | 5 + terraform/platform/lambdas/main.tf | 16 ++- terraform/platform/lambdas/variables.tf | 10 ++ terraform/platform/main.tf | 3 + 9 files changed, 362 insertions(+), 2 deletions(-) create mode 100644 terraform/lambda-src/auth_claims/handler.py create mode 100644 terraform/lambda-src/tests/test_auth_claims.py diff --git a/terraform/lambda-src/auth_claims/handler.py b/terraform/lambda-src/auth_claims/handler.py new file mode 100644 index 0000000..b40eb76 --- /dev/null +++ b/terraform/lambda-src/auth_claims/handler.py @@ -0,0 +1,91 @@ +"""Auth claims — Cognito pre-token-generation (V2) trigger for the internal pool. + +Federated users (GoogleSAML) sign in with only an email; the pool has no group +membership for them. Registry membership is written to the memberships table +by team-provisioner: + + {pk: "group#", members: ["first.last@java.no", ...]} + +At every sign-in and token refresh this trigger: + 1. looks up the user's email in those groups and sets cognito:groups in the + ID and access token (kept alongside any native Cognito groups); + 2. reads {pk: "client#", allowed_groups: [...]} (written by + app-auth-sync from app.yaml auth.groups). When a client lists allowed + groups and the user is in none of them, raising here makes Cognito + refuse the sign-in, so the app never receives a token. + +Clients without an entry, or with an empty list, accept any pool user. +""" + +import logging +import os +import time + +import boto3 + +logger = logging.getLogger(__name__) +logger.setLevel(logging.INFO) + +TABLE = os.environ.get("MEMBERSHIPS_TABLE", "javabin-auth-memberships") +CACHE_SECONDS = int(os.environ.get("CACHE_SECONDS", "60")) + +dynamodb = boto3.client("dynamodb") + +_cache = {"groups": None, "loaded_at": 0} + + +class AccessDenied(Exception): + """Raised to make Cognito refuse the token. Cognito shows the message.""" + + +def _strings(attr): + """Values of a DynamoDB L (of S) or SS attribute.""" + if not attr: + return [] + if "SS" in attr: + return attr["SS"] + return [v["S"] for v in attr.get("L", []) if "S" in v] + + +def group_members(): + """{group: {lowercased emails}} — scanned and cached per container.""" + if _cache["groups"] is None or time.time() - _cache["loaded_at"] > CACHE_SECONDS: + groups, kwargs = {}, {"TableName": TABLE} + while True: + page = dynamodb.scan(**kwargs) + for item in page.get("Items", []): + pk = item["pk"]["S"] + if pk.startswith("group#"): + groups[pk[len("group#"):]] = {e.lower() for e in _strings(item.get("members"))} + if "LastEvaluatedKey" not in page: + break + kwargs["ExclusiveStartKey"] = page["LastEvaluatedKey"] + _cache.update(groups=groups, loaded_at=time.time()) + return _cache["groups"] + + +def allowed_groups(client_id): + item = dynamodb.get_item(TableName=TABLE, Key={"pk": {"S": f"client#{client_id}"}}).get("Item") + return set(_strings(item.get("allowed_groups"))) if item else set() + + +def handler(event, context): + request = event.get("request", {}) + email = (request.get("userAttributes", {}).get("email") or "").lower() + client_id = event.get("callerContext", {}).get("clientId", "") + native = (request.get("groupConfiguration") or {}).get("groupsToOverride") or [] + + registry = sorted(g for g, members in group_members().items() if email and email in members) + groups = sorted(set(native) | set(registry)) + + allowed = allowed_groups(client_id) if client_id else set() + if allowed and not allowed & set(groups): + logger.info("Denied %s for client %s: needs one of %s", email or "", client_id, sorted(allowed)) + raise AccessDenied("You are not in a group that has access to this app.") + + event["response"] = { + "claimsAndScopeOverrideDetails": { + "groupOverrideDetails": {"groupsToOverride": groups}, + } + } + return event diff --git a/terraform/lambda-src/team_provisioner/handler.py b/terraform/lambda-src/team_provisioner/handler.py index 772c34e..916542b 100644 --- a/terraform/lambda-src/team_provisioner/handler.py +++ b/terraform/lambda-src/team_provisioner/handler.py @@ -34,6 +34,7 @@ ssm = boto3.client("ssm") budgets_client = boto3.client("budgets") cognito_client = boto3.client("cognito-idp") +dynamodb_client = boto3.client("dynamodb") identitystore_client = boto3.client("identitystore") GOOGLE_SA_PARAM = os.environ.get( @@ -835,6 +836,27 @@ def sync_budget(team): # Cognito + Identity Center — stubs with clear extension points # --------------------------------------------------------------------------- +MEMBERSHIPS_TABLE = os.environ.get("MEMBERSHIPS_TABLE", "") + + +def _write_membership(group_name, emails): + """Record a group's registry members for the auth-claims trigger. + + Cognito has no membership for federated users until they sign in, so the + pre-token trigger puts groups into tokens from this table instead. + """ + if not MEMBERSHIPS_TABLE: + return + dynamodb_client.put_item( + TableName=MEMBERSHIPS_TABLE, + Item={ + "pk": {"S": f"group#{group_name}"}, + "members": {"L": [{"S": e.lower()} for e in sorted(set(emails)) if e]}, + "source": {"S": "registry"}, + }, + ) + + def sync_cognito_group(team): """Create or update a Cognito group in the internal user pool and sync membership. @@ -925,6 +947,7 @@ def sync_cognito_group(team): ) logger.info("Removed %s from Cognito group %s", email, group_name) + _write_membership(group_name, desired_emails) return {"synced": True, "group": group_name, "member_count": len(desired_emails)} @@ -1075,13 +1098,18 @@ def _get_google_group_members(group_email): def sync_group_to_cognito(group): - """Ensure a Cognito group exists. No member sync — JIT handles it at sign-in.""" + """Ensure a Cognito group exists and record its registry members. + + Members reach tokens through the auth-claims pre-token trigger, which + reads the memberships table — Cognito group membership is not synced. + """ if not COGNITO_INTERNAL_POOL_ID: return {"skipped": True, "reason": "cognito_pool_not_configured"} if not group.get("cognito"): return {"skipped": True, "reason": "cognito_not_enabled"} group_name = group["name"] + _write_membership(group_name, group.get("members") or []) google_email = group["google"] pool_id = COGNITO_INTERNAL_POOL_ID diff --git a/terraform/lambda-src/tests/test_auth_claims.py b/terraform/lambda-src/tests/test_auth_claims.py new file mode 100644 index 0000000..c607e3f --- /dev/null +++ b/terraform/lambda-src/tests/test_auth_claims.py @@ -0,0 +1,88 @@ +"""Tests for auth_claims — run: python3 -m unittest discover terraform/lambda-src/tests""" + +import importlib.util +import os +import sys +import types +import unittest + + +class FakeDynamo: + def __init__(self, items): + self.items = {i["pk"]["S"]: i for i in items} + self.scans = 0 + + def scan(self, TableName, **kw): + self.scans += 1 + return {"Items": list(self.items.values())} + + def get_item(self, TableName, Key): + item = self.items.get(Key["pk"]["S"]) + return {"Item": item} if item else {} + + +def group(name, *emails): + return {"pk": {"S": f"group#{name}"}, "members": {"L": [{"S": e} for e in emails]}} + + +def client(cid, *groups): + return {"pk": {"S": f"client#{cid}"}, "allowed_groups": {"L": [{"S": g} for g in groups]}} + + +def load(dynamo): + boto3 = types.ModuleType("boto3") + boto3.client = lambda svc: dynamo + sys.modules["boto3"] = boto3 + path = os.path.join(os.path.dirname(__file__), "..", "auth_claims", "handler.py") + spec = importlib.util.spec_from_file_location("auth_claims_handler", path) + mod = importlib.util.module_from_spec(spec) + spec.loader.exec_module(mod) + return mod + + +def event(email, client_id="frosting", native=()): + return { + "callerContext": {"clientId": client_id}, + "request": {"userAttributes": {"email": email}, + "groupConfiguration": {"groupsToOverride": list(native)}}, + "response": {}, + } + + +def groups_of(result): + return result["response"]["claimsAndScopeOverrideDetails"]["groupOverrideDetails"]["groupsToOverride"] + + +class AuthClaimsTest(unittest.TestCase): + def setUp(self): + self.dynamo = FakeDynamo([ + group("helter", "ada.lovelace@java.no", "alan.turing@java.no"), + group("team-cupcake", "ada.lovelace@java.no"), + client("frosting", "helter"), + ]) + self.h = load(self.dynamo) + + def test_registry_groups_go_into_the_token_case_insensitively(self): + result = self.h.handler(event("Ada.Lovelace@java.no"), None) + self.assertEqual(groups_of(result), ["helter", "team-cupcake"]) + + def test_native_cognito_groups_are_kept(self): + result = self.h.handler(event("alan.turing@java.no", native=["admins"]), None) + self.assertEqual(groups_of(result), ["admins", "helter"]) + + def test_user_outside_allowed_groups_is_refused(self): + with self.assertRaises(self.h.AccessDenied): + self.h.handler(event("grace.hopper@java.no"), None) + + def test_client_without_restriction_accepts_anyone(self): + result = self.h.handler(event("grace.hopper@java.no", client_id="other-app"), None) + self.assertEqual(groups_of(result), []) + + def test_memberships_are_cached_between_invocations(self): + self.h.handler(event("ada.lovelace@java.no"), None) + self.h.handler(event("alan.turing@java.no"), None) + self.assertEqual(self.dynamo.scans, 1) + + +if __name__ == "__main__": + unittest.main() diff --git a/terraform/platform/identity/main.tf b/terraform/platform/identity/main.tf index 9f27620..e176986 100644 --- a/terraform/platform/identity/main.tf +++ b/terraform/platform/identity/main.tf @@ -14,6 +14,13 @@ resource "aws_cognito_user_pool" "internal" { name = "${var.project}-internal" + lambda_config { + pre_token_generation_config { + lambda_arn = aws_lambda_function.auth_claims.arn + lambda_version = "V2_0" + } + } + auto_verified_attributes = ["email"] username_attributes = ["email"] @@ -181,3 +188,107 @@ resource "aws_route53_record" "external_auth" { evaluate_target_health = false } } + +################################################################################ +# Registry group claims — javabin-auth-claims (pre-token-generation V2) +# +# Federated users (GoogleSAML) arrive with only an email; no Cognito group +# membership exists for them. team-provisioner writes registry membership +# (groups.yaml with cognito: true, team- groups) to the memberships +# table; this trigger puts the user's groups into cognito:groups of the ID +# and access token at every sign-in and refresh. +# +# The same table holds each app client's allowed groups (written by +# app-auth-sync from app.yaml auth.groups). If a client has allowed groups +# and the user is in none of them, the trigger refuses the token. +################################################################################ + +locals { + lambda_src_path = "${path.module}/../../lambda-src" +} + +resource "aws_dynamodb_table" "auth_memberships" { + name = "${var.project}-auth-memberships" + billing_mode = "PAY_PER_REQUEST" + hash_key = "pk" + + attribute { + name = "pk" + type = "S" + } + + point_in_time_recovery { + enabled = true + } + + tags = { + Name = "${var.project}-auth-memberships" + } +} + +data "archive_file" "auth_claims" { + type = "zip" + output_path = "${path.module}/builds/auth_claims.zip" + output_file_mode = "0644" + source_dir = "${local.lambda_src_path}/auth_claims" +} + +resource "aws_iam_role" "auth_claims" { + name = "${var.project}-auth-claims" + # The org boundary denies creating any role without one + permissions_boundary = var.org_boundary_arn + + assume_role_policy = jsonencode({ + Version = "2012-10-17" + Statement = [{ + Effect = "Allow" + Principal = { Service = "lambda.amazonaws.com" } + Action = "sts:AssumeRole" + }] + }) +} + +resource "aws_iam_role_policy" "auth_claims" { + name = "${var.project}-auth-claims" + role = aws_iam_role.auth_claims.id + + policy = jsonencode({ + Version = "2012-10-17" + Statement = [{ + Sid = "ReadMemberships" + Effect = "Allow" + Action = ["dynamodb:Scan", "dynamodb:GetItem"] + Resource = aws_dynamodb_table.auth_memberships.arn + }] + }) +} + +resource "aws_iam_role_policy_attachment" "auth_claims_logs" { + role = aws_iam_role.auth_claims.name + policy_arn = "arn:aws:iam::aws:policy/service-role/AWSLambdaBasicExecutionRole" +} + +resource "aws_lambda_function" "auth_claims" { + function_name = "${var.project}-auth-claims" + role = aws_iam_role.auth_claims.arn + handler = "handler.handler" + runtime = "python3.12" + timeout = 5 + memory_size = 256 + filename = data.archive_file.auth_claims.output_path + source_code_hash = data.archive_file.auth_claims.output_base64sha256 + + environment { + variables = { + MEMBERSHIPS_TABLE = aws_dynamodb_table.auth_memberships.name + } + } +} + +resource "aws_lambda_permission" "auth_claims_cognito" { + statement_id = "AllowCognitoInternalPool" + action = "lambda:InvokeFunction" + function_name = aws_lambda_function.auth_claims.function_name + principal = "cognito-idp.amazonaws.com" + source_arn = aws_cognito_user_pool.internal.arn +} diff --git a/terraform/platform/identity/outputs.tf b/terraform/platform/identity/outputs.tf index 9aafe38..149cdfe 100644 --- a/terraform/platform/identity/outputs.tf +++ b/terraform/platform/identity/outputs.tf @@ -43,3 +43,13 @@ output "external_user_pool_domain" { description = "Custom domain of the external Cognito user pool (empty if not configured)" value = var.certificate_arn != "" ? "auth-external.${var.domain}" : "" } + +output "auth_memberships_table_name" { + description = "DynamoDB table with registry group memberships and per-client allowed groups" + value = aws_dynamodb_table.auth_memberships.name +} + +output "auth_memberships_table_arn" { + description = "ARN of the auth memberships table" + value = aws_dynamodb_table.auth_memberships.arn +} diff --git a/terraform/platform/identity/variables.tf b/terraform/platform/identity/variables.tf index 62b11a0..e6faa86 100644 --- a/terraform/platform/identity/variables.tf +++ b/terraform/platform/identity/variables.tf @@ -38,3 +38,8 @@ variable "certificate_arn" { type = string default = "" } + +variable "org_boundary_arn" { + description = "Permissions boundary for roles created here (required by the org boundary)" + type = string +} diff --git a/terraform/platform/lambdas/main.tf b/terraform/platform/lambdas/main.tf index 5a02b73..268806e 100644 --- a/terraform/platform/lambdas/main.tf +++ b/terraform/platform/lambdas/main.tf @@ -608,6 +608,12 @@ resource "aws_iam_role_policy" "team_provisioner" { policy = jsonencode({ Version = "2012-10-17" Statement = [ + { + Sid = "WriteAuthMemberships" + Effect = "Allow" + Action = ["dynamodb:PutItem"] + Resource = var.auth_memberships_table_arn + }, { Sid = "SSMRead" Effect = "Allow" @@ -903,6 +909,7 @@ resource "aws_lambda_function" "team_provisioner" { GITHUB_ORG = "javaBin" ALERTS_TOPIC_ARN = var.alerts_topic_arn COGNITO_INTERNAL_POOL_ID = var.internal_user_pool_id + MEMBERSHIPS_TABLE = var.auth_memberships_table_name IDENTITY_STORE_ID = var.identity_store_id SSO_INSTANCE_ARN = var.sso_instance_arn PROJECT = var.project @@ -1633,6 +1640,12 @@ resource "aws_iam_role_policy" "app_auth_sync" { "arn:aws:ssm:${var.region}:${var.aws_account_id}:parameter/${var.project}/apps/*", ] }, + { + Sid = "WriteClientAllowedGroups" + Effect = "Allow" + Action = ["dynamodb:PutItem", "dynamodb:DeleteItem"] + Resource = var.auth_memberships_table_arn + }, { Sid = "WriteClientCredentials" Effect = "Allow" @@ -1672,7 +1685,8 @@ resource "aws_lambda_function" "app_auth_sync" { environment { variables = { - PROJECT = var.project + PROJECT = var.project + MEMBERSHIPS_TABLE = var.auth_memberships_table_name } } } diff --git a/terraform/platform/lambdas/variables.tf b/terraform/platform/lambdas/variables.tf index 3f38c2c..364f4a2 100644 --- a/terraform/platform/lambdas/variables.tf +++ b/terraform/platform/lambdas/variables.tf @@ -127,3 +127,13 @@ variable "cur_data_bucket_arn" { type = string } + +variable "auth_memberships_table_name" { + description = "Auth memberships table (registry groups + per-client allowed groups)" + type = string +} + +variable "auth_memberships_table_arn" { + description = "ARN of the auth memberships table" + type = string +} diff --git a/terraform/platform/main.tf b/terraform/platform/main.tf index e0112f2..a6ebf19 100644 --- a/terraform/platform/main.tf +++ b/terraform/platform/main.tf @@ -83,6 +83,8 @@ module "lambdas" { internal_user_pool_id = module.identity.internal_user_pool_id internal_user_pool_arn = module.identity.internal_user_pool_arn external_user_pool_arn = module.identity.external_user_pool_arn + auth_memberships_table_name = module.identity.auth_memberships_table_name + auth_memberships_table_arn = module.identity.auth_memberships_table_arn identity_store_id = var.identity_store_id sso_instance_arn = var.sso_instance_arn alert_dedup_table_name = module.monitoring.alert_dedup_table_name @@ -108,6 +110,7 @@ module "identity" { google_client_id = var.google_client_id google_client_secret = var.google_client_secret certificate_arn = var.certificate_arn + org_boundary_arn = module.iam.org_boundary_arn } module "dns" { From 067db3d2c5ab2690dd8dd93dc515b33378b23d93 Mon Sep 17 00:00:00 2001 From: Alexanderamiri Date: Fri, 23 Oct 2026 16:29:00 +0200 Subject: [PATCH 7/8] feat(app-yaml): public and validate-only auth clients, env aliases - 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) --- CLAUDE.md | 5 +- docs/app-yaml-reference.md | 45 ++++++++---- docs/lambda-functions.md | 11 ++- docs/reusable-modules.md | 4 +- scripts/generate-modules.py | 69 +++++++++++++----- scripts/tests/test_generate_modules.py | 46 +++++++++++- terraform/lambda-src/app_auth_sync/handler.py | 70 +++++++++++++++++-- .../lambda-src/tests/test_app_auth_sync.py | 63 +++++++++++++++-- terraform/modules/cognito-app-client/main.tf | 14 ++-- .../modules/cognito-app-client/outputs.tf | 9 ++- .../modules/cognito-app-client/variables.tf | 30 ++++++-- 11 files changed, 305 insertions(+), 61 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 3d1a8c8..4cd7517 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -97,7 +97,7 @@ terraform/platform/ compute/ ECS cluster, ECR base config monitoring/ SNS, EventBridge, Config, GuardDuty, Security Hub lambdas/ slack-alert, cost-report, daily-cost-check, compliance-reporter, resource-tagger, budget-enforcer, override-cleanup, team-provisioner, apply-gate, securityhub-summary, password-set, ci-broker, app-auth-sync - identity/ Cognito user pools (internal + external). Identity Center is in terraform/org/ + identity/ Cognito user pools (internal + external), auth-claims trigger, auth memberships table. Identity Center is in terraform/org/ ``` ### Terraform — Org (human-applied, no CI) @@ -169,7 +169,8 @@ terraform/state/ | `budget-enforcer` | Scales ECS services to zero when team exceeds 200% budget | | `resource-tagger` | EventBridge-triggered, auto-tags created-by + commit on new resources | | `ci-broker` | Verifies the job's GitHub OIDC token (repo from signed claims), checks repo ∈ app.yaml team, vends short-lived team role credentials | -| `app-auth-sync` | Creates/deletes Cognito app clients requested by app.yaml `auth:` (SSM request → client + credentials) | +| `app-auth-sync` | Creates/deletes Cognito app clients requested by app.yaml `auth:` (SSM request → client + credentials + allowed groups) | +| `auth-claims` | Internal pool pre-token trigger: registry groups → `cognito:groups`, enforces app `auth.groups` (in `identity/`) | ### Scripts | Script | What | diff --git a/docs/app-yaml-reference.md b/docs/app-yaml-reference.md index 65c3275..b6b4429 100644 --- a/docs/app-yaml-reference.md +++ b/docs/app-yaml-reference.md @@ -191,26 +191,40 @@ Full form: ```yaml auth: pool: internal # 'internal' (default) or 'external' - callback_urls: # optional, https only + client: confidential # confidential (default) | public | none — see below + callback_urls: # optional; https, or http://localhost for local dev - https://app.javazone.no/auth/callback - logout_urls: # optional, https only + - http://localhost:3000/auth/callback + logout_urls: # optional; same rules - https://app.javazone.no/ scopes: # optional, default ['openid', 'email', 'profile'] - openid - email - profile - groups: # optional, reference-only — exposed as COGNITO_GROUPS env var - - admins - - editors + groups: # optional — only members of these groups can sign in + - helter + env: # optional — also inject values under your own names + COGNITO_CLIENT_ID: NUXT_PUBLIC_OIDC_CLIENT_ID + COGNITO_ISSUER_URL: NUXT_PUBLIC_OIDC_AUTHORITY ``` +#### `client` + +| Value | For | You get | +|---|---|---| +| `confidential` | Server-side login (the server does the code exchange) | Client ID **and** secret | +| `public` | Browser apps doing OIDC with PKCE (oidc-client-ts, etc.) — a secret can't be kept in a browser | Client ID, no secret | +| `none` | APIs that only *verify* tokens issued to another app's client | Pool/issuer values only; no client, no callbacks, no `routing.host` needed | + +A typical split frontend/backend: the frontend uses `client: public`, the API uses `client: none` and validates tokens against `COGNITO_ISSUER_URL` / `COGNITO_WELL_KNOWN_URL`. + Both forms are supported and produce the same wiring. - `internal` — Javabin heroes. Sign-in offers every identity provider on the pool (Google Workspace SAML) plus Cognito users. - `external` — public users. - `none` or omitted — no Cognito integration. -`auth:` requires `routing.host` — callback and logout URL defaults are derived from it. +`auth:` with a client needs `routing.host` or explicit `callback_urls` — the defaults are derived from the host. #### Defaults @@ -233,14 +247,21 @@ On the very first deploy with `auth:`, the first task can start a few seconds be |---|---|---| | `COGNITO_USER_POOL_ID` | Pool looked up by name | Used to construct the issuer URL on the backend | | `COGNITO_DOMAIN` | Hosted-UI FQDN of the pool | e.g. `javabin-internal.auth.eu-central-1.amazoncognito.com`; empty for external when no custom domain is configured | -| `COGNITO_ISSUER_URL` | `https://cognito-idp.{region}.amazonaws.com/{pool_id}` | OIDC issuer for JWT validation (`{issuer}/.well-known/openid-configuration`) | -| `COGNITO_CLIENT_ID` | SSM (ECS task `secrets`) | The OAuth client ID | -| `COGNITO_CLIENT_SECRET` | SSM SecureString (ECS task `secrets`) | Always generated — server-side apps only | -| `COGNITO_GROUPS` | Comma-joined list from `auth.groups` | Optional; for app-level authorization on the `cognito:groups` JWT claim | +| `COGNITO_ISSUER_URL` | `https://cognito-idp.{region}.amazonaws.com/{pool_id}` | OIDC issuer for JWT validation | +| `COGNITO_WELL_KNOWN_URL` | `{issuer}/.well-known/openid-configuration` | OIDC discovery document | +| `COGNITO_CLIENT_ID` | SSM (ECS task `secrets`) | `confidential` and `public` only | +| `COGNITO_CLIENT_SECRET` | SSM SecureString (ECS task `secrets`) | `confidential` only | +| `COGNITO_GROUPS` | Comma-joined list from `auth.groups` | Only when `groups` is set | + +`auth.env` adds the same values under extra names (both names are injected). Only names injected for your `client` type can be aliased. + +#### Groups + +Tokens from the internal pool carry the user's registry groups in `cognito:groups` (ID and access token): every group in `javaBin/registry` `groups/groups.yaml` with `cognito: true` the user is a member of, plus `team-{team}` for teams they belong to. The `javabin-auth-claims` trigger adds them at every sign-in and refresh, from membership team-provisioner records on each registry merge. A registry change reaches tokens at the user's next sign-in or token refresh. -#### Groups are reference-only +`auth.groups` restricts the client: a user in none of the listed groups is refused at sign-in, so the app never receives a token for them. Apps should still check `cognito:groups` for finer-grained authorization (e.g. admin features). -`auth.groups` is a list of group names that the app *expects* to see on the `cognito:groups` JWT claim. **No Cognito groups are created or modified from `app.yaml`.** Groups are managed by the team-provisioner Lambda (e.g. `team-{team}` groups synced from the registry). +No Cognito groups are created from `app.yaml`; add groups in the registry. #### `pool: both` is not supported diff --git a/docs/lambda-functions.md b/docs/lambda-functions.md index 4a38c85..f6bf6ae 100644 --- a/docs/lambda-functions.md +++ b/docs/lambda-functions.md @@ -110,12 +110,21 @@ Idempotent: skips resources that already have a `created-by` tag (preserves orig **Trigger:** EventBridge "Parameter Store Change" for `/javabin/apps/*/*/auth` (Create/Update/Delete), plus a daily `{"action": "reconcile"}` at 03:30 UTC **Purpose:** Creates, updates and deletes the Cognito app clients requested by `app.yaml` `auth:`. The request parameter's path identifies the team (only that team can write there), so no caller-supplied identity is trusted. -**Flow:** Re-read the request → validate (pool, https URLs, scopes) → upsert client `{team}-{app}` in the requested pool with every pool IdP + `COGNITO` → remove it from the other pool → write `/javabin/apps/{team}/{app}/cognito-client-id` (String) and `cognito-client-secret` (SecureString), tagged `team={team}`. A missing request deletes the client and both parameters. Reconcile also removes clients whose credential parameter exists but whose request is gone; clients created by other means are never touched. +**Flow:** Re-read the request → validate (pool, client_type, https/localhost URLs, scopes, allowed_groups) → upsert client `{team}-{app}` in the requested pool with every pool IdP + `COGNITO` (a client_type change replaces the client, since the secret setting is immutable) → remove it from the other pool → write `/javabin/apps/{team}/{app}/cognito-client-id` (String) and, for confidential clients, `cognito-client-secret` (SecureString), tagged `team={team}` → record `allowed_groups` in `javabin-auth-memberships`. A missing request deletes the client and both parameters. Reconcile also removes clients whose credential parameter exists but whose request is gone; clients created by other means are never touched. **Concurrency:** reserved concurrency 1, so two changes to one app cannot race into duplicate clients. **Manual run:** `aws lambda invoke --function-name javabin-app-auth-sync --payload '{"action":"reconcile"}' --cli-binary-format raw-in-base64-out /dev/stdout` +## auth-claims + +**Trigger:** Cognito pre-token-generation (V2) on the internal pool (`javabin-internal`), every sign-in and token refresh +**Purpose:** Puts the user's registry groups into `cognito:groups` (ID and access token), and refuses sign-in when the app client has `allowed_groups` and the user is in none of them. + +**Data:** `javabin-auth-memberships` (DynamoDB). `group#` items (members' emails) are written by team-provisioner on registry merges; `client#` items (`allowed_groups`) by app-auth-sync. Memberships are cached for 60 s per container. + +**Lives in:** `terraform/platform/identity/` (the pool references it, so it can't sit in `lambdas/` without a module cycle). + ## Shared Module: pricing `terraform/lambda-src/shared/pricing.py` — provides `estimate_cost()` for enriching alerts with cost impact. Uses AWS Pricing API (us-east-1 endpoint). Caches pricing data across Lambda invocations. diff --git a/docs/reusable-modules.md b/docs/reusable-modules.md index b247cdf..605b00a 100644 --- a/docs/reusable-modules.md +++ b/docs/reusable-modules.md @@ -150,8 +150,8 @@ Auto-wires: resource env vars (bucket names, table names, queue URLs), secret AR Requests a Cognito OAuth client for the app. It does **not** create the client: it writes the request to SSM `/{project}/apps/{team}/{app_name}/auth`, and the platform `app-auth-sync` Lambda creates the client `{team}-{app_name}` and writes its credentials next to the request (`cognito-client-id`, `cognito-client-secret`). See [app-yaml-reference.md#auth](app-yaml-reference.md#auth) for why. -**Inputs:** `app_name`, `team`, `pool` (internal/external), `callback_urls`, `logout_urls`, `allowed_oauth_scopes`, `external_pool_custom_domain` -**Outputs:** `user_pool_id`, `user_pool_domain`, `issuer_url`, `client_id_arn`, `client_secret_arn` (SSM ARNs for ECS `secrets`), `request_parameter_name` +**Inputs:** `app_name`, `team`, `pool` (internal/external), `client_type` (confidential/public/none), `callback_urls`, `logout_urls`, `allowed_oauth_scopes`, `allowed_groups`, `external_pool_custom_domain` +**Outputs:** `user_pool_id`, `user_pool_domain`, `issuer_url`, `well_known_url`, `client_id_arn`, `client_secret_arn` (SSM ARNs for ECS `secrets`), `request_parameter_name` ## Policy Composition Pattern diff --git a/scripts/generate-modules.py b/scripts/generate-modules.py index 53f1694..c596ee6 100755 --- a/scripts/generate-modules.py +++ b/scripts/generate-modules.py @@ -63,7 +63,7 @@ class AppYamlError(ValueError): "routing": {"host", "priority", "deregistration_delay"}, "domain": None, "resources": {"buckets", "databases", "secrets", "queues"}, - "auth": {"pool", "callback_urls", "logout_urls", "scopes", "groups"}, + "auth": {"pool", "client", "callback_urls", "logout_urls", "scopes", "groups", "env"}, "environment": None, "alarms": {"enabled", "cpu_threshold", "memory_threshold", "error_5xx_threshold"}, } @@ -84,6 +84,8 @@ class AppYamlError(ValueError): "budget_alert_nok": "budgets are per team — set budget_nok in javaBin/registry teams/.yaml", } +AUTH_CLIENT_TYPES = ("confidential", "public", "none") + NAME_RE = re.compile(r"^[a-z][a-z0-9-]{0,19}$") TEAM_RE = re.compile(r"^[a-z]{1,12}$") RESOURCE_NAME_RE = re.compile(r"^[a-z0-9][a-z0-9_-]*$") @@ -404,37 +406,66 @@ def src(mod): if pool not in ("internal", "external"): raise AppYamlError(f"auth.pool must be 'internal' or 'external' (got {pool!r})") - host = routing.get("host") - if not host: - raise AppYamlError("auth: requires routing.host (callback URLs default to it)") + client = auth.get("client", "confidential") + if client not in AUTH_CLIENT_TYPES: + raise AppYamlError(f"auth.client must be one of {', '.join(AUTH_CLIENT_TYPES)} (got {client!r})") - callback_urls = auth.get("callback_urls") or [ - f"https://{host}/", - f"https://{host}/auth/callback", - ] - logout_urls = auth.get("logout_urls") or [f"https://{host}/"] + groups = auth.get("groups") or [] + if not isinstance(groups, list) or not all(isinstance(g, str) and g for g in groups): + raise AppYamlError("auth.groups must be a list of group names") auth_attrs = { "app_name": name, "team": team, "pool": pool, - "callback_urls": callback_urls, - "logout_urls": logout_urls, + "client_type": client, "project": PROJECT, } + if client != "none": + host = routing.get("host") + if not host and not auth.get("callback_urls"): + raise AppYamlError("auth: requires routing.host or auth.callback_urls (callback URLs default to the host)") + auth_attrs["callback_urls"] = auth.get("callback_urls") or [ + f"https://{host}/", + f"https://{host}/auth/callback", + ] + auth_attrs["logout_urls"] = auth.get("logout_urls") or ([f"https://{host}/"] if host else []) + if groups: + auth_attrs["allowed_groups"] = groups if auth.get("scopes"): auth_attrs["allowed_oauth_scopes"] = auth["scopes"] blocks.append(module_block("auth", src("cognito-app-client"), auth_attrs)) - env_map["COGNITO_USER_POOL_ID"] = Ref("module.auth.user_pool_id") - env_map["COGNITO_DOMAIN"] = Ref("module.auth.user_pool_domain") - env_map["COGNITO_ISSUER_URL"] = Ref("module.auth.issuer_url") - secret_map["COGNITO_CLIENT_ID"] = Ref("module.auth.client_id_arn") - secret_map["COGNITO_CLIENT_SECRET"] = Ref("module.auth.client_secret_arn") - - if auth.get("groups"): - env_map["COGNITO_GROUPS"] = ",".join(auth["groups"]) + auth_env = { + "COGNITO_USER_POOL_ID": Ref("module.auth.user_pool_id"), + "COGNITO_DOMAIN": Ref("module.auth.user_pool_domain"), + "COGNITO_ISSUER_URL": Ref("module.auth.issuer_url"), + "COGNITO_WELL_KNOWN_URL": Ref("module.auth.well_known_url"), + } + if groups: + auth_env["COGNITO_GROUPS"] = ",".join(groups) + auth_secrets = {} + if client != "none": + auth_secrets["COGNITO_CLIENT_ID"] = Ref("module.auth.client_id_arn") + if client == "confidential": + auth_secrets["COGNITO_CLIENT_SECRET"] = Ref("module.auth.client_secret_arn") + + # auth.env: expose the same values under names the app already reads, + # e.g. {COGNITO_CLIENT_ID: NUXT_PUBLIC_OIDC_CLIENT_ID} + aliases = auth.get("env") or {} + if not isinstance(aliases, dict): + raise AppYamlError("auth.env must map injected names to extra variable names") + for source, alias in aliases.items(): + if source not in auth_env and source not in auth_secrets: + available = ", ".join(sorted({*auth_env, *auth_secrets})) + raise AppYamlError(f"auth.env: {source} is not injected for this app (available: {available})") + if not ENV_RE.match(str(alias)): + raise AppYamlError(f"auth.env.{source}: {alias!r} is not a valid variable name") + (auth_env if source in auth_env else auth_secrets)[alias] = (auth_env | auth_secrets)[source] + + env_map.update(auth_env) + secret_map.update(auth_secrets) # Task role (always) blocks.append(module_block("task_role", src("service-role"), { diff --git a/scripts/tests/test_generate_modules.py b/scripts/tests/test_generate_modules.py index 76931fe..adb6f1a 100644 --- a/scripts/tests/test_generate_modules.py +++ b/scripts/tests/test_generate_modules.py @@ -122,12 +122,56 @@ def test_worker_without_routing_has_no_target_group(self): self.assertNotIn('module "alarms"', main) +class AuthFeaturesTest(unittest.TestCase): + def test_public_client_gets_id_but_no_secret(self): + app = {**FROSTING, "auth": {**FROSTING["auth"], "client": "public"}} + main = generate(app)["main.tf"] + self.assertIn('"public"', main) + self.assertIn("COGNITO_CLIENT_ID", main) + self.assertNotIn("COGNITO_CLIENT_SECRET", main) + + def test_validate_only_injects_issuer_without_client_or_host(self): + app = {"name": "api", "team": "core", "auth": {"pool": "internal", "client": "none"}} + main = generate(app)["main.tf"] + self.assertIn("module.auth.well_known_url", main) + self.assertNotIn("COGNITO_CLIENT_ID", main) + self.assertNotIn("callback_urls", main) + + def test_env_aliases_expose_values_under_app_names(self): + app = {**FROSTING, "auth": {"pool": "internal", "client": "public", + "env": {"COGNITO_CLIENT_ID": "NUXT_PUBLIC_OIDC_CLIENT_ID", + "COGNITO_ISSUER_URL": "NUXT_PUBLIC_OIDC_AUTHORITY"}}} + main = generate(app)["main.tf"] + self.assertRegex(main, r"NUXT_PUBLIC_OIDC_CLIENT_ID\s+= module.auth.client_id_arn") + self.assertRegex(main, r"NUXT_PUBLIC_OIDC_AUTHORITY\s+= module.auth.issuer_url") + + def test_alias_of_value_not_injected_is_rejected(self): + app = {**FROSTING, "auth": {"pool": "internal", "client": "public", + "env": {"COGNITO_CLIENT_SECRET": "SECRET"}}} + with self.assertRaises(gen.AppYamlError) as cm: + generate(app) + self.assertIn("not injected", str(cm.exception)) + + def test_groups_restrict_the_client(self): + app = {**CUPCAKE, "auth": {"pool": "internal", "groups": ["helter"]}} + main = generate(app)["main.tf"] + self.assertIn('allowed_groups', main) + self.assertIn('["helter"]', main) + + def test_localhost_callbacks_are_passed_through(self): + app = {**FROSTING, "auth": {"pool": "internal", "client": "public", + "callback_urls": ["https://cupcake.javazone.no/", "http://localhost:3000/"]}} + self.assertIn('"http://localhost:3000/"', generate(app)["main.tf"]) + + @unittest.skipUnless(shutil.which("terraform"), "terraform not installed") class FmtTest(unittest.TestCase): """tf-plan runs `terraform fmt -check`; generated files must pass it.""" def test_generated_files_are_fmt_clean(self): - for app in (CUPCAKE, FROSTING, FULL, {"name": "worker", "team": "core"}): + public = {**FROSTING, "auth": {"pool": "internal", "client": "public", "groups": ["helter"], + "env": {"COGNITO_CLIENT_ID": "NUXT_PUBLIC_OIDC_CLIENT_ID"}}} + for app in (CUPCAKE, FROSTING, FULL, public, {"name": "worker", "team": "core"}): with tempfile.TemporaryDirectory() as d: for name, content in generate(app).items(): with open(os.path.join(d, name), "w") as f: diff --git a/terraform/lambda-src/app_auth_sync/handler.py b/terraform/lambda-src/app_auth_sync/handler.py index c69825b..7568cac 100644 --- a/terraform/lambda-src/app_auth_sync/handler.py +++ b/terraform/lambda-src/app_auth_sync/handler.py @@ -11,7 +11,12 @@ /{project}/apps/{team}/{app}/cognito-client-id (String) /{project}/apps/{team}/{app}/cognito-client-secret (SecureString) -where the app's ECS task picks them up via the shared execution role. +where the app's ECS task picks them up via its team's execution role. +Public clients (browser apps using PKCE) get no secret and no secret parameter. + +The request's allowed groups are written to the auth memberships table as +{pk: "client#", allowed_groups: [...]}; the auth-claims trigger +refuses tokens for users in none of them. Triggers: - EventBridge "Parameter Store Change" for */auth under /{project}/apps/ @@ -35,7 +40,11 @@ "internal": os.environ.get("INTERNAL_POOL_NAME", f"{PROJECT}-internal"), "external": os.environ.get("EXTERNAL_POOL_NAME", f"{PROJECT}-external"), } +MEMBERSHIPS_TABLE = os.environ.get("MEMBERSHIPS_TABLE", "") DEFAULT_SCOPES = ["openid", "email", "profile"] +CLIENT_TYPES = ("confidential", "public") +# Cognito allows plain http only for localhost, for local development +LOCAL_URL = re.compile(r"^http://(localhost|127\.0\.0\.1)(:\d+)?(/.*)?$") ALLOWED_SCOPES = {"openid", "email", "profile", "phone", "aws.cognito.signin.user.admin"} APPS_PREFIX = f"/{PROJECT}/apps/" @@ -43,6 +52,7 @@ ssm = boto3.client("ssm") cognito = boto3.client("cognito-idp") +dynamodb = boto3.client("dynamodb") class InvalidRequest(ValueError): @@ -74,8 +84,10 @@ def parse_request(raw): def urls(key, required): value = req.get(key) or [] - if not isinstance(value, list) or not all(isinstance(u, str) and u.startswith("https://") for u in value): - raise InvalidRequest(f"{key} must be a list of https:// URLs") + if not isinstance(value, list) or not all( + isinstance(u, str) and (u.startswith("https://") or LOCAL_URL.match(u)) for u in value + ): + raise InvalidRequest(f"{key} must be a list of https:// (or http://localhost) URLs") if required and not value: raise InvalidRequest(f"{key} must not be empty") return value @@ -84,11 +96,21 @@ def urls(key, required): if not isinstance(scopes, list) or not set(scopes) <= ALLOWED_SCOPES: raise InvalidRequest(f"scopes must be a subset of {sorted(ALLOWED_SCOPES)}") + client_type = req.get("client_type") or "confidential" + if client_type not in CLIENT_TYPES: + raise InvalidRequest(f"client_type must be one of {list(CLIENT_TYPES)}, got {client_type!r}") + + groups = req.get("allowed_groups") or [] + if not isinstance(groups, list) or not all(isinstance(g, str) and g for g in groups): + raise InvalidRequest("allowed_groups must be a list of group names") + return { "pool": pool, + "client_type": client_type, "callback_urls": urls("callback_urls", required=True), "logout_urls": urls("logout_urls", required=False), "scopes": scopes, + "allowed_groups": sorted(set(groups)), } @@ -181,25 +203,58 @@ def sync(team, app, req): stale = find_client(other_id, client_name) if stale: cognito.delete_user_pool_client(UserPoolId=other_id, ClientId=stale) + _delete_client_groups(stale) logger.info("Removed %s from %s pool", client_name, other) upid = pool_id(req["pool"]) settings = client_settings(upid, client_name, req) + want_secret = req["client_type"] == "confidential" client_id = find_client(upid, client_name) + if client_id: + has_secret = bool(cognito.describe_user_pool_client(UserPoolId=upid, ClientId=client_id) + ["UserPoolClient"].get("ClientSecret")) + if has_secret != want_secret: + # A client's secret setting is immutable; switching type means a new client + cognito.delete_user_pool_client(UserPoolId=upid, ClientId=client_id) + _delete_client_groups(client_id) + logger.info("Replacing %s: client_type changed to %s", client_name, req["client_type"]) + client_id = None if client_id: cognito.update_user_pool_client(ClientId=client_id, **settings) logger.info("Updated client %s (%s) in %s pool", client_name, client_id, req["pool"]) else: - client_id = cognito.create_user_pool_client(GenerateSecret=True, **settings)["UserPoolClient"]["ClientId"] - logger.info("Created client %s (%s) in %s pool", client_name, client_id, req["pool"]) + client_id = cognito.create_user_pool_client(GenerateSecret=want_secret, **settings)["UserPoolClient"]["ClientId"] + logger.info("Created %s client %s (%s) in %s pool", req["client_type"], client_name, client_id, req["pool"]) - secret = cognito.describe_user_pool_client(UserPoolId=upid, ClientId=client_id)["UserPoolClient"]["ClientSecret"] id_param, secret_param = _cred_names(team, app) _put_param(id_param, client_id, False, team, app) - _put_param(secret_param, secret, True, team, app) + if want_secret: + secret = cognito.describe_user_pool_client(UserPoolId=upid, ClientId=client_id)["UserPoolClient"]["ClientSecret"] + _put_param(secret_param, secret, True, team, app) + else: + ssm.delete_parameters(Names=[secret_param]) + _put_client_groups(client_id, req["allowed_groups"], team, app) return client_id +def _put_client_groups(client_id, groups, team, app): + if not MEMBERSHIPS_TABLE: + return + dynamodb.put_item( + TableName=MEMBERSHIPS_TABLE, + Item={ + "pk": {"S": f"client#{client_id}"}, + "allowed_groups": {"L": [{"S": g} for g in groups]}, + "app": {"S": f"{team}-{app}"}, + }, + ) + + +def _delete_client_groups(client_id): + if MEMBERSHIPS_TABLE: + dynamodb.delete_item(TableName=MEMBERSHIPS_TABLE, Key={"pk": {"S": f"client#{client_id}"}}) + + def remove(team, app): """Delete the client from every pool and its credential parameters.""" client_name = f"{team}-{app}" @@ -208,6 +263,7 @@ def remove(team, app): client_id = find_client(upid, client_name) if client_id: cognito.delete_user_pool_client(UserPoolId=upid, ClientId=client_id) + _delete_client_groups(client_id) logger.info("Deleted client %s from %s pool", client_name, pool) ssm.delete_parameters(Names=list(_cred_names(team, app))) diff --git a/terraform/lambda-src/tests/test_app_auth_sync.py b/terraform/lambda-src/tests/test_app_auth_sync.py index 9a6cee2..40ab733 100644 --- a/terraform/lambda-src/tests/test_app_auth_sync.py +++ b/terraform/lambda-src/tests/test_app_auth_sync.py @@ -51,6 +51,17 @@ def get_paginator(self, name): ], "Parameters") +class FakeDynamo: + def __init__(self): + self.items = {} + + def put_item(self, TableName, Item): + self.items[Item["pk"]["S"]] = Item + + def delete_item(self, TableName, Key): + self.items.pop(Key["pk"]["S"], None) + + class FakeCognito: POOLS = {"pool-int": "javabin-internal", "pool-ext": "javabin-external"} @@ -75,7 +86,7 @@ def get_paginator(self, name): def create_user_pool_client(self, GenerateSecret, **settings): self.n += 1 cid = f"client{self.n}" - self.clients[cid] = {**settings, "ClientSecret": f"secret{self.n}"} + self.clients[cid] = {**settings, **({"ClientSecret": f"secret{self.n}"} if GenerateSecret else {})} return {"UserPoolClient": {"ClientId": cid}} def update_user_pool_client(self, ClientId, **settings): @@ -88,9 +99,10 @@ def describe_user_pool_client(self, UserPoolId, ClientId): return {"UserPoolClient": self.clients[ClientId]} -def load_handler(ssm, cognito): +def load_handler(ssm, cognito, dynamo=None): + os.environ["MEMBERSHIPS_TABLE"] = "javabin-auth-memberships" boto3 = types.ModuleType("boto3") - boto3.client = lambda svc: {"ssm": ssm, "cognito-idp": cognito}[svc] + boto3.client = lambda svc: {"ssm": ssm, "cognito-idp": cognito, "dynamodb": dynamo or FakeDynamo()}[svc] sys.modules["boto3"] = boto3 path = os.path.join(os.path.dirname(__file__), "..", "app_auth_sync", "handler.py") spec = importlib.util.spec_from_file_location("app_auth_sync_handler", path) @@ -110,8 +122,8 @@ def event(name, operation): class AppAuthSyncTest(unittest.TestCase): def setUp(self): - self.ssm, self.cognito = FakeSSM(), FakeCognito() - self.h = load_handler(self.ssm, self.cognito) + self.ssm, self.cognito, self.dynamo = FakeSSM(), FakeCognito(), FakeDynamo() + self.h = load_handler(self.ssm, self.cognito, self.dynamo) def put_request(self, req=REQUEST, name=REQ): self.ssm.params[name] = json.dumps(req) @@ -203,5 +215,46 @@ def test_reconcile_syncs_requests_and_removes_orphans(self): self.assertEqual(client["ClientName"], "cupcake-frosting") +class AuthFeaturesTest(AppAuthSyncTest): + SECRET = "/javabin/apps/cupcake/frosting/cognito-client-secret" + + def test_public_client_has_no_secret_parameter(self): + self.put_request({**REQUEST, "client_type": "public"}) + self.h.handler(event(REQ, "Create"), None) + _, client = self.only_client() + self.assertNotIn("ClientSecret", client) + self.assertNotIn(self.SECRET, self.ssm.params) + self.assertIn("/javabin/apps/cupcake/frosting/cognito-client-id", self.ssm.params) + + def test_switching_client_type_replaces_the_client(self): + self.put_request() + self.h.handler(event(REQ, "Create"), None) + old_id, _ = self.only_client() + self.put_request({**REQUEST, "client_type": "public"}) + self.h.handler(event(REQ, "Update"), None) + new_id, client = self.only_client() + self.assertNotEqual(new_id, old_id) + self.assertNotIn("ClientSecret", client) + self.assertNotIn(self.SECRET, self.ssm.params) + self.assertNotIn(f"client#{old_id}", self.dynamo.items) + + def test_allowed_groups_are_recorded_and_cleaned_up(self): + self.put_request({**REQUEST, "allowed_groups": ["helter", "pkom"]}) + self.h.handler(event(REQ, "Create"), None) + cid, _ = self.only_client() + item = self.dynamo.items[f"client#{cid}"] + self.assertEqual([v["S"] for v in item["allowed_groups"]["L"]], ["helter", "pkom"]) + + del self.ssm.params[REQ] + self.h.handler(event(REQ, "Delete"), None) + self.assertEqual(self.dynamo.items, {}) + + def test_localhost_callbacks_allowed_but_not_other_http(self): + self.put_request({**REQUEST, "callback_urls": ["https://cupcake.javazone.no/", "http://localhost:3000/cb"]}) + self.assertEqual(self.h.handler(event(REQ, "Create"), None)["status"], "synced") + self.put_request({**REQUEST, "callback_urls": ["http://localhost.evil.com/cb"]}) + self.assertEqual(self.h.handler(event(REQ, "Update"), None)["status"], "invalid") + + if __name__ == "__main__": unittest.main() diff --git a/terraform/modules/cognito-app-client/main.tf b/terraform/modules/cognito-app-client/main.tf index 539a304..8f34e8d 100644 --- a/terraform/modules/cognito-app-client/main.tf +++ b/terraform/modules/cognito-app-client/main.tf @@ -40,14 +40,20 @@ locals { pool_domain = var.pool == "internal" ? "${var.internal_pool_name}.auth.${local.region}.amazoncognito.com" : var.external_pool_custom_domain } +# Validate-only apps (client_type = "none") only verify tokens issued to +# other clients; they get pool metadata but request no client. resource "aws_ssm_parameter" "auth_request" { + count = var.client_type == "none" ? 0 : 1 + name = "${local.param_prefix}/auth" description = "Cognito client request for ${var.team}-${var.app_name} — read by the platform app-auth-sync Lambda" type = "String" value = jsonencode({ - pool = var.pool - callback_urls = var.callback_urls - logout_urls = var.logout_urls - scopes = var.allowed_oauth_scopes + pool = var.pool + client_type = var.client_type + callback_urls = var.callback_urls + logout_urls = var.logout_urls + scopes = var.allowed_oauth_scopes + allowed_groups = var.allowed_groups }) } diff --git a/terraform/modules/cognito-app-client/outputs.tf b/terraform/modules/cognito-app-client/outputs.tf index 6195e53..5d4a506 100644 --- a/terraform/modules/cognito-app-client/outputs.tf +++ b/terraform/modules/cognito-app-client/outputs.tf @@ -13,6 +13,11 @@ output "issuer_url" { value = "https://cognito-idp.${local.region}.amazonaws.com/${local.pool_id}" } +output "well_known_url" { + description = "OIDC discovery document URL" + value = "https://cognito-idp.${local.region}.amazonaws.com/${local.pool_id}/.well-known/openid-configuration" +} + output "client_id_arn" { description = "ARN of the SSM parameter the platform writes the client ID to" value = "${local.param_arn}/cognito-client-id" @@ -24,6 +29,6 @@ output "client_secret_arn" { } output "request_parameter_name" { - description = "SSM parameter holding the client request" - value = aws_ssm_parameter.auth_request.name + description = "SSM parameter holding the client request (empty for client_type = none)" + value = one(aws_ssm_parameter.auth_request[*].name) } diff --git a/terraform/modules/cognito-app-client/variables.tf b/terraform/modules/cognito-app-client/variables.tf index 7f86181..1805e5a 100644 --- a/terraform/modules/cognito-app-client/variables.tf +++ b/terraform/modules/cognito-app-client/variables.tf @@ -36,27 +36,45 @@ variable "external_pool_custom_domain" { default = "" } +variable "client_type" { + description = "confidential (server-side, with secret), public (browser/PKCE, no secret) or none (validate tokens only, no client)" + type = string + default = "confidential" + + validation { + condition = contains(["confidential", "public", "none"], var.client_type) + error_message = "client_type must be confidential, public or none." + } +} + variable "callback_urls" { - description = "Allowed OAuth callback URLs (https only)" + description = "Allowed OAuth callback URLs (https, or http://localhost for local development)" type = list(string) + default = [] validation { - condition = length(var.callback_urls) > 0 && alltrue([for u in var.callback_urls : startswith(u, "https://")]) - error_message = "callback_urls must be a non-empty list of https:// URLs." + condition = alltrue([for u in var.callback_urls : can(regex("^(https://|http://(localhost|127\\.0\\.0\\.1)(:[0-9]+)?(/|$))", u))]) + error_message = "callback_urls must be https:// URLs or http://localhost URLs." } } variable "logout_urls" { - description = "Allowed logout URLs (https only)" + description = "Allowed logout URLs (https, or http://localhost for local development)" type = list(string) default = [] validation { - condition = alltrue([for u in var.logout_urls : startswith(u, "https://")]) - error_message = "logout_urls must be https:// URLs." + condition = alltrue([for u in var.logout_urls : can(regex("^(https://|http://(localhost|127\\.0\\.0\\.1)(:[0-9]+)?(/|$))", u))]) + error_message = "logout_urls must be https:// URLs or http://localhost URLs." } } +variable "allowed_groups" { + description = "Only users in at least one of these groups can sign in to this client (empty = any pool user)" + type = list(string) + default = [] +} + variable "allowed_oauth_scopes" { description = "Allowed OAuth scopes" type = list(string) From 25aed70c5733ca29871ccc4e6ef1b571703cb4e2 Mon Sep 17 00:00:00 2001 From: Alexanderamiri Date: Fri, 23 Oct 2026 17:09:13 +0200 Subject: [PATCH 8/8] fix(auth): prune stale group memberships and fail closed on allowed groups 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- 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) --- docs/app-yaml-reference.md | 4 ++ terraform/lambda-src/app_auth_sync/handler.py | 3 + .../lambda-src/team_provisioner/handler.py | 37 ++++++++++- .../test_team_provisioner_memberships.py | 64 +++++++++++++++++++ terraform/platform/lambdas/main.tf | 2 +- 5 files changed, 108 insertions(+), 2 deletions(-) create mode 100644 terraform/lambda-src/tests/test_team_provisioner_memberships.py diff --git a/docs/app-yaml-reference.md b/docs/app-yaml-reference.md index b6b4429..c393817 100644 --- a/docs/app-yaml-reference.md +++ b/docs/app-yaml-reference.md @@ -218,6 +218,10 @@ auth: A typical split frontend/backend: the frontend uses `client: public`, the API uses `client: none` and validates tokens against `COGNITO_ISSUER_URL` / `COGNITO_WELL_KNOWN_URL`. +A `client: none` API has no client ID of its own to check, so it accepts tokens issued to *any* client in the pool. Check `cognito:groups` (or the token's `client_id`) for anything that matters. + +Changing `client` between `confidential` and `public` replaces the Cognito client, so its ID changes. Running tasks keep the old ID until they restart: redeploy after the change. + Both forms are supported and produce the same wiring. - `internal` — Javabin heroes. Sign-in offers every identity provider on the pool (Google Workspace SAML) plus Cognito users. diff --git a/terraform/lambda-src/app_auth_sync/handler.py b/terraform/lambda-src/app_auth_sync/handler.py index 7568cac..7fa166a 100644 --- a/terraform/lambda-src/app_auth_sync/handler.py +++ b/terraform/lambda-src/app_auth_sync/handler.py @@ -239,6 +239,9 @@ def sync(team, app, req): def _put_client_groups(client_id, groups, team, app): if not MEMBERSHIPS_TABLE: + if groups: + # Fail closed: without the table the restriction would silently vanish + raise RuntimeError("MEMBERSHIPS_TABLE is not set; cannot record allowed groups") return dynamodb.put_item( TableName=MEMBERSHIPS_TABLE, diff --git a/terraform/lambda-src/team_provisioner/handler.py b/terraform/lambda-src/team_provisioner/handler.py index 916542b..51f0750 100644 --- a/terraform/lambda-src/team_provisioner/handler.py +++ b/terraform/lambda-src/team_provisioner/handler.py @@ -857,6 +857,30 @@ def _write_membership(group_name, emails): ) +def _prune_memberships(current_groups): + """Delete recorded registry groups that are gone or no longer cognito-enabled. + + team- rows are owned by team sync and are left alone. + """ + if not MEMBERSHIPS_TABLE: + return [] + pruned, kwargs = [], {"TableName": MEMBERSHIPS_TABLE, "ProjectionExpression": "pk"} + while True: + page = dynamodb_client.scan(**kwargs) + for item in page.get("Items", []): + pk = item["pk"]["S"] + name = pk[len("group#"):] + if pk.startswith("group#") and not name.startswith("team-") and name not in current_groups: + dynamodb_client.delete_item(TableName=MEMBERSHIPS_TABLE, Key={"pk": {"S": pk}}) + pruned.append(name) + if "LastEvaluatedKey" not in page: + break + kwargs["ExclusiveStartKey"] = page["LastEvaluatedKey"] + if pruned: + logger.info("Pruned stale auth memberships: %s", pruned) + return pruned + + def sync_cognito_group(team): """Create or update a Cognito group in the internal user pool and sync membership. @@ -1109,7 +1133,10 @@ def sync_group_to_cognito(group): return {"skipped": True, "reason": "cognito_not_enabled"} group_name = group["name"] - _write_membership(group_name, group.get("members") or []) + # Only payloads with resolved members (sync_groups_and_heros) carry a + # member list; others must not wipe the recorded membership. + if "members" in group: + _write_membership(group_name, group["members"]) google_email = group["google"] pool_id = COGNITO_INTERNAL_POOL_ID @@ -1775,6 +1802,14 @@ def handle_sync_groups_and_heros(event): except Exception as e: logger.error("Slack notification failed: %s", e) + + # Full group list: drop memberships of removed / no-longer-cognito groups + try: + results["auth_memberships_pruned"] = _prune_memberships( + {g["name"] for g in groups if g.get("cognito")}) + except Exception as e: + logger.error("Pruning auth memberships failed: %s", e) + return { "statusCode": 200, "body": json.dumps( diff --git a/terraform/lambda-src/tests/test_team_provisioner_memberships.py b/terraform/lambda-src/tests/test_team_provisioner_memberships.py new file mode 100644 index 0000000..a401744 --- /dev/null +++ b/terraform/lambda-src/tests/test_team_provisioner_memberships.py @@ -0,0 +1,64 @@ +"""Tests for team_provisioner auth-membership recording — 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 FakeDynamo: + def __init__(self, pks): + self.items = {pk: {"pk": {"S": pk}} for pk in pks} + + def put_item(self, TableName, Item): + self.items[Item["pk"]["S"]] = Item + + def delete_item(self, TableName, Key): + del self.items[Key["pk"]["S"]] + + def scan(self, TableName, **kw): + return {"Items": [{"pk": i["pk"]} for i in self.items.values()]} + + +def load(dynamo): + os.environ["MEMBERSHIPS_TABLE"] = "javabin-auth-memberships" + os.environ["COGNITO_INTERNAL_POOL_ID"] = "pool" + cognito = mock.MagicMock() + cognito.exceptions.ResourceNotFoundException = KeyError + boto3 = types.ModuleType("boto3") + boto3.client = lambda svc, **kw: {"dynamodb": dynamo, "cognito-idp": cognito}.get(svc, mock.MagicMock()) + sys.modules["boto3"] = boto3 + sys.path.insert(0, SRC) + spec = importlib.util.spec_from_file_location("team_provisioner_handler", os.path.join(SRC, "team_provisioner", "handler.py")) + mod = importlib.util.module_from_spec(spec) + spec.loader.exec_module(mod) + return mod + + +class MembershipsTest(unittest.TestCase): + def test_group_sync_records_members_lowercased(self): + dynamo = FakeDynamo([]) + h = load(dynamo) + h.sync_group_to_cognito({"name": "helter", "google": "helter@java.no", "cognito": True, + "members": ["Ada.Lovelace@java.no"]}) + self.assertEqual(dynamo.items["group#helter"]["members"], {"L": [{"S": "ada.lovelace@java.no"}]}) + + def test_payload_without_members_does_not_wipe_membership(self): + dynamo = FakeDynamo(["group#helter"]) + h = load(dynamo) + h.sync_group_to_cognito({"name": "helter", "google": "helter@java.no", "cognito": True}) + self.assertNotIn("members", dynamo.items["group#helter"]) # untouched + + def test_prune_removes_gone_groups_but_keeps_team_and_client_rows(self): + dynamo = FakeDynamo(["group#helter", "group#drift", "group#team-cupcake", "client#abc"]) + h = load(dynamo) + self.assertEqual(h._prune_memberships({"helter"}), ["drift"]) + self.assertEqual(set(dynamo.items), {"group#helter", "group#team-cupcake", "client#abc"}) + + +if __name__ == "__main__": + unittest.main() diff --git a/terraform/platform/lambdas/main.tf b/terraform/platform/lambdas/main.tf index 268806e..ec82143 100644 --- a/terraform/platform/lambdas/main.tf +++ b/terraform/platform/lambdas/main.tf @@ -611,7 +611,7 @@ resource "aws_iam_role_policy" "team_provisioner" { { Sid = "WriteAuthMemberships" Effect = "Allow" - Action = ["dynamodb:PutItem"] + Action = ["dynamodb:PutItem", "dynamodb:DeleteItem", "dynamodb:Scan"] Resource = var.auth_memberships_table_arn }, {