Repository navigation
fix: scope cache entries to host-authenticated job refs - #14
Conversation
linyiru
left a comment
There was a problem hiding this comment.
Review of the direction and the riskier details.
The design matches GitHub's model, and I'd keep it. Authority comes only from the daemon's authenticated data, never from the guest. Records are per-slot with a generation, revoked before the slot is reused, and leased. The source pinning sits before the established-connection and cache-port accepts in both tap and netns modes, and the injection test exercises it. Every RPC and blob route authorizes the caller, and the restore order is own ref → PR base → default, exactly as GitHub documents.
To fix before merging
- The default-ref fallback ignores the job's repository (
scope.go:45). With an organisation-scoped scale set, a job from another repository, including a fork PR, can read this repository's default-branch cache while its record is pending or unresolvable. - Record permissions force the cache-server, which parses guest HTTP, to share the daemon's uid and with it the App key (
identity.go:126). Running it as another user fails silently. - One
--repositoryper cache-server (scope.go:19). Namespacing by the record's repository would remove the flag and fix (1).
Hit rate: these checks are stricter than GitHub and refuse common runs.
- Branch writes need the branch to still sit at the run's SHA (
github.go:240). - PR resolution needs the PR head to still be the run's SHA (
github.go:197). JobWorkflowRefwould settle most of the ambiguity for push and pull_request without extra calls (github.go:247).
Smaller: per-run memoization and a narrowly scoped installation token (github.go:66), slashed branch names (github.go:237), observability for unresolved identities (registry.go:195), the upgrade order (README), and an empty-set guard in the egress ruleset (egress.go:154).
I read the code; I didn't build or run it.
There was a problem hiding this comment.
🟡 Changes recommended
Triggering-ref inference still permits unauthorized trusted-branch cache writes.
1 open finding
What changed in this PR
Scopes caches to host-authenticated repository and job refs to address cross-ref cache poisoning in #13.
Changes:
- Adds authenticated job records, ref-scoped authorization, and VM-generation checks.
- Enforces guest source addresses and documents isolated cache-service deployment.
- Expands unit, protocol, firewall, and real-VM validation.
| File | Description |
|---|---|
| test/e2e/smoke_init.go | Exercises cache isolation from guests. |
| test/e2e/run.sh | Runs KVM tests in disposable namespaces. |
| test/e2e/README.md | Documents real-VM validation. |
| test/e2e/e2e_test.go | Tests cache scopes and VM cleanup. |
| test/e2e/build-smoke-rootfs.sh | Updates usage comments. |
| test/cache-protocol/worker.mjs | Runs real cache-toolkit operations. |
| test/cache-protocol/test.mjs | Validates archive transfers and isolation. |
| test/cache-protocol/README.md | Documents integration and rollout checks. |
| test/cache-protocol/package.json | Defines toolkit test dependencies. |
| test/cache-protocol/package-lock.json | Locks toolkit dependencies. |
| README.md | Documents authorization and migration. |
| Makefile | Expands coverage and integration targets. |
| internal/provisioner/firecracker.go | Registers identities and quarantines failed revocations. |
| internal/provisioner/firecracker_test.go | Tests revocation before slot reuse. |
| internal/provisioner/egress.go | Enforces exact guest source bindings. |
| internal/provisioner/egress_test.go | Tests binding generation and validation. |
| internal/provisioner/egress_netns_test.go | Tests kernel source-spoof rejection. |
| internal/listener/scaleset.go | Dispatches authenticated job metadata. |
| internal/listener/listener_test.go | Tests identity callback wiring. |
| internal/jobidentity/registry.go | Publishes and revokes expiring identities. |
| internal/jobidentity/permissions_linux_test.go | Validates separate-user permissions. |
| internal/jobidentity/identity.go | Validates records and trusted ownership. |
| internal/jobidentity/identity_test.go | Tests identity lifecycle and rejection. |
| internal/jobidentity/github.go | Resolves GitHub-backed cache policies. |
| internal/jobidentity/github_test.go | Tests resolver policies and memoization. |
| internal/config/config.go | Configures the identity directory. |
| internal/cacheserver/scope.go | Binds authorization to VM generations. |
| internal/cacheserver/scope_test.go | Tests scope isolation and stale requests. |
| internal/cacheserver/cacheserver.go | Authorizes cache operations by repository/ref. |
| internal/cacheserver/cacheserver_test.go | Adapts tests to authenticated identities. |
| internal/cacheserver/artifactproxy_test.go | Uses authorized test stores. |
| go.mod | Declares JWT as a direct dependency. |
| deploy/firerunner.service | Adds daemon runtime-directory ownership. |
| deploy/firerunner-jailer@.service | Adds instance-specific identity directories. |
| deploy/firerunner-cache.service | Adds an isolated cache service. |
| cmd/firerunner/main.go | Wires identity resolution and cache authorization. |
| cmd/firerunner/main_test.go | Tests unsafe configuration rejection. |
| .gitignore | Excludes protocol-test dependencies. |
| .github/workflows/ci.yml | Adds permissions and protocol validation. |
Files not reviewed (1)
- test/cache-protocol/package-lock.json: Generated file
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
babysit-pr · head |
|
babysit-pr · head |
|
babysit-pr · head |
|
babysit-pr · head |
|
babysit-pr · head |
|
babysit-pr · head |
|
babysit-pr · head |
There was a problem hiding this comment.
🟡 Changes recommended
Generation binding leaves an authorization gap for queued connections, and the documented URL-only cache configuration is firewall-blocked.
2 open findings
1 resolved since last review
Files not reviewed (1)
- test/cache-protocol/package-lock.json: Generated file
🧠 Review effort: Balanced
| host, _, err := net.SplitHostPort(remote) | ||
| if err == nil && s.lookup != nil { | ||
| i, _ := s.lookup(host) // a valid pending lease has a generation but no job yet | ||
| generation = i.Generation |
There was a problem hiding this comment.
Flagged for human review — the mechanism is real. ConnContext runs at Accept, so a connection the old VM completed but the listener had not accepted yet takes whatever generation record exists at accept time. Exploiting it needs the accept loop to stall for longer than revoke plus a VM boot, which takes seconds. That is unlikely on a healthy host, but it can happen under host overload, and the README promises that queued requests never acquire the next occupant's authority.
A candidate fix that stays within this design: store the generation's issue time in the record. In ConnContext, read TCP_INFO (tcpi_last_data_recv / tcpi_last_ack_recv, milliseconds since the peer last sent anything). If the peer's last packet is older than the current generation, bind the connection to no generation. This is Linux-only and needs a socket-level regression that delays accept. Per-generation addresses or credentials are the heavier alternative. This is a security design decision for the author, so I have not changed the code.
| // --cache-url-only deployment (same host, see CacheURL) is not opened here: the | ||
| // operator must accept that address and port on INPUT. It first |
There was a problem hiding this comment.
Flagged for human review — the finding holds. The iifname <prefix>* drop in firerunner's own input chain was already on main. Before this PR, though, --cache-url meant a remote server reached through FORWARD. Now that remote servers cannot be supported (the server authorizes by guest source IP), a URL-only setup has no working path at all. My earlier doc fixes (56422ee, aecb70d, 804ba83) also say the operator can open INPUT themselves, and that is wrong: an accept in another table cannot override this drop.
The decision needed is what --cache-url should be now:
(1) --cache-url requires --cache-port with the same port, and the firewall keeps keying off --cache-port. This is a small change: config validation, docs and a test.
(2) firerunner derives the destination and port from the URL and opens them itself, including netns mode. This is a larger change.
(3) Remove --cache-url.
|
babysit-pr · head
|
There was a problem hiding this comment.
🔵 Needs a closer look
The authorization redesign crosses VM, network, and filesystem trust boundaries, with prior design questions still requiring human security review.
2 open findings
Files not reviewed (1)
- test/cache-protocol/package-lock.json: Generated file
🧠 Review effort: Balanced


A fork PR could previously reserve a cache key that a later default-branch job restored. Cache entries now belong to
(repository, ref, key, version), with authority derived exclusively from the daemon's authenticated job/run metadata and the guest's source address. One cache server supports all repositories served by its daemon.--repository/--default-refoverrides. Resolved low-trust events get default-only reads for their own repository.*.ghe.comAPI routing and the scaleset client's GHES override.--jobs-owner-uid, startup readability checks, and owner/permission checks on every read. Include a cache systemd unit with read-only/run, no capabilities and separate state storage. Document daemon-first deployment after stopping the old insecure cache server, runtime directory ownership and private App keys.firerunner_cache_identity_denied_total.Validation:
CI on
c6b2f69passes all three PR jobs. Linux coverage is 82.2% (minimum 70%).Local full Go race/coverage gate: 85.0% (minimum 70%); build/vet and E2E Linux compilation pass.
Real
@actions/cachearchive save/extract integration passes, including repository isolation, own/base/default precedence, sibling PR denial, reruns, resolved read-only access and unknown-identity denial.starship, 2026-10-14 22:31 UTC, rerun at
c6b2f69: rebuilt the smoke rootfs from this commit. Firecracker 1.15.1 with guest Linux 6.1.102 passes boot/self-destruct plus 10 cache guest scenarios (9.60 seconds total): main, base, PR, sibling PR, rerun, main-after, unknown identity, repository B, repository A again, and read-only. Every guest reports success, reuses the same IP, and reclaims its tap/slot; each cache guest revokes its job record. The disposable network namespace is removed (before/after namespace lists match). Logs retained on starship at/var/tmp/firerunner-pr14-c6b2f69-e2e.H94F2S/e2e.log.Kernel netns firewall/source-spoof probes pass on starship in tap/veth and open/allowlist modes.
A real Linux subprocess running as UID 65534 can read daemon-owned records through its group but cannot create, overwrite or delete them, or read the daemon's 0600 secret. Foreign-owner records are rejected. This test also runs in CI.
API regressions cover PR retargeting, missing/invalid run base metadata, commit fallback with live base metadata, memoized and expired policies, hosted/GHES URL routing and override presence, advanced heads, slashed branches, repository-scoped token reuse, 30 concurrent matrix jobs (one three-request resolution), memo expiry and repository/event isolation. HTTP tests cover pending/expired/unknown repositories, foreign blob URLs and stale VM connections.
Review decisions:
JobWorkflowRefis retained as metadata but deliberately cannot select triggering scope. Even a same-repository reusable workflow can reference another branch (GitHub context documentation). Branch/tag REST probes therefore remain and require Contents read; resolver tokens request only Actions/Pull requests/Metadata/Contents read for the requested repository. Ambiguous PR associations, tags and deleted branches still fail closed. Guest requests do not initiate privileged resolver retries after the host's three-attempt/30-second window.The real-VM suite replaces only GitHub identity resolution with a host fixture; cache HTTP, registry, firewall, guest execution and teardown are real. Live GitHub assignment/API resolution remains a rollout check. KVM CI is manually dispatched; PR CI runs Go/race/coverage, separate-UID permissions, kernel netns probes and the real toolkit integration. Source-NAT/proxy deployments and guest
cache-modeoverrides are unsupported. Legacy entries without a ref are never served.Closes #13.