Skip to content

fix(iac/aws): extend OIDC trust allowlist to cover every job assuming cudly_deploy - #1683

Open
cristim wants to merge 3 commits into
mainfrom
sec/1648-aws-oidc-allowlist
Open

fix(iac/aws): extend OIDC trust allowlist to cover every job assuming cudly_deploy#1683
cristim wants to merge 3 commits into
mainfrom
sec/1648-aws-oidc-allowlist

Conversation

@cristim

@cristim cristim commented Jul 29, 2026

Copy link
Copy Markdown
Member

Refs #1648, #1674

Not applied. Do not apply.

terraform/environments/aws/ci-cd-permissions/ is bootstrap-only, applied manually by a privileged human, never by a deploy workflow. Live AWS currently trusts the pre-b876d7fa0 wildcard repo:LeanerCloud/CUDly:*, not even the 4-entry allowlist already committed to role.tf, which is why that role has never been re-applied: doing so as currently written would immediately break every job whose OIDC subject isn't one of the 4 listed values. This PR extends the allowlist so that when it is applied, nothing breaks. The apply itself is a separate, manual step for whoever holds the credentials.

Owner constraint: only main may deploy. This PR's code half does not fully deliver that yet.

pull_request is not in this allowlist, and was never a candidate. I checked whether any job assuming this specific role (cudly_deploy) runs on pull_request: none does (see the judgment-call section below for the evidence). Nothing to report, nothing to allowlist.

