CI: let the runner token be forwarded to a local reusable workflow - #759
Merged
Conversation
The runner-token policy required every mention of EC2_RUNNER_TOKEN to be
literally `github-token: ${{ secrets.EC2_RUNNER_TOKEN }}`, which forbade a
`secrets:` block outright. That made every EC2 start/stop job inline the
machulav action, and it is the reason the same nine-line block is copy-pasted
across tests.yml, cu130.yml, cu132.yml, publish.yml and nightly-publish.yml.
The prohibition was too blunt in one direction and not blunt enough in the
other. The rule's purpose (openvdb#672) is to keep the token's reachable surface
enumerable by grep, so that a PR cannot quietly move it somewhere it can escape
and have that land on main. But:
* `secrets: inherit` never spells the token's name, so it slipped through the
textual scan entirely -- including a forward to a *remote* reusable
workflow, which takes the token out of the repo with the check reporting OK;
* forwarding to a *local* reusable workflow is safe by the rule's own
reasoning. The callee is a file in .github/workflows/, so this same scan
covers it and rules 2-4 apply to it directly; under pull_request_target it
is base-controlled like every other workflow; and a caller job has no steps
and no workspace, so rule 4 ("no untrusted code beside the token") has
nothing to bite on.
So the checker rejected the careful form and accepted the blanket one. Make the
rule structural instead of textual:
* allow `EC2_RUNNER_TOKEN: ${{ secrets.EC2_RUNNER_TOKEN }}` in a job's
`secrets:` block, but only when that job's `uses:` is
`./.github/workflows/<name>.yml` and that file exists here;
* require the forwarded name to be the token's own, so a single
`git grep EC2_RUNNER_TOKEN` still enumerates every file that touches it;
* allow the matching `on.workflow_call.secrets` declaration on the callee;
* reject `secrets: inherit` anywhere (new rule 6);
* check every remaining mention structurally, so shapes that merely look like
a forward -- a workflow-level `env:`, a job-level `with:` -- are still
rejected.
Net effect is strictly tighter than before: it closes the `inherit` hole and
the remote-callee hole, and opens only the one shape this scan can actually
verify.
Signed-off-by: Mark Harris <mharris@nvidia.com>
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: Mark Harris <mharris@nvidia.com>
This was referenced Sep 3, 2026
matthewdcong
approved these changes
Sep 3, 2026
harrism
added a commit
that referenced
this pull request
Sep 3, 2026
…ckoff (#760) Replaces #758. CI keeps failing because us-east-2 has no capacity for the requested instance type at the moment the runner is requested. `machulav/ec2-github-runner` sweeps the availability zones in `availability-zones-config` within one attempt, but it never retries the sweep, so a transient region-wide shortage fails the whole run. Retrying twice, 90s and then 180s later, turns most of those into a slow pass instead of a red build. #758 inlined that retry ladder at every call site. That is ~30 lines of near-identical YAML in nine places that can drift independently, which is why this replaces it: the ladder goes in a reusable workflow instead. ## What changed Two new local reusable workflows hold the retry policy, the pinned action SHA, the AWS OIDC step and the teardown: * `.github/workflows/start-ec2-runner.yml` * `.github/workflows/stop-ec2-runner.yml` `tests.yml`, `cu130.yml`, `cu132.yml`, `publish.yml` and `nightly-publish.yml` now call them — nine start jobs and nine stop jobs. **401 lines of duplicated YAML removed, 171 added.** The reusable workflows absorb both call shapes the repo already had: * `runner-label` / `aws-resource-tags` for the matrix fan-outs in `publish.yml` and `nightly-publish.yml`, which need a predictable label because they find the instance again by tag at teardown. * `stagger-seconds`, replacing the hand-rolled `RANDOM % 16` sleep those same matrix jobs used to spread out runner registrations. `stop-ec2-runner.yml` takes the instance id directly when the caller has one from a start job's output, and otherwise resolves it from the `RunnerLabel` tag — the lookup the publish workflows had to do inline, because a matrix job has no single output to read. ## Two details worth a reviewer's eye * `aws-resource-tags` defaults to `'[]'`, not `''`. The pinned action does a bare `JSON.parse()` on that input ([`src/config.js`](https://github.com/machulav/ec2-github-runner/blob/343a1b2ae682e681c3cec9a235d882da17ff04ef/src/config.js#L39)) with no fallback, so an empty string throws and would break every caller that does not set tags. An empty `label`, by contrast, is explicitly fine — the action generates a unique one. * Retry semantics are unchanged from #758: only one attempt can succeed, since each is skipped unless every earlier one failed, so at most one of the three step outputs is non-empty and `||` picks it. ## Testing * `check_runner_token_policy.py` (with #759 applied): OK on all 17 workflow files. * Policy unit tests: 26 passed. * `actionlint` 1.7.7: clean. `zizmor` 1.26.1 `--persona=regular`: no findings. * Parsed every reusable-workflow caller job to confirm none carries a key incompatible with `uses:` (`runs-on`, `steps`, `env`, `container`, …). Because CI runs on `pull_request_target` from the base branch, this change cannot exercise itself on its own PR — the first real run of the new provisioning path is the one after merge. 🤖 Generated with [Claude Code](https://claude.com/claude-code) ## Update: retry-path fix folded in #759 has merged, so the Workflow Security gate now passes here. While confirming the ladder had not yet fired in production (it hasn't — every start since the merge succeeded on attempt 1), I audited the never-executed path and found a defect, now fixed in `c90f2d7`: The action publishes its outputs as soon as the instance launches, **before** waiting for the runner to register. So a "no capacity" failure leaves outputs empty, but a *registration* failure leaves them populated. Selecting outputs with `a1 || a2 || a3` takes the first **non-empty** value rather than the **successful** one — which in that second case hands back a dead label (dependent job queues until timeout instead of failing fast) and the wrong instance id (the retry's instance leaks). Fixed by selecting on `outcome`, and by stopping the instance a failed attempt left running before retrying. Verified against all five outcome combinations. Same fix for frc is openvdb/fvdb-reality-capture#330. Not a regression from this refactor — the inline version in #758 had the same `||` chain. The refactor is why the fix is one file rather than nine. --------- Signed-off-by: Mark Harris <mharris@nvidia.com> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Amends the EC2 runner-token policy (added in #672) so the token may be forwarded, by name, to a local reusable workflow — and rejects
secrets: inherit, which currently slips through.This unblocks #760, which removes ~400 lines of duplicated runner YAML. It is worth landing on its own, because it makes the policy strictly tighter than it is today regardless of what follows.
Why the current rule is both too strict and not strict enough
Rule 2 requires every mention of the token to be literally
github-token: ${{ secrets.EC2_RUNNER_TOKEN }}, which forbids asecrets:block outright. The rule's stated purpose is to keep the token's reachable surface enumerable by grep, so a PR cannot quietly move it somewhere it can escape and have that land onmain(the two-stage, time-of-merge attack described in #672). Measured against that purpose:secrets: inheritnever spells the token's name, so the textual scan is blind to the forwarding entirely. Forward to a remote reusable workflow (uses: other-org/repo/.github/workflows/x.yml@main) and the token leaves this repository with the check reporting OK. Verified againstmain: that workflow passes today..github/workflows/, so this same scan covers it and rules 2–4 apply to it directly; underpull_request_targetit is base-controlled like every other workflow; and a reusable-workflow caller job has nosteps:and no workspace, so rule 4's rationale ("no untrusted code beside the privileged context") has nothing to bite on.Put bluntly: the checker currently passes blanket
inheritand fails the explicit single-secret mapping — it rejects the more careful of the two.What changed
The rule becomes structural rather than textual:
EC2_RUNNER_TOKEN: ${{ secrets.EC2_RUNNER_TOKEN }}inside a job'ssecrets:block, only when that job'suses:is./.github/workflows/<name>.ymland that file exists in this directory (i.e. only when the callee is covered by this same scan).EC2_RUNNER_TOKENitself, so the token keeps one name across caller and callee and a singlegit grep EC2_RUNNER_TOKENstill enumerates every file that touches it.on.workflow_call.secretsdeclaration on the callee.secrets: inheritanywhere in a workflow.env:, a job-levelwith:— are still rejected.Net effect is strictly tighter than before: it closes the
inherithole and the remote-callee hole, and opens only the one shape this scan can actually verify.Testing
check_runner_token_policy.py: OK on all 17 workflow files.secrets: inheritrejected (local and remote), workflow-levelenv:rejected, callee declaration allowed.actionlint1.7.7 andzizmor1.26.1--persona=regular: clean.Reviewer note on CI signal
workflow-security.ymldeliberately runs the policy script and its tests from the trusted base checkout, overlaying only the PR's.github/workflowsas data. This PR touches only.github/scripts/, so the gate here exercises the old script against the old workflows and will go green without testing any of this. That is the design working as intended (a PR must not be able to edit the check that guards it) — the new code is exercised by thepush: mainrun after merge, and by the 26 tests above locally.@swahtz — this is your rule from #672, so flagging you directly.
🤖 Generated with Claude Code