From b820743f91c37a5d4f4f8fa62827b05c20bbf471 Mon Sep 17 00:00:00 2001 From: Oleg Kalnichevski Date: Sat, 31 Oct 2026 14:53:11 +0100 Subject: [PATCH 1/5] Moved request cancellation integration tests into separate groups to be executed manually during release preparation due to those tests failing intermittently in CI --- httpclient5-testing/pom.xml | 2 +- .../org/apache/hc/client5/testing/async/TestHttp1Async.java | 2 ++ .../hc/client5/testing/sync/TestClientRequestExecution.java | 5 +++-- pom.xml | 2 +- 4 files changed, 7 insertions(+), 4 deletions(-) diff --git a/httpclient5-testing/pom.xml b/httpclient5-testing/pom.xml index 97c1e96401..36062df988 100644 --- a/httpclient5-testing/pom.xml +++ b/httpclient5-testing/pom.xml @@ -126,7 +126,7 @@ org.apache.maven.plugins maven-surefire-plugin - slow,time-out,stale-conn + slow,time-out,stale-conn,cancellation diff --git a/httpclient5-testing/src/test/java/org/apache/hc/client5/testing/async/TestHttp1Async.java b/httpclient5-testing/src/test/java/org/apache/hc/client5/testing/async/TestHttp1Async.java index 700c631b36..cf5b0a477f 100644 --- a/httpclient5-testing/src/test/java/org/apache/hc/client5/testing/async/TestHttp1Async.java +++ b/httpclient5-testing/src/test/java/org/apache/hc/client5/testing/async/TestHttp1Async.java @@ -46,6 +46,7 @@ import org.apache.hc.core5.http.HttpHost; import org.apache.hc.core5.http.URIScheme; import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.Tag; import org.junit.jupiter.api.Test; import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.ValueSource; @@ -139,6 +140,7 @@ void testSharedPool() throws Exception { Assertions.assertEquals(2048, body3.length()); } + @Tag("cancellation") @Test void testRequestCancellation() throws Exception { configureServer(bootstrap -> bootstrap.register("/random/*", AsyncRandomHandler::new)); diff --git a/httpclient5-testing/src/test/java/org/apache/hc/client5/testing/sync/TestClientRequestExecution.java b/httpclient5-testing/src/test/java/org/apache/hc/client5/testing/sync/TestClientRequestExecution.java index 17173a4253..0eb8de18fa 100644 --- a/httpclient5-testing/src/test/java/org/apache/hc/client5/testing/sync/TestClientRequestExecution.java +++ b/httpclient5-testing/src/test/java/org/apache/hc/client5/testing/sync/TestClientRequestExecution.java @@ -65,7 +65,7 @@ import org.apache.hc.core5.net.URIBuilder; import org.apache.hc.core5.util.TimeValue; import org.junit.jupiter.api.Assertions; -import org.junit.jupiter.api.Disabled; +import org.junit.jupiter.api.Tag; import org.junit.jupiter.api.Test; /** @@ -316,7 +316,8 @@ void testAbsoluteRequestURIWithFragment() throws Exception { Assertions.assertEquals(uri, location); } - @Test @Disabled("Fails intermittently with GitHub Actions") + @Tag("cancellation") + @Test void testRequestCancellation() throws Exception { startServer(); final HttpHost target = startServer(); diff --git a/pom.xml b/pom.xml index 36db97227e..ac7e99dc0e 100644 --- a/pom.xml +++ b/pom.xml @@ -60,7 +60,7 @@ - 8 + 17 17 ${javaVersion} ${javaVersion} From 9e45136c42f7226469b0f7d1fa1e7fb3999719ae Mon Sep 17 00:00:00 2001 From: Arturo Bernal Date: Sun, 1 Nov 2026 10:14:30 +0100 Subject: [PATCH 2/5] Do not skip hostname verification when the CLIENT policy has no verifier Since the default verifier fallback was removed, CLIENT or BOTH policy with a null HostnameVerifier performed no hostname verification at all. ConscryptClientTlsStrategy and the deprecated DefaultClientTlsStrategy constructor now select the BUILTIN policy when no verifier is given, and AbstractClientTlsStrategy rejects CLIENT or BOTH without a verifier. --- .../http/ssl/AbstractClientTlsStrategy.java | 4 + .../http/ssl/ConscryptClientTlsStrategy.java | 3 +- .../http/ssl/DefaultClientTlsStrategy.java | 3 +- .../ssl/AbstractClientTlsStrategyTest.java | 30 ++++ .../ssl/TestNullHostnameVerifierPolicy.java | 147 ++++++++++++++++++ httpclient5/src/test/resources/test.keystore | Bin 0 -> 2243 bytes 6 files changed, 185 insertions(+), 2 deletions(-) create mode 100644 httpclient5/src/test/java/org/apache/hc/client5/http/ssl/TestNullHostnameVerifierPolicy.java create mode 100644 httpclient5/src/test/resources/test.keystore diff --git a/httpclient5/src/main/java/org/apache/hc/client5/http/ssl/AbstractClientTlsStrategy.java b/httpclient5/src/main/java/org/apache/hc/client5/http/ssl/AbstractClientTlsStrategy.java index d403ff18df..0aba6a75ae 100644 --- a/httpclient5/src/main/java/org/apache/hc/client5/http/ssl/AbstractClientTlsStrategy.java +++ b/httpclient5/src/main/java/org/apache/hc/client5/http/ssl/AbstractClientTlsStrategy.java @@ -99,6 +99,10 @@ abstract class AbstractClientTlsStrategy implements TlsStrategy, TlsSocketStrate this.sslBufferManagement = sslBufferManagement != null ? sslBufferManagement : SSLBufferMode.STATIC; this.hostnameVerificationPolicy = hostnameVerificationPolicy != null ? hostnameVerificationPolicy : (hostnameVerifier != null ? HostnameVerificationPolicy.BOTH : HostnameVerificationPolicy.BUILTIN); + Args.check(this.hostnameVerificationPolicy != HostnameVerificationPolicy.CLIENT + && this.hostnameVerificationPolicy != HostnameVerificationPolicy.BOTH + || hostnameVerifier != null, + "Hostname verifier is required for hostname verification policy " + this.hostnameVerificationPolicy); this.hostnameVerifier = hostnameVerifier; } diff --git a/httpclient5/src/main/java/org/apache/hc/client5/http/ssl/ConscryptClientTlsStrategy.java b/httpclient5/src/main/java/org/apache/hc/client5/http/ssl/ConscryptClientTlsStrategy.java index bcf9bfd344..d635866f73 100644 --- a/httpclient5/src/main/java/org/apache/hc/client5/http/ssl/ConscryptClientTlsStrategy.java +++ b/httpclient5/src/main/java/org/apache/hc/client5/http/ssl/ConscryptClientTlsStrategy.java @@ -69,7 +69,8 @@ public ConscryptClientTlsStrategy( final String[] supportedCipherSuites, final SSLBufferMode sslBufferManagement, final HostnameVerifier hostnameVerifier) { - this(sslContext, supportedProtocols, supportedCipherSuites, sslBufferManagement, HostnameVerificationPolicy.CLIENT, hostnameVerifier); + this(sslContext, supportedProtocols, supportedCipherSuites, sslBufferManagement, + hostnameVerifier != null ? HostnameVerificationPolicy.CLIENT : HostnameVerificationPolicy.BUILTIN, hostnameVerifier); } /** diff --git a/httpclient5/src/main/java/org/apache/hc/client5/http/ssl/DefaultClientTlsStrategy.java b/httpclient5/src/main/java/org/apache/hc/client5/http/ssl/DefaultClientTlsStrategy.java index c52997473f..2792df98f0 100644 --- a/httpclient5/src/main/java/org/apache/hc/client5/http/ssl/DefaultClientTlsStrategy.java +++ b/httpclient5/src/main/java/org/apache/hc/client5/http/ssl/DefaultClientTlsStrategy.java @@ -101,7 +101,8 @@ public DefaultClientTlsStrategy( final SSLBufferMode sslBufferManagement, final HostnameVerifier hostnameVerifier, final Factory tlsDetailsFactory) { - super(sslContext, supportedProtocols, supportedCipherSuites, sslBufferManagement, HostnameVerificationPolicy.CLIENT, hostnameVerifier); + super(sslContext, supportedProtocols, supportedCipherSuites, sslBufferManagement, + hostnameVerifier != null ? HostnameVerificationPolicy.CLIENT : HostnameVerificationPolicy.BUILTIN, hostnameVerifier); this.tlsDetailsFactory = tlsDetailsFactory; } diff --git a/httpclient5/src/test/java/org/apache/hc/client5/http/ssl/AbstractClientTlsStrategyTest.java b/httpclient5/src/test/java/org/apache/hc/client5/http/ssl/AbstractClientTlsStrategyTest.java index 20a2bbd407..a052ca16e7 100644 --- a/httpclient5/src/test/java/org/apache/hc/client5/http/ssl/AbstractClientTlsStrategyTest.java +++ b/httpclient5/src/test/java/org/apache/hc/client5/http/ssl/AbstractClientTlsStrategyTest.java @@ -27,17 +27,21 @@ package org.apache.hc.client5.http.ssl; +import static org.junit.jupiter.api.Assertions.assertDoesNotThrow; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertThrows; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; import java.security.cert.X509Certificate; +import javax.net.ssl.HostnameVerifier; import javax.net.ssl.SSLEngine; import javax.net.ssl.SSLParameters; import javax.net.ssl.SSLSession; import javax.security.auth.x500.X500Principal; +import org.apache.hc.core5.function.Factory; import org.apache.hc.core5.reactor.ssl.SSLBufferMode; import org.apache.hc.core5.reactor.ssl.TlsDetails; import org.apache.hc.core5.ssl.SSLContexts; @@ -110,5 +114,31 @@ TlsDetails createTlsDetails(final SSLEngine sslEngine) { } + @Test + void testClientPolicyRequiresHostnameVerifier() { + assertThrows(IllegalArgumentException.class, () -> new DefaultClientTlsStrategy( + SSLContexts.createDefault(), HostnameVerificationPolicy.CLIENT, null)); + assertThrows(IllegalArgumentException.class, () -> new DefaultClientTlsStrategy( + SSLContexts.createDefault(), HostnameVerificationPolicy.BOTH, null)); + } + + @Test + void testBuilderClientPolicyRequiresHostnameVerifier() { + final ClientTlsStrategyBuilder builder = ClientTlsStrategyBuilder.create() + .setSslContext(SSLContexts.createDefault()) + .setHostVerificationPolicy(HostnameVerificationPolicy.CLIENT); + assertThrows(IllegalArgumentException.class, builder::buildClassic); + } + + @Test + @SuppressWarnings("deprecation") + void testNullHostnameVerifierDefaultsToBuiltinPolicy() { + assertDoesNotThrow(() -> new DefaultClientTlsStrategy( + SSLContexts.createDefault(), null, null, SSLBufferMode.STATIC, + (HostnameVerifier) null, (Factory) null)); + assertDoesNotThrow(() -> new DefaultClientTlsStrategy(SSLContexts.createDefault())); + assertDoesNotThrow(() -> new ConscryptClientTlsStrategy(SSLContexts.createDefault())); + } + } \ No newline at end of file diff --git a/httpclient5/src/test/java/org/apache/hc/client5/http/ssl/TestNullHostnameVerifierPolicy.java b/httpclient5/src/test/java/org/apache/hc/client5/http/ssl/TestNullHostnameVerifierPolicy.java new file mode 100644 index 0000000000..02b9ee22b0 --- /dev/null +++ b/httpclient5/src/test/java/org/apache/hc/client5/http/ssl/TestNullHostnameVerifierPolicy.java @@ -0,0 +1,147 @@ +/* + * ==================================================================== + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + * ==================================================================== + * + * This software consists of voluntary contributions made by many + * individuals on behalf of the Apache Software Foundation. For more + * information on the Apache Software Foundation, please see + * . + * + */ + + +package org.apache.hc.client5.http.ssl; + +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertThrows; + +import java.io.IOException; +import java.net.InetAddress; +import java.net.Socket; + +import javax.net.ssl.HostnameVerifier; +import javax.net.ssl.SSLContext; +import javax.net.ssl.SSLEngine; +import javax.net.ssl.SSLException; +import javax.net.ssl.SSLServerSocket; +import javax.net.ssl.SSLSocket; + +import org.apache.hc.core5.function.Factory; +import org.apache.hc.core5.reactor.ssl.SSLBufferMode; +import org.apache.hc.core5.reactor.ssl.TlsDetails; +import org.apache.hc.core5.ssl.SSLContexts; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; + +/** + * Handshakes against a server whose certificate is issued for {@code localhost} to make sure + * that a strategy created without a {@link javax.net.ssl.HostnameVerifier} still verifies the host name. + */ +class TestNullHostnameVerifierPolicy { + + private static final char[] PASSWORD = "nopassword".toCharArray(); + + private SSLContext sslContext; + private SSLServerSocket serverSocket; + private Thread serverThread; + + @BeforeEach + void setUp() throws Exception { + sslContext = SSLContexts.custom() + .loadTrustMaterial(getClass().getResource("/test.keystore"), PASSWORD) + .loadKeyMaterial(getClass().getResource("/test.keystore"), PASSWORD, PASSWORD) + .build(); + serverSocket = (SSLServerSocket) sslContext.getServerSocketFactory() + .createServerSocket(0, 1, InetAddress.getLoopbackAddress()); + serverThread = new Thread(() -> { + try (final SSLSocket socket = (SSLSocket) serverSocket.accept()) { + socket.startHandshake(); + } catch (final IOException ignore) { + // the client may abort the handshake + } + }); + serverThread.setDaemon(true); + serverThread.start(); + } + + @AfterEach + void tearDown() throws Exception { + serverSocket.close(); + serverThread.join(5000); + } + + private SSLSocket upgrade(final AbstractClientTlsStrategy strategy, final String target) throws IOException { + final Socket socket = new Socket(InetAddress.getLoopbackAddress(), serverSocket.getLocalPort()); + try { + return strategy.upgrade(socket, target, serverSocket.getLocalPort(), null, null); + } catch (final IOException | RuntimeException ex) { + socket.close(); + throw ex; + } + } + + @SuppressWarnings("deprecation") + private AbstractClientTlsStrategy defaultStrategy() { + return new DefaultClientTlsStrategy( + sslContext, null, null, SSLBufferMode.STATIC, + (HostnameVerifier) null, (Factory) null); + } + + @SuppressWarnings("deprecation") + private AbstractClientTlsStrategy conscryptStrategy() { + return new ConscryptClientTlsStrategy( + sslContext, null, null, SSLBufferMode.STATIC, (HostnameVerifier) null); + } + + @Test + void testDefaultStrategyRejectsMismatchingHost() { + assertThrows(SSLException.class, () -> upgrade(defaultStrategy(), "example.invalid")); + } + + @Test + void testDefaultStrategyAcceptsMatchingHost() throws Exception { + try (final SSLSocket socket = upgrade(defaultStrategy(), "localhost")) { + assertNotNull(socket.getSession().getPeerCertificates()); + } + } + + @Test + void testConscryptStrategyRejectsMismatchingHost() { + assertThrows(SSLException.class, () -> upgrade(conscryptStrategy(), "example.invalid")); + } + + @Test + void testConscryptStrategyAcceptsMatchingHost() throws Exception { + try (final SSLSocket socket = upgrade(conscryptStrategy(), "localhost")) { + assertNotNull(socket.getSession().getPeerCertificates()); + } + } + + @Test + void testConscryptDefaultConstructorRejectsMismatchingHost() { + assertThrows(SSLException.class, () -> upgrade(new ConscryptClientTlsStrategy(sslContext), "example.invalid")); + } + + @Test + void testDefaultConstructorRejectsMismatchingHost() { + assertThrows(SSLException.class, () -> upgrade(new DefaultClientTlsStrategy(sslContext), "example.invalid")); + } + +} diff --git a/httpclient5/src/test/resources/test.keystore b/httpclient5/src/test/resources/test.keystore new file mode 100644 index 0000000000000000000000000000000000000000..f8d5ace1ad25f92675c2b94caeec3c117f857fb8 GIT binary patch literal 2243 zcmcIl`8(8$7oN=?V>ie$6eY`NWDjKtl{J(lWj8XI7=$bh5sjrrl4vlpPLY{wU$SQ_ zk`cnil*qnhxt6c)bARX``2O(z@}Bp3&U4;#p7$Iq9xQ@DAjlEG|AIHr+ZT)RP(b5w zScO23fMAb+qmt*NWs)ff!~_NKutR{0nf(ki7zUPu@PNTgAQ&F@b3!rNw!Lo5RlIWV zGuxtCA&jD%y>C~5HS!SlRBxWVG9JCltLe2=c~M>dM&{0W)i2MwLdfY$U;U~AvqysV zcI1Tu#Msu9ELF{7g_)-9Phw-lXxNW=lP4>tMQ_@^p>J}UbPEOdkXPS44z9{?8+$9B zSNJGlKce|uoXtDKAA0f~!YNratK7t=%AS4mzV7vJkcJncwnC;8v@I8L%3TC^T_Y{l zCtammHQk;j*1Yk?Ny#uo7@2vnaQ0b z9z)6DcaLj)58<6MZvbr~Y($%N?kdY%9Xbup4NC3D>^s&6FR|(AO;)Ht7Ae`i*M*O% zCh``SKLqzM#uYiGCz3Pfex(pUUkL2lfMb!^2$7PD^-f{CwP%|JO6D5}7^2^o<{X-?iX+8;jdDdSHbSq^24>Ug{UYFJTk8`bj<#j;d0_nZeR! zjc#tb-PYch-PTC;o|7LmNctE?YP3w{y3uHI-LZvc68|n*8bg1!vM$=Fu3wwv@BAR& zfM8Ch_O__lMru;d5u}$flGb8rKkyA{6(_vJ)n{L9E!%S*+uYQPE!C3K57%kpUW)p) zGkZ{Y-HU{di18hyI`rqnsIA@>gnr_j@3fo~8NDe}K+{U^^~L7b8%0SAyeZxFia6C- zHQ?9QmeZ#fm6uav2*dx7NyDT0 zr@Jw?$}8uL)kKMNkvWB(sSZ2U>a8q;C@mWKmg;%;8J{#P2@S8#sjJ%$ zN0_mJ>^1rmd70UcgF^Q{2<1y!Q)6E~GTtOD+j#b^Y4a9Mt^0V53D)>1_6U<{_E-tR z<&j_MO9O{rkqRD5{bduwq^(G^^C1qako0w)B%_=It2jY`wct6DX+AoWagj>S$leO|1$OhO^(Z# z9azzxQTtTd{U523I^~|~HW>zxxTW_m3J_R_CQ`d9hs-~4x-Sc{-X_pliJ%!Rju)bd zlaomn>#z)#rBS5g6w)o3N6~k1pskRrJnIxim#6oW3{+_rHmjDk*BmW>jcNd(uzaL5*IR0D2!6)#a$mA%44g7tv{(c^QxIh#y z!2P=pw@@c>u0Z22e`Y7L1>>^tam>!`yajhOp$t(~u8 z|CV_?ZIKmZNPHrxKk(94r<{y#=eKwt#^CenxKF)xX}V=;(I~2@G(&4s-f?!&%yhhA zo5RQh>W_@RO(VDlf+Nmv1R7KmL!>qZ-SkEHHy0Fe$z7M@?!5QQsAj*ihUb^^UU`}l ziLTZvmEfZC1|?UHRfgJ|hCS)%RzTCdQ3jq0Wr*G~$7{j^J5)033?Z-Yo!bs9n=X$h zT5R(i6nwTZ(KpETA1*uXeuG@{U0RqvjU#Y=c+C>c{-8`(t!_B2R;wkGALun9a&*|i zEzWdZD%q)lK*1m|SshRX_>UGR#slL7IPiZgPwY3#1rvnv$3@3pzLK!EKXF*$e8%n* z<%Ibx<4_+s=O}~w_3^nkr7t0 j+=-YtJ;!r<`!BItX5^5E808gChqv2)op0*N=MVn_*7f=z literal 0 HcmV?d00001 From f805a8cfed0bf123e73beda9f9bd0cf65571aea1 Mon Sep 17 00:00:00 2001 From: Arturo Bernal Date: Sun, 1 Nov 2026 10:14:30 +0100 Subject: [PATCH 3/5] Do not treat a 407 received through a tunnel as a proxy challenge Once the CONNECT tunnel is established a 407 comes from the origin. It was evaluated against the proxy auth exchange, which failed with a NullPointerException or IllegalStateException. The response is now returned to the caller as is. --- .../http/impl/async/AsyncProtocolExec.java | 9 +- .../http/impl/classic/ProtocolExec.java | 9 +- .../impl/async/TestAsyncProtocolExec.java | 111 ++++++++++++++++++ .../http/impl/classic/TestProtocolExec.java | 104 ++++++++++++++++ 4 files changed, 225 insertions(+), 8 deletions(-) diff --git a/httpclient5/src/main/java/org/apache/hc/client5/http/impl/async/AsyncProtocolExec.java b/httpclient5/src/main/java/org/apache/hc/client5/http/impl/async/AsyncProtocolExec.java index 706793e98b..3a7de2326a 100644 --- a/httpclient5/src/main/java/org/apache/hc/client5/http/impl/async/AsyncProtocolExec.java +++ b/httpclient5/src/main/java/org/apache/hc/client5/http/impl/async/AsyncProtocolExec.java @@ -217,7 +217,8 @@ public AsyncDataConsumer handleResponse( if (needAuthentication( targetAuthExchange, proxyAuthExchange, - proxy != null ? proxy : target, + // A 407 received through a tunnel comes from the origin, not from the proxy + route.isTunnelled() ? null : proxy, target, pathPrefix, response, @@ -319,11 +320,11 @@ private boolean needAuthentication( } } - final boolean proxyAuthRequested = authenticator.isChallenged( + final boolean proxyAuthRequested = proxy != null && authenticator.isChallenged( proxy, ChallengeType.PROXY, response, proxyAuthExchange, context); - final boolean proxyMutualAuthRequired = authenticator.isChallengeExpected(proxyAuthExchange); + final boolean proxyMutualAuthRequired = proxy != null && authenticator.isChallengeExpected(proxyAuthExchange); - if (authCacheKeeper != null) { + if (proxy != null && authCacheKeeper != null) { if (proxyAuthRequested) { authCacheKeeper.updateOnChallenge(proxy, null, proxyAuthExchange, context); } else { diff --git a/httpclient5/src/main/java/org/apache/hc/client5/http/impl/classic/ProtocolExec.java b/httpclient5/src/main/java/org/apache/hc/client5/http/impl/classic/ProtocolExec.java index 4514ddfe94..cc73b224e5 100644 --- a/httpclient5/src/main/java/org/apache/hc/client5/http/impl/classic/ProtocolExec.java +++ b/httpclient5/src/main/java/org/apache/hc/client5/http/impl/classic/ProtocolExec.java @@ -207,7 +207,8 @@ public ClassicHttpResponse execute( if (needAuthentication( targetAuthExchange, proxyAuthExchange, - proxy != null ? proxy : target, + // A 407 received through a tunnel comes from the origin, not from the proxy + route.isTunnelled() ? null : proxy, target, pathPrefix, response, @@ -280,11 +281,11 @@ private boolean needAuthentication( } } - final boolean proxyAuthRequested = authenticator.isChallenged( + final boolean proxyAuthRequested = proxy != null && authenticator.isChallenged( proxy, ChallengeType.PROXY, response, proxyAuthExchange, context); - final boolean proxyMutualAuthRequired = authenticator.isChallengeExpected(proxyAuthExchange); + final boolean proxyMutualAuthRequired = proxy != null && authenticator.isChallengeExpected(proxyAuthExchange); - if (authCacheKeeper != null) { + if (proxy != null && authCacheKeeper != null) { if (proxyAuthRequested) { authCacheKeeper.updateOnChallenge(proxy, null, proxyAuthExchange, context); } else { diff --git a/httpclient5/src/test/java/org/apache/hc/client5/http/impl/async/TestAsyncProtocolExec.java b/httpclient5/src/test/java/org/apache/hc/client5/http/impl/async/TestAsyncProtocolExec.java index dff99e94e6..7fcae912dc 100644 --- a/httpclient5/src/test/java/org/apache/hc/client5/http/impl/async/TestAsyncProtocolExec.java +++ b/httpclient5/src/test/java/org/apache/hc/client5/http/impl/async/TestAsyncProtocolExec.java @@ -27,11 +27,14 @@ package org.apache.hc.client5.http.impl.async; import java.io.IOException; +import java.util.ArrayList; import java.util.Collections; +import java.util.List; import java.util.concurrent.atomic.AtomicInteger; import org.apache.hc.client5.http.AuthenticationStrategy; import org.apache.hc.client5.http.HttpRoute; +import org.apache.hc.client5.http.RouteInfo; import org.apache.hc.client5.http.async.AsyncExecCallback; import org.apache.hc.client5.http.async.AsyncExecChain; import org.apache.hc.client5.http.async.AsyncExecRuntime; @@ -39,6 +42,7 @@ import org.apache.hc.client5.http.auth.AuthScope; import org.apache.hc.client5.http.auth.ChallengeType; import org.apache.hc.client5.http.auth.StandardAuthScheme; +import org.apache.hc.client5.http.impl.auth.BasicAuthCache; import org.apache.hc.client5.http.impl.auth.BasicScheme; import org.apache.hc.client5.http.impl.auth.CredentialsProviderBuilder; import org.apache.hc.client5.http.protocol.HttpClientContext; @@ -157,4 +161,111 @@ public void failed(final Exception cause) { final AuthExchange authExchange = context.getAuthExchange(target); Assertions.assertEquals("/blah/", authExchange.getPathPrefix()); } + + private static final class RecordingCallback implements AsyncExecCallback { + + private final List codes = new ArrayList<>(); + private final AtomicInteger completed = new AtomicInteger(0); + + @Override + public AsyncDataConsumer handleResponse(final HttpResponse response, final EntityDetails entityDetails) { + codes.add(response.getCode()); + return Mockito.mock(AsyncDataConsumer.class); + } + + @Override + public void handleInformationResponse(final HttpResponse response) { + } + + @Override + public void completed() { + completed.incrementAndGet(); + } + + @Override + public void failed(final Exception cause) { + Assertions.fail(cause); + } + + } + + private void stubChain(final AtomicInteger proceedCount, final List requests, final int... codes) + throws Exception { + Mockito.doAnswer(invocation -> { + requests.add(invocation.getArgument(0, HttpRequest.class)); + final AsyncExecCallback cb = invocation.getArgument(3, AsyncExecCallback.class); + final int code = codes[Math.min(proceedCount.getAndIncrement(), codes.length - 1)]; + final HttpResponse resp = new BasicHttpResponse(code, "Huh?"); + if (code == 407) { + resp.setHeader(HttpHeaders.PROXY_AUTHENTICATE, StandardAuthScheme.BASIC + " realm=test"); + } + cb.handleResponse(resp, (EntityDetails) null); + cb.completed(); + return null; + }).when(chain).proceed(Mockito.any(), Mockito.any(), Mockito.any(), Mockito.any()); + } + + @Test + void testProxyChallengeThroughTunnelWithAuthCacheIsReturnedToCaller() throws Exception { + final AsyncProtocolExec cachingExec = new AsyncProtocolExec(targetAuthStrategy, proxyAuthStrategy, null, false); + final HttpHost httpsTarget = new HttpHost("https", "foo", 443); + final HttpHost proxy = new HttpHost("http", "proxy", 8080); + final HttpRoute route = new HttpRoute(httpsTarget, null, proxy, true, + RouteInfo.TunnelType.TUNNELLED, RouteInfo.LayerType.LAYERED); + final BasicAuthCache authCache = new BasicAuthCache(); + final HttpClientContext context = HttpClientContext.create(); + context.setAuthCache(authCache); + context.setCredentialsProvider(CredentialsProviderBuilder.create() + .add(new AuthScope(proxy), "proxyuser", "proxypass".toCharArray()) + .build()); + Mockito.when(execRuntime.isEndpointConnected()).thenReturn(true); + final AtomicInteger proceedCount = new AtomicInteger(0); + final List requests = new ArrayList<>(); + stubChain(proceedCount, requests, 407); + + final HttpRequest request = BasicRequestBuilder.get("https://foo/test").build(); + final AsyncExecChain.Scope scope = new AsyncExecChain.Scope("test", route, request, + Mockito.mock(CancellableDependency.class), context, execRuntime, null, new AtomicInteger(1)); + final RecordingCallback callback = new RecordingCallback(); + + cachingExec.execute(request, (AsyncEntityProducer) null, scope, chain, callback); + + Assertions.assertEquals(Collections.singletonList(407), callback.codes); + Assertions.assertEquals(1, callback.completed.get()); + Assertions.assertEquals(1, proceedCount.get()); + Assertions.assertNull(requests.get(0).getFirstHeader(HttpHeaders.PROXY_AUTHORIZATION)); + Mockito.verifyNoInteractions(proxyAuthStrategy); + Assertions.assertEquals(AuthExchange.State.UNCHALLENGED, context.getAuthExchange(proxy).getState()); + Assertions.assertNull(authCache.get(proxy)); + } + + @Test + void testProxyChallengeOnPlainRouteStillAuthenticates() throws Exception { + final HttpHost proxy = new HttpHost("http", "proxy", 8080); + final HttpRoute route = new HttpRoute(target, null, proxy, false); + final HttpClientContext context = HttpClientContext.create(); + context.setCredentialsProvider(CredentialsProviderBuilder.create() + .add(new AuthScope(proxy), "proxyuser", "proxypass".toCharArray()) + .build()); + Mockito.when(proxyAuthStrategy.select( + Mockito.eq(ChallengeType.PROXY), + Mockito.any(), + Mockito.any())) + .thenReturn(Collections.singletonList(new BasicScheme())); + Mockito.when(execRuntime.isEndpointConnected()).thenReturn(true); + final AtomicInteger proceedCount = new AtomicInteger(0); + final List requests = new ArrayList<>(); + stubChain(proceedCount, requests, 407, 200); + + final HttpRequest request = BasicRequestBuilder.get("http://foo/test").build(); + final AsyncExecChain.Scope scope = new AsyncExecChain.Scope("test", route, request, + Mockito.mock(CancellableDependency.class), context, execRuntime, null, new AtomicInteger(1)); + final RecordingCallback callback = new RecordingCallback(); + + protocolExec.execute(request, (AsyncEntityProducer) null, scope, chain, callback); + + Assertions.assertEquals(2, proceedCount.get()); + Assertions.assertEquals(Collections.singletonList(200), callback.codes); + Assertions.assertNotNull(requests.get(1).getFirstHeader(HttpHeaders.PROXY_AUTHORIZATION)); + } } diff --git a/httpclient5/src/test/java/org/apache/hc/client5/http/impl/classic/TestProtocolExec.java b/httpclient5/src/test/java/org/apache/hc/client5/http/impl/classic/TestProtocolExec.java index 3417e66ff5..9588a42972 100644 --- a/httpclient5/src/test/java/org/apache/hc/client5/http/impl/classic/TestProtocolExec.java +++ b/httpclient5/src/test/java/org/apache/hc/client5/http/impl/classic/TestProtocolExec.java @@ -34,6 +34,7 @@ import org.apache.hc.client5.http.AuthenticationStrategy; import org.apache.hc.client5.http.HttpRoute; +import org.apache.hc.client5.http.RouteInfo; import org.apache.hc.client5.http.auth.AuthExchange; import org.apache.hc.client5.http.auth.AuthScope; import org.apache.hc.client5.http.auth.ChallengeType; @@ -43,6 +44,7 @@ import org.apache.hc.client5.http.classic.methods.HttpGet; import org.apache.hc.client5.http.classic.methods.HttpPost; import org.apache.hc.client5.http.entity.EntityBuilder; +import org.apache.hc.client5.http.impl.auth.BasicAuthCache; import org.apache.hc.client5.http.impl.auth.BasicScheme; import org.apache.hc.client5.http.impl.auth.CredentialsProviderBuilder; import org.apache.hc.client5.http.protocol.HttpClientContext; @@ -58,6 +60,7 @@ import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.ExtendWith; +import org.mockito.ArgumentCaptor; import org.mockito.Mock; import org.mockito.Mockito; import org.mockito.junit.jupiter.MockitoExtension; @@ -295,4 +298,105 @@ void testAuthExchangePathPrefixRestoredAfterChallenge() throws Exception { final AuthExchange authExchange = context.getAuthExchange(target); Assertions.assertEquals("/blah/", authExchange.getPathPrefix()); } + + @Test + void testProxyChallengeThroughTunnelIsReturnedToCaller() throws Exception { + final HttpHost httpsTarget = new HttpHost("https", "foo", 443); + final HttpHost proxy = new HttpHost("http", "proxy", 8080); + final HttpRoute route = new HttpRoute(httpsTarget, null, proxy, true, + RouteInfo.TunnelType.TUNNELLED, RouteInfo.LayerType.LAYERED); + final HttpClientContext context = HttpClientContext.create(); + context.setCredentialsProvider(CredentialsProviderBuilder.create() + .add(new AuthScope(proxy), "proxyuser", "proxypass".toCharArray()) + .build()); + final ClassicHttpRequest request = new HttpGet("https://foo/test"); + final ClassicHttpResponse response = new BasicClassicHttpResponse(407, "Huh?"); + response.setHeader(HttpHeaders.PROXY_AUTHENTICATE, StandardAuthScheme.BASIC + " realm=test"); + + Mockito.when(chain.proceed(Mockito.same(request), Mockito.any())).thenReturn(response); + + final ExecChain.Scope scope = new ExecChain.Scope("test", route, request, execRuntime, context); + final ClassicHttpResponse finalResponse = protocolExec.execute(request, scope, chain); + Assertions.assertEquals(407, finalResponse.getCode()); + Mockito.verify(chain, Mockito.times(1)).proceed(request, scope); + Mockito.verifyNoInteractions(proxyAuthStrategy); + Assertions.assertNull(context.getAuthExchange(proxy).getAuthScheme()); + } + + @Test + void testProxyChallengeWithoutProxyIsReturnedToCaller() throws Exception { + final HttpRoute route = new HttpRoute(target); + final HttpClientContext context = HttpClientContext.create(); + context.setCredentialsProvider(CredentialsProviderBuilder.create() + .add(new AuthScope(target), "user", "pass".toCharArray()) + .build()); + final ClassicHttpRequest request = new HttpGet("http://foo/test"); + final ClassicHttpResponse response = new BasicClassicHttpResponse(407, "Huh?"); + response.setHeader(HttpHeaders.PROXY_AUTHENTICATE, StandardAuthScheme.BASIC + " realm=test"); + + Mockito.when(chain.proceed(Mockito.same(request), Mockito.any())).thenReturn(response); + + final ExecChain.Scope scope = new ExecChain.Scope("test", route, request, execRuntime, context); + final ClassicHttpResponse finalResponse = protocolExec.execute(request, scope, chain); + Assertions.assertEquals(407, finalResponse.getCode()); + Mockito.verify(chain, Mockito.times(1)).proceed(request, scope); + Mockito.verifyNoInteractions(proxyAuthStrategy); + } + + @Test + void testProxyChallengeThroughTunnelWithAuthCacheIsReturnedToCaller() throws Exception { + final ProtocolExec cachingExec = new ProtocolExec(targetAuthStrategy, proxyAuthStrategy, null, false); + final HttpHost httpsTarget = new HttpHost("https", "foo", 443); + final HttpHost proxy = new HttpHost("http", "proxy", 8080); + final HttpRoute route = new HttpRoute(httpsTarget, null, proxy, true, + RouteInfo.TunnelType.TUNNELLED, RouteInfo.LayerType.LAYERED); + final BasicAuthCache authCache = new BasicAuthCache(); + final HttpClientContext context = HttpClientContext.create(); + context.setAuthCache(authCache); + context.setCredentialsProvider(CredentialsProviderBuilder.create() + .add(new AuthScope(proxy), "proxyuser", "proxypass".toCharArray()) + .build()); + final ClassicHttpRequest request = new HttpGet("https://foo/test"); + final ClassicHttpResponse response = new BasicClassicHttpResponse(407, "Huh?"); + response.setHeader(HttpHeaders.PROXY_AUTHENTICATE, StandardAuthScheme.BASIC + " realm=test"); + + Mockito.when(chain.proceed(Mockito.same(request), Mockito.any())).thenReturn(response); + + final ExecChain.Scope scope = new ExecChain.Scope("test", route, request, execRuntime, context); + final ClassicHttpResponse finalResponse = cachingExec.execute(request, scope, chain); + Assertions.assertSame(response, finalResponse); + Mockito.verify(chain, Mockito.times(1)).proceed(request, scope); + Mockito.verifyNoInteractions(proxyAuthStrategy); + Assertions.assertNull(request.getFirstHeader(HttpHeaders.PROXY_AUTHORIZATION)); + Assertions.assertEquals(AuthExchange.State.UNCHALLENGED, context.getAuthExchange(proxy).getState()); + Assertions.assertNull(authCache.get(proxy)); + } + + @Test + void testProxyChallengeOnPlainRouteStillAuthenticates() throws Exception { + final HttpHost proxy = new HttpHost("http", "proxy", 8080); + final HttpRoute route = new HttpRoute(target, null, proxy, false); + final HttpClientContext context = HttpClientContext.create(); + context.setCredentialsProvider(CredentialsProviderBuilder.create() + .add(new AuthScope(proxy), "proxyuser", "proxypass".toCharArray()) + .build()); + final ClassicHttpRequest request = new HttpGet("http://foo/test"); + final ClassicHttpResponse response1 = new BasicClassicHttpResponse(407, "Huh?"); + response1.setHeader(HttpHeaders.PROXY_AUTHENTICATE, StandardAuthScheme.BASIC + " realm=test"); + final ClassicHttpResponse response2 = new BasicClassicHttpResponse(200, "OK"); + + Mockito.when(chain.proceed(Mockito.any(), Mockito.any())).thenReturn(response1, response2); + Mockito.when(proxyAuthStrategy.select( + Mockito.eq(ChallengeType.PROXY), + Mockito.any(), + Mockito.any())).thenReturn(Collections.singletonList(new BasicScheme())); + Mockito.when(execRuntime.isConnectionReusable()).thenReturn(true); + + final ExecChain.Scope scope = new ExecChain.Scope("test", route, request, execRuntime, context); + final ClassicHttpResponse finalResponse = protocolExec.execute(request, scope, chain); + Assertions.assertEquals(200, finalResponse.getCode()); + final ArgumentCaptor captor = ArgumentCaptor.forClass(ClassicHttpRequest.class); + Mockito.verify(chain, Mockito.times(2)).proceed(captor.capture(), Mockito.same(scope)); + Assertions.assertNotNull(captor.getAllValues().get(1).getFirstHeader(HttpHeaders.PROXY_AUTHORIZATION)); + } } From 7c7ae932d854ec97c9fc64e976a44e73e42008e5 Mon Sep 17 00:00:00 2001 From: Arturo Bernal Date: Sun, 1 Nov 2026 10:14:30 +0100 Subject: [PATCH 4/5] Verify the TLS endpoint of an HTTPS proxy against the proxy host The target name was passed as the endpoint name when connecting to the first hop, so the proxy certificate was verified against, and the SNI set to, the target virtual host instead of the proxy. The target name is now used only when upgrading to TLS for the target. --- .../io/BasicHttpClientConnectionManager.java | 3 +- .../PoolingHttpClientConnectionManager.java | 3 +- .../PoolingAsyncClientConnectionManager.java | 3 +- ...estPoolingHttpClientConnectionManager.java | 48 +++++++++++++++++++ 4 files changed, 54 insertions(+), 3 deletions(-) diff --git a/httpclient5/src/main/java/org/apache/hc/client5/http/impl/io/BasicHttpClientConnectionManager.java b/httpclient5/src/main/java/org/apache/hc/client5/http/impl/io/BasicHttpClientConnectionManager.java index 78e9476b1e..49cef36be2 100644 --- a/httpclient5/src/main/java/org/apache/hc/client5/http/impl/io/BasicHttpClientConnectionManager.java +++ b/httpclient5/src/main/java/org/apache/hc/client5/http/impl/io/BasicHttpClientConnectionManager.java @@ -491,7 +491,8 @@ public void connect(final ConnectionEndpoint endpoint, final TimeValue timeout, this.connectionOperator.connect( connection, firstHop, - route.getTargetName(), + // The target name applies to the target only, never to an intermediate proxy + route.getProxyHost() != null ? null : route.getTargetName(), route.getLocalSocketAddress(), connectTimeout, socketConfig, diff --git a/httpclient5/src/main/java/org/apache/hc/client5/http/impl/io/PoolingHttpClientConnectionManager.java b/httpclient5/src/main/java/org/apache/hc/client5/http/impl/io/PoolingHttpClientConnectionManager.java index 2945e8ed7d..6d670bf3e6 100644 --- a/httpclient5/src/main/java/org/apache/hc/client5/http/impl/io/PoolingHttpClientConnectionManager.java +++ b/httpclient5/src/main/java/org/apache/hc/client5/http/impl/io/PoolingHttpClientConnectionManager.java @@ -565,7 +565,8 @@ public void connect(final ConnectionEndpoint endpoint, final TimeValue timeout, this.connectionOperator.connect( conn, firstHop, - route.getTargetName(), + // The target name applies to the target only, never to an intermediate proxy + route.getProxyHost() != null ? null : route.getTargetName(), unixDomainSocket, route.getLocalSocketAddress(), connectTimeout, diff --git a/httpclient5/src/main/java/org/apache/hc/client5/http/impl/nio/PoolingAsyncClientConnectionManager.java b/httpclient5/src/main/java/org/apache/hc/client5/http/impl/nio/PoolingAsyncClientConnectionManager.java index c5215a4156..d030b9b3f3 100644 --- a/httpclient5/src/main/java/org/apache/hc/client5/http/impl/nio/PoolingAsyncClientConnectionManager.java +++ b/httpclient5/src/main/java/org/apache/hc/client5/http/impl/nio/PoolingAsyncClientConnectionManager.java @@ -523,7 +523,8 @@ public Future connect( connectionInitiator, firstHop, unixDomainSocket, - route.getTargetName(), + // The target name applies to the target only, never to an intermediate proxy + route.getProxyHost() != null ? null : route.getTargetName(), route.getLocalSocketAddress(), connectTimeout, connectAttachment, diff --git a/httpclient5/src/test/java/org/apache/hc/client5/http/impl/io/TestPoolingHttpClientConnectionManager.java b/httpclient5/src/test/java/org/apache/hc/client5/http/impl/io/TestPoolingHttpClientConnectionManager.java index b045626750..e5f8416018 100644 --- a/httpclient5/src/test/java/org/apache/hc/client5/http/impl/io/TestPoolingHttpClientConnectionManager.java +++ b/httpclient5/src/test/java/org/apache/hc/client5/http/impl/io/TestPoolingHttpClientConnectionManager.java @@ -53,6 +53,7 @@ import org.apache.hc.core5.http.HttpHost; import org.apache.hc.core5.http.config.Lookup; import org.apache.hc.core5.http.io.SocketConfig; +import org.apache.hc.core5.net.URIAuthority; import org.apache.hc.core5.http2.HttpVersionPolicy; import org.apache.hc.core5.pool.PoolEntry; import org.apache.hc.core5.pool.StrictConnPool; @@ -407,5 +408,52 @@ void testConcurrentShutdown() throws InterruptedException { Assertions.assertTrue(mgr.isClosed(), "Connection manager should be shutdown after concurrent calls to shutdown."); } + @Test + void testHttpsProxyVerifiedAgainstProxyNotTargetName() throws Exception { + final HttpHost target = new HttpHost("https", "somehost", 443); + final URIAuthority targetName = new URIAuthority("virtualhost", 443); + final HttpHost proxy = new HttpHost("https", "someproxy", 8443); + final InetAddress remote = InetAddress.getByAddress(new byte[] {10, 0, 0, 1}); + final HttpRoute route = new HttpRoute(target, targetName, null, proxy, true); + Assertions.assertTrue(route.isTunnelled()); + + final PoolEntry entry = new PoolEntry<>(route, TimeValue.NEG_ONE_MILLISECOND); + entry.assignConnection(conn); + + Mockito.when(conn.isOpen()).thenReturn(false); + Mockito.when(future.get(1, TimeUnit.SECONDS)).thenReturn(entry); + Mockito.when(pool.lease( + Mockito.eq(route), + Mockito.eq(null), + Mockito.any(), + Mockito.eq(null))) + .thenReturn(future); + + final LeaseRequest connRequest = mgr.lease("some-id", route, null); + final ConnectionEndpoint endpoint = connRequest.get(Timeout.ofSeconds(1)); + Assertions.assertNotNull(endpoint); + + final HttpClientContext context = HttpClientContext.create(); + final TlsConfig tlsConfig = TlsConfig.custom().build(); + mgr.setDefaultTlsConfig(tlsConfig); + + Mockito.when(dnsResolver.resolve("someproxy", 8443)).thenReturn(Collections.singletonList(new InetSocketAddress(remote, 8443))); + Mockito.when(schemePortResolver.resolve(proxy.getSchemeName(), proxy)).thenReturn(8443); + Mockito.when(tlsSocketStrategyLookup.lookup("https")).thenReturn(tlsSocketStrategy); + Mockito.when(detachedSocketFactory.create(Mockito.any(), Mockito.any())).thenReturn(socket); + Mockito.when(tlsSocketStrategy.upgrade( + Mockito.any(), Mockito.any(), Mockito.anyInt(), Mockito.any(), Mockito.any())).thenReturn(upgradedSocket); + + mgr.connect(endpoint, null, context); + + Mockito.verify(tlsSocketStrategy).upgrade(socket, "someproxy", 8443, null, context); + + Mockito.when(conn.isOpen()).thenReturn(true); + Mockito.when(conn.getSocket()).thenReturn(socket); + + mgr.upgrade(endpoint, context); + + Mockito.verify(tlsSocketStrategy).upgrade(socket, "virtualhost", 443, tlsConfig, context); + } } From c942e460733c5341356431adbe8da641e4791e30 Mon Sep 17 00:00:00 2001 From: Arturo Bernal Date: Sun, 1 Nov 2026 12:41:57 +0100 Subject: [PATCH 5/5] Do not put the query string of a request into observation names and tags The span name and the per-URI I/O tag carried the query string, which may hold tokens and makes the tag cardinality unbounded. They now contain the scheme, host and path only. --- .../impl/ObservationAsyncExecInterceptor.java | 2 +- .../ObservationClassicExecInterceptor.java | 4 +- .../http/observation/impl/SafeUri.java | 93 +++++++ .../interceptors/AsyncIoByteCounterExec.java | 3 +- .../interceptors/IoByteCounterExec.java | 3 +- .../impl/ObservationUriRedactionTest.java | 241 ++++++++++++++++++ .../http/observation/impl/SafeUriTest.java | 84 ++++++ 7 files changed, 425 insertions(+), 5 deletions(-) create mode 100644 httpclient5-observation/src/main/java/org/apache/hc/client5/http/observation/impl/SafeUri.java create mode 100644 httpclient5-observation/src/test/java/org/apache/hc/client5/http/observation/impl/ObservationUriRedactionTest.java create mode 100644 httpclient5-observation/src/test/java/org/apache/hc/client5/http/observation/impl/SafeUriTest.java diff --git a/httpclient5-observation/src/main/java/org/apache/hc/client5/http/observation/impl/ObservationAsyncExecInterceptor.java b/httpclient5-observation/src/main/java/org/apache/hc/client5/http/observation/impl/ObservationAsyncExecInterceptor.java index d4f442a406..1504e775fc 100644 --- a/httpclient5-observation/src/main/java/org/apache/hc/client5/http/observation/impl/ObservationAsyncExecInterceptor.java +++ b/httpclient5-observation/src/main/java/org/apache/hc/client5/http/observation/impl/ObservationAsyncExecInterceptor.java @@ -79,7 +79,7 @@ public void execute(final HttpRequest request, final Observation observation = Observation .createNotStarted("http.client.request", () -> observationContext, registry) - .contextualName(request.getMethod() + " " + request.getRequestUri()) + .contextualName(request.getMethod() + " " + SafeUri.of(request.getRequestUri())) .lowCardinalityKeyValue("http.method", request.getMethod()) .lowCardinalityKeyValue("net.peer.name", scope.route.getTargetHost().getHostName()) .start(); diff --git a/httpclient5-observation/src/main/java/org/apache/hc/client5/http/observation/impl/ObservationClassicExecInterceptor.java b/httpclient5-observation/src/main/java/org/apache/hc/client5/http/observation/impl/ObservationClassicExecInterceptor.java index 9f8f494307..480ce4211f 100644 --- a/httpclient5-observation/src/main/java/org/apache/hc/client5/http/observation/impl/ObservationClassicExecInterceptor.java +++ b/httpclient5-observation/src/main/java/org/apache/hc/client5/http/observation/impl/ObservationClassicExecInterceptor.java @@ -109,9 +109,9 @@ public ClassicHttpResponse execute(final ClassicHttpRequest request, private static String safeUriForName(final ClassicHttpRequest req) { try { - return req.getUri().toString(); + return SafeUri.of(req.getUri().toString()); } catch (final URISyntaxException e) { - return req.getRequestUri(); + return SafeUri.of(req.getRequestUri()); } } diff --git a/httpclient5-observation/src/main/java/org/apache/hc/client5/http/observation/impl/SafeUri.java b/httpclient5-observation/src/main/java/org/apache/hc/client5/http/observation/impl/SafeUri.java new file mode 100644 index 0000000000..e1ad15a40f --- /dev/null +++ b/httpclient5-observation/src/main/java/org/apache/hc/client5/http/observation/impl/SafeUri.java @@ -0,0 +1,93 @@ +/* + * ==================================================================== + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + * ==================================================================== + * + * This software consists of voluntary contributions made by many + * individuals on behalf of the Apache Software Foundation. For more + * information on the Apache Software Foundation, please see + * . + * + */ +package org.apache.hc.client5.http.observation.impl; + +import org.apache.hc.core5.util.Args; + +/** + * Reduces a request URI to a form that is safe to use as a span name or a metric tag. + * User info, query and fragment are removed, as they may carry credentials and are + * unbounded in cardinality. + * + * @since 5.7 + */ +public final class SafeUri { + + private SafeUri() { + } + + /** + * Returns the given request URI without user info, query and fragment. + * + * @param uri the absolute URI or request target. + * @return the redacted URI. + */ + public static String of(final String uri) { + Args.notNull(uri, "URI"); + int end = uri.length(); + final int query = uri.indexOf('?'); + if (query >= 0) { + end = query; + } + final int fragment = uri.indexOf('#'); + if (fragment >= 0 && fragment < end) { + end = fragment; + } + final int authorityStart = authorityStart(uri, end); + if (authorityStart >= 0) { + int authorityEnd = uri.indexOf('/', authorityStart); + if (authorityEnd < 0 || authorityEnd > end) { + authorityEnd = end; + } + final int at = uri.lastIndexOf('@', authorityEnd - 1); + if (at >= authorityStart) { + return uri.substring(0, authorityStart) + uri.substring(at + 1, end); + } + } + return uri.substring(0, end); + } + + private static int authorityStart(final String uri, final int end) { + if (end == 0 || !isAlpha(uri.charAt(0))) { + return -1; + } + int i = 1; + while (i < end && (isAlpha(uri.charAt(i)) || isDigit(uri.charAt(i)) || "+-.".indexOf(uri.charAt(i)) >= 0)) { + i++; + } + return uri.startsWith("://", i) && i + 3 <= end ? i + 3 : -1; + } + + private static boolean isAlpha(final char ch) { + return ch >= 'a' && ch <= 'z' || ch >= 'A' && ch <= 'Z'; + } + + private static boolean isDigit(final char ch) { + return ch >= '0' && ch <= '9'; + } + +} diff --git a/httpclient5-observation/src/main/java/org/apache/hc/client5/http/observation/interceptors/AsyncIoByteCounterExec.java b/httpclient5-observation/src/main/java/org/apache/hc/client5/http/observation/interceptors/AsyncIoByteCounterExec.java index a53bccbc3c..4d519e3818 100644 --- a/httpclient5-observation/src/main/java/org/apache/hc/client5/http/observation/interceptors/AsyncIoByteCounterExec.java +++ b/httpclient5-observation/src/main/java/org/apache/hc/client5/http/observation/interceptors/AsyncIoByteCounterExec.java @@ -40,6 +40,7 @@ import org.apache.hc.client5.http.async.AsyncExecChainHandler; import org.apache.hc.client5.http.observation.MetricConfig; import org.apache.hc.client5.http.observation.ObservingOptions; +import org.apache.hc.client5.http.observation.impl.SafeUri; import org.apache.hc.core5.http.EntityDetails; import org.apache.hc.core5.http.HttpException; import org.apache.hc.core5.http.HttpRequest; @@ -141,7 +142,7 @@ private void record() { final String protocol = scope.route.getTargetHost().getSchemeName(); final String target = scope.route.getTargetHost().getHostName(); - final String uri = request.getRequestUri(); + final String uri = SafeUri.of(request.getRequestUri()); final List tags = buildTags(request.getMethod(), status, protocol, target, uri); diff --git a/httpclient5-observation/src/main/java/org/apache/hc/client5/http/observation/interceptors/IoByteCounterExec.java b/httpclient5-observation/src/main/java/org/apache/hc/client5/http/observation/interceptors/IoByteCounterExec.java index 8cad4b0eb1..bad33659b7 100644 --- a/httpclient5-observation/src/main/java/org/apache/hc/client5/http/observation/interceptors/IoByteCounterExec.java +++ b/httpclient5-observation/src/main/java/org/apache/hc/client5/http/observation/interceptors/IoByteCounterExec.java @@ -37,6 +37,7 @@ import org.apache.hc.client5.http.classic.ExecChainHandler; import org.apache.hc.client5.http.observation.MetricConfig; import org.apache.hc.client5.http.observation.ObservingOptions; +import org.apache.hc.client5.http.observation.impl.SafeUri; import org.apache.hc.core5.http.ClassicHttpRequest; import org.apache.hc.core5.http.ClassicHttpResponse; import org.apache.hc.core5.http.HttpEntity; @@ -94,7 +95,7 @@ public ClassicHttpResponse execute(final ClassicHttpRequest request, final int status = response != null ? response.getCode() : 599; final String protocol = scope.route.getTargetHost().getSchemeName(); final String target = scope.route.getTargetHost().getHostName(); - final String uri = request.getRequestUri(); + final String uri = SafeUri.of(request.getRequestUri()); final List tags = buildTags(request.getMethod(), status, protocol, target, uri); diff --git a/httpclient5-observation/src/test/java/org/apache/hc/client5/http/observation/impl/ObservationUriRedactionTest.java b/httpclient5-observation/src/test/java/org/apache/hc/client5/http/observation/impl/ObservationUriRedactionTest.java new file mode 100644 index 0000000000..bf62cf9706 --- /dev/null +++ b/httpclient5-observation/src/test/java/org/apache/hc/client5/http/observation/impl/ObservationUriRedactionTest.java @@ -0,0 +1,241 @@ +/* + * ==================================================================== + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + * ==================================================================== + * + * This software consists of voluntary contributions made by many + * individuals on behalf of the Apache Software Foundation. For more + * information on the Apache Software Foundation, please see + * . + * + */ +package org.apache.hc.client5.http.observation.impl; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertInstanceOf; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertThrows; + +import java.net.InetAddress; +import java.net.URI; +import java.util.EnumSet; +import java.util.concurrent.ExecutionException; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicReference; + +import io.micrometer.core.instrument.Counter; +import io.micrometer.core.instrument.MeterRegistry; +import io.micrometer.core.instrument.simple.SimpleMeterRegistry; +import io.micrometer.observation.Observation; +import io.micrometer.observation.ObservationHandler; +import io.micrometer.observation.ObservationRegistry; +import org.apache.hc.client5.http.async.methods.SimpleHttpRequest; +import org.apache.hc.client5.http.ClientProtocolException; +import org.apache.hc.client5.http.async.methods.SimpleRequestBuilder; +import org.apache.hc.client5.http.classic.methods.HttpGet; +import org.apache.hc.client5.http.impl.async.CloseableHttpAsyncClient; +import org.apache.hc.client5.http.impl.async.HttpAsyncClients; +import org.apache.hc.client5.http.impl.classic.CloseableHttpClient; +import org.apache.hc.client5.http.impl.classic.HttpClients; +import org.apache.hc.client5.http.observation.MetricConfig; +import org.apache.hc.client5.http.observation.ObservingOptions; +import org.apache.hc.client5.http.observation.interceptors.AsyncIoByteCounterExec; +import org.apache.hc.client5.http.observation.interceptors.IoByteCounterExec; +import org.apache.hc.core5.http.ClassicHttpRequest; +import org.apache.hc.core5.http.ClassicHttpResponse; +import org.apache.hc.core5.http.ContentType; +import org.apache.hc.core5.http.HttpStatus; +import org.apache.hc.core5.http.ProtocolException; +import org.apache.hc.core5.http.impl.bootstrap.HttpServer; +import org.apache.hc.core5.http.impl.bootstrap.ServerBootstrap; +import org.apache.hc.core5.http.io.entity.StringEntity; +import org.apache.hc.core5.http.io.support.ClassicRequestBuilder; +import org.apache.hc.core5.io.CloseMode; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; + +class ObservationUriRedactionTest { + + private static final String SECRET_URI = "user:secret@127.0.0.1:%d/path?token=SECRET#frag"; + + private HttpServer server; + private int port; + + @BeforeEach + void setUp() throws Exception { + server = ServerBootstrap.bootstrap() + .setLocalAddress(InetAddress.getByAddress(new byte[]{127, 0, 0, 1})) + .setListenerPort(0) + .register("127.0.0.1", "/path", (request, response, context) -> { + response.setCode(HttpStatus.SC_OK); + response.setEntity(new StringEntity("ACK", ContentType.TEXT_PLAIN)); + }) + .create(); + server.start(); + port = server.getLocalPort(); + } + + @AfterEach + void tearDown() { + if (server != null) { + server.close(CloseMode.GRACEFUL); + } + } + + private static final class NameCapture implements ObservationHandler { + final AtomicReference name = new AtomicReference<>(); + + @Override + public boolean supportsContext(final Observation.Context context) { + return true; + } + + @Override + public void onStart(final Observation.Context context) { + name.set(context.getContextualName()); + } + } + + private static ObservingOptions spanOnly() { + return ObservingOptions.builder() + .metrics(EnumSet.noneOf(ObservingOptions.MetricSet.class)) + .build(); + } + + private String classicSpanName(final String uri, final boolean rejected) throws Exception { + final ObservationRegistry reg = ObservationRegistry.create(); + final NameCapture capture = new NameCapture(); + reg.observationConfig().observationHandler(capture); + try (final CloseableHttpClient client = HttpClients.custom() + .addExecInterceptorFirst("span", new ObservationClassicExecInterceptor(reg, spanOnly())) + .build()) { + execute(client, new HttpGet(uri), rejected); + } + return capture.name.get(); + } + + private String asyncSpanName(final String uri, final boolean rejected) throws Exception { + final ObservationRegistry reg = ObservationRegistry.create(); + final NameCapture capture = new NameCapture(); + reg.observationConfig().observationHandler(capture); + try (final CloseableHttpAsyncClient client = HttpAsyncClients.custom() + .addExecInterceptorFirst("span", new ObservationAsyncExecInterceptor(reg, spanOnly())) + .build()) { + client.start(); + final SimpleHttpRequest req = SimpleHttpRequest.create("GET", URI.create(uri)); + execute(client, req, rejected); + } + return capture.name.get(); + } + + private String classicUriTag(final String uri) throws Exception { + final MeterRegistry meters = new SimpleMeterRegistry(); + final MetricConfig mc = MetricConfig.builder().prefix("test").perUriIo(true).build(); + final ObservingOptions opts = ObservingOptions.builder() + .metrics(EnumSet.of(ObservingOptions.MetricSet.IO)) + .tagLevel(ObservingOptions.TagLevel.LOW) + .build(); + try (final CloseableHttpClient client = HttpClients.custom() + .addExecInterceptorFirst("io", + new IoByteCounterExec(meters, opts, mc)) + .build()) { + execute(client, ClassicRequestBuilder.post(uri) + .setEntity(new StringEntity("HELLO", ContentType.TEXT_PLAIN)).build(), false); + } + return uriTag(meters, mc); + } + + private String asyncUriTag(final String uri) throws Exception { + final MeterRegistry meters = new SimpleMeterRegistry(); + final MetricConfig mc = MetricConfig.builder().prefix("test").perUriIo(true).build(); + final ObservingOptions opts = ObservingOptions.builder() + .metrics(EnumSet.of(ObservingOptions.MetricSet.IO)) + .tagLevel(ObservingOptions.TagLevel.LOW) + .build(); + try (final CloseableHttpAsyncClient client = HttpAsyncClients.custom() + .addExecInterceptorFirst("io", + new AsyncIoByteCounterExec(meters, opts, mc)) + .build()) { + client.start(); + execute(client, SimpleRequestBuilder.post(uri).setBody("HELLO", ContentType.TEXT_PLAIN).build(), false); + } + return uriTag(meters, mc); + } + + // The client rejects userinfo downstream of the interceptors; the observation has already started by then. + private static void execute( + final CloseableHttpClient client, final ClassicHttpRequest request, final boolean rejected) throws Exception { + if (rejected) { + assertThrows(ClientProtocolException.class, () -> client.executeOpen(null, request, null).close()); + } else { + try (final ClassicHttpResponse rsp = client.executeOpen(null, request, null)) { + assertEquals(HttpStatus.SC_OK, rsp.getCode()); + } + } + } + + private static void execute( + final CloseableHttpAsyncClient client, final SimpleHttpRequest request, final boolean rejected) throws Exception { + if (rejected) { + final ExecutionException ex = assertThrows(ExecutionException.class, + () -> client.execute(request, null).get(10, TimeUnit.SECONDS)); + assertInstanceOf(ProtocolException.class, ex.getCause()); + } else { + assertEquals(HttpStatus.SC_OK, client.execute(request, null).get(10, TimeUnit.SECONDS).getCode()); + } + } + + private static String uriTag(final MeterRegistry meters, final MetricConfig mc) { + final Counter counter = meters.find(mc.prefix + ".request.bytes").counter(); + assertNotNull(counter); + final String tag = counter.getId().getTag("uri"); + assertNotNull(tag); + return tag; + } + + @Test + void classicSpanNameExcludesUserInfoQueryAndFragment() throws Exception { + assertEquals("GET http://127.0.0.1:" + port + "/path", + classicSpanName("http://" + String.format(SECRET_URI, port), true)); + } + + @Test + void asyncSpanNameExcludesUserInfoQueryAndFragment() throws Exception { + assertEquals("GET /path", asyncSpanName("http://" + String.format(SECRET_URI, port), true)); + } + + @Test + void classicUriTagExcludesQuery() throws Exception { + assertEquals("/path", classicUriTag("http://127.0.0.1:" + port + "/path?token=SECRET#frag")); + } + + @Test + void asyncUriTagExcludesQuery() throws Exception { + assertEquals("/path", asyncUriTag("http://127.0.0.1:" + port + "/path?token=SECRET#frag")); + } + + @Test + void plainUrlKeepsNameAndTag() throws Exception { + final String url = "http://127.0.0.1:" + port + "/path"; + assertEquals("GET http://127.0.0.1:" + port + "/path", classicSpanName(url, false)); + assertEquals("GET /path", asyncSpanName(url, false)); + assertEquals("/path", classicUriTag(url)); + assertEquals("/path", asyncUriTag(url)); + } +} diff --git a/httpclient5-observation/src/test/java/org/apache/hc/client5/http/observation/impl/SafeUriTest.java b/httpclient5-observation/src/test/java/org/apache/hc/client5/http/observation/impl/SafeUriTest.java new file mode 100644 index 0000000000..5bf328b4f6 --- /dev/null +++ b/httpclient5-observation/src/test/java/org/apache/hc/client5/http/observation/impl/SafeUriTest.java @@ -0,0 +1,84 @@ +/* + * ==================================================================== + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + * ==================================================================== + * + * This software consists of voluntary contributions made by many + * individuals on behalf of the Apache Software Foundation. For more + * information on the Apache Software Foundation, please see + * . + * + */ +package org.apache.hc.client5.http.observation.impl; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertThrows; + +import org.junit.jupiter.api.Test; + +class SafeUriTest { + + @Test + void removesUserInfoQueryAndFragment() { + assertEquals("http://host:8080/path", SafeUri.of("http://user:secret@host:8080/path?token=SECRET#frag")); + } + + @Test + void removesUserInfoWithoutPath() { + assertEquals("http://host", SafeUri.of("http://user:secret@host?token=SECRET")); + } + + @Test + void removesQueryFromRequestTarget() { + assertEquals("/path", SafeUri.of("/path?token=SECRET")); + assertEquals("/path", SafeUri.of("/path#frag?x")); + } + + @Test + void keepsAtSignInPath() { + assertEquals("http://host/a@b", SafeUri.of("http://host/a@b?x=1")); + } + + @Test + void keepsUriLikeTextInRelativePath() { + assertEquals("/redirect/http://user@host/x", SafeUri.of("/redirect/http://user@host/x?y=1")); + assertEquals("path/to://user@host", SafeUri.of("path/to://user@host#frag")); + assertEquals("/a://b@c", SafeUri.of("/a://b@c")); + } + + @Test + void recognizesSchemeByItsSyntax() { + assertEquals("git+ssh://host/repo", SafeUri.of("git+ssh://user:secret@host/repo?x=1")); + assertEquals("HTTP://host", SafeUri.of("HTTP://user@host")); + assertEquals("1http://user@host", SafeUri.of("1http://user@host")); + assertEquals("ht tp://user@host", SafeUri.of("ht tp://user@host")); + } + + @Test + void leavesPlainUriUntouched() { + assertEquals("http://host:8080/path", SafeUri.of("http://host:8080/path")); + assertEquals("/path", SafeUri.of("/path")); + assertEquals("*", SafeUri.of("*")); + } + + @Test + void rejectsNull() { + assertThrows(NullPointerException.class, () -> SafeUri.of(null)); + } + +}