Three separate GitHub-side controls keep getting conflated across this repo's issues. This PR implements only the first.

  1. Allowlist membership (role.tf's sub list, this PR): can the job authenticate to AWS at all? Nothing below matters if this says no.
  2. Deployment branch policy (a GitHub environment setting, not touched by this PR): which branch may trigger a deploy to that environment. This is what "only main may deploy" actually requires.
  3. Required reviewers / protection rules (a GitHub environment setting, not touched by this PR): who must approve before a job bound to that environment proceeds. The subject of sec(ci): destroy and rollback workflows have no environment binding, so no reviewer gate applies #1591/sec(ci): bind destroy jobs to an environment and narrow id-token on destroy workflows #1674/sec(ci): deployment environments have no protection rules, so environment-bound credentialed jobs run unapproved #1660, not this PR.

ref:refs/heads/main enforces (2) on its own for a job with no environment: binding: no environment, no policy needed, the ref check is the restriction. environment:<name> subjects enforce neither (2) nor (3) on their own, because that OIDC sub is ref-agnostic: it encodes which environment the job bound to, not which branch triggered the run. A workflow_dispatch fired from any branch against an environment-bound job presents the identical subject a main run would. So allowlisting environment:staging admits a staging deploy dispatched from an arbitrary branch, exactly what "only main deploys" is supposed to rule out. The only control that reattaches the branch requirement is a deployment branch policy (custom_branch_policies, restricted to main) configured per-environment on GitHub's side.

Live state, checked directly against the GitHub API just now (gh api repos/LeanerCloud/CUDly/environments), not assumed from an earlier issue's snapshot:

Environment Exists today? Protection rules Deployment branch policy
dev Yes none none
staging No n/a n/a
prod No n/a n/a
aws-fargate-dev Yes none none
aws-fargate-staging Yes none none
aws-fargate-prod No n/a n/a
aws-db-{dev,staging,prod} No, all three n/a n/a
aws-lambda-{dev,staging,prod}-rollback No, all three n/a n/a
aws-fargate-{dev,staging,prod}-rollback No, all three n/a n/a

Only 3 of the 15 environments this allowlist names exist yet (dev, aws-fargate-dev, aws-fargate-staging), and none of those 3, including dev which live deploy jobs already use today, has a branch policy or protection rules of any kind. Until (2) is configured on every environment above, and the 12 that don't exist yet are created, this allowlist delivers "only these environments deploy, from any branch", not "only main deploys". That is the honest characterization; I am not calling this "main-only" anywhere in this PR.

Correction from the previous revision of this table: an earlier pass here said "3 environments exist" without qualifying it against this allowlist's 15 names, which read as (and was pointed out to be) a different, broader claim than intended. The account has 5 environments in total; the table above covers the 3 that overlap with this allowlist. The other 2, azure- and gcp- (malformed, trailing-hyphen names), are unrelated to this PR: investigated and confirmed dead code (root-caused to a push-triggered job in deploy-azure.yml/deploy-gcp.yml at commit cb386249e, interpolating an empty inputs.environment; fixed by a later refactor, no deployment against either since 2026-03-25) and structurally unreachable by any job that assumes cudly_deploy (wrong cloud, wrong role, wrong action entirely). Filed as its own issue, #1684, cross-linked to #1660 rather than folded into this PR.

No job that assumes cudly_deploy can bind to an empty-suffix environment, so applying this allowlist is safe on that axis. Every compound name any AWS-role job currently produces carries a non-empty prefix segment or a suffix after the interpolation point (aws-fargate-, aws-db-, aws-lambda-...-rollback, aws-fargate-...-rollback), never a bare aws- or a name that could collapse to something outside the 16 entries above given the type: choice constraint on every reachable input. rollback.yml's gcp-*-rollback (google-github-actions/auth) and azure-*-rollback (azure/login) jobs, and database-migration.yml's migrate-gcp/migrate-azure jobs (same two actions), authenticate to their own clouds, not this role; only rollback.yml's two aws-*-rollback jobs and database-migration.yml's migrate-aws job use vars.AWS_ROLE_TO_ASSUME, confirmed by re-reading every credentialed step, not by pattern-matching names.

Deliverable is therefore two halves:

Half What Who Status
Code (control 1) role.tf's allowlist, exactly the subjects credentialed jobs present this PR done
Manual (controls 2 and 3) Create the 12 missing environments; set a deployment branch policy limited to main on all 15; set required reviewers where warranted privileged human, GitHub Settings -> Environments (no GitHub Terraform provider exists in this repo to do any of this declaratively, confirmed by grep: no provider "github" / github_repository_* resource anywhere under terraform/ or iac/; adding one is #1660's scope) not done

I did not solve the manual half in code, and I did not omit any of these environment entries to force branch-scoping by omission, that would just reproduce #1648: break the destroy and rollback jobs' authentication entirely. Landing the allowlist and stating the residual gap, per-environment, is the honest version of this change.

Method: evidence, not a list handed down

Grepped every workflow for role-to-assume: ${{ vars.AWS_ROLE_TO_ASSUME }} (the var that resolves to this role's ARN, confirmed via outputs.tf's description) rather than trusting any prior characterization of which jobs need what. 9 job-steps across 7 files assume this specific role. For each, traced the job's environment: binding to its concrete value(s) by reading the job's own resolution logic (a prepare job's if/elif chain, or a workflow_dispatch input's type: choice options:), not by assuming the binding is a literal.

An OIDC sub is either ...:ref:refs/heads/<branch> or ...:environment:<name>, never both, so a job's environment: binding replaces its ref-based subject. Every environment-bound job's exact subject has to be in the allowlist or that job cannot authenticate at all, independent of whether the environment has any protection rules (#1660 is a separate concern from the allowlist gap this PR closes).

The table

Workflow / job Trigger environment: binding Resolved subject(s) In allowlist before this PR?
deploy-aws-lambda.yml build-and-deploy, test-deployment push, workflow_dispatch, release, workflow_call ${{ needs.prepare.outputs.environment }} dev, staging, prod (prepare job: releaseprod, workflow_dispatch/workflow_callinputs.environment choice, else→dev) Yes
deploy-aws-fargate.yml deploy workflow_dispatch, workflow_call aws-fargate-${{ needs.prepare.outputs.environment }} aws-fargate-dev, aws-fargate-staging, aws-fargate-prod No
database-migration.yml migrate-aws workflow_dispatch, workflow_call aws-db-${{ inputs.environment }} aws-db-dev, aws-db-staging, aws-db-prod (workflow_dispatch path only, see below) No
rollback.yml rollback-aws-lambda workflow_dispatch aws-lambda-${{ inputs.environment }}-rollback aws-lambda-dev-rollback, aws-lambda-staging-rollback, aws-lambda-prod-rollback No
rollback.yml rollback-aws-fargate workflow_dispatch aws-fargate-${{ inputs.environment }}-rollback aws-fargate-dev-rollback, aws-fargate-staging-rollback, aws-fargate-prod-rollback No
cleanup-staging.yml destroy-aws-lambda, destroy-aws-fargate workflow_dispatch none on current main; staging once #1674 merges ref:refs/heads/main today, environment:staging after #1674 Yes (both forms)
destroy-fargate-dev.yml destroy workflow_dispatch none on current main; dev once #1674 merges ref:refs/heads/main today, environment:dev after #1674 Yes (both forms)

12 new entries added: aws-fargate-{dev,staging,prod}, aws-db-{dev,staging,prod}, aws-lambda-{dev,staging,prod}-rollback, aws-fargate-{dev,staging,prod}-rollback. Existing 4 entries (ref:refs/heads/main, environment:{dev,staging,prod}) kept unchanged. Total 16.

inputs.environment on every workflow_dispatch above is a type: choice with options: [dev, staging, prod] (checked each one individually, not assumed uniform), so every compound value is exhaustively enumerable, no StringLike pattern needed or used. Confirmed the deploy-all.yml orchestrator, the only caller of deploy-aws-fargate.yml/deploy-aws-lambda.yml via workflow_call, is itself constrained the same way (its own environment input is type: choice options: [dev, staging, prod], and its release-triggered path hardcodes prod), so the workflow_call trigger on those two files can't introduce a value outside the enumerated set either.

Two judgment calls, made explicitly

1. pull_request is excluded, not forgotten, and is now dropped as a matter of policy, not just because nothing needs it today. Checked whether any job assuming this role (cudly_deploy) runs on pull_request: none does. aws_sanity.yml is the only pull_request-triggered workflow calling configure-aws-credentials, and it uses role-to-assume: ${{ secrets.AWS_CICD_READONLY_ROLE_ARN }}, a separate, already-scoped-down role, confirmed by grep (role-to-assume appears in aws_sanity.yml exactly once, with that secret, not vars.AWS_ROLE_TO_ASSUME). Given the owner's "only main may deploy" constraint, pull_request would be excluded regardless of what I'd found; the wildcard's over-permission of it today is exactly the hole this allowlist exists to close, not a capability to carry forward. For the record: GitHub does not issue OIDC tokens to pull_request-triggered runs from forked repositories by default, independent of this trust policy. I could not find the readonly role's own trust policy anywhere under terraform/, so I can't speak to its scoping; out of scope for this PR since it's a different role entirely.

2. No StringLike patterns. Every value in the table above is a StringEquals exact match, drawn from a finite, enumerated set. The temptation with 16 entries is to collapse aws-fargate-*-shaped names into a pattern; resisted it, because a StringLike in a trust policy is exactly the substring-standing-in-for-equality shape that produced four separate defects elsewhere in this codebase today (#1544/#1667's GCP WIF mapping/condition bypasses, the AWS user-path contains() bypass in #1667's Terraform sibling). A trust policy is the worst place to add a fifth. If a pattern is ever proposed here later, it should have to prove what it admits beyond the intended set, which none of the current 16 values need.

Deliberately not covered, and why

database-migration.yml's workflow_call trigger declares environment as an unconstrained type: string (not choice), unlike its workflow_dispatch path (type: choice, options: [dev, staging, prod], cannot be empty). Grepped .github/workflows/ for uses:.*database-migration.yml: zero hits. Nothing in this repo calls it via workflow_call today, so that path is unreachable and its arbitrary-string environment value was not enumerated (there's nothing to enumerate, and adding a wildcard to pre-cover a hypothetical future caller would be exactly the kind of unjustified widening this PR argues against elsewhere). If a caller is ever added, its environment: binding needs an explicit new entry here before it can authenticate.

This is not a hypothetical failure shape, it is the concrete one: a workflow_call caller passing environment: "" would produce aws-db- on migrate-aws, the job at database-migration.yml:170/190 that does assume cudly_deploy. That is the exact same empty-interpolation defect class that produced the azure-/gcp- junk environments (#1684), just on a job that would authenticate to AWS instead of one that couldn't reach this role at all. migrate-gcp (:246/264, google-github-actions/auth) and migrate-azure (:313/331, azure/login) use their own clouds' credentials, not this role, same split confirmed for rollback.yml's GCP/Azure jobs below. Flagged in both role.tf's comment and the README so it doesn't silently rot into a "why doesn't this work" surprise later.

Also fixed: stale README

terraform/environments/aws/ci-cd-permissions/README.md's "Trust policy conditions" section said "The trust policy allows only workflows from repo:LeanerCloud/CUDly:*", describing the wildcard, not what role.tf has actually specified since b876d7fa0 (predates this PR). Directly adjacent to what I'm changing and actively misleading about what the committed policy does, so corrected it to describe the enumerated allowlist, added the "does not restrict to main by itself" section with the same environment list as above, and cross-linked #1648/#1660/#1674 rather than duplicating their content.

Verification

  • terraform fmt -check -diff on terraform/environments/aws/ci-cd-permissions/: exit 0.
  • terraform init -backend=false && terraform validate: exit 0, "Success! The configuration is valid."
  • pre-commit run on both changed files (both commits): exit 0, all applicable hooks passed (Terraform format/validate/lint, Trivy config scanner, secret scanners, Markdown lint); no hook skipped via --no-verify or similar.
  • Computed the resulting trust policy's compact JSON size with the full 16-entry list: 1234 characters, comfortably under AWS's 2048-character default limit for role trust policies (no quota increase needed).
  • No GitHub provider / github_repository_environment resource found anywhere under terraform/ or iac/ (grep), confirming the manual-half table above is accurate: these environments and their branch policies cannot be created declaratively in this repo today.
  • Three review passes on the diff across both commits; two corrections made: a comment conflated this issue (fix(iac/aws): rollback environments are absent from the OIDC trust allowlist, so AWS/Azure rollback cannot authenticate #1648) with sec(ci): deployment environments have no protection rules, so environment-bound credentialed jobs run unapproved #1660's separate "environments need protection rules" scope, corrected to cross-link the right issue for the right claim; and the ref-agnostic branch-scoping gap itself, added after the owner's follow-up constraint, not present in the first commit.

What this does not do

Does not touch Azure or GCP trust policies (separate concerns, separate files). Does not provision the GitHub environments themselves, their protection rules, or their deployment branch policies (#1660, and the manual half above). Does not apply anything to live AWS (bootstrap-only, manual). Does not add a reviewer gate; adding an entry here only lets an already environment-bound job authenticate, it says nothing about whether that environment has required reviewers. Does not, on its own, deliver "only main deploys" for environment-bound jobs; see the constraint section above.

Summary by CodeRabbit

  • Documentation

    • Expanded deployment authorization documentation with clearer guidance on trusted workflow environments, branch rules, forks, and new deployment jobs.
    • Added explanations of expected authorization behavior when new environments or workflows are not configured in advance.
  • Chores

    • Updated deployment permissions to explicitly support approved development, staging, production, rollback, database, and container deployment environments.

… cudly_deploy

The AWS deploy role's trust policy sub allowlist (role.tf) covered only
ref:refs/heads/main and the three bare environment:{dev,staging,prod}
subjects. Live AWS still runs the pre-b876d7fa0 wildcard
(repo:LeanerCloud/CUDly:*), so that allowlist was never applied. Tightening
it as committed would immediately break every job whose environment binding
isn't in it.

Enumerated every workflow job that assumes this role (role-to-assume:
${{ vars.AWS_ROLE_TO_ASSUME }}, 9 job-steps across 7 files) and traced each
environment: binding to its concrete value. Twelve subjects were missing:
deploy-aws-fargate.yml (aws-fargate-{dev,staging,prod}), database-migration.yml
(aws-db-{dev,staging,prod}), and rollback.yml's two AWS jobs
(aws-{lambda,fargate}-{dev,staging,prod}-rollback).

pull_request is deliberately not added: the only pull_request-triggered
workflow that calls configure-aws-credentials (aws_sanity.yml) assumes a
separate read-only role, not this one. database-migration.yml's
workflow_call path takes an unconstrained string environment input, but
nothing in this repo calls it that way today, so it's excluded and flagged
rather than covered with a wildcard.

Also corrects ci-cd-permissions/README.md's "Trust policy conditions"
section, which still described the wildcard rather than the allowlist
role.tf has actually specified since b876d7f.

Not applied here, this module is bootstrap-only, applied manually by a
privileged human. Refs #1648, #1674.
@cristim cristim added triaged Item has been triaged priority/p1 Next up; this sprint severity/high Significant harm urgency/this-sprint Within the current sprint impact/internal Team-internal only effort/s Hours type/bug Defect labels Jul 29, 2026
@coderabbitai

coderabbitai Bot commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

The AWS deployment role now explicitly allows additional GitHub Actions environment OIDC subjects for Fargate, database, and rollback workflows. The README documents the subject formats, exclusions, repository configuration, and required allowlist updates for new jobs or environments.

Changes

AWS OIDC trust policy

Layer / File(s) Summary
Expand OIDC subject allowlist and documentation
terraform/environments/aws/ci-cd-permissions/role.tf, terraform/environments/aws/ci-cd-permissions/README.md
The trust policy adds explicit Fargate, database, and Lambda/Fargate rollback environment subjects. Documentation describes the subject formats, exclusions, and required updates for new environment-assuming jobs.

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related issues

Possibly related PRs

  • LeanerCloud/CUDly#1602 — Also modifies AWS IAM OIDC trust-policy handling for the token.actions.githubusercontent.com:sub condition.
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the main change: expanding the AWS OIDC trust allowlist for the cudly_deploy role.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch sec/1648-aws-oidc-allowlist

Comment @coderabbitai help to get the list of available commands.

…citly

The owner constraint on this repo is that only main may deploy. ref:refs/heads/main
in the allowlist enforces that for unbound jobs, but environment:<name> subjects
are ref-agnostic: a workflow_dispatch from any branch against an environment-bound
job presents the identical subject a main run would. Reattaching the branch
requirement needs a deployment branch policy per environment, which none of the
15 environments this allowlist names currently has (several don't exist yet),
and this module has no GitHub provider to configure that declaratively.

Makes this explicit in role.tf's comment and adds a README section naming every
environment that needs the policy, so the allowlist isn't read as delivering
"only main deploys" before that manual step happens.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
terraform/environments/aws/ci-cd-permissions/role.tf (1)

74-105: 🔒 Security & Privacy | 🔵 Trivial

New environments have no protection rules — self-service credential minting.

The comment at Line 96-99 already flags that aws-lambda-*-rollback/aws-fargate-*-rollback (and, by the same reasoning, the new aws-fargate-*/aws-db-* entries) are auto-created bare with no reviewer gate. GitHub's own guidance backs treating this as a real gap, not just documentation: When environments are used in workflows or in OIDC policies, we recommend adding protection rules to the environment for additional security. For example, you can configure deployment rules on an environment to restrict which branches and tags can deploy to the environment or access environment secrets. Until #1660 lands, anyone able to trigger these workflow_dispatch jobs can self-mint credentials for cudly_deploy with no human approval step — worth confirming that's an acceptable interim risk before this bootstrap change is manually applied.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@terraform/environments/aws/ci-cd-permissions/role.tf` around lines 74 - 105,
Update the Terraform change around the rollback, Fargate, and database
environment subjects to avoid enabling self-service credential minting for
environments without protection rules. Before applying these new OIDC subjects,
obtain explicit confirmation that the interim risk is acceptable, or
defer/remove the entries until the corresponding GitHub environment protection
rules are provisioned by `#1660`. Preserve only subjects backed by approved,
protected environments.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@terraform/environments/aws/ci-cd-permissions/role.tf`:
- Around line 74-105: Update the Terraform change around the rollback, Fargate,
and database environment subjects to avoid enabling self-service credential
minting for environments without protection rules. Before applying these new
OIDC subjects, obtain explicit confirmation that the interim risk is acceptable,
or defer/remove the entries until the corresponding GitHub environment
protection rules are provisioned by `#1660`. Preserve only subjects backed by
approved, protected environments.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 7f400399-78bb-475a-91e8-6959ee889ec9

📥 Commits

Reviewing files that changed from the base of the PR and between 6ded401 and 95cd177.

📒 Files selected for processing (2)
  • terraform/environments/aws/ci-cd-permissions/README.md
  • terraform/environments/aws/ci-cd-permissions/role.tf

…plicitly

Team feedback: the previous pass stated that this allowlist does not restrict
deploys to main, but didn't name the three separate GitHub-side controls that
keep getting conflated across this repo's issues (allowlist membership,
deployment branch policy, required reviewers), and didn't say which of the
15 named environments exist yet.

Checked live via `gh api repos/LeanerCloud/CUDly/environments`: only 3 of 15
exist today (dev, aws-fargate-dev, aws-fargate-staging), none with any
protection rules or branch policy.

Adds the three-way distinction to role.tf's top comment and a short existence
note next to each entry group, and replaces the README section with the same
distinction plus a per-environment existence table.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/s Hours impact/internal Team-internal only priority/p1 Next up; this sprint severity/high Significant harm triaged Item has been triaged type/bug Defect urgency/this-sprint Within the current sprint

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant