feat(agent): guest enforcement — ro mirror, rw grouped, expiry and revocation (G1c) - #102
Conversation
…vocation (G1c) Per G1 spec §3-§4. The offer path now dispatches on the principal that signed the binding and the SDP-bound challenge, not the routed owner_id: an offer proven by a key that is not a pinned owner is served as a guest iff a stored grant names this owner and machine, is bound to that key, verifies, is not tombstoned, and its window covers now — re-checked on EVERY attach. The owner path is byte-identical (same principal == routed owner). Read-only is a pane mirror, not a tmux client: capture-pane paints once, pipe-pane streams the scope's active pane, and guest input frames are read and dropped at the agent — no shell, no control channel, no window snapshot, so a ro guest sees one pane's output and nothing else. Read-write rides D4's grouped sessions as guest-<gid[:8]> (kill guard, sweep, and snapshot filter extended to guest-*) with tmuxPid=0 and no control handler, so a guest gets a real tmux client but no agent-level FrameControl. A per-session deadline at na and a gid-keyed revoke registry both cancel the serve context, so expiry and revoke-grant tear a live guest down at once. revoke-grant (same session-owner gate as add-grant) tombstones the gid, drops live sessions, and acks; startup sweeps grant files past na. Client gains Machine.Owner so a guest attach routes under the machine owner while authenticating as the guest. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KeiotDVE94wEzvc7wcvm1y
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c24b1e3e9e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| // Guest path: a stored, valid grant for THIS owner and machine, bound to the | ||
| // principal key. ValidAt + tombstone are re-checked here on every attach. | ||
| grant := findValidGuestGrant(rt.cfg.Dir, owner, rt.cfg.MachineID, principal, time.Now()) |
There was a problem hiding this comment.
If an owner revokes the grant after this check but before ICE and Noise finish, guests.drop sees no registered session; serveGuest subsequently registers the cached grant without checking the tombstone again and serves it until expiry. Register and then recheck revocation, or otherwise synchronize authorization with registration, so a revoke either cancels the session or prevents it from starting.
Useful? React with 👍 / 👎.
| if err := syscall.Mkfifo(fifo, 0o600); err != nil { | ||
| return err | ||
| } | ||
| if err := exec.Command("tmux", "pipe-pane", "-O", "-t", target, "cat > "+fifo).Run(); err != nil { |
There was a problem hiding this comment.
When two read-only grants observe the same pane, the second pipe-pane replaces the first because tmux permits only one pipe command per pane; the first reader then gets EOF, and its deferred no-command pipe-pane closes the second pipe as well. This also destroys any pre-existing user pipe such as pane logging. The tmux manual states that a pane may connect to only one command and an existing pipe is closed before the new command runs, so sharing needs one managed pipe fan-out rather than one pipe per guest.
Useful? React with 👍 / 👎.
| if err := ensureGroupedBase(grant.Scope); err != nil { | ||
| return err | ||
| } | ||
| name := guestSessionName(grant.GID) |
There was a problem hiding this comment.
If the same writable guest opens a second tab or reconnects before the old connection finishes, both attaches derive the identical session name. Since groupedLaunch uses new-session -A, the second client attaches to that existing session, and either connection's deferred killGroupedSession then kills the other live connection. The tmux manual documents that -A behaves like attach-session when the named session exists; add per-attach uniqueness while retaining the guest prefix used by cleanup.
Useful? React with 👍 / 👎.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit c24b1e3. Configure here.
| return rt.serveGuestRW(gctx, mc, sess, grant) | ||
| } | ||
| return rt.serveGuestRO(gctx, mc, sess, grant) | ||
| } |
There was a problem hiding this comment.
Revoke misses in-flight guest attaches
High Severity
authorizeOffer is the only tombstone check, then WebRTC and Noise can run for up to 20s before serveGuest registers the session. A revoke-grant in that window tombstones the gid and drops an empty registry, after which the in-flight attach still starts and stays up until na.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit c24b1e3. Configure here.
| if err := exec.Command("tmux", "pipe-pane", "-O", "-t", target, "cat > "+fifo).Run(); err != nil { | ||
| return err | ||
| } | ||
| defer exec.Command("tmux", "pipe-pane", "-t", target).Run() // stop piping |
There was a problem hiding this comment.
Concurrent RO guests steal the pane pipe
Medium Severity
Each read-only serve runs pipe-pane on the scope pane and later closes that pipe with a bare pipe-pane. tmux allows only one pipe per pane, so a second RO guest replaces the first stream, and the first guest's cleanup then tears down the second guest's pipe as well.
Reviewed by Cursor Bugbot for commit c24b1e3. Configure here.
1 participant
The security core of guest sharing (G1 spec §3 enforcement, §4 revocation), adversarial-tests-first. Part of #55.
How the guest offer path verifies
An offer carries the machine owner's id in its relay routing (so it reaches the agent's registration) but its binding + SDP-bound auth are signed by whoever is attaching — the principal.
authorizeOffernow parses the binding, verifies it, checks the auth overAttachChallenge(session, machine, sdp)against the principal key, and then:findValidGuestGrant(owner, machine, principalKey, now)— a stored grant that names this owner and machine, is bound to the principal key, verifies against the owner signature, is not tombstoned, and whose window covers now. Re-run on every attach, so expiry/revocation stop working without touching the file. Noise-KK then pins the guest's bound X25519.Enforcement (§3)
capture-pane -epaints once,pipe-pane -Ostreams the scope's active pane; guest input frames are read and dropped at the agent (logged), never written to any pane. No shell, no FrameControl, no FrameWindows — the guest sees one pane's output and nothing else. A missing scope session is an honest "[this share's session isn't running]", not a crash.guest-*), served withtmuxPid=0and no control handler — a real tmux client (Ctrl-B works in their own client) but no agent-level control channel.naand a gid-keyed revoke registry both cancel the serve context.revoke-grant(same session-owner gate as add-grant) tombstones the gid, drops every live session serving it, and acks with a HELLO; startup sweeps grant files pastna. No relay change — revocation is agent-local, backstopped by the 24 h TTL cap.Adversarial tests (all pass; live-tmux ones skip under -short / without tmux)
TestGuestROMirrorsOutputAndDropsInput— differential on a shell scope: the guest sees realsend-keysoutput (REALDATA) but its injectedecho INJECTEDnever echoes → input truly dropped.TestGuestRWKeystrokesLand— a rw guest'stouch <marker>runs in the shared shell; theguest-*session exists while serving and is gone after detach.TestGuestExpiryDropsLiveSession— na 1 s out → torn down with "[this share has ended]" and serveGuest returns.TestGuestRevokeDropsAndBars— revoke-grant drops the live session ("ended by the owner"), acks, and a later attach with the tombstoned grant refuses.TestAuthorizeOfferGuestRejections(no tmux): no grant / expired / not-yet-valid / tombstoned / wrong owner / wrong machine / wrong guest key / tampered auth / auth over a different SDP / session replay — every one refused.TestAuthorizeOfferOwnerStillWorks+ the existing owner/pin-set tests gate the byte-identical owner path.go test ./...+-raceon agent/client/identity green,gofmt -lempty,go vetclean, web 157/157. No vectors changed; relay untouched.🤖 Generated with Claude Code
https://claude.ai/code/session_01KeiotDVE94wEzvc7wcvm1y
Note
High Risk
Changes attach authorization, guest confinement (including read-write shell access), and revocation semantics—security-critical paths where mistakes could allow unauthorized access or insufficient guest isolation.
Overview
Implements G1c guest sharing enforcement on the agent and wires the client so guests can attach under the machine owner’s relay route while authenticating with their own keys.
Attach authorization now treats the offer’s signed binding as the principal (not only the routed
owner_id). Pinned owners keep the same path; everyone else must match a stored grant viafindValidGuestGrant(owner, machine, guest key, signature, validity window, tombstone check) on every attach, with session replay still keyed by principal.Guest sessions branch after Noise-KK: read-only mirrors one tmux scope with
capture-pane+pipe-paneand drops all guest input; read-write uses a groupedguest-<gid[:8]>tmux client with no agent control channel. Live guests honor grantnavia context deadline andrevoke-grant(owner control) tombstones the gid, removes the grant file, and cancels registered sessions. Startup sweeps expired grant files; grouped-session cleanup/snapshots now includeguest-*alongsidemir-*.Client:
Machine.Ownerroutes/attachto the machine owner while the guest signs the offer;mir joinpersists that field.RevokeGrantOverSessionsends revocation over an owner session (CLI command noted as G1d).Coverage includes authorization rejection cases and live-tmux tests for ro input drop, rw keystrokes, expiry, and revoke.
Reviewed by Cursor Bugbot for commit c24b1e3. Bugbot is set up for automated code reviews on this repo. Configure here.