From 6adf23b513f1375e88f184f2aff1d9df8f745213 Mon Sep 17 00:00:00 2001 From: AndreasS Date: Sat, 3 Oct 2026 05:42:14 +0200 Subject: [PATCH 1/4] docs(review-on-done): peer sender identity is a claim, not an authentication MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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: 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. --- .../review-on-done/findings-authenticity.md | 114 ++++++++++++++++++ 1 file changed, 114 insertions(+) create mode 100644 openspec/changes/review-on-done/findings-authenticity.md diff --git a/openspec/changes/review-on-done/findings-authenticity.md b/openspec/changes/review-on-done/findings-authenticity.md new file mode 100644 index 000000000000..cd5c524cdcc6 --- /dev/null +++ b/openspec/changes/review-on-done/findings-authenticity.md @@ -0,0 +1,114 @@ +# Finding: peer sender identity is a resolved claim, not an authentication + +Date: 2026-10-03. Branch: `review-identity-finding`, cut from `origin/dev` = `d7360d499f`. +Affects: `review-on-done` Phase 1b (record authenticity), and `project-server` D5. +Status: **decision required from the user.** This is the input to that decision, not the decision. + +## Summary + +Phase 1b.1 requires the review record to be written with "the authenticated session +identity of the REVIEWER". Two candidate sources exist: a shared project server, which +attributes the request itself, and today's per-process sidecar route, which resolves a +peer-supplied return address through a registry. + +The second is not a weaker authentication. It is **not an authentication at all**. Nothing +in `packages/` binds a peer's claimed identity to the connection the message arrived on. + +That distinction decides Phase 1b. A review record whose `reviewer.sessionID` is a +self-asserted string cannot enforce "the reviewer is not the author" (task 1b.3). Building +the writer on that basis yields records that *look* like a working review gate and enforce +nothing — worse than no gate, because the record's presence is the misleading part. + +## What the code claims, and what it does + +`packages/opencode/src/peer/claude/codec.ts`, in the doc comment on `parseEnvelope`, +states the rule correctly: + +> the returned `from`/`fromName` are for DISPLAY ONLY — a sender can put anything in these +> attributes, so provenance for authorization purposes is whatever authenticated socket +> connection this arrived on, never these fields. + +`packages/opencode/src/peer/claude/lifecycle.ts` then does the opposite: + +```ts +const sender = yield* Effect.promise(() => resolveOpencodeSender(inbound.from)) +const pid = claudePidOf(inbound.from) +``` + +It hands the envelope **field** to the resolver. The comment promises connection-derived +provenance; the implementation derives it from the claim the comment warns against. There +is no third path. + +## Evidence + +Measured on this checkout at `d7360d499f`: + +- `SO_PEERCRED`, `getpeereid`, `getPeerName`: **0 occurrences** in `packages/`. The + connection exposes no peer credential, so "whatever authenticated socket connection this + arrived on" does not exist as a value that could be read. +- `packages/opencode/src/peer/route.ts:203-209`, `resolveOpencodeSender`, has two branches + and both are functions of a string the sender chose: + - `opencodeSenderOf` strips the `uds:opencode-skein:` prefix and returns the remainder. + A sender that writes `uds:opencode-skein:ses_victim` **is** `ses_victim`. + - the UDS branch looks that socket path up in the registry. The path came from `from` too, + so it answers "whoever owns the socket path I named", not "who is speaking". +- `packages/opencode/src/peer/lead.ts:170` types `Sender` as "The authenticated sender, as + the delivery path resolved it from the socket." For both routes above, that comment + overstates the mechanism. +- `packages/opencode/src/peer/claude/sidecar-server.ts:95` gates each connection on + `frame.token === peerToken`, a `randomBytes(16)` value written into the registration at + `sidecar-server.ts:125`. That authenticates the **destination** — it proves the holder may + deliver *to* that session. It says nothing about who is delivering. + +## The check that would have caught this + +Stating the intent as a test rather than a comment, because a rule that reads as prose and +parses as a mapping is exactly the failure this project has been bitten by: + +> A peer sends a message whose envelope claims `from = uds:opencode-skein:`. +> Does the receiver attribute the message to that session? + +Today it does. `opencodeSenderOf` returns the claimed id verbatim, and +`verifyLead` (`packages/opencode/src/peer/lead.ts`) then compares +`sender.sessionID === grant.lead.sessionID` and grants the lead's scopes. A forged `from` +satisfies the grant check. + +No such test exists today. It should become a regression test when this is fixed, asserting +that a mismatched claim is either refused or attributed to the transport — never to the +claim. Until then, the honest statement is: **sender identity is unenforced.** + +## The decision + +**Option A — server-attributed (recommended).** The project server mediates the write, so +the requesting session is the one it served. Identity is intrinsic to the request rather +than asserted inside a payload, which is what makes 1b.3 enforceable. + +Cost, stated plainly: Phase 1b waits on the project server, and that is not a small +dependency. `project-server` findings 0.5 measured ~172-204 MB resident *per registered +session sidecar*, and D5 alone does not deliver the memory argument (findings, +"Consequences" #3). This is multi-phase work. + +**Option B — socket-resolved.** Reuses existing infrastructure and unblocks 1b.1 now. But +it rests on a claim, so 1b.3's author check is defeatable by any same-user process that can +read the registry directory. Records written under it would have to be invalidated later. + +**Recommendation: A**, on one narrow ground — B cannot close the hole it appears to close. +The cost of A is that authenticity stays open longer, which is uncomfortable and truthful. +An unenforceable check is worse than a visibly missing one. + +## Orthogonal: 1b.2 is not sound on its own + +Task 1b.2 denies model `edit`/`write` of `.skein/review.json`. That is worth doing and is +independent of A versus B — but it is only a control if the fenced path cannot be reached +another way, and scoped unattended bash can read and write arbitrary files today. A fence +covering `edit`/`write` alone would hold in tests and be bypassable in practice, which is +the more expensive failure because the code reads as though it is safe. + +The bash gap should be settled before 1b.2 is treated as done, or 1b.2 should carry an +explicit note that it is not a sandbox. + +## What this does not change + +Nothing here is a fix, and nothing here is wired. The review record merged in +`abadec9aea` remains a reader and a mapper. PR #111 covers 1b.3 only; 1b.1, 1b.2 and 1b.4 +wait on the decision recorded here. \ No newline at end of file From 35004b5602abb2fdc3ff4c5047aea9647dfc22da Mon Sep 17 00:00:00 2001 From: AndreasS Date: Sat, 3 Oct 2026 05:43:21 +0200 Subject: [PATCH 2/4] docs(review-on-done): correct line references in the authenticity finding route.ts:203-208 and lead.ts:195, verified against d7360d499f. The first pass cited lines I had read before #110 moved codec.ts, and a finding about citations drifting should not carry its own. --- openspec/changes/review-on-done/findings-authenticity.md | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/openspec/changes/review-on-done/findings-authenticity.md b/openspec/changes/review-on-done/findings-authenticity.md index cd5c524cdcc6..eebab193ef26 100644 --- a/openspec/changes/review-on-done/findings-authenticity.md +++ b/openspec/changes/review-on-done/findings-authenticity.md @@ -21,14 +21,14 @@ nothing — worse than no gate, because the record's presence is the misleading ## What the code claims, and what it does -`packages/opencode/src/peer/claude/codec.ts`, in the doc comment on `parseEnvelope`, +`packages/opencode/src/peer/claude/codec.ts:96-101`, in the doc comment on `parseEnvelope`, states the rule correctly: > the returned `from`/`fromName` are for DISPLAY ONLY — a sender can put anything in these > attributes, so provenance for authorization purposes is whatever authenticated socket > connection this arrived on, never these fields. -`packages/opencode/src/peer/claude/lifecycle.ts` then does the opposite: +`packages/opencode/src/peer/claude/lifecycle.ts:122-123` then does the opposite: ```ts const sender = yield* Effect.promise(() => resolveOpencodeSender(inbound.from)) @@ -46,13 +46,13 @@ Measured on this checkout at `d7360d499f`: - `SO_PEERCRED`, `getpeereid`, `getPeerName`: **0 occurrences** in `packages/`. The connection exposes no peer credential, so "whatever authenticated socket connection this arrived on" does not exist as a value that could be read. -- `packages/opencode/src/peer/route.ts:203-209`, `resolveOpencodeSender`, has two branches +- `packages/opencode/src/peer/route.ts:203-208`, `resolveOpencodeSender`, has two branches and both are functions of a string the sender chose: - `opencodeSenderOf` strips the `uds:opencode-skein:` prefix and returns the remainder. A sender that writes `uds:opencode-skein:ses_victim` **is** `ses_victim`. - the UDS branch looks that socket path up in the registry. The path came from `from` too, so it answers "whoever owns the socket path I named", not "who is speaking". -- `packages/opencode/src/peer/lead.ts:170` types `Sender` as "The authenticated sender, as +- `packages/opencode/src/peer/lead.ts:195` types `Sender` as "The authenticated sender, as the delivery path resolved it from the socket." For both routes above, that comment overstates the mechanism. - `packages/opencode/src/peer/claude/sidecar-server.ts:95` gates each connection on From e9fc04d02f3e9901e782ecdb42e8f12711f45ec2 Mon Sep 17 00:00:00 2001 From: AndreasS Date: Sat, 3 Oct 2026 05:43:49 +0200 Subject: [PATCH 3/4] docs(review-on-done): tighten citation ranges to the quoted blocks 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. --- openspec/changes/review-on-done/findings-authenticity.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/openspec/changes/review-on-done/findings-authenticity.md b/openspec/changes/review-on-done/findings-authenticity.md index eebab193ef26..fa6f875c0c49 100644 --- a/openspec/changes/review-on-done/findings-authenticity.md +++ b/openspec/changes/review-on-done/findings-authenticity.md @@ -21,7 +21,7 @@ nothing — worse than no gate, because the record's presence is the misleading ## What the code claims, and what it does -`packages/opencode/src/peer/claude/codec.ts:96-101`, in the doc comment on `parseEnvelope`, +`packages/opencode/src/peer/claude/codec.ts:95-102`, in the doc comment on `parseEnvelope`, states the rule correctly: > the returned `from`/`fromName` are for DISPLAY ONLY — a sender can put anything in these @@ -52,7 +52,7 @@ Measured on this checkout at `d7360d499f`: A sender that writes `uds:opencode-skein:ses_victim` **is** `ses_victim`. - the UDS branch looks that socket path up in the registry. The path came from `from` too, so it answers "whoever owns the socket path I named", not "who is speaking". -- `packages/opencode/src/peer/lead.ts:195` types `Sender` as "The authenticated sender, as +- `packages/opencode/src/peer/lead.ts:195-201` types `Sender` as "The authenticated sender, as the delivery path resolved it from the socket." For both routes above, that comment overstates the mechanism. - `packages/opencode/src/peer/claude/sidecar-server.ts:95` gates each connection on From 3fde1cdd1633b23ab68004ecb7e2a03d04bde95a Mon Sep 17 00:00:00 2001 From: AndreasS Date: Sat, 3 Oct 2026 05:52:05 +0200 Subject: [PATCH 4/4] docs(review-on-done): withdraw the server-attributed recommendation 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. --- .../review-on-done/findings-authenticity.md | 75 +++++++++++++------ 1 file changed, 52 insertions(+), 23 deletions(-) diff --git a/openspec/changes/review-on-done/findings-authenticity.md b/openspec/changes/review-on-done/findings-authenticity.md index fa6f875c0c49..964acc074a2f 100644 --- a/openspec/changes/review-on-done/findings-authenticity.md +++ b/openspec/changes/review-on-done/findings-authenticity.md @@ -79,36 +79,65 @@ claim. Until then, the honest statement is: **sender identity is unenforced.** ## The decision -**Option A — server-attributed (recommended).** The project server mediates the write, so -the requesting session is the one it served. Identity is intrinsic to the request rather -than asserted inside a payload, which is what makes 1b.3 enforceable. +Revised after peer consultation (2026-10-03). The original recommendation here was +server-attributed identity, per `project-server` D5. **That recommendation is withdrawn: +the user rejected the project server, so its premise no longer exists.** -Cost, stated plainly: Phase 1b waits on the project server, and that is not a small -dependency. `project-server` findings 0.5 measured ~172-204 MB resident *per registered -session sidecar*, and D5 alone does not deliver the memory argument (findings, -"Consequences" #3). This is multi-phase work. +### Correction to the evidence above -**Option B — socket-resolved.** Reuses existing infrastructure and unblocks 1b.1 now. But -it rests on a claim, so 1b.3's author check is defeatable by any same-user process that can -read the registry directory. Records written under it would have to be invalidated later. +I listed the absence of `SO_PEERCRED` / `getpeereid` as a gap in this codebase. It is +worse than that: the primitive is not available in the runtime. Probed directly against +Bun 1.3.14 on a live unix-socket connection, `Socket` exposes no `getPeerName`, +`getPeerCredentials` or `getPeerCert`. So an unforgeable *connection* identity is not +merely unimplemented here — it is not reachable without OS-level separation or a different +runtime. Any proposal resting on peer credentials is not implementable as written. -**Recommendation: A**, on one narrow ground — B cannot close the hole it appears to close. -The cost of A is that authenticity stays open longer, which is uncomfortable and truthful. -An unenforceable check is worse than a visibly missing one. +### Option 1 — callback confirmation (recommended) + +Before attributing a message, the receiver asks the claimed `from` socket: did you send +`msg_id` X to me? The real session answers from its own in-process sent-log. A forger +cannot make it say yes, because it does not have the log. Cost is roughly one new frame +type plus a sent-log — no server, no peer credentials. + +This bounds the claim rather than making it absolute, and it is the strongest thing +available in this runtime. + +### Option 2 — socket-resolved, as-is + +Unblocked today, and rejected. It rests on a claim, so any same-user process that can +write the registry can attribute itself as another session. + +### What no option delivers + +Against a same-user, shell-capable agent, **nothing short of OS separation makes sender +identity unforgeable.** A process that can replace a registry entry or a socket can +answer for it. This should be stated wherever sender identity is relied upon, so the +residual is a decision the user accepts rather than one discovered later. + +The real control is that claims stay **bounded**: human-granted, scoped, expiring, and +subject to a never-list. For the review record specifically, the durable part is that the +record is written by a tool from a session id the tool holds — never parsed out of message +text. + +### Sequence + +Callback confirmation first, since it hardens `verifyLead` as well; then 1b.1 on top of it. ## Orthogonal: 1b.2 is not sound on its own -Task 1b.2 denies model `edit`/`write` of `.skein/review.json`. That is worth doing and is -independent of A versus B — but it is only a control if the fenced path cannot be reached -another way, and scoped unattended bash can read and write arbitrary files today. A fence -covering `edit`/`write` alone would hold in tests and be bypassable in practice, which is -the more expensive failure because the code reads as though it is safe. +Task 1b.2 denies model `edit`/`write` of `.skein/review.json`. Worth doing, and independent +of the identity choice — but only a control if the fenced path cannot be reached another +way, and scoped unattended bash can read and write arbitrary files today. A fence covering +`edit`/`write` alone holds in tests and is bypassable in practice, which is the more +expensive failure because the code reads as though it were safe. -The bash gap should be settled before 1b.2 is treated as done, or 1b.2 should carry an -explicit note that it is not a sandbox. +Settle the bash gap before treating 1b.2 as done, or carry an explicit note that it is not +a sandbox. ## What this does not change -Nothing here is a fix, and nothing here is wired. The review record merged in -`abadec9aea` remains a reader and a mapper. PR #111 covers 1b.3 only; 1b.1, 1b.2 and 1b.4 -wait on the decision recorded here. \ No newline at end of file +Nothing here is a fix, and nothing here is wired. The review record merged in `abadec9aea` +remains a reader and a mapper. #111 covers 1b.3 only and is **inert as merged**: nothing +passes `authorSessionID` yet, so it enforces nothing. It is recorded here as driver-side +plumbing, not as a control in force — and that distinction matters only for as long as +nobody ticks 1b done.