Fix HTTP client proxy support #742
Conversation
Since the introduction of HTTP/2 support in the client, proxy handlers (requested via proxy-options) would be installed only after the HTTP version was negotiated. In case of a HTTPS connection, this would be attempted via ALPN which means the TLS handshake would have already taken place with the actual destination host, bypassing the configured proxy. This resulted in an error (see #731). With this patch, the proxy handlers are installed earlier again so that a configured proxy is also used during the TLS handshake phase.
Turns out this worked perfectly fine already.
|
FYI: There's a huge diff there due to wrapping + reindent, so I recommend to hide whitespace when looking at the diff once more! |
src/aleph/http/client.clj
Outdated
| @@ -396,7 +396,7 @@ | |||
|
|
|||
| :else | |||
| cause)] | |||
| (s/put! response-stream response) | |||
| (d/error! early-response-d response) | |||
There was a problem hiding this comment.
Oh yeah, so far, I was unable to even reach this code path and have it result in an error response anyway. It would probably be a good idea to come up with a bunch of test cases for the proxy functionality (currently there are none).
There was a problem hiding this comment.
Given how untested proxy code is, I'd strongly suggest writing tests before approving.
Maybe whoever needed the proxies could lend a hand?
There was a problem hiding this comment.
I was able to come up with a test that fails as expected without the patch - sanity check welcome 🙏
Using the test as a basis, I should be able to see how we can reach the error case referenced above. Stay tuned.
There was a problem hiding this comment.
OK turns out there was a bit of breakage in the proxy connection error handling and propagation due to the various recent changes in the client response pipeline. Pushed 9673ac9 for fixing those (see commit message for details) and added two proxy connection error test cases which indeed do hit those code paths now 💦
| false | ||
| stream-ch | ||
| nil)] | ||
| (assert (instance? LastHttpContent @(d/timeout! (s/take! stream) 1000))) |
There was a problem hiding this comment.
IIRC, LastHttpContent is Netty HTTP1-only. You'll need a new test if proxying is added to HTTP2.
There was a problem hiding this comment.
The request towards the proxy server is indeed still HTTP/1 even for HTTP/2! Note how the test uses the with-http-ssl-servers macro which already exercises both protocol versions and it works just fine. This is because for TLS connections, the proxy acts as a dumb tunnel. In other words: beyond this point, it only shoves opaque bytes back and forth and has no clue about which HTTP version is contained within. Not sure whether this also works for h2c but I am willing to leave that for some adventurous soul to find out 😋
There was a problem hiding this comment.
True...but what about the initial CONNECT? There's still one http request before shoveling bytes, and if the ALPN negotiates HTTP/2, it won't work. It should probably be documented to not allow http1 during ALPN if you're going to proxy.
There was a problem hiding this comment.
The initial CONNECT request currently will always use (plain) HTTP/1 as there is no option to make it connect to the proxy itself via HTTPS (see proxy-options where there would have to be an ssl-context option for this to work). Also, Netty's support for this is still quite buggy and HttpProxyHandler is hardcoded to HTTP/1.1. So tl;dr I think this is fine? 🔥 ☕ 🐶 🔥
There was a problem hiding this comment.
Ahh, I hadn't realized Aleph only supported unencrypted client<->proxy connections. I think ssl support should be mandatory, but I defer to you.
FWIW, I'm pretty sure think we'll also have the problem that netty issue did, with putting two ssl handlers in the pipeline. We have some code that looks for the handler by class instead of name, expecting only one.
To me, not supporting an encrypted client<->proxy connection isn't safe, since a client may not be encrypting the proxied stream (e.g. when the proxy is the gateway between the internet and a trusted VPC).
The point I was trying to raise is that, when Aleph acts as a secure http proxy (not the client), ALPN will occur before the CONNECT is sent, so it might negotiate http2 before it could learn that won't work.
Suggested changes
When Aleph is the client
When Aleph is the client, it should really support an encrypted client<->proxy connection.
Optionally, we could add proxy-specific ALPN options in proxy-options that default to accepting only http1, for better compatibility when the client and proxy are both Aleph.
When Aleph is the proxy
We should support ssl. If we only support it with http1, we should probably:
- Return an informative error and kill the connection if (1) https and ALPN are used, (2) a client negotiated http2, and then (3) sent a CONNECT
- Document these limits.
There was a problem hiding this comment.
Ahh, I hadn't realized Aleph only supported unencrypted client<->proxy connections.
Heh yeah I only learned about the ins and outs of the client proxy support myself while looking into this fix, never used the feature myself so far.
I think ssl support should be mandatory, but I defer to you.
I wouldn't go that far - AFAIK HTTP proxies are mainly used from within trusted networks for connecting to the outside world and might not even offer HTTPS. So I'd leave that as optional just like we do for direct connections. @David-Ongaro Do you have a perspective on this, too?
To me, not supporting an encrypted client<->proxy connection isn't safe, since a client may not be encrypting the proxied stream (e.g. when the proxy is the gateway between the internet and a trusted VPC).
I don't quite understand your point here. A client would always encrypt the proxied stream when the destination address is https (since the proxy only acts as a tunnel, not as an endpoint of a TLS session in this case). Only the initial CONNECT request towards the proxy is unencrypted in that scenario. This means a MITM could tamper with the host being connected to but then hostname verification should fail and the connection should be aborted 🤞. OK and there's the possibility of MITMs snooping which hosts clients are connecting to. But given that the network context is assumed to be trusted, both issues should be no problem in practice (except for defense in depth, of course, which is why I still find it worth supporting).
The point I was trying to raise is that, when Aleph acts as a secure http proxy (not the client), ALPN will occur before the CONNECT is sent, so it might negotiate http2 before it could learn that won't work.
Aaah 💡 Right, using Aleph as a proxy server is quite a different situation and there's no first-class support for that really (note how Iow-level I had to go in the test). IMHO first-class proxy support is out of scope for Aleph's HTTP server. There's enough special-purpose implementations of this already, I don't see how a (customized?) Clojure-based adds much to the table.
In any case, I would like to keep this PR focused on restoring the status quo ante and created #743 for adding HTTPS proxy support. Thanks your input and for thinking along! 🙏
There was a problem hiding this comment.
A client would always encrypt the proxied stream when the destination address is https (since the proxy only acts as a tunnel, not as an endpoint of a TLS session in this case).
Sure, but what if the destination address is insecure http? The whole session will be in cleartext. I think the default should be encryption in 2025.
IMHO first-class proxy support is out of scope for Aleph's HTTP server. There's enough special-purpose implementations of this already, I don't see how a (customized?) Clojure-based adds much to the table.
I don't recall, what will an Aleph server do at the moment when it gets a CONNECT?
I agree there's no particular need for Aleph to do this, though, other than for completeness. I doubt many servers are simultaneously Clojure web apps and proxies.
DerGuteMoritz
force-pushed
the
fix-http-client-proxy-support
branch
from
49cb9d2 to
d498758
Compare
* Ensure that proxy connection errors have precedence over subsequent errors so that they don't get wrapped in e.g. SSL handshake exceptions. * Fix proxy connection timeout detection (the check wasn't correct anymore). * Propagate ProxyConnectionTimeoutException directly instead of wrapping it in another ConnectionTimeoutException * Add tests for proxy connection errors
| (fn pipeline-builder* | ||
| [^ChannelPipeline pipeline] | ||
| (add-proxy-handlers pipeline proxy-connected proxy-options ssl?) |
There was a problem hiding this comment.
Should be after the SSL handlers, for security, no?
There was a problem hiding this comment.
As in .addAfter? Or code ordering wise? The former won't do the right thing and the latter I'm not sure makes a difference since the pipeline builder is run atomically before anything else happens. Since the proxy handlers are added to the front of the pipeline (via .addFirst) and the SSL handler is added to the back, I would tend to leave the code ordering as it is since it mirrors the pipeline order.
There was a problem hiding this comment.
Since the proxy handlers are added to the front of the pipeline (via .addFirst)
Dammit. This is one of the most annoying things about netty. I thought we consistentnly used .addLast.
I guess I should say, if we supported encrypted client<->proxy connections, we'd have to ensure there was a second SslHandler before the proxy handlers in the pipeline.
Fixes #731