feat(agent-proxy): broker credentials over WebSocket connections - #356
Open
saifsmailbox98 wants to merge 3 commits into
Open
feat(agent-proxy): broker credentials over WebSocket connections#356saifsmailbox98 wants to merge 3 commits into
saifsmailbox98 wants to merge 3 commits into
Conversation
The proxy MITMs every CONNECT and strips Upgrade as a hop-by-hop header, so a WebSocket handshake never survived it: real-time APIs failed outright rather than being brokered. - Preserve the upgrade headers across the hop-by-hop strip, dial the upstream on a connection the proxy owns, and take over both sockets after 101. A non-101 response is relayed normally so the agent sees the real rejection. - Add a 'websocket' substitution surface for services that expect the credential in the first message rather than the handshake. Frames are parsed on the way out and re-masked with a fresh key; binary, fragmented, compressed and oversized frames are forwarded unchanged. - Decline permessage-deflate where that surface is in use, since a compressed frame is unreadable and the substitution would silently never fire. - Clear the hijacked connection's deadlines and let the inner handler own the conn, so a long-lived WebSocket is not killed by the tunnel read timeout or closed underneath by handleConnect.
|
💬 Discussion in Slack: #pr-review-cli-356-feat-agent-proxy-broker-credentials-over-websocket-connections Posted by Review Police — reviews, comments, new commits, and CI failures will stream into this channel. |
11 tasks
Contributor
|
| Filename | Overview |
|---|---|
| packages/agentproxy/proxy.go | Refactors upstream preparation and transfers tunnel ownership after a nested WebSocket hijack; no independent defect was found here. |
| packages/agentproxy/rewrite.go | Adds WebSocket substitution selection and safe audit labels, but those labels are not consumed by the new close activity record. |
| packages/agentproxy/websocket.go | Implements handshake and frame brokering; independent per-direction deadlines can terminate an active one-way stream, and substitution audit metadata is omitted. |
| packages/agentproxy/websocket_test.go | Provides broad protocol coverage but does not test active one-way traffic across the idle timeout or credential metadata in close logs. |
Reviews (1): Last reviewed commit: "feat(agent-proxy): broker credentials ov..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![veria-ai[bot]](https://avatars.githubusercontent.com/in/1723776?s=60&v=4){kind=small}
Contributor
PR overviewAll previously flagged issues have been addressed. No open security concerns remain on this pull request. Security reviewNo open security issues remain on this pull request. Fixed/addressed: 2 · PR risk: 0/10 |
Comments here should earn their place by explaining why, per the rule in the infisical repo's CLAUDE.md. Drops the leading clauses that restated a function's signature (nothing in this file is exported, so godoc's name-first form buys nothing) and one change-history note in a test, keeping the constraints and reasons that are not visible from the code.
- Redact on the return path. The agent chooses where its placeholder goes, so it could plant one in a field the service echoes (a correlation id, an error message) and read the real credential out of the reply. Inbound text frames now swap the value back to the placeholder, which also stops a service that quotes the credential in an error from leaking it by accident. - Share the idle deadline across both directions. It was per-direction, and either side expiring closed the connection, so a subscribe-mostly stream that received for ten minutes while sending nothing was torn down mid-stream. - Bound the upstream dial. wsResponseTimeout only starts once TCP is up, so a dial at a black-holed address pinned a connection slot for as long as the OS took to give up. - Name the brokered credentials on the close record, and only once a frame was actually rewritten. The labels were built and then never logged, which left audit consumers unable to tell which credential a connection carried.
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.
Description 📣
The proxy MITMs every CONNECT and strips
Upgradeas a hop-by-hop header, so a WebSocket handshake never survived it. Real-time APIs failed outright rather than being brokered, which ruled out anything on Slack Socket Mode, the Discord gateway, or a streaming MCP transport.Two things now work.
Handshake brokering. The upgrade is an ordinary HTTP request, so header rewrites and
path/query/headersubstitution apply to it exactly as they do anywhere else. The upgrade headers are preserved across the hop-by-hop strip, the upstream is dialled on a connection the proxy owns (a hijacked WebSocket can't be driven throughhttp.Transport's pooled round tripper), and after 101 the proxy owns both sockets. A non-101 response is relayed normally so the agent sees the real rejection instead of a proxy error.Frame substitution. A new
websocketsubstitution surface, for services that expect the credential in the first message rather than in the handshake. Frames are parsed on the way out and re-masked with a fresh key per RFC 6455 §5.3 — reusing the client's key would leak the XOR relationship between the placeholder and the real value. Binary, fragmented, compressed and oversized (>1MB) frames are forwarded byte-for-byte, so an unsupported shape degrades to passthrough rather than corrupting the stream. Substitution runs client-to-upstream only; nothing is ever rewritten on the way back to the agent.Three things worth calling out, each of which was a bug before it was a decision:
permessage-deflateis removed from the upgrade offer where thewebsocketsurface is in use. A compressed frame carries RSV1 and is never substituted, and most clients offer compression by default, so leaving the offer in place made the surface silently do nothing. Only affects services that use that surface.Hijack, so the tunnel's 60sReadTimeoutkilled every WebSocket after a minute.serveTunnelnow reports whether the inner handler hijacked.Servereturns the moment the one-shot listener closes on hijack, sohandleConnect's deferred close was tearing the socket out from under a live pipe. Nothing had ever hijacked inside a tunnel before, so the invariant was untested.upstreamTLSConfigreads the TLS settings off the transport rather than hardcoding system roots, so a WebSocket verifies its upstream the same way the plain HTTP path does.Companion PR: Infisical/infisical#7581
Type ✨
Tests 🛠️
# Here's some code block to paste some code snippets