Skip to content

fix(ci): eliminate zero-click fork-PR RCE in terraform workflows - #1266

Merged
LeonardCareer merged 2 commits into
mainfrom
fix-msrc-127928-fork-pr-rce
Jul 27, 2026
Merged

fix(ci): eliminate zero-click fork-PR RCE in terraform workflows#1266
LeonardCareer merged 2 commits into
mainfrom
fix-msrc-127928-fork-pr-rce

Conversation

@LeonardCareer

@LeonardCareer LeonardCareer commented Jul 27, 2026

Copy link
Copy Markdown
Collaborator

Fixes MSRC #127928 / IcM 31000000665395.

Root cause

terraform-validation.yml was reachable from fork PRs and combined three capabilities that together enabled zero-click RCE and Azure-token minting under AZURE_MI:

  1. permissions: id-token: write on a pull_request-triggered job (added in Update CI Validations #327) — fork PRs are not stripped of OIDC minting.
  2. terraform test step (added in Add variable to customize AKS os disk type for all node pools #530) — defaults to command = apply and executes attacker-controlled HCL from the PR (null_resource + provisioner "local-exec").
  3. AWS creds gate removed in Remove aws creds from CI check #1135terraform test now runs on any fork PR without needing any secrets.

Plus an Entra federated identity credential on AZURE_MI (= telescope-github-oidc, sub AKS Airlock, rg telescope) trusting sub=repo:Azure/telescope:pull_request, which GitHub emits identically for same-repo and fork PRs — closing the chain.

Changes

.github/workflows/terraform-validation.yml (pull_request):

  • Remove permissions: id-token: write (both jobs).
  • Remove terraform test steps (AWS + Azure).
  • Remove azure/login step.
  • Remove the setup-matrix and terraform-plan jobs (moved).
  • Add top-level permissions: contents: read.
  • Use terraform init -backend=false to prevent backend-driven egress.
  • Keeps only fmt / validate / tflint — safe for any fork PR.

.github/workflows/terraform-integration.yml (new, pull_request_target):

  • Every PR (same-repo AND fork) is gated by an authorize job that uses the terraform-privileged environment. Required reviewers = the @Azure/telescope-oncall team. A compromised telescope-write account is part of the threat model, so same-repo PRs are not auto-approved.
  • terraform-test, setup-matrix, terraform-plan all needs: authorize and check out github.event.pull_request.head.sha from the PR's head repo only after human approval.
  • azure/login@v1@v2 (v1 is deprecated).
  • Fix pre-existing matrix_combinations double-assignment bug; simplify Create JSON Input to read the file directly via jq -c … "$FILE".

Out-of-band actions

1. Org admin — create team @Azure/telescope-oncall

Members: @wonderyl, @liyu-ma, @LeonardCareer, @xinWeiWei24, @vittoriasalim. Grant the team Write access on Azure/telescope.

2. Repo admin — create terraform-privileged environment

3. Tenant admin — update federated credentials on telescope-github-oidc

Managed Identity: telescope-github-oidc (client-id faa8d4b3-7b17-4588-bef2-95f0fb74abc5), subscription AKS Airlock (137f0351-8235-42a6-ac7a-6b46be2d21c7), resource group telescope.

Two new FICs already created (zero-downtime rollout):

Name Subject Status
gh-telescope-env-terraform-privileged repo:Azure/telescope:environment:terraform-privileged ✅ created
gh-telescope-env-terraform-privileged-numeric repository_owner_id:6844498:repository_id:718354598:environment:terraform-privileged ✅ created

Old FICs to be deleted after merge + validation:

Name Subject Action
gh-telescope-pr repo:Azure/telescope:pull_request 🗑 delete
gh-telescope-pr-numeric repository_owner_id:6844498:repository_id:718354598:pull_request 🗑 delete
az identity federated-credential delete -g telescope --identity-name telescope-github-oidc --name gh-telescope-pr --yes
az identity federated-credential delete -g telescope --identity-name telescope-github-oidc --name gh-telescope-pr-numeric --yes

Keep: gh-telescope-main and gh-telescope-main-numeric (needed for post-merge Actions on main).

Rollout order

  1. ✅ Add the two new FICs (done).
  2. Configure the team (§1) and the environment (§2).
  3. Merge this PR.
  4. Validate on the next Terraform-touching PR: terraform-validation runs green; terraform-integration / authorize pauses waiting for approval; after approve, azure/login succeeds.
  5. Delete the two old gh-telescope-pr* FICs. MSRC vulnerability is not considered closed until this step is done.
  6. Notify MSRC to re-test.

Copilot AI 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.

Pull request overview

This PR hardens the Terraform CI against fork-PR zero-click RCE by splitting Terraform checks into an unprivileged pull_request workflow (static validation only) and a privileged pull_request_target workflow that is gated behind a protected environment before any PR code is checked out and executed.

Changes:

  • Restricts .github/workflows/terraform-validation.yml to fmt/validate/tflint only, removes OIDC/azure login/terraform test/plan, and uses terraform init -backend=false.
  • Adds a new .github/workflows/terraform-integration.yml workflow that gates privileged terraform test/terraform plan behind an authorize job using the terraform-privileged environment.
  • Updates Azure login usage in the privileged workflow to azure/login@v2.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File Description
.github/workflows/terraform-validation.yml Removes privileged steps and limits fork-PR execution to static Terraform checks only.
.github/workflows/terraform-integration.yml Introduces an environment-gated privileged workflow for terraform test/plan and Azure auth.
Comments suppressed due to low confidence (2)

.github/workflows/terraform-integration.yml:75

  • Same as above: on pull_request_target, this checkout will fail for fork PRs unless you opt into unsafe PR checkout, and you should fetch from the fork repo explicitly when using the PR’s head SHA.
        uses: actions/checkout@v4
        with:
          ref: ${{ github.event.pull_request.head.sha }}
          fetch-depth: 0
          persist-credentials: false

.github/workflows/terraform-integration.yml:143

  • Same issue in this job: pull_request_target + fork PR checkout will fail without explicitly opting into unsafe PR checkout, and the fork repo should be specified when checking out github.event.pull_request.head.sha.
        uses: actions/checkout@v4
        with:
          ref: ${{ github.event.pull_request.head.sha }}
          persist-credentials: false

Comment thread .github/workflows/terraform-integration.yml
Comment thread .github/workflows/terraform-integration.yml Outdated
Comment thread .github/workflows/terraform-integration.yml Outdated
Fixes MSRC #127928 / IcM 31000000665395.

The `terraform-validation.yml` workflow was reachable from fork PRs and
combined three capabilities that together enabled zero-click RCE and
Azure-token minting under `AZURE_MI`:

  1. `permissions: id-token: write` on a `pull_request`-triggered job
     (added in #327) -- fork PRs are not stripped of OIDC minting.
  2. `terraform test` step (added in #530) -- defaults to `command = apply`
     and executes attacker-controlled HCL from the PR (`null_resource`
     + `provisioner "local-exec"`).
  3. AWS creds gate removed (#1135) -- `terraform test` now runs on any
     fork PR without needing any secrets.

Plus an Entra federated identity credential on `AZURE_MI`
(= `telescope-github-oidc`, sub `AKS Airlock`, rg `telescope`) trusting
`sub=repo:Azure/telescope:pull_request`, which GitHub emits identically
for same-repo and fork PRs -- closing the chain.

Changes:

* `.github/workflows/terraform-validation.yml` (`pull_request`):
  - Remove `permissions: id-token: write` (both jobs).
  - Remove `terraform test` steps (AWS + Azure).
  - Remove `azure/login` step.
  - Remove the `setup-matrix` and `terraform-plan` jobs (moved).
  - Add top-level `permissions: contents: read`.
  - Use `terraform init -backend=false` to prevent backend-driven egress.
  - Keeps only fmt / validate / tflint -- safe for any fork PR.

* `.github/workflows/terraform-integration.yml` (new,
  `pull_request_target`):
  - Every PR (same-repo AND fork) is gated by an `authorize` job that
    uses the `terraform-privileged` environment. Required reviewers on
    that environment = the `@Azure/telescope-oncall` team. A compromised
    `telescope-write` account is part of the threat model, so same-repo
    PRs are not auto-approved.
  - `terraform-test`, `setup-matrix`, `terraform-plan` all `needs:
    authorize` and check out `github.event.pull_request.head.sha` only
    after human approval.
  - `azure/login@v1` -> `@v2` (v1 is deprecated).
  - Fix pre-existing typo `['inlcude']` -> `['include']`, then simplify
    the null/empty check to avoid string-index on outputs.

Out-of-band actions required for this PR to be effective:

1. Org admin: create team `@Azure/telescope-oncall` with members:
     * @wonderyl
     * @liyu-ma
     * @LeonardCareer
     * @xinWeiWei24
     * @vittoriasalim
   Grant the team Write access on Azure/telescope.

2. Repo admin: create `terraform-privileged` environment in
   Settings -> Environments. Required reviewers = `@Azure/telescope-oncall`.

3. Tenant admin (Managed Identity Contributor on `telescope-github-oidc`,
   sub `AKS Airlock` = 137f0351-8235-42a6-ac7a-6b46be2d21c7, rg `telescope`):

     # Delete the two pull_request federated credentials:
     az identity federated-credential delete \
       -g telescope --identity-name telescope-github-oidc \
       --name gh-telescope-pr --yes
     az identity federated-credential delete \
       -g telescope --identity-name telescope-github-oidc \
       --name gh-telescope-pr-numeric --yes

     # Add the two environment-scoped federated credentials:
     az identity federated-credential create \
       -g telescope --identity-name telescope-github-oidc \
       --name gh-telescope-env-terraform-privileged \
       --issuer https://token.actions.githubusercontent.com \
       --subject 'repo:Azure/telescope:environment:terraform-privileged' \
       --audiences api://AzureADTokenExchange
     az identity federated-credential create \
       -g telescope --identity-name telescope-github-oidc \
       --name gh-telescope-env-terraform-privileged-numeric \
       --issuer https://token.actions.githubusercontent.com \
       --subject 'repository_owner_id:6844498:repository_id:718354598:environment:terraform-privileged' \
       --audiences api://AzureADTokenExchange

   Keep `gh-telescope-main` and `gh-telescope-main-numeric` intact
   (needed for post-merge Actions on main).

Merge order:
   Do actions 1-3 BEFORE merging this PR. Otherwise the next
   Terraform-touching PR will either (a) fail because the environment
   does not exist, (b) auto-pass because no reviewers are configured,
   or (c) fail at `azure/login` because the FIC subject does not match.
@LeonardCareer
LeonardCareer force-pushed the fix-msrc-127928-fork-pr-rce branch from 56885a3 to 287b609 Compare July 27, 2026 06:23
…x/JSON

- actions/checkout on pull_request_target must specify `repository`, else
  the PR head sha is looked up in the base repo and fork PRs fail.
  Added `repository: github.event.pull_request.head.repo.full_name` to all
  three PR-head checkouts.
- setup-matrix: drop the double-assignment / string-truncation of
  matrix_combinations; write the same value we log.
- terraform-plan: replace `echo $INPUT_JSON_OBJECT | jq` (shell splits +
  glob-expands the JSON) with `jq -c ... "$FILE"` directly.
@LeonardCareer
LeonardCareer merged commit 146bbe6 into main Jul 27, 2026
2 checks passed
@LeonardCareer
LeonardCareer deleted the fix-msrc-127928-fork-pr-rce branch July 27, 2026 23:47
LeonardCareer added a commit that referenced this pull request Jul 28, 2026
Follow-up to #1266 / MSRC #127928.

## Bug

The `authorize` job's `run:` line:
```yaml
run: echo "Authorized PR #${{ github.event.pull_request.number }} head=${{ github.event.pull_request.head.sha }}"
```

The `#` is interpreted as a **YAML comment**, so the shell only sees
`echo "Authorized PR ` — an unterminated double-quoted string — and
fails:
```
/home/runner/work/_temp/...sh: line 1: unexpected EOF while looking for matching `"'
##[error]Process completed with exit code 2.
```

That fails `authorize`, which skips `terraform-test` / `setup-matrix` /
`terraform-plan`.

## Fix

Wrap the whole `run:` value in single quotes and drop `#`:
```yaml
run: 'echo "Authorized PR ${{ github.event.pull_request.number }} head=${{ github.event.pull_request.head.sha }}"'
```

Scanned all workflows for the same pattern — no other occurrences.

## Verification

Reproduced in retest PR #1267 (authorize job
[30315458954](https://github.com/Azure/telescope/actions/runs/30315458954)):
- ✅ `Terraform Validation` on `pull_request` — success (static checks).
- ✅ `Terraform Integration` — paused waiting for `terraform-privileged`
approval, confirming the gate works.
- ❌ After approval, `authorize` failed with the shell-syntax error
above; downstream jobs skipped.

Once this merges I'll re-run the retest to confirm `authorize` →
`terraform-test` → `azure/login` succeeds end-to-end with the new
federated credential.

/cc @wonderyl @liyu-ma
LeonardCareer added a commit that referenced this pull request Jul 28, 2026
…rm-privileged environment (#1269)

Follow-up to #1266 / MSRC #127928.

## Two fixes

### 1. `terraform test` needs `terraform init` first

Before the split, `terraform-validation.yml` ran `terraform init &&
terraform validate` as one step, and the downstream `terraform test`
reused the resulting `.terraform/` directory as a side effect. After
moving `terraform test` into `terraform-integration.yml` (#1266), that
side effect is gone and `terraform test` fails with:

```
Error: Module not installed
Run "terraform init" to install all modules required by this configuration.
```

Fix: prepend `terraform init` to both AWS and Azure test steps.

### 2. Downstream jobs need `environment: terraform-privileged` to
change the OIDC subject

Currently only `authorize` has `environment: terraform-privileged`. The
other jobs (`terraform-test`, `terraform-plan`) present a JWT with
`subject=repo:Azure/telescope:pull_request` because that's the default
when a job has no environment. This only works today because the legacy
federated credential `gh-telescope-pr` is still on
`telescope-github-oidc` (retest run
[30317570861](https://github.com/Azure/telescope/actions/runs/30317570861/job/90146643975)
confirms the JWT subject).

**Once the two old FICs are removed to actually close MSRC #127928,
`azure/login` in these jobs would start failing.** Add `environment:
terraform-privileged` to both jobs so their JWT subject is
`environment:terraform-privileged`, matching the new FIC
`gh-telescope-env-terraform-privileged`.

## Approval prompts do NOT multiply

GitHub deduplicates environment approvals per `(workflow_run,
environment_name)`. `authorize`'s single approval covers all three jobs.
Verified in retest #1267 — after approving `authorize`, both
`terraform-test` and `terraform-plan` started immediately without a
second prompt (they just failed the login step due to the JWT subject
mismatch, which this PR fixes).

## Verification plan

1. Merge this PR.
2. Trigger a no-op retest PR (same shape as #1267).
3. Observe:
   - `authorize` waits for approval ✅ (same as before)
- After approve, `terraform-test` runs — its `azure/login` step's JWT
subject should now read `subject claim -
repo:Azure/telescope:environment:terraform-privileged` (or the numeric
equivalent), matching the new FIC.
   - `terraform-plan` matrix jobs the same.
4. Delete the two legacy FICs (`gh-telescope-pr`,
`gh-telescope-pr-numeric`).
5. Re-run one job — must still succeed. Confirms new FICs alone are
sufficient.
6. Close MSRC via retest.

/cc @wonderyl @liyu-ma
LeonardCareer added a commit that referenced this pull request Jul 28, 2026
…1270)

Follow-up to #1266 / #1269 / MSRC #127928 — approval-fatigue fix.

## Problem

With `max-parallel: 3` on `terraform-plan`, GitHub creates matrix jobs
in waves of 3. Each wave is a separate `terraform-privileged` deployment
request, so approving the environment covers only the current wave. For
~20 tfvars this means 7-8 approval clicks per push (see retest #1267 run
#4).

Additionally, the `authorize` job was a no-op placeholder that only
existed to attach `environment: terraform-privileged`. After #1269 both
`terraform-test` and `terraform-plan` carry that environment themselves,
so `authorize` no longer serves a purpose.

## Fix

Two changes:

1. **Delete `max-parallel: 3`.** All matrix jobs now pend simultaneously
as one batch. GitHub deduplicates approval per batch, so a single click
covers the entire matrix.

2. **Delete the `authorize` job** and the `needs: authorize` references
on `terraform-test`, `setup-matrix`, and `terraform-plan`. The approval
gate now lives directly on the jobs that actually use the OIDC token
(`terraform-test`, `terraform-plan`). Same security guarantee — an
environment approval is still required before any privileged step can
run — just one fewer job in the DAG.

## Runner concurrency

telescope has no self-hosted runners; workflows run on the Azure org's
shared GitHub-hosted Ubuntu pool. Azure is on GitHub Enterprise Cloud,
which caps Ubuntu concurrency at ~500 per org. With ~20 matrix jobs this
is well within budget; if org-wide contention ever exhausts the pool,
extra jobs simply queue behind the currently-approved batch — they don't
fail.

## Verification plan

After merge, trigger a Terraform-touching PR and confirm:
- All jobs (`terraform-test`, `setup-matrix`, `terraform-plan` matrix)
pend simultaneously
- One click on `terraform-privileged` approves the whole batch
- No further approval prompts appear as jobs finish

/cc @wonderyl @liyu-ma
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants