diff --git a/docs/design/autofix-gate-runner-isolation.md b/docs/design/autofix-gate-runner-isolation.md new file mode 100644 index 00000000000..fc743a484b9 --- /dev/null +++ b/docs/design/autofix-gate-runner-isolation.md @@ -0,0 +1,249 @@ +# Autofix: runner-level isolation for PAT-bearing steps + +Supersedes the in-step approach attempted in PR #9214 (frozen). +Tracks the structural close for issue #9089. Findings inventory: issue #9524. + +## Problem statement + +`review-address` and `issue-autofix` (`.github/workflows/qwen-autofix.yml`) run +**untrusted branch code** and **hold `CI_DEV_BOT_PAT`** in the same job, on the +same OS user, on a persistent self-hosted pool that shares one `HOME` across +~27 runner registrations. In `review-address` the sequence inside one job is: + +| Step | Executes branch code | Holds the PAT | +| -------------------------------- | -------------------- | ------------- | +| `Triage and address` (agent) | yes | no | +| `Verification gate` (build/test) | yes | no | +| `Repair …` / `Finalize …` | partly | no | +| `Push and report` | no | **yes** | + +Everything the later step trusts — `$GITHUB_ENV`, `$GITHUB_OUTPUT`, +`$RUNNER_TEMP`, `$HOME`, the workspace `.git`, live processes — is writable by +the earlier steps' code and by any co-resident survivor at the same uid. #9089 +enumerates the vectors that execute _before a step's first line_ +(`BASH_ENV`, `BASH_FUNC_*`, `LD_PRELOAD`/`LD_AUDIT`) and the runner-provided +shared channels that no in-step `unset` can reach. + +### What PR #9214 tried, and what it proved + +#9214 ran the verification gate in an ephemeral container and hardened the +surrounding steps: digest chains, type/timeout discipline, env pin blocks, +process-tree sweeps, fingerprint sets, and contract tests pinning all of it. +It did not converge, and two of its results are load-bearing for this design: + +1. **The approach is self-defeating at this scale.** Eleven review rounds took + the diff from 329 to 2,700 lines while the per-round Critical count rose + 8 → 9 → 15 → 19 → 29. Rounds 10 and 11 changed no code at all, and round 11 + still produced 14 new Criticals on byte-identical files — six of them of the + form _"the site this diff ADDS lacks the discipline this diff installs + elsewhere."_ Each added guard was the next round's attack surface. + +2. **A container around the executor cannot help, because the root of trust is + outside it** (R11-8). The gate's whole digest chain is rooted in + `steps.stage.outputs.*`. The runner parses a step's `$GITHUB_OUTPUT` + last-wins at step _end_, the file lives under attacker-writable + `$RUNNER_TEMP`, and no sweep of any kind runs before the staging step. A + same-uid co-resident can swap the staged bytes _and_ append forged digest + lines that displace the honest ones — both sides of every downstream + comparison are then attacker-steered. Wrapping the consumer in a container + changes nothing about that. + +Independently: the wrapper could never have run at all post-merge (R9-1: it +digest-verifies a copy of itself that the staging loop never creates; R9-2: the +helper digest is path-dependent and computed over two different path sets), and +CI stayed green because the pre-merge `-z` skip path masks it. That is a +symptom of the same problem — the design had grown past what its tests could +cover. + +## Design principle + +> A job that has executed branch code must never hold the PAT, and must never +> be able to influence the _inputs_ of the job that does. + +The boundary moves from _inside a step_ to _between jobs_, and the channel +between them narrows to one artifact that the consumer re-derives its decision +from rather than trusting. + +## Proposed design + +### A. Split `review-address` into `execute` and `publish` + +**`execute`** — everything that touches branch code: prepare, agent, gate, +repair, artifacts. No `CI_DEV_BOT_PAT`, no write-scoped `GITHUB_TOKEN`, no +push, no PR comment. It emits exactly two things as a job artifact: + +- `fix.patch` — `git format-patch`/`git diff` output against the recorded base + SHA, and +- `report.md` + a plain `verdict.txt` (advisory: what the gate concluded). + +**`publish`** — `needs: execute`, **never executes branch code**: no +`npm ci`, no build, no test, no repo script, no hooks. It checks out the base +branch fresh, downloads the artifact, re-derives its own decision (see B), +applies the patch, and pushes with the PAT. + +What crosses the boundary is an Actions artifact, not a runner-managed channel. +`$GITHUB_ENV`, `$GITHUB_OUTPUT`, `$HOME`, `$RUNNER_TEMP` and live processes do +not cross a job boundary; a forged line in the execute job's `$GITHUB_OUTPUT` +reaches nothing that the publish job reads. + +Hook safety in `publish` is explicit, not incidental: apply with +`git -c core.hooksPath=/dev/null apply --index`, never `git am` with a hostile +`.git`, and never check out the PR head into the publish job's workspace. + +### B. The publish job re-derives; it does not trust the verdict + +In this threat model the execute job is _assumed compromised_ — it runs branch +code by design. Therefore its verdict is untrusted by construction, and no +signature over that verdict can fix it. What the publish job can do is evaluate +the patch itself, with checks that need no branch execution: + +1. the patch applies cleanly to the recorded base SHA; +2. every touched path is inside the PR's declared footprint allowlist; +3. `.github/workflows/**`, `.github/actions/**`, lockfiles, and release + manifests are refused unless the run was explicitly authorised for them; +4. net added lines are within the round's growth budget (the brake that already + exists, moved to the trusted side); +5. the commit message matches the Conventional Commit contract; +6. the head SHA still matches the lease recorded before the agent ran + (`--force-with-lease`). + +Anything that requires _executing_ the branch (build, typecheck, unit tests) +stays in `execute` and is **advisory**. This is a real reduction in what +"verified" means, and it should be stated plainly in the status comment. The +honest comparison is not against a working guarantee: today's gate verdict is +forgeable (R11-8) and, on the #9214 branch, was never produced at all +(R9-1/R9-2). + +Optional defence in depth: `actions/attest-build-provenance` over the artifact +binds _which job produced it_. It does not make a compromised producer's output +trustworthy, so it is additive, not a substitute for (B). + +### C. Ephemeral registration for the `execute` leg + +The survivor/co-resident class exists because registrations are long-lived and +share `HOME`. Register the pool's autofix runners with `--ephemeral` (one job +per registration, fresh `HOME`), or, where the pool cannot be changed, run +`execute` in a job-level `container:` with a per-job `HOME`. The container is a +mitigation (concurrent legs still share the host kernel and the docker socket); +ephemeral registration is the close. + +This is an infrastructure change and is sequenced last — (A) and (B) already +remove the PAT from the shared host, which is the part that matters most. + +### D. Kill agent descendants by lineage, not by env marker + +Replace the `AUTOFIX_AGENT_TREE` marker sweeps (the source of R8-8, R9-3 and +R11-10) with a cgroup scope per agent invocation — `systemd-run --scope` or a +`cgcreate`/`cgclassify` pair — and kill the cgroup. Cgroup membership is not +forgeable from an environment variable, cross-leg kills become impossible +because each scope is per invocation, and self-kill becomes impossible because +the gate step is not a member of the scope it kills. Where cgroup delegation is +unavailable, `setsid` + process-group kill is a strictly better fallback than +marker matching. + +### E. Delete what A–D make redundant + +The point of this work is a **smaller** trust surface, not another layer. Once +the PAT is off the shared host and the decision is re-derived on the trusted +side, the in-step enumeration machinery (env pin blocks, staged-script digest +chains, fingerprint sets, marker sweeps and the contract tests pinning them) +protects a boundary that no longer carries a secret. Removing it is part of +this change, not a follow-up: a guard kept "just in case" is the thing that +regenerated findings in #9214. + +## Alternatives considered + +| Option | Verdict | +| -------------------------------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| In-step hardening (#9214) | Rejected — measured non-convergence, and its root of trust sits outside the step (R11-8). | +| Sign the verdict in `execute`, verify in `publish` | Rejected alone — signs an untrusted producer's claim. Useful only as (B)'s optional attestation. | +| Move the whole job to GitHub-hosted runners | Closes the class outright but loses the pool's cache/network locality and costs minutes. | +| Move **only `publish`** to GitHub-hosted | **Attractive.** The publish job is short, needs no repo cache, and runs no branch code — running it on `ubuntu-latest` removes the shared host from the PAT path entirely. Proposed as step 2 of the rollout. | + +## Rollout + +1. Land (A) + (B) behind a kill switch (`AUTOFIX_SPLIT_PUBLISH_DISABLED`), with + `publish` still on the self-hosted pool. +2. Move `publish` to `ubuntu-latest`. +3. Replace the marker sweeps with (D). +4. Ephemeral registrations for `execute` (C) — infra ticket. +5. Delete the machinery (E) that steps 1–4 make redundant, in one PR per + cluster so each deletion is reviewable on its own. + +## Test plan + +- **Contract (static, mutation-tested):** the `publish` job's step list contains + no `npm`, no `run: .*scripts/`, no checkout of the PR head, and no step that + sources anything from the artifact other than `fix.patch`. +- **Negative control:** a patch touching `.github/workflows/**` is refused by + the footprint check; a patch exceeding the growth budget is refused. +- **Probe — forged output:** during `execute`, append `outcome=fixed` to + `$GITHUB_OUTPUT` from a background process; assert `publish`'s decision is + unchanged. +- **Probe — hostile patch:** a patch that adds `.git/hooks/pre-applypatch` and + a `core.hooksPath` change; assert no hook executes in `publish`. +- **Probe — lineage kill (D):** start a grandchild that `exec`s with a forged + `AUTOFIX_AGENT_TREE`; assert the cgroup kill reaps the real tree and spares + the forger, and that the gate step survives its own sweep. + +## Risks and open questions + +- "Verified" weakens to "deterministically checked + advisory build/test". This + must be visible in the PR status comment, not buried. +- Artifact hand-off adds one upload/download per round and a second checkout. +- Does the org permit `--ephemeral` registrations on the ECS pool? +- Is a GitHub-hosted runner acceptable for the PAT-bearing `publish` job under + the current egress/IP policy? +- `issue-autofix` has the same shape and should follow the same split, but it + creates a branch and a PR rather than pushing to an existing head; its + publish-side checks differ and are not designed here. + +
+中文说明 + +# autofix：为携带 PAT 的步骤做 runner 级隔离 + +取代 PR #9214 中尝试的"步骤内加固"方案（该 PR 已冻结）。对应 issue #9089 的结构性收口，发现清单见 issue #9524。 + +## 问题 + +`review-address` 与 `issue-autofix` 在**同一个 job**里既执行**不可信的分支代码**（agent、构建、测试），又持有 `CI_DEV_BOT_PAT`（`Push and report`），且运行在常驻自建池上——同一 OS 用户、~27 个 runner 注册共享一个 `HOME`。后续步骤所信任的一切（`$GITHUB_ENV`、`$GITHUB_OUTPUT`、`$RUNNER_TEMP`、`$HOME`、工作区 `.git`、存活进程）都可被先前步骤的代码或同 uid 的幸存进程写入。 + +### #9214 证明了什么 + +1. **该路线在这个规模上自我挫败：** 11 轮评审把 diff 从 329 行推到 2700 行，每轮 Critical 数量为 8 → 9 → 15 → 19 → 29；第 10、11 轮没有任何代码改动，第 11 轮仍在逐字节相同的文件上产生 14 个新 Critical，其中 6 条的形态是"这个 diff 新加的地方，缺少这个 diff 在别处安装的纪律"。 +2. **把执行方装进容器无济于事，因为信任根在容器之外**（R11-8）：整条摘要链的根是 `steps.stage.outputs.*`，而 runner 在步骤**结束时**以 last-wins 方式解析位于可写 `$RUNNER_TEMP` 下的 `$GITHUB_OUTPUT`，且暂存步骤之前没有任何清扫。同 uid 的进程可以同时替换磁盘字节并追加伪造的摘要行，使下游比较的两侧都被操控。 + +此外该 wrapper 合入后根本无法运行（R9-1、R9-2），而 CI 保持全绿只是因为 pre-merge 的 `-z` 跳过路径把它挡住了。 + +## 设计原则 + +> 执行过分支代码的 job 绝不持有 PAT，也绝不能影响持有 PAT 的 job 的**输入**。 + +边界从"步骤内部"上移到"job 之间"，两者之间只经由一个产物传递，且消费方**自行重新推导**结论而不是信任它。 + +## 方案 + +- **A. 拆成 `execute` 与 `publish` 两个 job。** `execute` 承载一切分支代码，不持有任何写权限凭据，只产出 `fix.patch` + 报告 + 仅供参考的 `verdict.txt`；`publish` 全新检出基线分支，**不执行任何分支代码**（无 npm、无构建、无测试、无仓库脚本、无 git hook），下载产物、自行判定、应用补丁并用 PAT 推送。`$GITHUB_ENV`/`$GITHUB_OUTPUT`/`$HOME`/`$RUNNER_TEMP` 与存活进程都不跨 job 边界。 +- **B. `publish` 自行推导，不信任 verdict。** 威胁模型中 `execute` 被假定为已失陷，因此对其 verdict 的任何签名都无济于事。`publish` 只做无需执行分支代码的判定：补丁能否干净应用于记录的基线 SHA、路径是否在足迹白名单内、是否触碰 `.github/workflows/**` 与锁文件、净增行是否在增长预算内、提交信息是否符合规范、head SHA 是否仍匹配租约。构建与测试留在 `execute`，且**降级为仅供参考**——这一点必须写进状态评论。诚实的对比基准不是"原本可用的保证"：现有 verdict 本就可伪造（R11-8），而在 #9214 分支上它根本没被产生过（R9-1/R9-2）。 +- **C. `execute` 使用一次性注册（`--ephemeral`）**，或退而求其次用 job 级 `container:` 提供独立 `HOME`；后者是缓解，前者才是收口。属基础设施改动，排在最后。 +- **D. 按血缘杀进程树，而不是按 env 标记。** 用 `systemd-run --scope` / cgroup 取代 `AUTOFIX_AGENT_TREE` 标记匹配（R8-8、R9-3、R11-10 的来源）：cgroup 归属无法由环境变量伪造，跨 leg 误杀与自杀都不再可能。 +- **E. 删掉 A–D 让其失去意义的机制。** 本项工作的目的是**更小**的信任面，而不是再加一层。PAT 离开共享主机、判定移到可信侧之后，步骤内的枚举式机制（env 固定块、暂存脚本摘要链、指纹集、标记清扫及其契约测试）守护的是一个已不再承载秘密的边界，删除它属于本次改动的一部分——"以防万一"留下的守卫正是 #9214 里不断再生发现的东西。 + +## 备选方案 + +步骤内加固（#9214）：否决，已实测不收敛且信任根在步骤之外。仅对 verdict 签名：否决，签的是失陷方的断言。整个 job 迁到 GitHub 托管 runner：能彻底关闭该类问题，但损失缓存与网络就近性且有成本。**仅把 `publish` 迁到 GitHub 托管**：可取——该 job 很短、不需要仓库缓存、不执行分支代码，这样 PAT 路径上就不再有共享主机；列为推进步骤 2。 + +## 推进顺序 + +1. 带 kill switch 落地 A + B（`publish` 仍在自建池）；2. 把 `publish` 迁到 `ubuntu-latest`；3. 用 D 取代标记清扫；4. 为 `execute` 启用一次性注册（C，基础设施工单）；5. 按簇分 PR 删除 1–4 让其冗余的机制（E）。 + +## 测试计划 + +静态契约（做变异测试）：`publish` 的步骤列表中不得出现 `npm`、`run: .*scripts/`、检出 PR head，或读取 `fix.patch` 以外的产物内容。负向对照：触碰 `.github/workflows/**` 或超出增长预算的补丁必须被拒。探针：在 `execute` 中由后台进程向 `$GITHUB_OUTPUT` 追加 `outcome=fixed`，断言 `publish` 的判定不变；构造包含 `.git/hooks` 与 `core.hooksPath` 的恶意补丁，断言 `publish` 中没有 hook 被执行；伪造 `AUTOFIX_AGENT_TREE` 的孙进程，断言 cgroup 清杀命中真实进程树、放过伪造者，且门步骤不会杀死自己。 + +## 风险与未决问题 + +"已验证"会弱化为"确定性检查 + 仅供参考的构建/测试"，必须在状态评论中明示；产物传递增加一次上传/下载与一次检出；ECS 池是否允许 `--ephemeral` 注册；当前出网/IP 策略下，携带 PAT 的 `publish` 能否放在 GitHub 托管 runner 上；`issue-autofix` 形态相同但它是新建分支并开 PR，其发布侧判定不同，本文未涵盖。 + +