Conversation
…5575) async-http-client 2.x's setProxyServer(ProxyType.SOCKS_V5) silently bypasses the proxy. ChannelManager#getBootstrap composes two ChannelInitializers that share a single ChannelHandlerContext -- the inner one removes the context before the outer initChannel can install the SOCKS handler. Traffic goes directly to the target host while the call still succeeds, making the failure invisible. Both AsyncHttpConnector (PulsarAdmin) and HttpClient (PulsarClient) are affected. Verified against AHC 2.16.1 with JDK 17: the SOCKS5 server receives zero bytes while the HTTP request succeeds with 200. Fix: - Add Socks5ProxyChannelConfigurer in pulsar-client that installs Socks5ProxyHandler directly on the Netty pipeline via setHttpAdditionalChannelInitializer, matching the approach used by PulsarChannelInitializer#initSocks5IfConfig for binary connections. - Register the handler under name "socks" (matching AHC's ChannelManager.SOCKS_HANDLER constant). Socks5ProxyHandler.protocol() returns "socks5" which would cause NoSuchElementException on any addAfter(SOCKS_HANDLER, ...) lookup. - Add Socks5HandshakeAwaitHandler that defers the channel connect promise until ProxyHandler.connectFuture() completes, so AHC's SslHandler is inserted after the tunnel is established rather than ahead of the pending SOCKS5 handshake (fixes HTTPS-over-SOCKS5). - Replace broken setProxyServer calls in both AsyncHttpConnector and HttpClient with the shared Socks5ProxyChannelConfigurer. - Add testConfigureSocks5_pipelineIsHttpsSafe regression test. - Replace mock-only verification tests with end-to-end tests using a real Netty SOCKS5 server.
lhotari
left a comment
There was a problem hiding this comment.
Thanks for investigating the SOCKS5 behavior and adding a live proxy test. The workaround introduces an unresolved-proxy-address regression, and its rationale needs to be checked against the AHC version currently used by this branch.
CI also fails in PulsarAdminBuilderImplTest.testSocks5ProxyAddressIsConfiguredOnHttpClient, which still expects an AHC ProxyServer; please reconcile that test with the chosen approach. Failing job.
| } | ||
| Socks5ProxyHandler socks5ProxyHandler = StringUtils.isNotBlank(username) | ||
| ? new Socks5ProxyHandler(socks5Address, username, password) | ||
| : new Socks5ProxyHandler(socks5Address); |
There was a problem hiding this comment.
[BUG] Resolve the proxy address before passing it to Netty
A caller can supply InetSocketAddress.createUnresolved("localhost", port) through either client builder. This passes that address unchanged to Socks5ProxyHandler, whose connect calls ctx.connect(proxyAddress, ...) directly; it does not resolve the proxy hostname. NIO connections therefore fail with UnresolvedAddressException. The existing AHC ProxyServer path resolves the proxy asynchronously before constructing the handler. Please preserve that resolution and cover an unresolved proxy address in the live test.
| * <p><b>Why not async-http-client's own {@code setProxyServer(...)}?</b> AHC 2.x builds a | ||
| * dedicated bootstrap for SOCKS proxies in {@code ChannelManager#getBootstrap} by wrapping the | ||
| * regular HTTP {@code ChannelInitializer} in a second one: | ||
| * |
There was a problem hiding this comment.
[INTENT MISMATCH] Check the workaround against the current AHC version
This branch uses AHC 3.0.13, rather than the 2.16.1 cited in the description:
pulsar/gradle/libs.versions.toml
Lines 84 to 88 in 066d687
In 3.0.13, configureSocksBootstrap adds the HTTP initializer as a separate pipeline entry, avoiding the shared-context bug described here, and TLS is inserted after the SOCKS handler when ProxyServer is configured. Could you first run the live regression test with the existing setProxyServer wiring and add an actual HTTPS roundtrip? The current HTTPS test only checks handler presence/order. If the existing wiring passes, we can keep the regression coverage without adding this workaround and handshake barrier.
Motivation
PR #25575 added SOCKS5 proxy support for
PulsarAdminandPulsarClientHTTP paths. However, the proxy is silently bypassed at runtime — requests succeed with HTTP 200 but go directly to the target host without ever touching the SOCKS5 proxy.Root cause: async-http-client 2.x's
setProxyServer(ProxyType.SOCKS_V5)is broken.ChannelManager#getBootstrapcomposes twoChannelInitializers — the regular HTTP bootstrap handler and a SOCKS wrapper — by calling the inner handler'shandlerAddedbefore its owninitChannel. The innerhandlerAddedtriggersChannelInitializer#initChannel, which removes the sharedChannelHandlerContextin itsfinallyblock. The outerinitChannel(the one that would calladdFirst(SOCKS_HANDLER, socksProxyHandler)) then never executes. Net effect: the SOCKS handler is never installed, and traffic goes direct.Verified empirically against AHC 2.16.1 (used by master) with JDK 17: a local SOCKS5 server with password auth receives zero bytes while the HTTP request returns 200.
Both
AsyncHttpConnector(PulsarAdmin) andHttpClient(PulsarClient) are affected.Modifications
New
Socks5ProxyChannelConfigurer(pulsar-client): InstallsSocks5ProxyHandlerdirectly on the Netty pipeline viasetHttpAdditionalChannelInitializer, matching the approach already used byPulsarChannelInitializer#initSocks5IfConfigfor binary protocol connections.Handler naming: Registers the handler under the name
"socks"(matching AHC'sChannelManager.SOCKS_HANDLERconstant) instead ofSocks5ProxyHandler.protocol()which returns"socks5"and would causeNoSuchElementExceptionon anyaddAfter(SOCKS_HANDLER, ...)lookup.HTTPS-over-SOCKS5 fix: Adds
Socks5HandshakeAwaitHandlerthat defers the channel's connect promise untilProxyHandler.connectFuture()completes. Without this, AHC inserts itsSslHandleron connect-promise completion (which Netty fires at TCP-connect-to-proxy time, before the SOCKS5 handshake finishes), placing TLS ahead of the proxy handler. TheClientHellothen races ahead of the SOCKS5 negotiation, causing a handshake timeout.Both call sites fixed: Replace broken
setProxyServercalls inAsyncHttpConnectorandHttpClientwith the sharedSocks5ProxyChannelConfigurer.Test overhaul: Replace mock-only
verify(builder).setProxyServer(...)tests with end-to-end tests using a real Netty SOCKS5 server (password auth, CONNECT verification, origin-server hit confirmation). AddtestConfigureSocks5_pipelineIsHttpsSaferegression test that locks in the handler name and handshake barrier requirements.Gradle dependency: Add explicit
netty-handler-proxy,netty-codec-socks,netty-transporttopulsar-client-admin/build.gradle.kts(previously only available transitively).Verifying this change
End-to-end verification matrix (all scenarios tested with real AHC 2.16.1):
Does this pull request potentially affect one of the following parts?
Documentation
doc-requireddoc-not-neededdocdoc-complete