Skip to content

fix: scope cache entries to host-authenticated job refs - #14

Merged
linyiru merged 12 commits into
mainfrom
fix/cache-ref-isolation
Oct 16, 2026
Merged

linyiru merged 12 commits into
mainfrom
fix/cache-ref-isolation

Conversation

@linyiru

@linyiru linyiru commented Oct 14, 2026 •

Copy link
Copy Markdown
Member

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.

  • Publish atomic, expiring job records tied to VM generations. Persist authenticated repository/job metadata before REST resolution; missing, pending, invalid, expired and unresolved identities get no cache reads or writes. Remove global --repository / --default-ref overrides. Resolved low-trust events get default-only reads for their own repository.
  • Revoke records before slot reuse, reject late lookups, and quarantine slots on revocation failure. Pin TCP connections and delayed requests to the VM generation that opened them.
  • Bind tap and netns host-veth traffic to each slot's exact guest IP before connection/cache accepts, including firewall refresh. Reject invalid source-binding configuration rather than install an empty set.
  • Authorize every RPC and blob operation. Reservations belong to their creating job; completed archives stay immutable. Restore exhausts own ref, PR base, then default, including prefix matches. Repository A/B can save identical keys without reading or overwriting each other's entries.
  • Restrict optional PR base reads to a matching base in both the workflow-run association and current PR. Retargeted PRs, absent run base metadata and commit-association fallback retain PR-own/default reads without granting the live PR's new base. Support Enterprise Cloud *.ghe.com API routing and the scaleset client's GHES override.
  • Support a separate cache-server UID: daemon-owned 0750 directories / 0640 records, dedicated reader group, explicit --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.
  • Allow queued/rerun jobs after branch/PR heads advance. Preserve slash-separated branch paths, coalesce concurrent matrix resolution, cache successful run policy for two minutes, and scope installation tokens to one repository plus required read permissions. Log unresolved jobs/timeouts and bounded first lookup failures; expose firerunner_cache_identity_denied_total.

Validation:

  • CI on c6b2f69 passes 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/cache archive 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: JobWorkflowRef is 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-mode overrides are unsupported. Legacy entries without a ref are never served.

Closes #13.

@linyiru
linyiru marked this pull request as ready for review October 14, 2026 18:56

@linyiru linyiru left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  1. 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.
  2. 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.
  3. One --repository per 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).
  • JobWorkflowRef would 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.

Comment thread internal/cacheserver/scope.go Outdated
Comment thread internal/cacheserver/scope.go Outdated
Comment thread internal/jobidentity/identity.go Outdated
Comment thread internal/jobidentity/github.go Outdated
Comment thread internal/jobidentity/github.go Outdated
Comment thread internal/jobidentity/github.go Outdated
Comment thread internal/jobidentity/registry.go Outdated
Comment thread internal/listener/scaleset.go Outdated
Comment thread README.md Outdated
Comment thread internal/provisioner/egress.go

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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.

Comment thread internal/jobidentity/github.go
@linyiru

linyiru commented Oct 15, 2026

Copy link
Copy Markdown
Member Author

babysit-pr · head 4a44446
Fixed: review threads: 11 of 12 were already addressed by 8dd0e63 and c6b2f69, and each has a reply. Review body: its items 1–3, the hit-rate items and the smaller items map onto those same threads.
Declined: JobWorkflowRef as trigger ref: a same-repo reusable workflow pinned to @main would grant a PR job the main write scope.
CI: green (4 passed) at c6b2f69.
Local review (subagent): 40e3b79..c6b2f69. Adopted 2 doc findings: stale remote --cache-url advice (56422ee) and a stale handleGetDownloadURL precedence comment (4a44446).

@linyiru

linyiru commented Oct 15, 2026

Copy link
Copy Markdown
Member Author

babysit-pr · head aecb70d
Needs a person: deleted-tag rerun gains branch write. This is Copilot's high finding, and it is real. The fix is a choice between three options on the thread: tip-SHA equality, compare ancestry, or deny when ambiguous. That choice is the PR author's.
CI: green (4 passed) at 4a44446.
Local review (subagent): c6b2f69..4a44446. Adopted 1 finding: the ensureHostCacheInput comment still described a remote --cache-url (aecb70d).

@linyiru

linyiru commented Oct 15, 2026

Copy link
Copy Markdown
Member Author

babysit-pr · head 804ba83
Fixed: resolved the 11 Fixed threads. Checks are green on aecb70d, which contains every fix.
Needs a person: deleted-tag rerun gains branch write. Unchanged: pick (a), (b) or (c) on the thread.
Local review (subagent): 4a44446..aecb70d. Adopted 1 finding: the inline INPUT comment contradicted its function comment (804ba83). This was the third finding on the stale --cache-url docs, so the run stops here and leaves them to the author rather than looping on wording.

@linyiru

linyiru commented Oct 15, 2026

Copy link
Copy Markdown
Member Author

babysit-pr · head 00efc4c
Fixed: deleted-tag rerun gains branch write in 9d8c2ce, with the author's chosen option (d). Push runs need a repository-activity record that updated the branch to the run's SHA; dispatch and schedule runs, and GHES without the activity endpoint, need compare ancestry. The thread stays open until CI is green.
Fixed (local review): 00efc4c makes an unproven run keep default-only reads instead of losing its whole identity. It also reads only status from compare, whose payload can exceed the 2 MiB limit, and skips compare when the branch still points at the SHA.
Local review (subagent): aecb70d..9d8c2ce. 2 findings, both adopted, in 00efc4c.

@linyiru

linyiru commented Oct 15, 2026

Copy link
Copy Markdown
Member Author

babysit-pr · head fe10a9a
Local review (subagent): 9d8c2ce..00efc4c. Adopted 1 test gap: nothing pinned the compare skip at the branch tip, now covered in fe10a9a. Without the skip, the new case fails.

@linyiru

linyiru commented Oct 15, 2026

Copy link
Copy Markdown
Member Author

babysit-pr · head fe10a9a
Correction: the previous comment's reviewed marker carried a malformed SHA. The local review covered 00efc4cf7ac77e5473b32e64d98a67323085ed4a.

@linyiru

linyiru commented Oct 15, 2026

Copy link
Copy Markdown
Member Author

babysit-pr · head fe10a9a
Resolved the deleted-tag thread: checks are green on a head that contains the fix.
Local review (subagent): 00efc4c..fe10a9a. No findings.

@linyiru
linyiru requested a balanced review from Copilot October 15, 2026 04:51

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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

Comment on lines +35 to +38
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

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +564 to +565
// --cache-url-only deployment (same host, see CacheURL) is not opened here: the
// operator must accept that address and port on INPUT. It first

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@linyiru

linyiru commented Oct 15, 2026

Copy link
Copy Markdown
Member Author

babysit-pr · head fe10a9a
Needs a person (PR author):

  • Queued connections inherit the next generation (Copilot, high). This is a security design choice: a TCP_INFO last-received check against the generation issue time, or per-generation credentials.
  • URL-only --cache-url is firewall-blocked (Copilot, medium). The choice is what --cache-url should be: require --cache-port, open the port from the URL, or remove the flag. My earlier doc fixes for this flag were wrong about INPUT.
    CI: green (4/4). Copilot has reviewed this head (2 requests this run).

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 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

@linyiru
linyiru merged commit e69d4cd into main Oct 16, 2026
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

cache-server: scope cache entries by the job's ref, as GitHub does

2 participants