From 1a3e8109030253df3ae39c539fea5ad4edfae569 Mon Sep 17 00:00:00 2001 From: Mark Harris Date: Thu, 3 Sep 2026 02:06:19 +0000 Subject: [PATCH] CI: let the runner token be forwarded to a local reusable workflow 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 (#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/.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 Co-authored-by: Claude Opus 5 (1M context) Signed-off-by: Mark Harris --- .github/scripts/check_runner_token_policy.py | 237 +++++++++++++++--- .../scripts/test_check_runner_token_policy.py | 178 ++++++++++++- 2 files changed, 367 insertions(+), 48 deletions(-) diff --git a/.github/scripts/check_runner_token_policy.py b/.github/scripts/check_runner_token_policy.py index 83333f2bd..ad6a5467a 100755 --- a/.github/scripts/check_runner_token_policy.py +++ b/.github/scripts/check_runner_token_policy.py @@ -9,20 +9,43 @@ tooling under ``.github/scripts/`` (this enforcement script and its tests). It must not leak into product source, docs, etc. - 2. Every textual occurrence in a workflow must be EXACTLY the action input: - github-token: ${{ secrets.EC2_RUNNER_TOKEN }} - (whitespace inside ``${{ }}`` is tolerated). This single rule already - forbids putting the token in ``env:``, ``GH_TOKEN``/``GITHUB_TOKEN``, - ``with.token``, a ``run:`` script, or a reusable-workflow ``secrets:`` - block, because none of those match this pattern. - - 3. The step that consumes the token must ``uses: machulav/ec2-github-runner``. - - 4. A job that references the token must not pull untrusted code into its - workspace alongside the privileged context: no local actions + 2. Every textual occurrence in a workflow must be one of exactly three forms + (whitespace inside ``${{ }}`` is tolerated): + + github-token: ${{ secrets.EC2_RUNNER_TOKEN }} # the action input + EC2_RUNNER_TOKEN: ${{ secrets.EC2_RUNNER_TOKEN }} # caller forwarding + EC2_RUNNER_TOKEN: # callee declaration + + Each is a bare interpolation that is the *whole* value of a scalar key, so + this rule alone forbids the token in a ``run:`` script or in any + concatenated string. Which of the three a line is allowed to be is then + settled structurally by rules 3 and 4 -- e.g. an ``env:`` entry happens to + have the same shape as the forwarding form, and is rejected below. + + The forwarded name is required to be ``EC2_RUNNER_TOKEN`` itself, so the + token keeps one name across caller and callee and a single + ``git grep EC2_RUNNER_TOKEN`` still enumerates every file that touches it. + + 3. The token may be consumed in exactly two ways: + + a. by a *step* whose ``uses:`` is ``machulav/ec2-github-runner``, via its + ``github-token`` input; or + b. by a *job* whose ``uses:`` is a local reusable workflow + (``./.github/workflows/.yml``), via a ``secrets:`` mapping. + + (b) is safe precisely because the callee is itself a file in + ``.github/workflows/``: this same scan covers it, so rules 2-4 apply to it + directly, and under ``pull_request_target`` it is base-controlled like + every other workflow. Forwarding to anything else -- a remote reusable + workflow, or a callee that is not present in this directory -- would send + the token somewhere the scan cannot see, and is rejected. + + 4. A job that references the token *in a step* must not pull untrusted code + into its workspace alongside the privileged context: no local actions (``uses: ./...``) and no ``actions/checkout``. (Sibling ``run:`` steps are - fine -- rule 2 guarantees they can never reference the token, since it - only ever appears as the ``github-token`` input.) + fine -- rule 2 guarantees they can never reference the token.) A + reusable-workflow caller job under 3(b) has no ``steps:`` and no workspace + at all, so there is nothing for this rule to bite on. 5. No dynamic secret access (``secrets[...]``) anywhere in a workflow. Rules 1-4 are textual: they key on the literal name ``EC2_RUNNER_TOKEN``. Dynamic @@ -32,6 +55,12 @@ reject it outright (this check runs on every workflow, even ones that never mention the token by name). + 6. No ``secrets: inherit`` anywhere in a workflow. It forwards *every* secret, + including the admin token, without ever spelling a name -- so it is + invisible to rules 1-3 and would let a caller hand the token to a remote + reusable workflow with this scan reporting OK. Forward the one secret + explicitly instead (rule 3b), which is both narrower and checkable. + Usage: check_runner_token_policy.py [WORKFLOW_DIR] [--repo-root DIR] @@ -52,9 +81,25 @@ TOKEN_REF = f"secrets.{TOKEN_NAME}" ALLOWED_ACTION = "machulav/ec2-github-runner" -# The one and only allowed textual form, e.g. -# github-token: ${{ secrets.EC2_RUNNER_TOKEN }} -ALLOWED_LINE = re.compile(r"^\s*github-token:\s*\$\{\{\s*secrets\." + re.escape(TOKEN_NAME) + r"\s*\}\}\s*$") +# Rule 2's three allowed textual forms. Each is a bare interpolation that is the +# whole value of a scalar key, so none of them can hide inside a `run:` script or +# a concatenated string. +_INTERP = r"\$\{\{\s*secrets\." + re.escape(TOKEN_NAME) + r"\s*\}\}" +# github-token: ${{ secrets.EC2_RUNNER_TOKEN }} -- the action input +ACTION_INPUT_LINE = re.compile(r"^\s*github-token:\s*" + _INTERP + r"\s*$") +# EC2_RUNNER_TOKEN: ${{ secrets.EC2_RUNNER_TOKEN }} -- caller forwarding +FORWARD_LINE = re.compile(r"^\s*" + re.escape(TOKEN_NAME) + r":\s*" + _INTERP + r"\s*$") +# EC2_RUNNER_TOKEN: -- callee declaration +DECLARE_LINE = re.compile(r"^\s*" + re.escape(TOKEN_NAME) + r":\s*$") +ALLOWED_LINES = (ACTION_INPUT_LINE, FORWARD_LINE, DECLARE_LINE) + +# The only `uses:` a job may have if it is forwarded the token (rule 3b): a +# reusable workflow in this repo's own .github/workflows, which this same scan +# therefore covers. The capture group is the callee's file name, checked to exist. +LOCAL_REUSABLE = re.compile(r"^\./\.github/workflows/([^/]+\.ya?ml)$") + +# Rule 6: blanket secret forwarding, which names nothing and so evades rules 1-3. +INHERIT_LINE = re.compile(r"^\s*secrets:\s*inherit\s*$") # Dynamic secret access, e.g. secrets['X'], secrets[matrix.y], secrets[format(...)]. # The \b avoids matching identifiers that merely end in "secrets" (e.g. mysecrets[0]). @@ -85,12 +130,45 @@ def iter_steps(job: dict): yield step +def _on_block(data: dict) -> dict: + """The workflow's trigger mapping. + + YAML 1.1 parses a bare ``on:`` key as the boolean ``True``, so accept both + spellings rather than depending on how the file happens to quote it. + """ + for key in ("on", True): + value = data.get(key) + if isinstance(value, dict): + return value + return {} + + +def _token_secret_declaration(data: dict) -> dict | None: + """The ``on.workflow_call.secrets`` mapping, if it declares the token. + + This is the callee side of rule 3b -- the one legitimate mention of the + token name outside a job. Returns the enclosing mapping so the caller can + remove the declaration before scanning what is left. + """ + call = _on_block(data).get("workflow_call") + if not isinstance(call, dict): + return None + secrets = call.get("secrets") + if isinstance(secrets, dict) and TOKEN_NAME in secrets: + return secrets + return None + + +def _mentions_token(value) -> bool: + return TOKEN_NAME in yaml.safe_dump(value) + + def check_workflow_file(path: Path, violations: list[str]) -> None: text = path.read_text() - # --- Rule 5: no dynamic secret access (runs on every workflow). ----------- - # Must run before the token-name early return below: the whole point is that - # dynamic indexing can reach the token without naming it. + # --- Rules 5 & 6: blanket / unnamed secret access (every workflow). ------- + # These must run before the token-name early return below: the whole point + # of both is that they can reach the token without ever naming it. for lineno, line in enumerate(text.splitlines(), start=1): if DYNAMIC_SECRET.search(line): fail( @@ -101,21 +179,34 @@ def check_workflow_file(path: Path, violations: list[str]) -> None: f"'secrets.', not 'secrets[...]' (it can resolve " f"'{TOKEN_NAME}' without naming it): {line.strip()!r}", ) + if INHERIT_LINE.match(line): + fail( + violations, + path, + None, + f"line {lineno}: 'secrets: inherit' is not allowed -- it forwards " + f"every secret, including '{TOKEN_NAME}', without naming one, so no " + f"textual check can see where the token goes. Forward the single " + f"secret explicitly: '{TOKEN_NAME}: ${{{{ secrets.{TOKEN_NAME} }}}}'.", + ) if TOKEN_REF not in text and TOKEN_NAME not in text: return - # --- Rule 2: every line mentioning the token must be the exact input. ----- + # --- Rule 2: every line naming the token must be one of the three forms. -- for lineno, line in enumerate(text.splitlines(), start=1): if TOKEN_NAME not in line: continue - if not ALLOWED_LINE.match(line): + if not any(rx.match(line) for rx in ALLOWED_LINES): fail( violations, path, None, f"line {lineno}: '{TOKEN_NAME}' may only appear as " - f"'github-token: ${{{{ secrets.{TOKEN_NAME} }}}}', got: {line.strip()!r}", + f"'github-token: ${{{{ secrets.{TOKEN_NAME} }}}}' (action input), " + f"'{TOKEN_NAME}: ${{{{ secrets.{TOKEN_NAME} }}}}' (forward to a local " + f"reusable workflow), or '{TOKEN_NAME}:' (workflow_call declaration), " + f"got: {line.strip()!r}", ) # --- Structural rules 3 & 4 via parsed YAML. ------------------------------ @@ -125,22 +216,73 @@ def check_workflow_file(path: Path, violations: list[str]) -> None: fail(violations, path, None, f"could not parse YAML: {exc}") return + # Outside of `jobs:`, the only legitimate mention is the workflow_call + # declaration; drop it and anything left over is a leak (workflow-level + # `env:`, a default, a trigger expression, ...). + declaration = _token_secret_declaration(data) + if declaration is not None: + declaration.pop(TOKEN_NAME) + outside_jobs = {k: v for k, v in data.items() if k != "jobs"} + if _mentions_token(outside_jobs): + fail( + violations, + path, + None, + "token referenced outside any job; the only legitimate mention there " + "is an 'on.workflow_call.secrets' declaration", + ) + jobs = data.get("jobs") or {} for job_name, job in jobs.items(): if not isinstance(job, dict): continue - job_text = yaml.safe_dump(job) - if TOKEN_NAME not in job_text: + if not _mentions_token(job): continue + # --- Rule 3b: forwarding to a local reusable workflow. --------------- + secrets_block = job.get("secrets") + forwarded = {} + if isinstance(secrets_block, dict): + forwarded = {k: v for k, v in secrets_block.items() if _mentions_token({k: v})} + + if forwarded: + renamed = sorted(k for k in forwarded if k != TOKEN_NAME) + if renamed: + fail( + violations, + path, + job_name, + f"token forwarded under a different name {renamed}; it must be " + f"forwarded as '{TOKEN_NAME}' so that a single " + f"'git grep {TOKEN_NAME}' still finds every file that touches it", + ) + uses = job.get("uses") or "" + callee = LOCAL_REUSABLE.match(uses) + if callee is None: + fail( + violations, + path, + job_name, + f"token forwarded via 'secrets:' to {uses or ''!r}; " + f"it may only be forwarded to a local reusable workflow " + f"('./.github/workflows/.yml'), which this same scan covers", + ) + elif not (path.parent / callee.group(1)).is_file(): + fail( + violations, + path, + job_name, + f"token forwarded to {uses!r}, which does not exist in " + f"{path.parent}; the callee must be a workflow this scan covers", + ) + + # --- Rule 3a: only the EC2 runner action may consume it in a step. ---- token_steps = [] for step in iter_steps(job): - step_text = yaml.safe_dump(step) - if TOKEN_NAME not in step_text: + if not _mentions_token(step): continue token_steps.append(step) - # Rule 3: the consuming step must be the EC2 runner action. uses = (step.get("uses") or "").split("@")[0] if uses != ALLOWED_ACTION: fail( @@ -153,9 +295,7 @@ def check_workflow_file(path: Path, violations: list[str]) -> None: # Rule 2 (structural backstop): only via the github-token input. with_block = step.get("with") or {} - offending = { - k: v for k, v in with_block.items() if k != "github-token" and TOKEN_NAME in yaml.safe_dump({k: v}) - } + offending = {k: v for k, v in with_block.items() if k != "github-token" and _mentions_token({k: v})} if offending: fail( violations, @@ -164,19 +304,30 @@ def check_workflow_file(path: Path, violations: list[str]) -> None: f"token passed via disallowed input(s): {sorted(offending)}", ) - if not token_steps: - # Token appears in the job but in no step (e.g. job-level env: or a - # reusable-workflow `secrets:` block). That is never allowed. + # Anything left -- job-level `env:`, a `with:` input, a `if:` expression + # -- is a mention in a position neither rule 3a nor 3b sanctions. + leftover = {k: v for k, v in job.items() if k not in ("steps", "secrets")} + if _mentions_token(leftover): fail( violations, path, job_name, - "token referenced at job level (env/secrets/with), not as an " f"'{ALLOWED_ACTION}' step input", + f"token referenced at job level (env/with/...), not as an " + f"'{ALLOWED_ACTION}' step input or a 'secrets:' forward to a local " + f"reusable workflow", + ) + elif not token_steps and not forwarded: + # The job names the token but in none of the shapes above (e.g. a + # `secrets:` block that is not a mapping). Fail closed. + fail( + violations, + path, + job_name, + "token referenced in this job in an unrecognised position", ) - continue - # Rule 4: a privileged job must not pull untrusted code into its - # workspace (no local actions, no checkout) next to the token. + # --- Rule 4: no untrusted code in a job that touches the token. ------- + # A rule 3b caller job has no steps, so this only ever applies to 3a. for step in iter_steps(job): uses = step.get("uses") or "" bare = uses.split("@")[0] @@ -291,18 +442,22 @@ def main() -> int: for v in violations: print(f" - {v}", file=sys.stderr) print( - "\nThe admin-scoped runner token may ONLY be used as:\n" + "\nThe admin-scoped runner token may ONLY be used in one of two ways:\n" f" github-token: ${{{{ secrets.{TOKEN_NAME} }}}}\n" f" in a step that uses '{ALLOWED_ACTION}', inside a job that does\n" - " not check out code or run local actions. See " - ".github/scripts/check_runner_token_policy.py.", + " not check out code or run local actions; or\n" + f" secrets:\n {TOKEN_NAME}: ${{{{ secrets.{TOKEN_NAME} }}}}\n" + " on a job whose 'uses:' is a local './.github/workflows/.yml'\n" + " (which this same scan covers). 'secrets: inherit' is never allowed.\n" + " See .github/scripts/check_runner_token_policy.py.", file=sys.stderr, ) return 1 print( f"✅ EC2 runner token policy: OK ({len(files)} workflow file(s) scanned; " - f"token used only as '{ALLOWED_ACTION}' input)." + f"token reaches only '{ALLOWED_ACTION}', directly or via a local " + f"reusable workflow)." ) return 0 diff --git a/.github/scripts/test_check_runner_token_policy.py b/.github/scripts/test_check_runner_token_policy.py index 3ef549070..b07141577 100644 --- a/.github/scripts/test_check_runner_token_policy.py +++ b/.github/scripts/test_check_runner_token_policy.py @@ -3,9 +3,11 @@ """Unit tests for the EC2 runner-token CI policy. Exercises check_runner_token_policy.py -- the security gate that enforces that -``secrets.EC2_RUNNER_TOKEN`` is only ever used as the ``github-token`` input to -``machulav/ec2-github-runner``. Run by .github/workflows/workflow-security.yml -on every PR (it needs only pyyaml + pytest, no fvdb build). +``secrets.EC2_RUNNER_TOKEN`` only ever reaches ``machulav/ec2-github-runner``, +either as that action's ``github-token`` input or forwarded by name to a *local* +reusable workflow that this same scan covers. Run by +.github/workflows/workflow-security.yml on every PR (it needs only pyyaml + +pytest, no fvdb build). """ from __future__ import annotations @@ -34,8 +36,15 @@ def _load_policy_module(): policy = _load_policy_module() -def _check(tmp_path: Path, yaml_text: str) -> list[str]: - """Write a workflow file and return the policy violations it produces.""" +def _check(tmp_path: Path, yaml_text: str, callees: tuple[str, ...] = ()) -> list[str]: + """Write a workflow file and return the policy violations it produces. + + ``callees`` names sibling workflow files to create first, for the rule-3b + check that a forwarding target actually exists in this directory (and is + therefore covered by the same scan). + """ + for name in callees: + (tmp_path / name).write_text("name: callee\n") wf = tmp_path / "wf.yml" wf.write_text(textwrap.dedent(yaml_text)) violations: list[str] = [] @@ -272,7 +281,54 @@ def test_local_action_in_token_job_is_rejected(tmp_path): assert any("LOCAL action" in v for v in violations) -def test_job_level_token_with_no_step_is_rejected(tmp_path): +# --- rule 3b: forwarding to a local reusable workflow ------------------------ + + +def test_forward_to_local_reusable_workflow_is_allowed(tmp_path): + """The DRY shape: the callee is a workflow file this same scan covers.""" + violations = _check( + tmp_path, + """ + name: ok + on: [push] + jobs: + call: + uses: ./.github/workflows/start-ec2-runner.yml + secrets: + EC2_RUNNER_TOKEN: ${{ secrets.EC2_RUNNER_TOKEN }} + """, + callees=("start-ec2-runner.yml",), + ) + assert violations == [] + + +def test_callee_declaring_the_token_secret_is_allowed(tmp_path): + """The other half of 3b: `on.workflow_call.secrets` naming the token.""" + violations = _check( + tmp_path, + """ + name: ok + on: + workflow_call: + secrets: + EC2_RUNNER_TOKEN: + required: true + jobs: + start: + runs-on: ubuntu-latest + steps: + - uses: machulav/ec2-github-runner@343a1b2ae682e681c3cec9a235d882da17ff04ef + with: + mode: start + github-token: ${{ secrets.EC2_RUNNER_TOKEN }} + """, + ) + assert violations == [] + + +def test_forward_to_remote_reusable_workflow_is_rejected(tmp_path): + """The hole `secrets: inherit` used to walk through: the token leaves the + repo, where no scan of ours can see what the callee does with it.""" violations = _check( tmp_path, """ @@ -280,14 +336,122 @@ def test_job_level_token_with_no_step_is_rejected(tmp_path): on: [push] jobs: call: - uses: ./.github/workflows/reusable.yml + uses: other-org/other-repo/.github/workflows/x.yml@main + secrets: + EC2_RUNNER_TOKEN: ${{ secrets.EC2_RUNNER_TOKEN }} + """, + ) + assert any("only be forwarded to a local reusable workflow" in v for v in violations) + + +def test_forward_to_missing_local_workflow_is_rejected(tmp_path): + """A callee that is not in this directory would not be scanned.""" + violations = _check( + tmp_path, + """ + name: bad + on: [push] + jobs: + call: + uses: ./.github/workflows/nope.yml + secrets: + EC2_RUNNER_TOKEN: ${{ secrets.EC2_RUNNER_TOKEN }} + """, + ) + assert any("does not exist" in v for v in violations) + + +def test_forward_under_a_different_name_is_rejected(tmp_path): + """One name everywhere, so a single grep still enumerates every file.""" + violations = _check( + tmp_path, + """ + name: bad + on: [push] + jobs: + call: + uses: ./.github/workflows/start-ec2-runner.yml secrets: gh-token: ${{ secrets.EC2_RUNNER_TOKEN }} """, + callees=("start-ec2-runner.yml",), ) assert violations +def test_forward_from_a_job_with_no_uses_is_rejected(tmp_path): + violations = _check( + tmp_path, + """ + name: bad + on: [push] + jobs: + call: + runs-on: ubuntu-latest + secrets: + EC2_RUNNER_TOKEN: ${{ secrets.EC2_RUNNER_TOKEN }} + steps: + - run: echo hi + """, + ) + assert any("no job-level uses" in v for v in violations) + + +def test_workflow_level_env_is_rejected(tmp_path): + """Same textual shape as a legitimate forward -- caught structurally.""" + violations = _check( + tmp_path, + """ + name: bad + on: [push] + env: + EC2_RUNNER_TOKEN: ${{ secrets.EC2_RUNNER_TOKEN }} + jobs: + leak: + runs-on: ubuntu-latest + steps: + - run: env + """, + ) + assert any("outside any job" in v for v in violations) + + +# --- rule 6: no blanket secret forwarding ------------------------------------ + + +def test_secrets_inherit_is_rejected(tmp_path): + """`inherit` forwards the admin token while naming nothing, so no textual + check can see where it goes -- including a forward out of the repo.""" + violations = _check( + tmp_path, + """ + name: bad + on: [push] + jobs: + call: + uses: ./.github/workflows/start-ec2-runner.yml + secrets: inherit + """, + callees=("start-ec2-runner.yml",), + ) + assert any("secrets: inherit" in v for v in violations) + + +def test_secrets_inherit_is_rejected_even_to_a_remote_workflow(tmp_path): + violations = _check( + tmp_path, + """ + name: bad + on: [push] + jobs: + call: + uses: other-org/other-repo/.github/workflows/x.yml@main + secrets: inherit + """, + ) + assert any("secrets: inherit" in v for v in violations) + + # --- the real fvdb workflows must all pass -----------------------------------