feat(coderd): accept delegated API key ID from in-process aibridge callers#25625
Conversation
…llers aibridged's in-memory transport now requires callers to attach a delegated API key ID via aibridge.WithDelegatedAPIKeyID, enforced at the RoundTrip boundary. The handler uses it to authenticate the request as the named user without the key secret; IsAuthorized validates only liveness for this path. Orthogonal to BYOK: a delegated request still forwards user-supplied LLM credentials and strips the Coder governance token. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This stack of pull requests is managed by Graphite. Learn more about stacking. |
|
/coder-agents-review |
|
Review posted | Chat Review historydeep-review v0.5.0 | Round 2 | Last posted: Round 2, 12 findings (1 P2, 7 P3, 2 Nit, 2 Note), APPROVE. Review Finding inventoryFindings
Contested and acknowledgedCRF-7 (P3, aibridgedserver.go:579) - No format validation on delegated key_id
CRF-10 (Nit, PR description) - "currently: chatd" but chatd doesn't use this yet
CRF-8 (Note, aibridged.proto:139) - key/key_id not oneof
Round logRound 1Panel. 1 P2, 6 P3, 1 Nit, 2 Note. Reviewed against ef3f95a..2cda32c. Round 2Churn guard: 7 addressed, 1 accepted (Note), 2 contested. Panel: 5 reviewers (Bisky, Hisoka, Mafuuu, Mafu-san, Pariston). CRF-7 closed (3/3 accept). CRF-10 closed (2/2 accept). 1 P3, 1 Nit new. All R1 fixes verified. Reviewed against ef3f95a..ef81de8. About deep-reviewCRF = Coder Review Finding (P0-P4, Nit, Note)
|
![coder-agents-review[bot]](https://avatars.githubusercontent.com/in/3454270?s=60&v=4){kind=small}
There was a problem hiding this comment.
Clean, well-layered mechanism for delegated API key authentication. The trust boundary design is sound: context values are unexported types unreachable from HTTP, the transport enforces the invariant before the handler runs, the DRPC Authorizer sits behind MemTransportPipe. Test suite covers the security-critical negative cases (secret-hash-mismatch-ignored, empty-identity rejection, both-fields-set, BYOK orthogonality end-to-end).
1 P2, 6 P3, 1 Nit.
The P2 is a factual error in an exported doc comment (claims "revoked" but no revocation check exists). The P3s are observability, test coverage, error disambiguation, and format validation. No functional bugs found.
The full panel confirmed the security model is architecturally correct. Hisoka noted the IsAuthorized RPC has no way to know whether it was called over in-memory or network transport. If the DRPC service is ever exposed over a network, key_id becomes user impersonation with just a 10-character ID. Current architecture makes this safe; a // SECURITY comment on the RPC documenting the transport invariant would protect future refactors.
Proto uses two separate string fields rather than oneof. Discussed by 7 reviewers. Consensus: oneof would be more idiomatic but proto3 oneof scalar quirks exist, and the runtime enforcement with explicit both-set rejection is arguably better than oneof's silent last-wins. Current approach is defensible.
Comment verbosity pattern noted by Gon: "the caller never has the secret" appears 4 times across the diff, and several local comments restate what the code already shows. The minimum-draft suggestions are sound.
"There are no second chances when you do not know the identity of your opponent, but you also cannot win without taking a risk." ~ Pariston
🤖 This review was automatically generated with Coder Agents.
- http.go: add auth_delegated + transport source to the request logger - aibridge/factory.go: correct doc to describe the actual liveness checks - aibridged/mcp.go: skip Coder MCP proxy when no session key (delegated) - aibridgedserver: introduce ErrAmbiguousAuth for both-fields-set case - aibridgedserver: document the in-memory transport invariant on IsAuthorized - aibridgedserver_test: cover deleted/system user on the delegated path
|
/coder-agents-review |
There was a problem hiding this comment.
All 7 addressed R1 findings verified by the panel. Fixes are clean and proportional.
CRF-7 (contested, no format validation on delegated key_id): panel closed 3/3. Hisoka, Mafuuu, and Pariston accept the author's defense. The delegated interface should treat key_id as opaque; coupling it to the 10-char format leaks an implementation detail across an abstraction boundary. Consequence of a malformed ID is one wasted DB roundtrip and ErrUnknownKey, which becomes 403 at the HTTP layer regardless.
CRF-10 (contested, "currently: chatd"): panel closed 2/2. Forward-looking language in a PR description is not worth blocking.
1 P3, 1 Nit new this round. Both are in the fix commit, neither blocks approval.
The security model holds up well under adversarial probing. Hisoka verified the three-layer defense (transport guard, Go context type safety, in-memory DRPC pipe) and could not construct a scenario where a network request reaches the delegated path. Pariston confirmed the design choice (delegated auth with liveness check) over four alternative framings. Test suite covers the load-bearing properties.
"I tried to build a case against this PR and could not." ~ Pariston
🤖 This review was automatically generated with Coder Agents.
| // SECURITY: when in.KeyId is set (the "delegated" path), this method trusts the | ||
| // caller's claim of identity and skips the key-secret check. This is safe only | ||
| // because the DRPCServer is reachable solely via the in-process | ||
| // [aibridged.MemTransportPipe]; the handler itself cannot tell whether it was |
There was a problem hiding this comment.
Nit [CRF-11] Broken godoc link: [aibridged.MemTransportPipe] references a symbol that does not exist. The actual function is drpcsdk.MemTransportPipe() (called at coderd/aibridged.go:61). Since this SECURITY comment's entire purpose is to help future developers trace the trust boundary, the cross-reference should be accurate.
"Writing a security-critical cross-reference without verification, in a comment whose entire purpose is accurate documentation of a trust invariant." ~ Mafu-san
Fix: replace with [drpcsdk.MemTransportPipe] or describe the transport in prose. (Netero Nit, Mafu-san P3)
🤖
| // secret and so cannot authenticate against the Coder MCP server. | ||
| // Skip the proxy in that case rather than attempting a connection | ||
| // with an empty bearer token, which will fail upstream. | ||
| if req.SessionKey == "" { |
There was a problem hiding this comment.
P3 [CRF-12] The SessionKey == "" guard was added to fix CRF-4, but no test exercises this branch. The integration tests use pool mocks that bypass MCP; mcp_internal_test.go only tests regex compilation; pool_test.go always passes SessionKey: "key".
If someone reverts this guard, delegated requests would silently attempt MCP proxy creation with empty bearer tokens. The upstream server would reject (401), and the proxy factory would log a warning and proceed without the Coder MCP server. Not catastrophic, but a regression in fix-commit behavior that would go undetected.
(Bisky P3)
🤖
3 participants
Allows an
api_key_idto be passed from a trusted in-memory transport (currently:chatd) toaibridgedfor use in authenticating LLM requests.This value can only be passed via context, and all users of the in-memory transport must provide it.
It can be used in conjunction with BYOK headers.