feat: add upstream proxy support to aiproxy for passthrough requests#21512
feat: add upstream proxy support to aiproxy for passthrough requests#21512ssncferreira merged 3 commits intomainfrom
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
e4a81dd to
d843b9a
Compare
dannykopping
left a comment
There was a problem hiding this comment.
Very impressive work!
| // Set transport without Proxy to ensure MITM'd requests go directly to aibridge, | ||
| // not through any upstream proxy. | ||
| proxy.Tr = &http.Transport{ | ||
| TLSClientConfig: &tls.Config{ |
There was a problem hiding this comment.
What role is the TLS config playing here when we forward requests to aibridge?
I see that goproxy defines default TLS settings which seem undesirable, so I'm guessing that is it?
https://github.com/elazarl/goproxy/blob/master/certs.go#L26-L30
The tests seem to pass without this, which suggest that we don't have specific-enough coverage. This can be handled in a follow-up though.
There was a problem hiding this comment.
That is a good point. We probably should set the proxy transport every time to override goproxy's default TLS settings (which use InsecureSkipVerify: true), not just when an upstream proxy is configured. I can address that.
However, this is not a problem currently because aibridge is HTTP, so there is no TLS involved. In the future, if aiproxy and aibridge run as two independent processes with HTTPS between them, then we'd want secure TLS settings. But for now, it's safe.
There was a problem hiding this comment.
Actually, this change breaks some tests 😞 The problem is that the tests use a TLS target server with a self-signed certificate. For the proxy to forward requests to this server, it needs to trust the server's certificate. In a real world scenario, the target server would be github.com or google.com for instance, so the certificate would be in the proxy's system cert pool, but the test server's self-signed cert is not.
Reverted the change: e770c6e
I'll address this in a separate PR to not block this one.
There was a problem hiding this comment.
For sure 👍 can even be after GA, not urgent.
Thanks for testing this out!
| // the upstream proxy (true) or MITM'd by aiproxy (false). | ||
| // When true, the target domain is NOT in the allowlist. | ||
| // When false, the target domain IS in the allowlist. | ||
| passthrough bool |
There was a problem hiding this comment.
Nit: this terminology might get confusing since we have a concept of passthrough in bridge.
If we went for relayed/proxied/tunneled that might be clearer.
There was a problem hiding this comment.
Hum, that is true 🤔
I think I prefer tunneled as it seems the more accurate description of what happens. I can address this in a follow-up PR, because I have been using passthough in other tests (and possibly comments as well). And that way, I can update everywhere 👍
d843b9a to
9f9bf3a
Compare
Description
Adds upstream proxy support for AI Bridge Proxy passthrough requests. This allows aiproxy to forward non-allowlisted requests through an upstream proxy. Currently, the only supported configuration is when aiproxy is the first proxy in the chain (client → aiproxy → upstream proxy).
Changes
--aibridge-proxy-upstreamoption to configure an upstream HTTP/HTTPS proxy URL for passthrough requests--aibridge-proxy-upstream-caoption to trust custom CA certificates for HTTPS upstream proxiesCloses: coder/internal#1204