diff --git a/CHANGELOG.md b/CHANGELOG.md index b9c69f4..1c1b897 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,37 @@ ## [Unreleased] +- Walk the action graph instead of glancing at it. `check_transitive_action_pins` + claimed that every reference reachable from a pinned action is itself immutable, + and established none of it. Three gaps, each found by testing the check rather + than reading it: + + It read YAML with a regular expression anchored on `uses:` as the first token of + a line, so the ordinary composite form `- uses: owner/action@ref` matched + nothing — while the sibling expression for this repository's own workflows + handled `(?:-\s*)?`, a difference of one group. It did not recurse: a nested + reference that was itself a SHA ended the walk, so depth two and beyond were + never looked at. And it could not tell throttling from a refusal — about 43 + sequential API calls with no pacing hit a secondary rate limit, and the 403 was + reported as "unreachable", which reads as a broken third party rather than as + this check asking too fast. That is what `maintenance.yml` filed on its first + successful run. + + References are now read from the parsed document at any nesting, which covers + composite actions, reusable workflows, `jobs..uses`, and both step + spellings without encoding any of them. The walk is breadth-first with a + visited set, bounded in depth and node count, so a cycle terminates and a + hostile graph cannot run without end. Rate limits are retried with backoff and + reported as rate limits; a missing definition stays a finding about the pin, + and an unreadable one is a finding about the run rather than silence. + + Eleven self-tests run the walk over in-memory graphs — no network — covering + both step forms, depth three, a cycle, a reusable workflow, `jobs..uses`, a + missing definition, an unreadable one, an undigested image, and a local + reference that must not be chased. Six mutations were each caught, including + reverting to the regex and removing recursion. The tree's real closure is clean + at every depth. + - Make a tier's requirements data, and hold every caller to them. `--tier scheduled` needs a GitHub token, two external hosts and the tag refs. That was written down once, as comments beside `maintenance.yml`'s egress allow-list, and diff --git a/catalog/python-execution.yml b/catalog/python-execution.yml index beebe87..6f51dbb 100644 --- a/catalog/python-execution.yml +++ b/catalog/python-execution.yml @@ -186,7 +186,7 @@ "check_scorecard_evidence_contract.py": ["_strict_yaml", "_workflow_yaml", "check_harden_runner_contract", "check_python_execution_contract"], "check_secret_scan_contract.py": ["_strict_yaml", "_workflow_yaml", "check_python_execution_contract"], "check_sdk_runtime_fixtures.py": ["_gradle_lockfile", "_sdk_environment", "_strict_yaml", "_workflow_yaml", "generate_sdk_runtime_manifest"], - "check_transitive_action_pins.py": ["_workflow_yaml"], + "check_transitive_action_pins.py": ["_strict_yaml", "_workflow_yaml"], "check_side_effect_fixture_contract.py": ["_strict_yaml", "_workflow_yaml", "check_python_execution_contract"], "check_skills.py": ["_strict_yaml"], "check_tool_pinning.py": ["_workflow_yaml"], diff --git a/scripts/check_transitive_action_pins.py b/scripts/check_transitive_action_pins.py index 239dac1..9d2ad7a 100644 --- a/scripts/check_transitive_action_pins.py +++ b/scripts/check_transitive_action_pins.py @@ -1,26 +1,32 @@ #!/usr/bin/env python3 """Pinning an action by SHA does not pin what that action calls. -`check_pinned_actions.py` proves every `uses:` written here is a full commit -SHA. That is one level deep. A composite action's own `action.yml` may refer to -further actions by tag, and GitHub resolves the **entire** nested graph at job -setup — so under the repository control "require actions to be pinned to a -full-length commit SHA", which `docs/00` and the consumer-adoption skill both -recommend, the job is refused before a single step runs: +`check_pinned_actions.py` proves every `uses:` written here is a full commit SHA. +That is one level deep. A composite action's own `action.yml` may name further +actions by tag, and GitHub resolves the **entire** nested graph at job setup -- +so under the repository control "require actions to be pinned to a full-length +commit SHA", which `docs/00` and the consumer-adoption skill both recommend, the +job is refused before a single step runs: The action actions/cache@v5 is not allowed in NDDev-it-com/ci-workflows because all actions must be pinned to a full-length commit SHA. That is how `dart-flutter-ci.yml` and `qt-ci.yml` turned out to be unusable in -any organisation following this library's own advice, and nothing static could -see it: `subosito/flutter-action` and `jurplel/install-qt-action` are correctly -pinned *here*, and reach `actions/cache@v5`, `actions/setup-python@v6` and -`jurplel/install-qt-action/action@v4` from inside. See issue #150. +any organisation following this library's own advice. See issue #150. -This resolves each pinned third-party action at its pinned SHA and reports every -nested reference that is not itself a commit SHA — so the next one is found by -reading, not by a fixture failing in a way that takes a runner round trip to -diagnose. +The first version of this check claimed that property and did not establish it. +Three gaps, each found by testing it rather than reading it: + +* **It read YAML with a regular expression** anchored on `uses:` as the first + token of a line, so the ordinary composite form `- uses: owner/action@ref` + matched nothing. The sibling expression for this repository's own workflows + handled `(?:-\\s*)?`; this one did not, and the difference was one group. +* **It did not recurse.** A nested reference that was itself a SHA ended the + walk, so anything at depth two or beyond was never looked at. +* **It could not tell throttling from a refusal.** ~43 sequential API calls with + no pacing hit a secondary rate limit, and the 403 was reported as + "unreachable", which reads as a broken third party rather than as this check + asking too fast. `maintenance.yml` filed exactly that on its first run. Advisory tier. It reads other repositories over the network, and what a third party writes in its own `action.yml` is not a property of this tree. @@ -29,17 +35,33 @@ import os import re +import time import urllib.error import urllib.request +from typing import Any, Callable +from ci_workflows_tools._strict_yaml import strict_loads from ci_workflows_tools._workflow_yaml import WORKFLOWS_DIR, workflow_files USES = re.compile(r"^\s*(?:-\s*)?uses:\s*(?P[^\s#]+)", re.MULTILINE) SHA = re.compile(r"^[0-9a-f]{40}$") -NESTED_USES = re.compile(r"^\s*uses:\s*(?P[^\s#]+)", re.MULTILINE) TIMEOUT_SECONDS = 30 API = "https://api.github.com/repos/{repo}/contents/{path}?ref={ref}" +# Bounds, so a hostile or merely circular graph cannot run forever. They are +# generous: the tree's own graph is two layers and a few dozen nodes. +MAX_DEPTH = 6 +MAX_NODES = 400 +RETRY_DELAYS = (2, 8, 20) + + +class Unavailable(Exception): + """A definition could not be read, for a reason other than "not there". + + Separate from a missing definition because the two mean opposite things: a + 404 is a finding about the pin, and this is a finding about the run. + """ + def _candidate_paths(subdirectory: str) -> tuple[str, ...]: """Where the definition of a pinned reference lives, by kind. @@ -48,9 +70,6 @@ def _candidate_paths(subdirectory: str) -> tuple[str, ...]: `action.yaml` in its directory, or a reusable workflow, which *is* the file. Treating the second as the first asks for `.github/workflows/x.yml/action.yml`, gets a 404, and reports a missing definition for a perfectly good pin. - Reusable workflows carry `uses:` of their own, so they belong in this audit - rather than being skipped: a third party's workflow can name unpinned - actions exactly as a third party's composite action can. """ if subdirectory.endswith((".yml", ".yaml")): return (subdirectory,) @@ -59,20 +78,34 @@ def _candidate_paths(subdirectory: str) -> tuple[str, ...]: return ("action.yml", "action.yaml") -def _selftest() -> list[str]: - problems: list[str] = [] - for subdirectory, expected in ( - ("", ("action.yml", "action.yaml")), - ("setup", ("setup/action.yml", "setup/action.yaml")), - (".github/workflows/release.yml", (".github/workflows/release.yml",)), - (".github/workflows/release.yaml", (".github/workflows/release.yaml",)), - ): - actual = _candidate_paths(subdirectory) - if actual != expected: - problems.append( - f"reference-kind selftest: {subdirectory!r} resolved to {actual}, " - f"expected {expected}") - return problems +def _uses_in(node: Any) -> set[str]: + """Every `uses:` value reachable in a parsed definition, at any nesting. + + Read from the parsed document rather than matched in the text. An action + definition puts them under `runs.steps`; a reusable workflow puts them under + `jobs.` directly and under `jobs..steps`. Walking the structure + covers both without encoding either, and cannot be defeated by key order or + by which of the two step spellings the author used. + """ + found: set[str] = set() + if isinstance(node, dict): + for key, value in node.items(): + if key == "uses" and isinstance(value, str): + found.add(value.strip()) + else: + found |= _uses_in(value) + elif isinstance(node, list): + for item in node: + found |= _uses_in(item) + return found + + +def _references(text: str, origin: str) -> set[str]: + """The nested references a definition declares.""" + try: + return _uses_in(strict_loads(text, origin)) + except Exception as exc: # noqa: BLE001 - a third party's YAML, not ours + raise Unavailable(f"{origin}: definition does not parse as YAML: {exc}") from exc def _third_party_pins() -> dict[str, set[str]]: @@ -90,7 +123,26 @@ def _third_party_pins() -> dict[str, set[str]]: return pins +def _is_rate_limited(exc: urllib.error.HTTPError) -> bool: + """Distinguish "you are asking too fast" from "you may not read this". + + GitHub answers both with 403. The primary limit sets `X-RateLimit-Remaining: + 0`; the secondary limit sets `Retry-After` or says so in the body. Reporting + a throttle as a permission failure sent a maintainer looking at the wrong + repository -- which is what the first filed sweep finding did. + """ + if exc.code not in (403, 429): + return False + headers = exc.headers or {} + if str(headers.get("X-RateLimit-Remaining", "")).strip() == "0": + return True + if headers.get("Retry-After"): + return True + return "rate limit" in str(exc.reason).lower() + + def _fetch(repo: str, path: str, ref: str, token: str | None) -> str | None: + """The definition's bytes, `None` if there is none, `Unavailable` otherwise.""" request = urllib.request.Request( API.format(repo=repo, path=path, ref=ref), headers={"Accept": "application/vnd.github.raw+json", @@ -98,75 +150,242 @@ def _fetch(repo: str, path: str, ref: str, token: str | None) -> str | None: ) if token: request.add_header("Authorization", f"Bearer {token}") - try: - with urllib.request.urlopen(request, timeout=TIMEOUT_SECONDS) as response: # noqa: S310 - return response.read().decode("utf-8") - except urllib.error.HTTPError as exc: - if exc.code == 404: - return None - raise + for attempt, delay in enumerate((*RETRY_DELAYS, None)): + try: + with urllib.request.urlopen(request, timeout=TIMEOUT_SECONDS) as response: # noqa: S310 + return response.read().decode("utf-8") + except urllib.error.HTTPError as exc: + if exc.code == 404: + return None + if _is_rate_limited(exc) and delay is not None: + time.sleep(delay) + continue + reason = "rate limited" if _is_rate_limited(exc) else f"HTTP {exc.code}" + raise Unavailable(f"{repo}: {reason} after {attempt + 1} attempt(s)") from exc + except (urllib.error.URLError, TimeoutError, OSError) as exc: + if delay is None: + raise Unavailable(f"{repo}: unreachable: {exc}") from exc + time.sleep(delay) + raise Unavailable(f"{repo}: exhausted retries") -def _token() -> str | None: - """The token comes from the environment, never from shelling out to `gh`. +Fetcher = Callable[[str, str, str], str | None] - Reading it with `gh auth token` would add a process edge for a value the - caller already has, and the same rule the brief states for zizmor applies - here: pass it in. Unauthenticated the API allows 60 requests an hour, which - this exhausts, so a missing token is reported rather than worked around. - """ - for name in ("GH_TOKEN", "GITHUB_TOKEN"): - value = os.environ.get(name) - if value: - return value - return None +def walk(roots: dict[str, set[str]], fetch: Fetcher) -> list[str]: + """Every reference reachable from the tree's pins must itself be immutable. -def check() -> list[str]: - problems: list[str] = _selftest() - token = _token() - if token is None: - return problems + [ - "nested action pins unverified: set GH_TOKEN; the unauthenticated " - "API rate limit cannot cover every pinned action"] - for pin, callers in sorted(_third_party_pins().items()): - location, _, revision = pin.partition("@") + Breadth-first with a visited set, so a cycle terminates instead of recursing + for ever, and bounded in depth and node count so a hostile graph cannot make + this run without end. + """ + problems: list[str] = [] + seen: set[str] = set() + # (reference, depth, the tree paths that reach it) + queue: list[tuple[str, int, set[str]]] = [ + (pin, 0, callers) for pin, callers in sorted(roots.items()) + ] + while queue: + reference, depth, callers = queue.pop(0) + if reference in seen: + continue + seen.add(reference) + if len(seen) > MAX_NODES: + problems.append( + f"action graph exceeded {MAX_NODES} nodes; refusing to keep walking") + break + if depth > MAX_DEPTH: + problems.append( + f"{reference}: action graph deeper than {MAX_DEPTH}; refusing to " + "keep walking") + continue + + location, _, revision = reference.partition("@") parts = location.split("/") repo = "/".join(parts[:2]) - subdirectory = "/".join(parts[2:]) - candidates = _candidate_paths(subdirectory) + candidates = _candidate_paths("/".join(parts[2:])) definition = None try: for candidate in candidates: - definition = _fetch(repo, candidate, revision, token) + definition = fetch(repo, candidate, revision) if definition is not None: break - except (urllib.error.URLError, TimeoutError, OSError) as exc: - problems.append(f"{pin}: nested pins unverified, {repo} unreachable: {exc}") + except Unavailable as exc: + problems.append(f"{reference}: nested pins unverified, {exc}") continue if definition is None: - problems.append(f"{pin}: no definition at that ref (looked for {', '.join(candidates)})") + problems.append( + f"{reference}: no definition at that ref (looked for " + f"{', '.join(candidates)})") + continue + try: + nested_refs = _references(definition, reference) + except Unavailable as exc: + problems.append(f"{reference}: nested pins unverified, {exc}") continue - # An action may call the same thing from several steps; the finding is - # about the reference, not about how often it appears. - for nested in sorted({m.group("ref") for m in NESTED_USES.finditer(definition)}): + + for nested in sorted(nested_refs): if nested.startswith("./"): + # A local reference resolves inside the repository already being + # walked, at the same revision; it introduces no new mutability. continue if nested.startswith("docker://"): if "@sha256:" not in nested: problems.append( - f"{pin} calls {nested} without a digest " + f"{reference} calls {nested} without a digest " f"(used by {', '.join(sorted(callers))})") continue _, _, nested_revision = nested.partition("@") if not SHA.fullmatch(nested_revision): problems.append( - f"{pin} calls {nested}, which is not pinned to a commit SHA — " - f"a caller enforcing SHA pinning cannot start " + f"{reference} calls {nested}, which is not pinned to a commit SHA " + f"-- a caller enforcing SHA pinning cannot start " f"{', '.join(sorted(callers))}") + continue + queue.append((nested, depth + 1, callers)) + return problems + + +def _selftest() -> list[str]: + """The walk, on graphs built in memory rather than fetched. + + Each case here was first observed failing against the previous + implementation; none of them needs the network, which is why they can be + asserted every run instead of whenever GitHub is reachable. + """ + problems: list[str] = [] + + for subdirectory, expected in ( + ("", ("action.yml", "action.yaml")), + ("setup", ("setup/action.yml", "setup/action.yaml")), + (".github/workflows/release.yml", (".github/workflows/release.yml",)), + (".github/workflows/release.yaml", (".github/workflows/release.yaml",)), + ): + actual = _candidate_paths(subdirectory) + if actual != expected: + problems.append( + f"reference-kind selftest: {subdirectory!r} resolved to {actual}, " + f"expected {expected}") + + sha = "a" * 40 + other = "b" * 40 + third = "c" * 40 + + def graph(definitions: dict[str, str]) -> Fetcher: + """Serve definitions keyed by the reference string that reaches them. + + The path matters: a reusable workflow is addressed by its own file while + an action is addressed by `action.yml` inside its directory. Keying on + the repository alone conflated the two, which this self-test caught. + """ + def fetch(repo: str, path: str, ref: str) -> str | None: + reference = f"{repo}@{ref}" if path in ("action.yml", "action.yaml") \ + else f"{repo}/{path}@{ref}" + return definitions.get(reference) + return fetch + + # The step spelling the regular expression could not see. + dash_form = "runs:\n using: composite\n steps:\n - uses: o/b@v1\n" + named_form = "runs:\n using: composite\n steps:\n - name: n\n uses: o/b@v1\n" + for label, body in (("dash", dash_form), ("named", named_form)): + found = walk({f"o/a@{sha}": {"w.yml"}}, graph({f"o/a@{sha}": body})) + if not any("o/b@v1" in problem for problem in found): + problems.append( + f"walk selftest: the {label} step form did not surface a mutable " + f"nested reference; got {found}") + + # Depth two and three: a nested SHA is not the end of the walk. + deep = { + f"o/a@{sha}": f"runs:\n steps:\n - uses: o/b@{other}\n", + f"o/b@{other}": f"runs:\n steps:\n - uses: o/c@{third}\n", + f"o/c@{third}": "runs:\n steps:\n - uses: o/d@v9\n", + } + found = walk({f"o/a@{sha}": {"w.yml"}}, graph(deep)) + if not any("o/d@v9" in problem for problem in found): + problems.append( + f"walk selftest: a mutable reference at depth three was not found; got {found}") + + # A cycle terminates rather than recursing for ever. + cycle = { + f"o/a@{sha}": f"runs:\n steps:\n - uses: o/b@{other}\n", + f"o/b@{other}": f"runs:\n steps:\n - uses: o/a@{sha}\n", + } + if walk({f"o/a@{sha}": {"w.yml"}}, graph(cycle)): + problems.append("walk selftest: a clean cycle reported a problem") + + # A reusable workflow names actions under `jobs`, not under `runs.steps`. + reusable = { + f"o/a/.github/workflows/r.yml@{sha}": + "on:\n workflow_call:\njobs:\n j:\n steps:\n - uses: o/b@v2\n", + } + found = walk({f"o/a/.github/workflows/r.yml@{sha}": {"w.yml"}}, graph(reusable)) + if not any("o/b@v2" in problem for problem in found): + problems.append( + f"walk selftest: a reusable workflow's nested reference was missed; got {found}") + + # A job that *is* a reusable-workflow call, with no steps at all. + called = {f"o/a@{sha}": "jobs:\n j:\n uses: o/b/.github/workflows/x.yml@v3\n"} + found = walk({f"o/a@{sha}": {"w.yml"}}, graph(called)) + if not any("x.yml@v3" in problem for problem in found): + problems.append( + f"walk selftest: a `jobs..uses` reference was missed; got {found}") + + # A missing definition is a finding about the pin. + found = walk({f"o/a@{sha}": {"w.yml"}}, graph({})) + if not any("no definition at that ref" in problem for problem in found): + problems.append(f"walk selftest: a missing definition was not reported; got {found}") + + # An unreadable definition is a finding about the run, and must not be + # silently treated as "nothing nested here". + def refuses(repo: str, path: str, ref: str) -> str | None: + raise Unavailable(f"{repo}: rate limited after 4 attempt(s)") + + found = walk({f"o/a@{sha}": {"w.yml"}}, refuses) + if not any("rate limited" in problem for problem in found): + problems.append(f"walk selftest: an unreadable definition was swallowed; got {found}") + + # Docker references need a digest, and a digest-pinned one is fine. + docker = {f"o/a@{sha}": "runs:\n steps:\n - uses: docker://alpine:3\n"} + found = walk({f"o/a@{sha}": {"w.yml"}}, graph(docker)) + if not any("without a digest" in problem for problem in found): + problems.append(f"walk selftest: an undigested image was accepted; got {found}") + + # A local reference introduces no new mutability and must not be chased. + local = {f"o/a@{sha}": "runs:\n steps:\n - uses: ./nested\n"} + if walk({f"o/a@{sha}": {"w.yml"}}, graph(local)): + problems.append("walk selftest: a local reference was treated as a finding") + return problems +def _token() -> str | None: + """The token comes from the environment, never from shelling out to `gh`. + + Reading it with `gh auth token` would add a process edge for a value the + caller already has, and the same rule the brief states for zizmor applies + here: pass it in. Unauthenticated the API allows 60 requests an hour, which + this exhausts, so a missing token is reported rather than worked around. + """ + for name in ("GH_TOKEN", "GITHUB_TOKEN"): + value = os.environ.get(name) + if value: + return value + return None + + +def check() -> list[str]: + problems: list[str] = _selftest() + token = _token() + if token is None: + return problems + [ + "nested action pins unverified: set GH_TOKEN; the unauthenticated " + "API rate limit cannot cover every pinned action"] + return problems + walk( + _third_party_pins(), + lambda repo, path, ref: _fetch(repo, path, ref, token), + ) + + def main() -> int: problems = check() if problems: