✨ feat(forwardproxy): support HTTPS CONNECT as raw TCP passthrough#430
Merged
huang195 merged 2 commits intoMay 22, 2026
Merged
Conversation
The forward proxy previously rejected every CONNECT request with 405,
which meant agents in proxy-sidecar mode couldn't reach external HTTPS
endpoints — the kagenti-operator injects HTTPS_PROXY env var on every
agent container, Python's openai/httpx honors it and sends CONNECT to
the forward proxy, and the 405 surfaced to the agent as
`openai.APIConnectionError: Connection error.`
That was the root cause of every HyperShift CI weather-agent failure
since kagenti-operator commit 21a2258 (May 14) flipped the cluster
default mode from envoy-sidecar to proxy-sidecar. Pre-flip, Envoy's
TLS-passthrough filter chain handled external HTTPS transparently;
post-flip, the forward proxy was the only outbound path and it refused.
Implement CONNECT as a raw TCP tunnel — match envoy-sidecar's
TLS-passthrough behavior. The bytes flowing through a CONNECT tunnel
are the agent's end-to-end TLS, opaque to the proxy by definition, so
the protocol parsers (mcp-parser, inference-parser) and token-exchange
are no-ops on this path. Pipeline gates that policy on host/identity
(ibac, etc.) still run on the CONNECT request itself and can deny
based on destination host before the tunnel opens.
Implementation notes:
- Run the outbound pipeline first. Reject paths use the existing
httpx.WriteRejection + recordOutboundReject machinery so abctl /
/v1/sessions surfaces denied CONNECTs the same way as denied HTTP
POSTs.
- Hijack BEFORE dialing upstream so a hijack failure produces a
clean 500 instead of a half-opened upstream socket. Dial timeout
bounded at 30s; once tunnel is up no timeout (the agent's TLS
handshake and subsequent traffic flow at their own pace).
- mTLS is intentionally NOT applied to the upstream dial. The bytes
flowing through this tunnel ARE the agent's own end-to-end TLS;
terminating that with sidecar-to-sidecar mTLS would break the
agent's trust path. CONNECT targets are opaque externals (LiteMaaS,
Bedrock, GitHub API, etc.) where the agent's TLS is the right
answer.
- Bidirectional copy with explicit Close on both sides — net.Conn
Close is idempotent so the duplicate close on the loser-side is
harmless.
Tests:
- TestForwardProxy_CONNECT_TunnelsBytes: real TCP echo upstream,
verifies CONNECT 200, then bytes round-trip both directions
through the tunnel.
- TestForwardProxy_CONNECT_PipelineDeny: pipeline rejects before
dial; client sees a non-200 status line. Plugin policy still
governs CONNECT.
- TestForwardProxy_CONNECT_BadGatewayOnDialFailure: upstream
refuses; client sees 502 (not a half-opened tunnel).
The pre-existing TestForwardProxy_CONNECT_Rejected (which asserted 405)
is replaced by these three.
Verified:
- go test -race ./authlib/... — all packages green.
- go build ./cmd/{authbridge-proxy,authbridge-envoy,authbridge-lite,abctl} — clean.
- go vet, gofmt — clean.
Closes rossoctl#428.
Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com>
Signed-off-by: Hai Huang <huang195@gmail.com>
Four review comments, all applied:
1. Observability gap: allowed CONNECTs were silent in /v1/sessions
while denied ones were recorded via recordOutboundReject. Added a
SessionRequest event after the pipeline allows + before the tunnel
opens, mirroring the HTTP path's post-Allow recording. Same gating
as the HTTP path (record only when Invocations or plugin-public
Plugins entries are non-nil) so we don't spam empty events for
pipelines that don't actually run gate plugins on outbound.
2. Misleading comment: "Hijack BEFORE dialing upstream" implied the
hijacker.Hijack() call happens before net.DialTimeout, but it
doesn't (and shouldn't — http.Error needs an un-hijacked
ResponseWriter to deliver the dial-failure 502). The PR review
author is right that what we actually do is verify hijack
capability via type-assertion before dialing, then hijack only
after dial succeeds. Rewrote the comment to say so and moved the
actual Hijack() call to use w.(http.Hijacker) directly so the
type-assertion-as-capability-check is visually distinct from the
commit-to-tunnel hijack.
3. Missing TCP keepalive: streaming LLM completions can hold the
tunnel open for minutes, and a vanished peer (NAT entry expiry,
peer reboot, network partition) parks the io.Copy goroutines
until the OS finally times the socket out. Added enableKeepalive
helper that sets SO_KEEPALIVE + 30s probe interval on both the
upstream conn and the hijacked client conn. No-op when the conn
doesn't unwrap to *net.TCPConn (defensive).
4. Single-roundtrip test: a real TLS handshake is multi-RTT with
interleaved reads and writes; one write/read in
TestForwardProxy_CONNECT_TunnelsBytes catches basic plumbing but
could miss a half-duplex regression in the bidirectional copy.
Reworked the upstream echo loop to keep accepting and added a
three-payload round-trip in the test ("hello", "world", "third").
Verified:
go test -race -count=1 ./authlib/... — green
go vet, gofmt — clean
Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com>
Signed-off-by: Hai Huang <huang195@gmail.com>
pdettori
approved these changes
May 22, 2026
pdettori
left a comment
Member
There was a problem hiding this comment.
Solid implementation of CONNECT tunneling. The design is well-reasoned: pipeline runs before dial, hijack capability is checked early, keepalives handle idle detection, and the bidirectional copy pattern is standard Go TCP proxying. Tests cover the happy path (multi-round-trip), policy rejection, and upstream failure — good edge case coverage.
Areas reviewed: Go (implementation + tests), Security, Commit Conventions
Commits: 2 commits, all signed-off ✓
CI status: all checks passing ✓
| _, _ = io.Copy(upstream, clientConn) | ||
| _ = upstream.Close() | ||
| _ = clientConn.Close() | ||
| }() |
Member
There was a problem hiding this comment.
nit: io.Copy errors are silently discarded in both directions. Consider logging at slog.Debug level on non-EOF errors — it won't affect correctness but helps diagnose weird disconnects during incident triage (e.g. a network partition that leaves one side writeable but the other already RST'd).
6 tasks
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
3 participants
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.
Summary
The forward proxy previously rejected every HTTPS
CONNECTwith405 Method Not Allowed. After kagenti-operator commit21a2258(May 14, 2026) flipped the cluster default mode from envoy-sidecar to proxy-sidecar, this broke external HTTPS for every agent in CI: the operator injectsHTTPS_PROXY=http://127.0.0.1:<port>on every agent container, Python's openai/httpx client honors it and sendsCONNECT litellm-prod...:443to the forward proxy, the 405 propagates back, and the agent reportsopenai.APIConnectionError: Connection error.This was the actual root cause of issue #428 (the original "proxy-init iptables" diagnosis was misframed — the failing pods are in proxy-sidecar mode and have noproxy-initcontainer at all).This PR implements
CONNECTas a raw TCP tunnel — matching envoy-sidecar's existing TLS-passthrough behavior. The bytes flowing through the tunnel are the agent's end-to-end TLS, opaque to the proxy by definition, so the protocol parsers (mcp-parser, inference-parser) and token-exchange are no-ops on this path. Pipeline gates that policy on host/identity (ibac, etc.) still run on theCONNECTrequest itself and can deny based on destination host before the tunnel opens.Key design points
CONNECTrequest before the upstream is dialed. Reject paths use the existinghttpx.WriteRejection+recordOutboundRejectmachinery, so abctl //v1/sessionssurfaces deniedCONNECTs the same way as denied HTTP POSTs.CONNECTtargets are opaque externals (LiteMaaS, Bedrock, GitHub API, etc.) where the agent's TLS is the right answer. ThemtlsDialercontinues to apply only to the plain-HTTP forwarding path (where it actually makes sense — sidecar-to-sidecar plaintext upgrade).net.Conn.Closeis idempotent so the duplicate close on the loser side is harmless.Tests
TestForwardProxy_CONNECT_TunnelsByteshello→echo:hello).TestForwardProxy_CONNECT_PipelineDenyTestForwardProxy_CONNECT_BadGatewayOnDialFailureThe pre-existing
TestForwardProxy_CONNECT_Rejected(which asserted 405) is replaced by these three.Verification
go test -race -count=1 ./authlib/...— all packages greengo build ./cmd/{authbridge-proxy,authbridge-envoy,authbridge-lite,abctl}— cleango vet,gofmt— clean on touched filesOut of scope
HTTPS_PROXYinjection — also a valid fix path (option A in the investigation), but with this PR's CONNECT support, the existingHTTPS_PROXYinjection becomes correct rather than wrong, so no operator change is needed.Closes
Assisted-By: Claude (Anthropic AI) noreply@anthropic.com