diff --git a/httpclient5/src/main/java/org/apache/hc/client5/http/ssl/AbstractClientTlsStrategy.java b/httpclient5/src/main/java/org/apache/hc/client5/http/ssl/AbstractClientTlsStrategy.java index d403ff18df..0aba6a75ae 100644 --- a/httpclient5/src/main/java/org/apache/hc/client5/http/ssl/AbstractClientTlsStrategy.java +++ b/httpclient5/src/main/java/org/apache/hc/client5/http/ssl/AbstractClientTlsStrategy.java @@ -99,6 +99,10 @@ abstract class AbstractClientTlsStrategy implements TlsStrategy, TlsSocketStrate this.sslBufferManagement = sslBufferManagement != null ? sslBufferManagement : SSLBufferMode.STATIC; this.hostnameVerificationPolicy = hostnameVerificationPolicy != null ? hostnameVerificationPolicy : (hostnameVerifier != null ? HostnameVerificationPolicy.BOTH : HostnameVerificationPolicy.BUILTIN); + Args.check(this.hostnameVerificationPolicy != HostnameVerificationPolicy.CLIENT + && this.hostnameVerificationPolicy != HostnameVerificationPolicy.BOTH + || hostnameVerifier != null, + "Hostname verifier is required for hostname verification policy " + this.hostnameVerificationPolicy); this.hostnameVerifier = hostnameVerifier; } diff --git a/httpclient5/src/main/java/org/apache/hc/client5/http/ssl/ConscryptClientTlsStrategy.java b/httpclient5/src/main/java/org/apache/hc/client5/http/ssl/ConscryptClientTlsStrategy.java index bcf9bfd344..d635866f73 100644 --- a/httpclient5/src/main/java/org/apache/hc/client5/http/ssl/ConscryptClientTlsStrategy.java +++ b/httpclient5/src/main/java/org/apache/hc/client5/http/ssl/ConscryptClientTlsStrategy.java @@ -69,7 +69,8 @@ public ConscryptClientTlsStrategy( final String[] supportedCipherSuites, final SSLBufferMode sslBufferManagement, final HostnameVerifier hostnameVerifier) { - this(sslContext, supportedProtocols, supportedCipherSuites, sslBufferManagement, HostnameVerificationPolicy.CLIENT, hostnameVerifier); + this(sslContext, supportedProtocols, supportedCipherSuites, sslBufferManagement, + hostnameVerifier != null ? HostnameVerificationPolicy.CLIENT : HostnameVerificationPolicy.BUILTIN, hostnameVerifier); } /** diff --git a/httpclient5/src/main/java/org/apache/hc/client5/http/ssl/DefaultClientTlsStrategy.java b/httpclient5/src/main/java/org/apache/hc/client5/http/ssl/DefaultClientTlsStrategy.java index c52997473f..2792df98f0 100644 --- a/httpclient5/src/main/java/org/apache/hc/client5/http/ssl/DefaultClientTlsStrategy.java +++ b/httpclient5/src/main/java/org/apache/hc/client5/http/ssl/DefaultClientTlsStrategy.java @@ -101,7 +101,8 @@ public DefaultClientTlsStrategy( final SSLBufferMode sslBufferManagement, final HostnameVerifier hostnameVerifier, final Factory tlsDetailsFactory) { - super(sslContext, supportedProtocols, supportedCipherSuites, sslBufferManagement, HostnameVerificationPolicy.CLIENT, hostnameVerifier); + super(sslContext, supportedProtocols, supportedCipherSuites, sslBufferManagement, + hostnameVerifier != null ? HostnameVerificationPolicy.CLIENT : HostnameVerificationPolicy.BUILTIN, hostnameVerifier); this.tlsDetailsFactory = tlsDetailsFactory; } diff --git a/httpclient5/src/test/java/org/apache/hc/client5/http/ssl/AbstractClientTlsStrategyTest.java b/httpclient5/src/test/java/org/apache/hc/client5/http/ssl/AbstractClientTlsStrategyTest.java index 20a2bbd407..a052ca16e7 100644 --- a/httpclient5/src/test/java/org/apache/hc/client5/http/ssl/AbstractClientTlsStrategyTest.java +++ b/httpclient5/src/test/java/org/apache/hc/client5/http/ssl/AbstractClientTlsStrategyTest.java @@ -27,17 +27,21 @@ package org.apache.hc.client5.http.ssl; +import static org.junit.jupiter.api.Assertions.assertDoesNotThrow; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertThrows; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; import java.security.cert.X509Certificate; +import javax.net.ssl.HostnameVerifier; import javax.net.ssl.SSLEngine; import javax.net.ssl.SSLParameters; import javax.net.ssl.SSLSession; import javax.security.auth.x500.X500Principal; +import org.apache.hc.core5.function.Factory; import org.apache.hc.core5.reactor.ssl.SSLBufferMode; import org.apache.hc.core5.reactor.ssl.TlsDetails; import org.apache.hc.core5.ssl.SSLContexts; @@ -110,5 +114,31 @@ TlsDetails createTlsDetails(final SSLEngine sslEngine) { } + @Test + void testClientPolicyRequiresHostnameVerifier() { + assertThrows(IllegalArgumentException.class, () -> new DefaultClientTlsStrategy( + SSLContexts.createDefault(), HostnameVerificationPolicy.CLIENT, null)); + assertThrows(IllegalArgumentException.class, () -> new DefaultClientTlsStrategy( + SSLContexts.createDefault(), HostnameVerificationPolicy.BOTH, null)); + } + + @Test + void testBuilderClientPolicyRequiresHostnameVerifier() { + final ClientTlsStrategyBuilder builder = ClientTlsStrategyBuilder.create() + .setSslContext(SSLContexts.createDefault()) + .setHostVerificationPolicy(HostnameVerificationPolicy.CLIENT); + assertThrows(IllegalArgumentException.class, builder::buildClassic); + } + + @Test + @SuppressWarnings("deprecation") + void testNullHostnameVerifierDefaultsToBuiltinPolicy() { + assertDoesNotThrow(() -> new DefaultClientTlsStrategy( + SSLContexts.createDefault(), null, null, SSLBufferMode.STATIC, + (HostnameVerifier) null, (Factory) null)); + assertDoesNotThrow(() -> new DefaultClientTlsStrategy(SSLContexts.createDefault())); + assertDoesNotThrow(() -> new ConscryptClientTlsStrategy(SSLContexts.createDefault())); + } + } \ No newline at end of file diff --git a/httpclient5/src/test/java/org/apache/hc/client5/http/ssl/TestNullHostnameVerifierPolicy.java b/httpclient5/src/test/java/org/apache/hc/client5/http/ssl/TestNullHostnameVerifierPolicy.java new file mode 100644 index 0000000000..02b9ee22b0 --- /dev/null +++ b/httpclient5/src/test/java/org/apache/hc/client5/http/ssl/TestNullHostnameVerifierPolicy.java @@ -0,0 +1,147 @@ +/* + * ==================================================================== + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + * ==================================================================== + * + * This software consists of voluntary contributions made by many + * individuals on behalf of the Apache Software Foundation. For more + * information on the Apache Software Foundation, please see + * . + * + */ + + +package org.apache.hc.client5.http.ssl; + +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertThrows; + +import java.io.IOException; +import java.net.InetAddress; +import java.net.Socket; + +import javax.net.ssl.HostnameVerifier; +import javax.net.ssl.SSLContext; +import javax.net.ssl.SSLEngine; +import javax.net.ssl.SSLException; +import javax.net.ssl.SSLServerSocket; +import javax.net.ssl.SSLSocket; + +import org.apache.hc.core5.function.Factory; +import org.apache.hc.core5.reactor.ssl.SSLBufferMode; +import org.apache.hc.core5.reactor.ssl.TlsDetails; +import org.apache.hc.core5.ssl.SSLContexts; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; + +/** + * Handshakes against a server whose certificate is issued for {@code localhost} to make sure + * that a strategy created without a {@link javax.net.ssl.HostnameVerifier} still verifies the host name. + */ +class TestNullHostnameVerifierPolicy { + + private static final char[] PASSWORD = "nopassword".toCharArray(); + + private SSLContext sslContext; + private SSLServerSocket serverSocket; + private Thread serverThread; + + @BeforeEach + void setUp() throws Exception { + sslContext = SSLContexts.custom() + .loadTrustMaterial(getClass().getResource("/test.keystore"), PASSWORD) + .loadKeyMaterial(getClass().getResource("/test.keystore"), PASSWORD, PASSWORD) + .build(); + serverSocket = (SSLServerSocket) sslContext.getServerSocketFactory() + .createServerSocket(0, 1, InetAddress.getLoopbackAddress()); + serverThread = new Thread(() -> { + try (final SSLSocket socket = (SSLSocket) serverSocket.accept()) { + socket.startHandshake(); + } catch (final IOException ignore) { + // the client may abort the handshake + } + }); + serverThread.setDaemon(true); + serverThread.start(); + } + + @AfterEach + void tearDown() throws Exception { + serverSocket.close(); + serverThread.join(5000); + } + + private SSLSocket upgrade(final AbstractClientTlsStrategy strategy, final String target) throws IOException { + final Socket socket = new Socket(InetAddress.getLoopbackAddress(), serverSocket.getLocalPort()); + try { + return strategy.upgrade(socket, target, serverSocket.getLocalPort(), null, null); + } catch (final IOException | RuntimeException ex) { + socket.close(); + throw ex; + } + } + + @SuppressWarnings("deprecation") + private AbstractClientTlsStrategy defaultStrategy() { + return new DefaultClientTlsStrategy( + sslContext, null, null, SSLBufferMode.STATIC, + (HostnameVerifier) null, (Factory) null); + } + + @SuppressWarnings("deprecation") + private AbstractClientTlsStrategy conscryptStrategy() { + return new ConscryptClientTlsStrategy( + sslContext, null, null, SSLBufferMode.STATIC, (HostnameVerifier) null); + } + + @Test + void testDefaultStrategyRejectsMismatchingHost() { + assertThrows(SSLException.class, () -> upgrade(defaultStrategy(), "example.invalid")); + } + + @Test + void testDefaultStrategyAcceptsMatchingHost() throws Exception { + try (final SSLSocket socket = upgrade(defaultStrategy(), "localhost")) { + assertNotNull(socket.getSession().getPeerCertificates()); + } + } + + @Test + void testConscryptStrategyRejectsMismatchingHost() { + assertThrows(SSLException.class, () -> upgrade(conscryptStrategy(), "example.invalid")); + } + + @Test + void testConscryptStrategyAcceptsMatchingHost() throws Exception { + try (final SSLSocket socket = upgrade(conscryptStrategy(), "localhost")) { + assertNotNull(socket.getSession().getPeerCertificates()); + } + } + + @Test + void testConscryptDefaultConstructorRejectsMismatchingHost() { + assertThrows(SSLException.class, () -> upgrade(new ConscryptClientTlsStrategy(sslContext), "example.invalid")); + } + + @Test + void testDefaultConstructorRejectsMismatchingHost() { + assertThrows(SSLException.class, () -> upgrade(new DefaultClientTlsStrategy(sslContext), "example.invalid")); + } + +} diff --git a/httpclient5/src/test/resources/test.keystore b/httpclient5/src/test/resources/test.keystore new file mode 100644 index 0000000000..f8d5ace1ad Binary files /dev/null and b/httpclient5/src/test/resources/test.keystore differ