Plan, Phase 0, and the #191 trust model revision #1

Open

oap wants to merge 6 commits from claude/plan-rev-191 into main

pull from: claude/plan-rev-191

merge into: oap:main

oap:main

Conversation 2 Commits 6 Files changed 21 +4100

oap commented 2026-10-08 23:20:54 -07:00

Owner

Copy link

The plan, Phase 0, and a revision of the plan for oap/nix-config#191.

Commits

1. 59192ea adds PLAN.md: a Forgejo Runner v13.1 plugin that runs each Actions job in its own Firecracker microVM.
2. 15fdfa7 Phase 0: Go module with the vendored plugin.proto, a stub daemon, flake.nix with the checks (lint, lint-nix, proto-drift, license-boundary, unit, kvm-smoke), the workflows and the review prompt.
3. 851a4d8 revises the plan after the oap/nix-config#191 test:
   • §5 new trust model. A nix:host job is not a boundary. The plugin gets its own runner instance and uid, with only firecracker:// labels. Its socket belongs to that uid's group, with a SO_PEERCRED check. End state: remove nix:host.
   • §7.1 the runner is now user-scoped under oap (oap/nix-config#130, oap/nix-config#190).
   • §7.3 CI stage 1: jobs run only on the docker label. The VM tier runs by hand from the Mac before a merge. Stage 2 (after dogfooding) moves all jobs to Firecracker.
   • §7.4 dogfood steps end with the #191 acceptance test, then the removal of nix:host and a rotation of the secrets.
   • New fixtures leftover.yaml and token-probe.yaml. §8 has new open questions: nix in the guest, and egress per label.
   • Phase 0 text corrected: --builders ssh-ng://nixos fails because oap/nix-config#178 removed @wheel from trusted-users. scripts/vm-check is replaced by --eval-store auto --store ssh-ng://nixos (README).
4. 49f7c03 records in PLAN.md §7.5 that this repo has its own review token.
5. 474acf9 takes claude-code from a pinned nixpkgs-master input. nixos-26.05 has 2.1.223, and claude-opus-5-5 needs 2.1.280 or newer.
6. 9e2b186 fixes the first review's findings:
   • §3.4 the VM firewall is an allowlist of public addresses. The old blocklist let guests reach the tailnet (100.64.0.0/10) and IPv6. VM→host is an input rule on iifname "fc-*". No IPv6 in v1. DNS defaults to public resolvers, not MagicDNS.
   • vm-isolation covers the tailnet, MagicDNS and IPv6, and its fake upstream has a public address.
   • license-boundary fails when go list fails.

Review token

This repo has its own CLAUDE_CODE_OAUTH_TOKEN, separate from nix-config's. nas provisions it (oap/nix-config#193, deployed 2026-10-08). Anyone with write access to this repo, Renovate included, can read it. The review job runs on the docker label, so leftover host processes (oap/nix-config#191) cannot read it.

Tests

• lint, lint-nix, proto-drift, license-boundary, unit and kvm-smoke pass with --store ssh-ng://nixos.
• fast and review pass on the docker label with the nixos/nix container.

🤖 Generated with Claude Code

The plan, Phase 0, and a revision of the plan for oap/nix-config#191. ## Commits 1. `59192ea` adds PLAN.md: a Forgejo Runner v13.1 plugin that runs each Actions job in its own Firecracker microVM. 2. `15fdfa7` Phase 0: Go module with the vendored `plugin.proto`, a stub daemon, `flake.nix` with the checks (lint, lint-nix, proto-drift, license-boundary, unit, kvm-smoke), the workflows and the review prompt. 3. `851a4d8` revises the plan after the oap/nix-config#191 test: - §5 new trust model. A `nix:host` job is not a boundary. The plugin gets its own runner instance and uid, with only `firecracker://` labels. Its socket belongs to that uid's group, with a `SO_PEERCRED` check. End state: remove `nix:host`. - §7.1 the runner is now user-scoped under `oap` (oap/nix-config#130, oap/nix-config#190). - §7.3 CI stage 1: jobs run only on the `docker` label. The VM tier runs by hand from the Mac before a merge. Stage 2 (after dogfooding) moves all jobs to Firecracker. - §7.4 dogfood steps end with the #191 acceptance test, then the removal of `nix:host` and a rotation of the secrets. - New fixtures `leftover.yaml` and `token-probe.yaml`. §8 has new open questions: nix in the guest, and egress per label. - Phase 0 text corrected: `--builders ssh-ng://nixos` fails because oap/nix-config#178 removed `@wheel` from `trusted-users`. `scripts/vm-check` is replaced by `--eval-store auto --store ssh-ng://nixos` (README). 4. `49f7c03` records in PLAN.md §7.5 that this repo has its own review token. 5. `474acf9` takes `claude-code` from a pinned `nixpkgs-master` input. nixos-26.05 has 2.1.223, and `claude-opus-5-5` needs 2.1.280 or newer. 6. `9e2b186` fixes the first review's findings: - §3.4 the VM firewall is an allowlist of public addresses. The old blocklist let guests reach the tailnet (100.64.0.0/10) and IPv6. VM→host is an input rule on `iifname "fc-*"`. No IPv6 in v1. DNS defaults to public resolvers, not MagicDNS. - `vm-isolation` covers the tailnet, MagicDNS and IPv6, and its fake upstream has a public address. - `license-boundary` fails when `go list` fails. ## Review token This repo has its own `CLAUDE_CODE_OAUTH_TOKEN`, separate from nix-config's. nas provisions it (oap/nix-config#193, deployed 2026-10-08). Anyone with write access to this repo, Renovate included, can read it. The review job runs on the `docker` label, so leftover host processes (oap/nix-config#191) cannot read it. ## Tests - `lint`, `lint-nix`, `proto-drift`, `license-boundary`, `unit` and `kvm-smoke` pass with `--store ssh-ng://nixos`. - `fast` and `review` pass on the `docker` label with the `nixos/nix` container. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

oap added 3 commits 2026-10-08 23:20:54 -07:00

Add implementation plan for the Firecracker runner plugin ... 59192eab6d

Covers the v13.1.0 plugin protocol requirements, the plugin and guest
agent architecture, phased delivery, and CI on Forgejo Actions using
NixOS VM tests with nested Firecracker. Records the Go and MIT decisions.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Phase 0: flake, CI workflows and nested-KVM smoke test ... 15fdfa7e82

- Go module with the runner's plugin.proto vendored unchanged and code
  generated by buf; a minimal daemon that passes the runner's handshake
  (health SERVING, Capabilities "firecracker") and leaves the job RPCs
  Unimplemented.
- flake.nix pinned to nixos-26.05 like nix-config, with the runner
  v13.1.0 source as a non-flake input. Checks: lint, lint-nix,
  proto-drift, license-boundary, unit, and kvm-smoke.
- kvm-smoke boots a stock Firecracker guest nested inside a NixOS test
  VM. Verified on the nixos runner host (about 30s). Firecracker hung
  when it inherited the test driver's console as stdin; it now gets
  /dev/null, and PLAN.md §3.2 records the rule for the plugin.
- .forgejo: check.yaml (fast tier, then VM tier one at a time) and
  review.yaml with a project-specific prompt, ported from nix-config.
- scripts/vm-check runs checks on the CI host from the Mac.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Revise the plan for nix-config #191: no host jobs, own runner instance ... 851a4d8090

A test on 2026-10-08 showed that a `nix:host` job on nixos can leave
processes behind and read the runner token (nix-config #191). The plan ran
this repo's CI on that label and called it safe. It was not.

- PLAN.md §5: a new trust model. The plugin gets its own runner instance
  and uid with only firecracker:// labels; the socket group is that uid,
  plus a SO_PEERCRED check. End state: remove nix:host.
- PLAN.md §7.1: the runner is user-scoped under oap (nix-config #130/#190).
  §7.3: CI stage 1 runs only in the docker label; the VM tier runs by hand
  from the Mac until stage 2 (Firecracker jobs). §7.4: dogfood steps end
  with #191's acceptance test and the removal of nix:host.
- New e2e fixtures leftover.yaml and token-probe.yaml repeat the #191
  probes; the vm-chaos leak assertion covers leftover processes.
- §8: new open questions on nix in the guest and egress per label.
- Phase 0 text: --builders ssh-ng://nixos fails because nix-config #178
  removed @wheel from trusted-users on nixos, not because of the Mac's
  daemon. An untrusted --store ssh-ng://nixos build works (kvm-smoke and
  unit verified), so scripts/vm-check is replaced by that one-liner in the
  README.
- check.yaml and review.yaml move from runs-on: nix to docker with a
  nixos/nix container. Not run yet: nothing is pushed.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

oap added 1 commit 2026-10-08 23:34:10 -07:00

PLAN.md: record the review token for this repo ... 49f7c03c69

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

oap added 1 commit 2026-10-08 23:35:45 -07:00

review: take claude-code from a pinned nixpkgs master ... 474acf9e0c

The review job failed: nixos-26.05's claude-code is 2.1.223, and
claude-opus-5-5 needs 2.1.280 or newer. Add a nixpkgs-master input, as
nix-config does, and expose only claude-code from it (2.1.293), with the
unfree allowance on that import. The job no longer needs --impure.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

forgejo-actions commented 2026-10-08 23:39:18 -07:00

Copy link

Claude review

Two things to fix before merge: the planned VM firewall leaves the host's tailnet and IPv6 reachable from guests, and the description and PLAN.md disagree about whether this repo has a review token. No problems found in the Phase 0 code beyond one check that can pass when it should fail.

1. Guests can reach the tailnet and IPv6 (PLAN.md:370–371, :730)

§3.4 masquerades guest traffic out of the host. It then drops only 169.254.0.0/16 and the RFC1918 ranges. §7.1 says the host has tailnet access. Two gaps follow:

• Tailnet: Tailscale addresses are in 100.64.0.0/10 (CGNAT), which is not RFC1918. A guest's traffic to nas or any other tailnet node would be NATed to the host's tailnet IP and let through. The same applies to Tailscale's DNS resolver, 100.100.100.100. If the host's /etc/resolv.conf points there, the §3.4 DNS default sends guests to it.
• IPv6: the table is inet but the rules list no IPv6 ranges. fc00::/7, fe80::/10 and the host's own v6 addresses are not covered.

What to change in the plan:

• Write the policy as an allowlist of public addresses. That matches what nix-config's docker label already does ("TCP 80/443 to public IPv4 only").
• Write "drop VM→host" as an input-chain rule on iifname "fc-*", not as a destination-address match. The host has many addresses (LAN, tailnet, TAP .1, podman bridges).
• Add 100.64.0.0/10 and the IPv6 cases to the vm-isolation row.

That row (line 730) has a second problem. It expects a "fake upstream (a node on the test network)" to be reachable through NAT. The NixOS test VLANs are 192.168.x.0/24, which is RFC1918, so under §3.4 that node must be blocked. As written, the test can only pass if the RFC1918 drop is broken or the node is allowlisted. Say which, or give the upstream node a public-range address.

2. The description and PLAN.md disagree on the review token (PLAN.md:35, :906)

• The description says the review job skips itself because "this repo has no CLAUDE_CODE_OAUTH_TOKEN. It stays that way until #191 is done."
• The decision log (line 35) says "This repo has its own token (2026-10-08)".
• §7.5 (line 906) says "The token (provisioned 2026-10-08)" and describes how to rotate it.

One of them is wrong. This matters for the trust model: §7.5 itself notes that anyone with write access, Renovate included, can read the secret with a pushed workflow. If the token exists, the description's "until #191 is done" is not true. If it doesn't, §7.5 should say it is deferred.

3. license-boundary passes when go list fails (flake.nix:192)

if go list -deps ./cmd/... | grep '^code\.forgejo\.org/forgejo/runner'; then

If go list errors, grep sees no match and the if branch is skipped, so the check passes. With or without pipefail the result is the same. A policy check should fail closed:

deps=$(go list -deps ./cmd/...)
if grep '^code\.forgejo\.org/forgejo/runner' <<<"$deps"; then …

---

_{claude-opus-5-5 on subscription · 474acf9e0c · 3 turns · in 4 · out 5.2k · cache read 93k · cache write 91.6k · $0.85 at API rates}

## Claude review Two things to fix before merge: the planned VM firewall leaves the host's tailnet and IPv6 reachable from guests, and the description and PLAN.md disagree about whether this repo has a review token. No problems found in the Phase 0 code beyond one check that can pass when it should fail. ### 1. Guests can reach the tailnet and IPv6 (PLAN.md:370–371, :730) §3.4 masquerades guest traffic out of the host. It then drops only 169.254.0.0/16 and the RFC1918 ranges. §7.1 says the host has tailnet access. Two gaps follow: - **Tailnet:** Tailscale addresses are in 100.64.0.0/10 (CGNAT), which is not RFC1918. A guest's traffic to nas or any other tailnet node would be NATed to the host's tailnet IP and let through. The same applies to Tailscale's DNS resolver, 100.100.100.100. If the host's `/etc/resolv.conf` points there, the §3.4 DNS default sends guests to it. - **IPv6:** the table is `inet` but the rules list no IPv6 ranges. fc00::/7, fe80::/10 and the host's own v6 addresses are not covered. What to change in the plan: - Write the policy as an allowlist of public addresses. That matches what nix-config's `docker` label already does ("TCP 80/443 to public IPv4 only"). - Write "drop VM→host" as an input-chain rule on `iifname "fc-*"`, not as a destination-address match. The host has many addresses (LAN, tailnet, TAP .1, podman bridges). - Add 100.64.0.0/10 and the IPv6 cases to the `vm-isolation` row. That row (line 730) has a second problem. It expects a "fake upstream (a node on the test network)" to be reachable through NAT. The NixOS test VLANs are 192.168.x.0/24, which is RFC1918, so under §3.4 that node must be blocked. As written, the test can only pass if the RFC1918 drop is broken or the node is allowlisted. Say which, or give the upstream node a public-range address. ### 2. The description and PLAN.md disagree on the review token (PLAN.md:35, :906) - The description says the review job skips itself because "this repo has no `CLAUDE_CODE_OAUTH_TOKEN`. It stays that way until #191 is done." - The decision log (line 35) says "This repo has its own token (2026-10-08)". - §7.5 (line 906) says "The token (provisioned 2026-10-08)" and describes how to rotate it. One of them is wrong. This matters for the trust model: §7.5 itself notes that anyone with write access, Renovate included, can read the secret with a pushed workflow. If the token exists, the description's "until #191 is done" is not true. If it doesn't, §7.5 should say it is deferred. ### 3. `license-boundary` passes when `go list` fails (flake.nix:192) ```sh if go list -deps ./cmd/... | grep '^code\.forgejo\.org/forgejo/runner'; then ``` If `go list` errors, grep sees no match and the `if` branch is skipped, so the check passes. With or without `pipefail` the result is the same. A policy check should fail closed: ```sh deps=$(go list -deps ./cmd/...) if grep '^code\.forgejo\.org/forgejo/runner' <<<"$deps"; then … ``` --- _{claude-opus-5-5 on subscription · 474acf9e0c · 3 turns · in 4 · out 5.2k · cache read 93k · cache write 91.6k · $0.85 at API rates}

oap added 1 commit 2026-10-08 23:50:41 -07:00

Fix the review findings: VM egress allowlist, fail-closed license check ... 9e2b186e68

- PLAN.md §3.4: the VM firewall is now an allowlist of public addresses.
  The old blocklist (169.254/16 and RFC1918) let guests reach the tailnet
  (100.64.0.0/10) and IPv6. VM→host is an input rule on iifname "fc-*".
  Guests get no IPv6 in v1. DNS defaults to public resolvers, not the
  host's resolv.conf, which points at MagicDNS on nixos.
- vm-isolation: covers the tailnet range, MagicDNS and IPv6. The fake
  upstream gets a public address, because the test VLANs are RFC1918 and
  the old row could only pass if the RFC1918 drop was broken.
- license-boundary: a failing `go list` now fails the check. Before, grep
  found no match and the check passed.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

forgejo-actions commented 2026-10-08 23:53:12 -07:00

Copy link

Claude review

Verdict: request changes. Two parts of the plan contradict each other, and the description contradicts the plan. The Phase 0 code itself raises nothing serious.

1. PLAN.md:302 vs §3.4 (PLAN.md:371): the host firewall can't see a TAP that has been moved into the jailer's netns

§3.2 says each VM gets "a new PID and network namespace, with the TAP moved into it". §3.4 builds all VM isolation on rules in the host's inet runner_firecracker table that match iifname "fc-*": drop on input, allow public addresses only on forward, drop VM→VM. Once the TAP is inside the jailer's netns, no packet from it reaches the host netns with iifname fc-*. Egress then needs a veth pair or routing out of that namespace. The host rules would match the veth's host-side name, or there would be no egress path at all.

As written, an implementation that follows §3.2 makes the §3.4 rules match nothing. vm-isolation would catch this only if it happens to fail closed. The vm-chaos leak assertion "no fc-* TAP" (§7.2) would also pass trivially, because it looks in the host netns.

Pick one design and write it down:

• TAP in the host netns (no --netns for networking).
• TAP in a per-VM netns plus a veth to the host. Name the host-side interface so fc-* still matches. State which netns each rule and the masquerade live in, and change the leak assertion to look for the host-side veth.

2. PLAN.md:35, PLAN.md:922 vs the PR description: is there a review token or not?

• The description says: "this repo has no CLAUDE_CODE_OAUTH_TOKEN. It stays that way until #191 is done."
• The decision log (line 35) says "This repo has its own token (2026-10-08)". §7.5 (line 922) says "The token (provisioned 2026-10-08)" and describes how it is provisioned and revoked.

This review ran, so a token does exist. Either the description or the plan is wrong. If the intent really was "no token until #191", then provisioning it already is a change to the trust model that should be stated. §7.5 itself says anyone with write access, Renovate included, can read the token.

Separately, head commit 9e2b186 ("VM egress allowlist, fail-closed license check") is not among the three commits listed in the description. Please add it.

3. .forgejo/workflows/review.yaml:82-84 and claude-review.sh:103: the PR under review supplies its own reviewer

The step runs .forgejo/scripts/claude-review.sh from the PR head checkout. The script reads .forgejo/review-prompt.md from that same checkout as --append-system-prompt. So any PR can rewrite the instructions and tools its own review runs with, and so can push the token out through a modified script.

§7.5 accepts that the token is readable by anyone with write access. Still, a reviewer whose prompt is set by the change it reviews is weaker than it needs to be. Fetch the script and the prompt from BASE_REF, for example with git fetch --depth 1 "$URL" "$BASE_REF" into a separate directory, and keep only the code under review from the PR head.

4. cmd/runner-firecracker/main.go:51-55: shutdown doesn't do what its comment says, and won't do what §3.5 asks

• The comment is wrong. It says NOT_SERVING tells the runner to stop sending jobs "before the listener goes". But GracefulStop() is called right after SetServingStatus. It closes the listener and sends GOAWAY at once, so the runner never gets a chance to read the new health status.
• Shutdown can hang forever once Phase 1 adds streams. GracefulStop waits for every in-flight RPC with no deadline, so a long Exec such as sleep infinity would block it. §3.5 asks for "finish active jobs until a deadline, then tear everything down".

What to change:

• Call health.Shutdown(), then wait for a grace period.
• Then run GracefulStop under a timer that falls back to Stop().
• Or, for now, cut the comment down to what the code actually does.

---

_{claude-opus-5-5 on subscription · 9e2b186e68 · 3 turns · in 4 · out 6.5k · cache read 95.3k · cache write 91.7k · $0.88 at API rates}

## Claude review **Verdict: request changes.** Two parts of the plan contradict each other, and the description contradicts the plan. The Phase 0 code itself raises nothing serious. ### 1. PLAN.md:302 vs §3.4 (PLAN.md:371): the host firewall can't see a TAP that has been moved into the jailer's netns §3.2 says each VM gets "a new PID and network namespace, with the TAP moved into it". §3.4 builds all VM isolation on rules in the host's `inet runner_firecracker` table that match `iifname "fc-*"`: drop on input, allow public addresses only on forward, drop VM→VM. Once the TAP is inside the jailer's netns, no packet from it reaches the host netns with `iifname fc-*`. Egress then needs a veth pair or routing out of that namespace. The host rules would match the veth's host-side name, or there would be no egress path at all. As written, an implementation that follows §3.2 makes the §3.4 rules match nothing. `vm-isolation` would catch this only if it happens to fail closed. The `vm-chaos` leak assertion "no `fc-*` TAP" (§7.2) would also pass trivially, because it looks in the host netns. Pick one design and write it down: - **TAP in the host netns** (no `--netns` for networking). - **TAP in a per-VM netns plus a veth to the host.** Name the host-side interface so `fc-*` still matches. State which netns each rule and the masquerade live in, and change the leak assertion to look for the host-side veth. ### 2. PLAN.md:35, PLAN.md:922 vs the PR description: is there a review token or not? - The description says: "this repo has no `CLAUDE_CODE_OAUTH_TOKEN`. It stays that way until #191 is done." - The decision log (line 35) says "This repo has its own token (2026-10-08)". §7.5 (line 922) says "The token (provisioned 2026-10-08)" and describes how it is provisioned and revoked. This review ran, so a token does exist. Either the description or the plan is wrong. If the intent really was "no token until #191", then provisioning it already is a change to the trust model that should be stated. §7.5 itself says anyone with write access, Renovate included, can read the token. Separately, head commit `9e2b186` ("VM egress allowlist, fail-closed license check") is not among the three commits listed in the description. Please add it. ### 3. `.forgejo/workflows/review.yaml:82-84` and `claude-review.sh:103`: the PR under review supplies its own reviewer The step runs `.forgejo/scripts/claude-review.sh` from the PR head checkout. The script reads `.forgejo/review-prompt.md` from that same checkout as `--append-system-prompt`. So any PR can rewrite the instructions and tools its own review runs with, and so can push the token out through a modified script. §7.5 accepts that the token is readable by anyone with write access. Still, a reviewer whose prompt is set by the change it reviews is weaker than it needs to be. Fetch the script and the prompt from `BASE_REF`, for example with `git fetch --depth 1 "$URL" "$BASE_REF"` into a separate directory, and keep only the code under review from the PR head. ### 4. `cmd/runner-firecracker/main.go:51-55`: shutdown doesn't do what its comment says, and won't do what §3.5 asks - **The comment is wrong.** It says NOT_SERVING tells the runner to stop sending jobs "before the listener goes". But `GracefulStop()` is called right after `SetServingStatus`. It closes the listener and sends GOAWAY at once, so the runner never gets a chance to read the new health status. - **Shutdown can hang forever once Phase 1 adds streams.** `GracefulStop` waits for every in-flight RPC with no deadline, so a long `Exec` such as `sleep infinity` would block it. §3.5 asks for "finish active jobs until a deadline, then tear everything down". What to change: - Call `health.Shutdown()`, then wait for a grace period. - Then run `GracefulStop` under a timer that falls back to `Stop()`. - Or, for now, cut the comment down to what the code actually does. --- _{claude-opus-5-5 on subscription · 9e2b186e68 · 3 turns · in 4 · out 6.5k · cache read 95.3k · cache write 91.7k · $0.88 at API rates}

All checks were successful

Hide checks

check / fast (pull_request) Successful in 1m57s

Details

review / review (pull_request) Successful in 2m42s

Details

This pull request can be merged automatically.

You are not authorized to merge this pull request.

View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.

git fetch -u origin claude/plan-rev-191:claude/plan-rev-191

git switch claude/plan-rev-191

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels

No items

No labels

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

2 participants

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference

oap/runner-firecracker!1

No description provided.