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)); + } + +} 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/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/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/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/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)); + } } 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); + } } 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 0000000000..f8d5ace1ad Binary files /dev/null and b/httpclient5/src/test/resources/test.keystore differ 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}