Add https proxy support#6215
Conversation
01a7c60 to
93b7ef3
Compare
93b7ef3 to
d0021d1
Compare
d0021d1 to
0b5db35
Compare
| public class HttpsProxyTest extends HttpTestBase { | ||
|
|
||
| private Vertx proxyVertx; | ||
| private HttpProxy proxy; |
There was a problem hiding this comment.
instead of manually setting up the proxy, we should instead support that in the io.vertx.test.proxy.Proxy rule
|
|
||
| protected boolean supportsFileRegion() { | ||
| return vertx.transport().supportFileRegion() && !isSsl() &&!isTrafficShaped(); | ||
| boolean sslChannel = chctx.pipeline().get(SslHandler.class) != null; |
There was a problem hiding this comment.
what is the motivation of this change ?
There was a problem hiding this comment.
This function is used in sendFile to decide whether the connection is encrypted and if the connection is encrypted then it desides not to use zero-copy. The reasoning to my understanding is the the way zero copy writes to the socket bypasses the encryption which is why it is disabled.
In my PR I have opted for isSsl() to refer to how we connect to the origin and not if the connection is encrypted.
So the addition covers the case where we have an SSL connection to the proxy but the origin is plain http. In this case we would have encryption on the pipeline. In that case isSsl() would be false but encryption would exist in the pipeline.
Maybe we should write a test to actually see how this is working. @vietj Are you aware of any tests that exercise this zero-copy function?
There was a problem hiding this comment.
Thank you for your comment @vietj. It led to finding multiple problems with my implementation that I have now hopefully fixed. I have added a test to verify my motivation for the zero copy io.vertx.tests.http.HttpsProxyTest#testHttpsProxy_sendFileThroughTunnel and I have found the way that I addressed it above was not correct. Instead the fix should have been in NetClientImpl.initChannel. I have fixed that in the latest commited code.
This work also motivated me that I needed to be more precise with how I address my intention to keep isSsl() independent of the existence of an https proxy. The way this was working for not consistent before. I have added io.vertx.tests.http.ProxyIsSslConsistencyTest to confirm this and fixed the cases that were not working correctly.
Co-authored-by: Julien Viet <julien@julienviet.com>
e0ad60d to
8ae9b16
Compare
We want isSsl to return consistent results regardless of whether we connect via a http or https proxy. A proxy is a connection detail that should not affect how origin connections are viewed
Motivation:
Closes: #6207
Add HTTPS proxy support (
ProxyType.HTTPS)What this adds
Support for connecting to a forward proxy over TLS — i.e. the connection to
the proxy itself (leg 1) is encrypted. Enabled via:
The proxying semantics are unchanged from
ProxyType.HTTP:GET, single TLS leg to theproxy).
CONNECTtunnel (leg-1 TLS to the proxy, then a secondnested TLS to the origin). HTTP/2 over the tunnel negotiates via ALPN
end-to-end. Works for both
HttpClientandNetClient(upgradeToSsl).Key design decisions
ConnectionBase.isSsl()keeps meaning "the origin is TLS", not "the wireis TLS". Existing code and the public contract rely on
ConnectionBase.isSsl()reflecting the origin. Rather than overload it,transport-level TLS (TLS to an HTTPS proxy) is computed separately via the
transportSsl/proxyHttpshelpers inTcpHttpClientTransport. The newoverride
Http1ClientConnection.isSsl()returns the origin flag.Transport-level "how to connect" calculations live in the transport.
HttpConnectParamscarries intent (forwardProxyflag, originssl,original
proxyOptions);TcpHttpClientTransportderives peer/SNI, TLSoptions, ALPN eligibility, and proxy hostname verification at the point of
use. The forward-vs-CONNECT decision stays at endpoint-creation time because
it selects the pooling/connect target (
EndpointKey).Zero-copy file region is gated on the actual pipeline, not
ConnectionBase.isSsl(). BecauseConnectionBase.isSsl()now strictlymeans "origin",
VertxConnection.supportsFileRegion()checks for anSslHandlerin the pipeline instead of calling the inheritedConnectionBase.isSsl()— zero-copy must be disabled whenever the wire isencrypted (e.g. leg-1 TLS to the proxy), regardless of the logical origin
flag.
Two
SslHandlers in the CONNECT-tunnel pipeline.NetSocketImplinsertsthe origin (leg-2) handler after the proxy (leg-1) handler (
proxy-ssl→ssl), andapplicationLayerProtocol()reads the innermostSslHandler, since ALPN is negotiated on the origin handshake.Secure defaults for the proxy leg. Hostname verification defaults to
"HTTPS"for the proxy connection; ALPN is never offered on the leg-1(proxy) handshake — it belongs to the origin; there is no plaintext
fallback for an HTTPS proxy (a TLS handshake failure against a plaintext
proxy fails cleanly rather than downgrading). Mutual TLS to the proxy is
supported.
Proxy SSL options participate in pool identity.
EndpointKeyincludesProxyOptions.getSslOptions()inequals/hashCodeso connections withdifferent proxy TLS config aren't pooled together.
Tests
HttpsProxyTest: forward (plain origin),CONNECTtunnel (HTTP/1 andHTTP/2 origins), proxy auth, mutual TLS, and negative cases (untrusted proxy
cert, hostname mismatch, plaintext-proxy no-downgrade).
NetTestcases forCONNECTover a TLS proxy (NetClient+upgradeToSsl).ProxyOptionsTestfor the newsslOptionsfield.Issues encountered & how they were resolved
(
testHttpsProxy_Http2Origin_tunnelled): the response body was read acrossthe
await()boundary, so body chunks could arrive with no handler attached.Fixed by reading the body inside the same future composition
(
resp.body().map(...)), which also folds the negotiated version into theasserted value. (Commit
2b8895f51.)HttpsProxyTest: fixed (commit93b7ef36b).Note on CI failures
CI runs on this branch intermittently failed on tests that I have seen failing
also in unrelated PRs suggesting these are flaky tests.
HttpSendFileTest.testSendOpenRangeFileFromClasspath(empty HTTP body), andNetTest.testListenDomainSocketAddress(empty receive,-PNativeEpoll).Conformance:
You should have signed the Eclipse Contributor Agreement as explained in https://github.com/eclipse/vert.x/blob/master/CONTRIBUTING.md
Please also make sure you adhere to the code style guidelines: https://github.com/vert-x3/wiki/wiki/Vert.x-code-style-guidelines