diff --git a/.github/workflows/maven.yml b/.github/workflows/maven.yml index c130ab3b37..9fcf8a7347 100644 --- a/.github/workflows/maven.yml +++ b/.github/workflows/maven.yml @@ -43,7 +43,7 @@ jobs: steps: - uses: actions/checkout@v7 - - uses: actions/cache@v4 + - uses: actions/cache@v6 with: path: ~/.m2/repository key: ${{ runner.os }}-maven-${{ hashFiles('**/pom.xml') }} 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}