diff --git a/generator/src/tend/checks.py b/generator/src/tend/checks.py index 7a1d9a9c..b9473d9d 100644 --- a/generator/src/tend/checks.py +++ b/generator/src/tend/checks.py @@ -615,7 +615,7 @@ def check_environment_deployments(repo: str) -> CheckResult: f"{path} job '{job_id}'" for path, text in sorted(files.items()) if text is not None - for job_id in sorted(_parse_workflow(path, text).filed_deployments) + for job_id in sorted(_parse_workflow(path, text, repo).filed_deployments) ] if offenders: return CheckResult( @@ -703,7 +703,7 @@ class _WorkflowFacts: path: str steerable: frozenset[str] # bot-steerable triggers it carries - reusable: bool # declares `workflow_call` + call_only: bool # `workflow_call` is the only thing that starts it calls: frozenset[str] # local reusable workflows this one invokes environments: frozenset[str] # environments its jobs deploy to oidc_environments: frozenset[str] # …of those, ones a job mints OIDC in @@ -721,7 +721,25 @@ def _permissions_grant_oidc(permissions: object) -> bool: return False -def _parse_workflow(path: str, text: str) -> _WorkflowFacts: +def _called_workflow(uses: str, repo: str) -> str | None: + """The workflow file in this repo that a job-level `uses:` names, or None. + + Two spellings reach the same file: the relative form, and the + `owner/repo/.github/workflows/x.yaml@ref` form a repo may use on its own + workflow to pin the ref it runs. Only a call landing in this repo is + followed — another repo's reusable workflow runs against that repo's + environments, not this one's. + """ + relative = "./.github/workflows/" + if uses.startswith(relative): + return uses[len(relative) :] + absolute = f"{repo}/.github/workflows/" + if repo and uses.casefold().startswith(absolute.casefold()): + return uses[len(absolute) :].partition("@")[0] + return None + + +def _parse_workflow(path: str, text: str, repo: str) -> _WorkflowFacts: """Read one workflow's triggers, environments, and OIDC use. Anything the parse cannot decide (an unparsable file, an environment named @@ -775,8 +793,9 @@ def _parse_workflow(path: str, text: str) -> _WorkflowFacts: # A job that calls another workflow declares no environment of its # own — the called workflow's jobs do, and those are parsed there. # Its `permissions:` only caps what the callee may request. - if isinstance(uses, str) and uses.startswith("./.github/workflows/"): - calls.add(uses.split("/")[-1]) + called = _called_workflow(uses, repo) if isinstance(uses, str) else None + if called is not None: + calls.add(called) continue permissions = job.get("permissions", workflow_permissions) oidc = _permissions_grant_oidc(permissions) @@ -809,7 +828,7 @@ def _parse_workflow(path: str, text: str) -> _WorkflowFacts: return _WorkflowFacts( path=path, steerable=frozenset(steerable), - reusable="workflow_call" in triggers, + call_only=triggers == {"workflow_call"}, calls=frozenset(calls), environments=frozenset(environments), oidc_environments=frozenset(oidc_environments), @@ -824,38 +843,45 @@ def _effective_triggers( ) -> tuple[dict[str, frozenset[str]], frozenset[str]]: """Resolve each workflow's steerable triggers, following `workflow_call`. - A reusable workflow's own `on:` says only that it is callable; what can + `workflow_call` on its own says only that a workflow is callable; what can start it is whatever starts its callers. Callers within the repo are - followed to a fixpoint. A reusable workflow with no caller here is returned - as unreached — its callers may live in another repo, which this cannot + followed to a fixpoint, and a workflow no such chain reaches is returned as + unreached — its callers may live in another repo, which this cannot enumerate. + + A workflow carrying triggers of its own anchors the chain: `workflow_call` + widens the way in rather than replacing it, so its own `on:` starts it here + whatever else calls it. A callable workflow is then reached when one of its + callers is, and unreached when every route to it runs through a workflow + only an outside caller can start. """ resolved = {path: f.steerable for path, f in facts.items()} + reached = {path: not f.call_only for path, f in facts.items()} callers: dict[str, set[str]] = {path: set() for path in facts} for path, f in facts.items(): for callee in f.calls: if callee in callers: callers[callee].add(path) - # Each pass only adds triggers and the vocabulary is finite, so this - # settles; the iteration bound keeps a cyclic `uses:` graph from looping. + # Each pass only grows the trigger sets and only flips workflows to + # reached, so this settles; the iteration bound keeps a cyclic `uses:` + # graph from looping. for _ in range(len(facts) + 1): changed = False for path, sources in callers.items(): - grown = ( - resolved[path].union(*(resolved[s] for s in sources)) - if sources - else resolved[path] - ) + if not sources: + continue + grown = resolved[path].union(*(resolved[s] for s in sources)) if grown != resolved[path]: resolved[path] = grown changed = True + if not reached[path] and any(reached[s] for s in sources): + reached[path] = True + changed = True if not changed: break - unreached = frozenset( - path for path, f in facts.items() if f.reusable and not callers[path] - ) + unreached = frozenset(path for path, ok in reached.items() if not ok) return resolved, unreached @@ -870,7 +896,9 @@ class _CredentialSurface: unresolved: tuple[str, ...] -def _credential_surface(files: dict[str, str | None] | None) -> _CredentialSurface: +def _credential_surface( + repo: str, files: dict[str, str | None] | None +) -> _CredentialSurface: """Read the workflows into the facts the environment gates need. An unreadable tree yields an empty surface that says so, rather than no @@ -891,7 +919,7 @@ def _credential_surface(files: dict[str, str | None] | None) -> _CredentialSurfa if text is None: unresolved.append(f"{path} could not be read") continue - parsed = _parse_workflow(path, text) + parsed = _parse_workflow(path, text, repo) facts[path] = parsed unresolved.extend(parsed.unresolved) @@ -1088,7 +1116,7 @@ def check_credential_environments( name, None, f"Could not list environments: {listed.stderr.strip()}" ) - surface = _credential_surface(_fetch_workflow_files(repo)) + surface = _credential_surface(repo, _fetch_workflow_files(repo)) tags_ok = cache(lambda: _tags_admin_gated(repo, cfg.bot_name)) ungated: list[str] = [] diff --git a/generator/tests/test_checks.py b/generator/tests/test_checks.py index 54798994..cb9fd397 100644 --- a/generator/tests/test_checks.py +++ b/generator/tests/test_checks.py @@ -2067,6 +2067,77 @@ def test_credential_environments_reusable_caller_job_is_not_ungated_oidc() -> No assert result.passed is True +def test_credential_environments_absolute_self_call_inherits_caller_triggers() -> None: + """A repo calling its own reusable workflow by the `owner/repo/...@ref` + form reaches the same file as the relative one, so the callee inherits the + caller's triggers either way.""" + result = _credential_check( + {"pypi": (["PYPI_TOKEN"], _CUSTOM_POLICY, "branch main")}, + workflows={ + "dispatch.yaml": ( + "on:\n" + " repository_dispatch:\n" + " types: [publish]\n" + "jobs:\n" + " call:\n" + " uses: owner/repo/.github/workflows/publish.yaml@main\n" + ), + "publish.yaml": ( + "on:\n workflow_call:\njobs:\n publish:\n environment: pypi\n" + ), + }, + ) + assert result.passed is False + assert "`repository_dispatch`" in result.message + + +def test_credential_environments_own_triggers_reach_a_callable_workflow() -> None: + """`workflow_call` alongside triggers of its own widens the way in rather + than replacing it: the workflow's own `on:` still starts it here, so an + uncallable-from-here verdict would skip a surface tend can read.""" + result = _credential_check( + {"pypi": (["PYPI_TOKEN"], _CUSTOM_POLICY, "branch main")}, + workflows={ + "tests.yaml": ( + "on:\n" + " pull_request:\n" + " push:\n" + " branches: [main]\n" + " schedule:\n" + " - cron: '49 10 * * *'\n" + " workflow_dispatch:\n" + " workflow_call:\n" + "jobs:\n" + " deploy:\n" + " environment: pypi\n" + ) + }, + ) + assert result.passed is True, result.message + + +def test_credential_environments_unreached_through_a_caller_is_unverified() -> None: + """A caller that nothing here starts leaves its callee just as unreachable + — the chain has to anchor on a workflow with triggers of its own.""" + result = _credential_check( + {"pypi": (["PYPI_TOKEN"], _CUSTOM_POLICY, "branch main")}, + workflows={ + "wrapper.yaml": ( + "on:\n" + " workflow_call:\n" + "jobs:\n" + " call:\n" + " uses: ./.github/workflows/publish.yaml\n" + ), + "publish.yaml": ( + "on:\n workflow_call:\njobs:\n publish:\n environment: pypi\n" + ), + }, + ) + assert result.passed is None + assert "publish.yaml is only reachable" in result.message + + def test_credential_environments_unreached_reusable_workflow_is_unverified() -> None: """Its callers may live in another repo, which this cannot enumerate.""" result = _credential_check( diff --git a/plugins/install-tend/skills/install-tend/SKILL.md b/plugins/install-tend/skills/install-tend/SKILL.md index a325f4f4..9ef7ed4c 100644 --- a/plugins/install-tend/skills/install-tend/SKILL.md +++ b/plugins/install-tend/skills/install-tend/SKILL.md @@ -365,9 +365,8 @@ Bot-deleting an admin-pushed tag is brief availability damage at worst; repos that need stronger protection against published-tag deletion can add a no-bypass `deletion` ruleset (see the publisher uplift below). -**Environment ref policies.** A ruleset only helps once an Environment -names the refs it protects. A new Environment defaults to -`deployment_branch_policy: null`, which admits every ref — so a bot-pushed +**Environment gates.** A new Environment admits every ref and requires no +approval — `deployment_branch_policy: null`, no reviewers — so a bot-pushed branch or tag reaches its secrets and mints its OIDC token. Survey what exists, including environments GitHub created on the repo's behalf (`github-pages`) and ones that predate tend: @@ -377,9 +376,15 @@ gh api "repos/$REPO/environments" \ --jq '.environments[] | {name, deployment_branch_policy, rules: [.protection_rules[].type]}' ``` -Pin each environment that holds a secret, or that a job with -`id-token: write` names, to the admin-gated refs its workflows actually -use — all tags for a release, the default branch for a continuous deploy: +Each environment that holds a secret, or that a job with `id-token: write` +names, needs a gate: a deployment policy pinned to admin-gated refs, or +required reviewers who exclude the bot. Either clears +`credential-environments`, so an environment already behind reviewers +stays as it is. + +Pin the policy to the admin-gated refs its workflows actually use — all +tags for a release, the default branch for a continuous deploy. The +rulesets above are what hold those refs out of the bot's reach: ```bash gh api "repos/$REPO/environments/$ENV" --method PUT --input - << 'EOF' @@ -396,6 +401,18 @@ branch carrying a *classic* protection rule — a set that grows as branches are created, and that excludes a branch protected by the ruleset above. +Name required reviewers where no ref list fits — a deploy running from a +ref no policy can name, such as a preview published from +`refs/pull/N/merge`. Approval holds whatever ref the run starts from. Ask +which humans to name; the bot cannot be one of them: + +```bash +ID=$(gh api "users/" --jq .id) +gh api "repos/$REPO/environments/$ENV" --method PUT --input - << EOF +{"reviewers": [{"type": "User", "id": $ID}], "deployment_branch_policy": null} +EOF +``` + **Release/deploy workflow design.** Workflows that use release or deploy secrets must trigger on `push: tags:` (release) or `push: branches: [main]` (continuous deploy from the default branch), and reference an Environment @@ -418,9 +435,8 @@ the bot. **More complicated approaches are possible** (per-pattern tag rulesets, mixed bypass actors, layered no-bypass immutability rulesets for repos -that publish actions consumed via tag pins, required-reviewer environment -gates for per-deploy human approval). Install-tend packages the recipe -above because it is the simplest configuration that holds the chain; +that publish actions consumed via tag pins). Install-tend packages the +recipe above because it is the simplest configuration that holds the chain; adopters with stricter requirements can layer additional rulesets or environment protection rules on top.