Repository navigation
docs(review-on-done): peer sender identity is a claim, not an authentication - #112
Merged
Merged
Conversation
…ication Phase 1b.1 needs "the authenticated session identity of the REVIEWER". Before that can be built, the two candidate sources have to be compared, and the comparison is not close. codec.ts documents the correct rule: from/fromName are display only, and "provenance for authorization purposes is whatever authenticated socket connection this arrived on, never these fields". lifecycle.ts then passes the envelope field to resolveOpencodeSender. There is no third path, and no connection identity to fall back on: SO_PEERCRED and getpeereid appear zero times in packages/, verified with a control pattern (peerToken, 22 hits) so the zero cannot be a silently failing search. The sidecar peerToken does not close the gap. It is presented by any sender to deliver to a session, so it authenticates the destination, not the sender. So socket-resolved identity is a registry-resolved claim. A peer that writes from = uds:opencode-skein:<another session> is attributed to that session, and verifyLead then compares it against grant.lead.sessionID and grants the scopes. No test covers that, which is why this is written as the failing case rather than as a complaint. Recommendation: server-attributed. Not because it is better in degree, but because socket-resolved cannot close the hole it appears to close. The cost is that authenticity stays open longer while the project server lands — an unenforceable check being worse than a visibly missing one. Also flags that 1b.2 is not sound alone: denying edit/write of the record is not a control while scoped unattended bash can write the same path, and a fence that holds in tests but is bypassable in practice is the more expensive failure. No code change. This is the input to the decision, not the decision; publishing is the user's.
codec.ts:95-102 and lead.ts:195-201, each verified by reading the line at both ends of the range. An earlier check appeared to show lead.ts:195 was wrong; that was the check resolving src/cli/cmd/lead.ts instead of src/peer/lead.ts.
The user rejected the project server, so the premise of my recommendation is gone. Recording that rather than quietly rewriting it: the reasoning was sound against D5, and leaving it standing would point the decision at a destination that does not exist. Two corrections that came out of consulting the peer session that owns lead authority, after it re-checked the code path itself: 1. I cited the absence of SO_PEERCRED/getpeereid as a gap in this codebase. It is a platform constraint: probed on Bun 1.3.14 over a live unix socket, Socket exposes no getPeerName, getPeerCredentials or getPeerCert. An unforgeable connection identity is not reachable here at all, so any proposal resting on peer credentials is not implementable as written. 2. #111 is inert as merged. Nothing passes authorSessionID yet, so the driver check enforces nothing. Recorded as driver-side plumbing rather than a control in force. What survives is the conclusion, not the route: callback confirmation, where the receiver asks the claimed socket whether it sent a given msg_id and the real session answers from its own sent-log. Roughly one frame type plus a sent-log, and it hardens verifyLead as well. The residual belongs in the document rather than in a chat log: against a same-user shell-capable agent, nothing short of OS separation makes sender identity unforgeable. That is why the controls are bounded claims, and why 1b.1 should have the record written by a tool from a session id the tool holds, never parsed from message text.
|
This PR doesn't fully meet our contributing guidelines and PR template. What needs to be fixed:
Please edit this PR description to address the above within 2 hours, or it will be automatically closed. If you believe this was flagged incorrectly, please let a maintainer know. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this is
A finding, not a fix.
review-on-donePhase 1b needs the review record written with "the authenticated session identity of the REVIEWER". This documents what the codebase actually provides today, so the choice between topologies is made on evidence rather than on a chat log.The finding
packages/opencode/src/peer/claude/codec.ts:95-102documents the rule correctly:from/fromNameare display only, and "provenance for authorization purposes is whatever authenticated socket connection this arrived on, never these fields."lifecycle.ts:122-123then passes the envelope field toresolveOpencodeSender. There is no third path. Both branches ofroute.ts:203-208are functions of a string the sender chose:opencodeSenderOfstrips a prefix and returns the remainder, and the UDS branch looks up a socket path the sender also supplied.So a peer that writes
from = uds:opencode-skein:<another session>is attributed to that session, andverifyLeadgrants the lead's scopes off that match. Written up as a failing test rather than a complaint: no such test exists today, which is why it went unnoticed.Two corrections to my own first draft
Peer credentials are not available in this runtime. I cited the absence of
SO_PEERCRED/getpeereidas a gap in this codebase. Probed against Bun 1.3.14 over a live unix socket,Socketexposes nogetPeerName,getPeerCredentialsorgetPeerCert. An unforgeable connection identity is unreachable here, not merely unimplemented — so any proposal resting on peer credentials is not implementable as written.The server-attributed recommendation is withdrawn. My original draft recommended in-server attribution per
project-serverD5. The user rejected the project server, so the premise is gone. Recorded as withdrawn rather than deleted so the reasoning is not quietly rewritten.What survives
Callback confirmation: the receiver asks the claimed socket whether it sent a given
msg_id, and the real session answers from its own in-process sent-log, which a forger does not have. Roughly one frame type plus a sent-log, no server. It also hardensverifyLeadindependently of review gating.The residual is stated rather than implied: against a same-user shell-capable agent, nothing short of OS separation makes sender identity unforgeable. Callback confirmation bounds the claim; it does not make it absolute.
Also recorded
authorSessionIDyet, so the driver check enforces nothing. Plumbing, not a control in force.edit/writeof the record is not a control while scoped unattended bash can write the same path. A fence that holds in tests but is bypassable in practice is the more expensive failure.Verification
Docs only; no code change. Every line citation spot-checked against explicit paths after finding that two files named
lead.tsexist underpackages/.fork:verify250/250 owned, 117/117 patched.🤖 Generated with Claude Code