Skip to content

sec(ci): main is unprotected while ref:refs/heads/main is a trusted OIDC subject for the AWS deploy role #154

Description

@cristim

Summary

main has no required-review protection, while repo:LeanerCloud/CUDly:ref:refs/heads/main is a trusted OIDC subject for the cudly-terraform-deploy AWS role. Anyone with write access can therefore push a workflow edit directly to main and mint AWS deploy credentials, with no review and no human gate anywhere in the chain.

This is the exposure that makes LeanerCloud/cloud-commitments-cli#1649's release-tag injection collapse in severity: the tag path grants strictly less than an actor with write access already holds. Recording it separately because it is larger than the bug LeanerCloud/cloud-commitments-cli#1657 fixes, and it is not addressed by any open PR.

Evidence

1. main is not protected. Classic branch protection:

$ gh api repos/LeanerCloud/CUDly/branches/main/protection
{"message":"Branch not protected", "status":"404"}

Checked with a token reporting "admin": true on the repo, so this is a genuine 404 and not an insufficient-scope artifact.

2. The one active ruleset does not require review. protect-master-branch is enforcement: active, but the rules it actually contributes to main are:

$ gh api repos/LeanerCloud/CUDly/rules/branches/main --jq '[.[]|.type]'
["deletion","non_fast_forward"]

That blocks deleting main and force-pushing to it. It does not require a pull request, an approving review, or status checks. The ruleset's name suggests it may have been written for a master branch that no longer exists.

