Add https proxy support#6215
Conversation
01a7c60 to
93b7ef3
Compare
93b7ef3 to
d0021d1
Compare
d0021d1 to
0b5db35
Compare
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
|
|
||
| @Override | ||
| public boolean isSsl() { | ||
| // Report the origin ssl, not the pipeline: in forward-proxy mode the TLS leg to an HTTPS proxy is |
There was a problem hiding this comment.
so it means it should return false if the connection to the server uses a plain text connection over an HTTPS proxy ?
can we ensure that for both case there is an assertion for this ? I am thinking of
- a plain text connection through an HTTPS proxy
- an SSL connection through an HTTP proxy
| } | ||
|
|
||
| proxySslProviderFuture.onComplete(ar -> { | ||
| if (ar.failed()) { |
There was a problem hiding this comment.
we should encapsulate this code in a method instead of channelling it through the proxySslProviderFuture that is a succeeded future when there is no proxy to reduce the callback complexity in the normal case.
so when there is no https proxy we do call directly the method that setup the bootstrap, otherwise we do resolve the proxySslProviderFuture and continue from this.
| pipeline.addLast("logging", new LoggingHandler(logging)); | ||
| } | ||
| if (ssl || !vertx.transport().supportFileRegion()) { | ||
| boolean sslChannel = pipeline.get(SslHandler.class) != null; |
There was a problem hiding this comment.
if we use this, shouldn't we remove the boolean ssl from the initChannel method ?
| .setSsl(true) | ||
| .setUseAlpn(version == HttpVersion.HTTP_2) | ||
| .setKeyCertOptions(Cert.SERVER_JKS.get())); | ||
| server.requestHandler(req -> req.response().end("Hello from origin")); |
There was a problem hiding this comment.
I think the server should respond with the boolean ssl status of its connection in its message and we should assert it in tests.
vietj
left a comment
There was a problem hiding this comment.
A few comments have been made to this PR.
|
@gaganis ping, can you also look at the conflict to resolve ? |
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