3. No CODEOWNERS. No CODEOWNERS file at the repo root, in .github/, or in docs/. Nothing gives .github/workflows/** special review treatment.

4. main is a trusted OIDC subject. terraform/environments/aws/ci-cd-permissions/role.tf on main (cd4ee031f, after LeanerCloud/cloud-commitments-cli#1683) lists 16 subjects. The first is:

"repo:${var.github_repo}:ref:refs/heads/main",

The remaining 15 are environment:* subjects covering the deploy, DB-migration, fargate and rollback environment families. Only the ref:refs/heads/main entry is relevant here: a workflow job running on a push to main without an environment: binding presents sub = repo:LeanerCloud/CUDly:ref:refs/heads/main and matches it.

Reproduce the full list with:

git show origin/main:terraform/environments/aws/ci-cd-permissions/role.tf \
  | grep -n '"repo:${var.github_repo}'

Why it matters

The chain is: write access -> push a workflow to main -> job runs with id-token: write -> OIDC sub ref:refs/heads/main -> assume cudly-terraform-deploy -> full deploy permissions.

No step requires review, approval, or any second party.

Compounding factor (LeanerCloud/cloud-commitments-cli#1648): no GitHub Environment in this repo has required-reviewer protection configured. So the environment: bindings that scope the OIDC subject provide subject scoping but not a human gate. A release auto-deploys to prod with AWS credentials and nothing pauses it.

What this is NOT

Stating the limits explicitly, because several recent p0s in this backlog have had threat models that did not survive scrutiny:

  • This requires write access to the repository. It is an insider or compromised-credential exposure, not an anonymous or external one.
  • It is not remote code execution by an unauthenticated party, and not a data-exfiltration path on its own.
  • A fork pull request does not reach it: fork PRs do not receive id-token: write or repository secrets.
  • Severity is high rather than critical on that basis: real, unreviewed, and privilege-granting, but gated behind an already-trusted principal.

The honest framing is that this removes the second line of defence rather than the first. It means a single compromised contributor account, or a single bad merge, reaches production AWS with nothing in the way.

Suggested remediation

Not prescribing the exact policy, but the shape:

  1. Require pull requests plus at least one approving review on main (either as a ruleset rule or classic protection). The existing protect-master-branch ruleset appears to be the intended home and is currently near-empty for main.
  2. Add a CODEOWNERS entry for .github/workflows/** and terraform/environments/*/ci-cd-permissions/** so credential-granting configuration needs a named reviewer.
  3. Configure required reviewers on the prod Environment at minimum (this is LeanerCloud/cloud-commitments-cli#1648).
  4. Consider whether ref:refs/heads/main needs to remain a trusted subject at all, or whether every AWS-assuming job could bind an environment: instead, leaving only environment subjects in the trust policy. See the caveat in fix(iac/aws): extend OIDC trust allowlist to cover every job assuming cudly_deploy cloud-commitments-cli#1683 before narrowing further.

Related

Verification method

All four evidence points are reproducible read-only:

gh api repos/LeanerCloud/CUDly/branches/main/protection
gh api repos/LeanerCloud/CUDly/rulesets --jq '.[]|{name,target,enforcement}'
gh api repos/LeanerCloud/CUDly/rules/branches/main --jq '[.[]|.type]'
ls CODEOWNERS .github/CODEOWNERS docs/CODEOWNERS
sed -n '20,40p' terraform/environments/aws/ci-cd-permissions/role.tf

Activity

  1. cristim commented on Aug 3, 2026

    @cristim
    MemberAuthor

    Correction to evidence point 4: the original text quoted a 4-entry subject allowlist. That was read from role.tf at a stale commit (before LeanerCloud/cloud-commitments-cli#1683 landed). On main (cd4ee031f) the list has 16 entries. Body updated.

    This does not change the issue: repo:${var.github_repo}:ref:refs/heads/main is still present, and the finding is about main being an unreviewed and trusted subject, not about the size of the list.

  2. cristim commented on Aug 3, 2026

    @cristim
    MemberAuthor

    Verified your premise against live repo settings, since the rest of this issue depends on it.

    main is unprotected in the sense that matters. There is no classic branch protection at all:

    GET /repos/LeanerCloud/CUDly/branches/main/protection  ->  404 Branch not protected
    

    One ruleset is active and it does cover main (its name is protect-master-branch, but its condition is ~DEFAULT_BRANCH, and the default branch is main, so the misleading name is not itself the bug). Its complete rule set is:

    GET /repos/LeanerCloud/CUDly/rules/branches/main  ->  deletion, non_fast_forward
    

    So force-pushes and deletion are blocked, and that is all. No required pull request, no required status checks, no required reviews, no bypass restrictions. Anyone with write access can push directly to main with failing CI.

    Combined with ref:refs/heads/main being allowlist entry 1 in the cudly-terraform-deploy trust policy, that makes repo write access sufficient to obtain the deploy role, without review and without green checks.

    I have separately verified that the deploy role can then create a new role and attach AdministratorAccess to it, with no permissions boundary and no Deny covering that path, and re-triaged #1705 to p0 with the evidence. These two issues are one chain: this issue is the cheap entry, LeanerCloud/cloud-commitments-cli#1705 is the escalation. Fixing either one breaks it, but they should be assessed together rather than independently.

    One note on scope for whoever fixes this: adding a required-status-checks rule changes the merge path for every open PR in the repo, so it wants sequencing rather than a drive-by edit. Also worth confirming whether the ruleset should simply be renamed while it is being edited, since protect-master-branch on a main repo invites exactly the misreading I had to rule out here.

  3. cristim commented on Aug 6, 2026

    @cristim
    MemberAuthor

    Implemented. Ruleset 16369673 updated — main now requires a pull request and passing status checks.

    Before

    $ gh api repos/LeanerCloud/CUDly/rules/branches/main --jq "[.[]|.type]"
    ["deletion","non_fast_forward"]
    

    Force-push and deletion blocked; nothing else. Direct pushes with red CI were permitted.

    After

    rules on main:   deletion, non_fast_forward, pull_request, required_status_checks
    required checks: CI Success, Run pre-commit hooks
    PR required, approvals=0
    

    Also renamed protect-master-branch -> protect-default-branch. The old name suggested it targeted a master branch that does not exist; its condition was always ~DEFAULT_BRANCH, so the name was misleading rather than wrong, and this issue called that out.

    Why these two required checks specifically

    CI Success is a pure aggregator: ci-success in ci.yml:594-608 declares needs: on all ten jobs (lint, unit, integration, docker-build, terraform-validate, security-scan, e2e, azure-role-parity, aws-iam-parity, gcp-secret-scope) with if: always(), and fails if any of them failed. Requiring the single aggregate covers all ten without pinning ten names that can be renamed independently.

    Both are unconditional. ci.yml and pre-commit.yml each trigger on pull_request: branches: [main] with no paths: filter, so both run on every PR. This mattered: a required check that does not run on some PRs blocks those PRs permanently.

    CodeRabbit is deliberately NOT required. Its check entry has been observed with both status and conclusion empty indefinitely, and separately reading success while its description said "Review rate limited". Requiring it would have blocked every merge. Review coverage is verified out-of-band by comparing the review objects commit_id` to the PR head, which is a SHA identity rather than a check state.

    strict_required_status_checks_policy is false: branches need not be rebased onto the tip before merging. With a multi-PR queue, strict mode forces a rebase-and-rerun after every merge.

    Verified after applying

    What this does NOT do — the open policy question

    required_approving_review_count is 0. A pull request is now mandatory and CI must pass, which closes "push a workflow edit straight to main with red CI". It does not add a human gate: one actor can still open a PR and merge it themselves.

    I left it at 0 deliberately rather than deciding for you, because raising it to 1 stops the automated merge flow currently landing this queue — there is no second reviewer to approve. Raise it with:

    gh api -X PUT repos/LeanerCloud/CUDly/rulesets/16369673 --input <ruleset.json>   # with count: 1
    

    bypass_actors is empty, so the rules apply to admins too. An admin can still edit the ruleset, so this is a speed bump rather than a lockout.

    Leaving this issue open pending your call on the approval count, since that is the part of the original concern ("no review, no human gate anywhere in the chain") that a count of 0 does not close.

    Relevant to LeanerCloud/cloud-commitments-cli#1705: this was the cheap half of that escalation chain. The precondition was repo write access plus an unprotected main; the second half is now materially harder.

  4. cristim commented on Sep 28, 2026

    @cristim
    MemberAuthor

    Verified against the split repo (LeanerCloud/cloud-commitments-platform)

    Re-checked this issue's premise against the live repo this issue now lives
    in, since the split created a new repository that does not inherit the old
    monorepo's branch protection or rulesets.

    $ gh api repos/LeanerCloud/cloud-commitments-platform/branches/main/protection
    {"message":"Branch not protected","status":"404"}
    
    $ gh api repos/LeanerCloud/cloud-commitments-platform/rulesets
    []
    
    $ gh api repos/LeanerCloud/cloud-commitments-platform/rules/branches/main
    []
    

    So the issue is confirmed, and actually worse than the pre-split state this
    issue originally described: the old monorepo (LeanerCloud/CUDly) had a
    protect-master-branch -> protect-default-branch ruleset requiring a PR
    and passing status checks (see this issue's own history). The new split repo
    has zero rulesets and zero branch protection -- not even the
    "blocks force-push and deletion" floor the old repo started from before this
    issue was fixed there.

    terraform/environments/aws/ci-cd-permissions/role.tf's trust policy on
    main (current HEAD) still includes repo:${var.github_repo}:ref:refs/heads/main
    as a trusted OIDC subject, and (per #2) the live AWS/Azure/GCP trusts still
    name the old monorepo, so no branch in this repo can currently assume that
    role at all -- but that is a separate, orthogonal gap tracked in #2, not a
    reason to leave main unprotected. Once #2's bootstrap re-apply lands, this
    issue's chain (write access -> push to main -> job runs unreviewed ->
    mints deploy credentials) becomes live again immediately unless this is
    fixed first or in the same window.

    This is a repo-admin action, not something I can fix via API

    Per this repo's CLAUDE.md: "Repo settings (branch rulesets, environment
    protection rules, secrets) are admin actions. Don't try to change them via
    the API." Branch protection / rulesets fall squarely in that category, so I
    am not applying the fix myself -- flagging it here with the exact commands a
    privileged human should run, mirroring what was already done successfully on
    the old repo (see the "Implemented" comment above on this same issue,
    against LeanerCloud/CUDly).

    Suggested remediation (exact settings)

    1. Require a pull request and passing status checks on main. The
      precedent from the old repo's fix:

      # Create (or update) a ruleset targeting the default branch with:
      #   rules: deletion, non_fast_forward, pull_request, required_status_checks
      #   required checks: "CI Success", "Run pre-commit hooks"
      #   (both are unconditional on every PR to main -- no `paths:` filter --
      #   so neither can permanently block a PR the way a filtered check would)
      #   strict_required_status_checks_policy: false (no forced rebase-before-merge)
      #   bypass_actors: [] (rules apply to admins too)
      gh api -X POST repos/LeanerCloud/cloud-commitments-platform/rulesets \
        --input ruleset.json

      required_approving_review_count is a genuine open policy question, not
      a default to pick silently: 0 closes "push straight to main with red
      CI" but does not add a second-party human gate ("one actor opens a PR and
      merges it themselves" stays possible). The old repo's fix deliberately
      left it at 0 and left the issue open pending that call -- worth the same
      explicit decision here rather than copying a number.

    2. Add a CODEOWNERS entry for .github/workflows/** and
      terraform/environments/*/ci-cd-permissions/** so credential-granting
      configuration needs a named reviewer. I am not adding this file myself:
      it needs a real GitHub username or team as the owner, and guessing one
      risks assigning review responsibility to the wrong person silently. Once
      review is required (step 1), a human should add this with the correct
      owner(s).

    3. This is sequenced with sec(iac): rename-proof OIDC deploy trust + branch-restricted environments for the split platform repo #2 and sec(ci): deployment environments have no protection rules, so environment-bound credentialed jobs run unapproved #141: protecting main narrows who
      can reach the ref:refs/heads/main OIDC subject, but that subject and
      the environments it's paired with also need sec(iac): rename-proof OIDC deploy trust + branch-restricted environments for the split platform repo #2's bootstrap re-apply
      (rename-proof trust, this repo's name) and sec(ci): deployment environments have no protection rules, so environment-bound credentialed jobs run unapproved #141's environment protection
      rules before the full chain this issue describes is actually closed.

    Labeling status/blocked since the fix is a repo-settings change outside
    what I can apply.

  5. cristim commented on Oct 4, 2026

    @cristim
    MemberAuthor

    Applied on 2026-10-04.

    Created ruleset 24469860 protect-default-branch on this repo as a copy of cloud-commitments-cli's: main now requires a pull request (0 approvals) and the checks CI Success and Run pre-commit hooks, blocks deletion and force-push, and has no bypass actors.

    $ gh api repos/LeanerCloud/cloud-commitments-platform/rules/branches/main --jq '[.[].type]|sort'
    ["deletion","non_fast_forward","pull_request","required_status_checks"]
    

    Both required checks report on PRs here, so merging is not deadlocked.

    Limits: 0 required approvals and one admin, so this means "PR plus green CI", not "reviewed". Same trade-off as the cli repo. Not closing; leaving that to the owner.

  6. cristim commented on Oct 7, 2026

    @cristim
    MemberAuthor

    Re-verified the residual gap against live repo settings on 2026-10-07 (worker cc-platform-w4, claim).

    Remediation item 1 (PR + checks on main) is live: ruleset 24469860 protect-default-branch, enforcement: active, contributing [deletion,non_fast_forward,pull_request,required_status_checks] to main, bypass_actors: [].

    What still keeps this open, all of it an owner decision rather than a code change:

    1. required_approving_review_count: 0 and require_code_owner_review: false. So the current posture is "PR plus green CI", not "reviewed".
    2. No CODEOWNERS file exists at the repo root, .github/ or docs/, so .github/workflows/** and terraform/environments/*/ci-cd-permissions/** have no named reviewer.
    3. No Environment has required reviewers: all 14 (prod, staging, dev, {aws,azure,gcp}-{db,fargate}-{dev,staging,prod}) report 0 required_reviewers and 0 wait_timer protection rules. Tracked as LeanerCloud/cloud-commitments-cli#1648.

    Parked, not closed: the owner already declined to close this on 2026-10-04 with the same limits noted. Item 1's approval count and item 3 are owner decisions, so a worker cannot land them. The CODEOWNERS file (item 2) is the only repo-local piece and is safe to add without changing merge behaviour while require_code_owner_review stays false.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions