From 6b8a0ed374d9e3a85bda9f9d3b47bef9488a1fcd Mon Sep 17 00:00:00 2001 From: Norman Maurer Date: Mon, 1 Apr 2019 21:02:36 +0200 Subject: [PATCH] Remove call to SSL.setHostNameValidation(...) as it is done in the TrustManager (#8981) Motivation: We do not need to call SSL.setHostNameValidation(...) as it should be done as part of the TrustManager implementation. This is consistent with the JDK implementation of SSLEngine. Modifications: Remove call to SSL.setHostNameValidation(...) Result: More consistent behaviour between our SSLEngine implementation and the one that comes with the JDK. --- .../java/io/netty/handler/ssl/OpenSsl.java | 21 +++++++------------ .../ssl/ReferenceCountedOpenSslEngine.java | 16 -------------- .../ConscryptOpenSslEngineInteropTest.java | 14 ------------- .../OpenSslConscryptSslEngineInteropTest.java | 15 ------------- .../netty/handler/ssl/OpenSslEngineTest.java | 14 ------------- .../ssl/OpenSslJdkSslEngineInteroptTest.java | 14 ------------- .../io/netty/handler/ssl/SSLEngineTest.java | 5 ----- 7 files changed, 7 insertions(+), 92 deletions(-) diff --git a/handler/src/main/java/io/netty/handler/ssl/OpenSsl.java b/handler/src/main/java/io/netty/handler/ssl/OpenSsl.java index 8fc09b0eb8..3a45b5cad6 100644 --- a/handler/src/main/java/io/netty/handler/ssl/OpenSsl.java +++ b/handler/src/main/java/io/netty/handler/ssl/OpenSsl.java @@ -58,7 +58,6 @@ public final class OpenSsl { private static final Set AVAILABLE_OPENSSL_CIPHER_SUITES; private static final Set AVAILABLE_JAVA_CIPHER_SUITES; private static final boolean SUPPORTS_KEYMANAGER_FACTORY; - private static final boolean SUPPORTS_HOSTNAME_VALIDATION; private static final boolean USE_KEYMANAGER_FACTORY; private static final boolean SUPPORTS_OCSP; private static final boolean TLSV13_SUPPORTED; @@ -200,7 +199,6 @@ public final class OpenSsl { final Set availableOpenSslCipherSuites = new LinkedHashSet<>(128); boolean supportsKeyManagerFactory = false; boolean useKeyManagerFactory = false; - boolean supportsHostNameValidation = false; boolean tlsv13Supported = false; IS_BORINGSSL = "BoringSSL".equals(versionString()); @@ -257,16 +255,10 @@ public final class OpenSsl { "AEAD-AES256-GCM-SHA384", "AEAD-CHACHA20-POLY1305-SHA256"); } - try { - SSL.setHostNameValidation(ssl, 0, "netty.io"); - supportsHostNameValidation = true; - } catch (Throwable ignore) { - logger.debug("Hostname Verification not supported."); - } PemEncoded privateKey = PemPrivateKey.toPEM(UnpooledByteBufAllocator.DEFAULT, true, KEY_BYTES); try { - X509Certificate certificate = selfSignedCertificate(); + X509Certificate certificate = selfSignedCertificate(); certBio = ReferenceCountedOpenSslContext.toBIO(ByteBufAllocator.DEFAULT, certificate); cert = SSL.parseX509Chain(certBio); @@ -338,7 +330,6 @@ public final class OpenSsl { AVAILABLE_CIPHER_SUITES = availableCipherSuites; SUPPORTS_KEYMANAGER_FACTORY = supportsKeyManagerFactory; - SUPPORTS_HOSTNAME_VALIDATION = supportsHostNameValidation; USE_KEYMANAGER_FACTORY = useKeyManagerFactory; Set protocols = new LinkedHashSet<>(6); @@ -381,7 +372,6 @@ public final class OpenSsl { AVAILABLE_JAVA_CIPHER_SUITES = Collections.emptySet(); AVAILABLE_CIPHER_SUITES = Collections.emptySet(); SUPPORTS_KEYMANAGER_FACTORY = false; - SUPPORTS_HOSTNAME_VALIDATION = false; USE_KEYMANAGER_FACTORY = false; SUPPORTED_PROTOCOLS_SET = Collections.emptySet(); SUPPORTS_OCSP = false; @@ -540,11 +530,14 @@ public final class OpenSsl { } /** - * Returns {@code true} if Hostname Validation - * is supported when using OpenSSL. + * Always returns {@code true} if {@link #isAvailable()} returns {@code true}. + * + * @deprecated Will be removed because hostname validation is always done by a + * {@link javax.net.ssl.TrustManager} implementation. */ + @Deprecated public static boolean supportsHostnameValidation() { - return SUPPORTS_HOSTNAME_VALIDATION; + return isAvailable(); } static boolean useKeyManagerFactory() { diff --git a/handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslEngine.java b/handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslEngine.java index e5d6fc9869..ad904af876 100644 --- a/handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslEngine.java +++ b/handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslEngine.java @@ -120,10 +120,6 @@ public class ReferenceCountedOpenSslEngine extends SSLEngine implements Referenc SSL.SSL_OP_NO_TLSv1_2, SSL.SSL_OP_NO_TLSv1_3 }; - /** - * The flags argument is usually 0. - */ - private static final int DEFAULT_HOSTNAME_VALIDATION_FLAGS = 0; /** * Depends upon tcnative ... only use if tcnative is available! @@ -1962,18 +1958,6 @@ public class ReferenceCountedOpenSslEngine extends SSLEngine implements Referenc final String endPointIdentificationAlgorithm = sslParameters.getEndpointIdentificationAlgorithm(); final boolean endPointVerificationEnabled = isEndPointVerificationEnabled(endPointIdentificationAlgorithm); - final boolean wasEndPointVerificationEnabled = - isEndPointVerificationEnabled(this.endPointIdentificationAlgorithm); - - if (wasEndPointVerificationEnabled && !endPointVerificationEnabled) { - // Passing in null will disable hostname verification again so only do so if it was enabled before. - SSL.setHostNameValidation(ssl, DEFAULT_HOSTNAME_VALIDATION_FLAGS, null); - } else { - String host = endPointVerificationEnabled ? getPeerHost() : null; - if (host != null && !host.isEmpty()) { - SSL.setHostNameValidation(ssl, DEFAULT_HOSTNAME_VALIDATION_FLAGS, host); - } - } // If the user asks for hostname verification we must ensure we verify the peer. // If the user disables hostname verification we leave it up to the user to change the mode manually. if (clientMode && endPointVerificationEnabled) { diff --git a/handler/src/test/java/io/netty/handler/ssl/ConscryptOpenSslEngineInteropTest.java b/handler/src/test/java/io/netty/handler/ssl/ConscryptOpenSslEngineInteropTest.java index 9cb5bb6100..8782ca24e1 100644 --- a/handler/src/test/java/io/netty/handler/ssl/ConscryptOpenSslEngineInteropTest.java +++ b/handler/src/test/java/io/netty/handler/ssl/ConscryptOpenSslEngineInteropTest.java @@ -109,20 +109,6 @@ public class ConscryptOpenSslEngineInteropTest extends ConscryptSslEngineTest { super.testMutualAuthInvalidIntermediateCAFailWithRequiredClientAuth(); } - @Override - @Test - public void testClientHostnameValidationSuccess() throws InterruptedException, SSLException { - assumeTrue(OpenSsl.supportsHostnameValidation()); - super.testClientHostnameValidationSuccess(); - } - - @Override - @Test - public void testClientHostnameValidationFail() throws InterruptedException, SSLException { - assumeTrue(OpenSsl.supportsHostnameValidation()); - super.testClientHostnameValidationFail(); - } - @Override @Test public void testSessionAfterHandshakeKeyManagerFactoryMutualAuth() throws Exception { diff --git a/handler/src/test/java/io/netty/handler/ssl/OpenSslConscryptSslEngineInteropTest.java b/handler/src/test/java/io/netty/handler/ssl/OpenSslConscryptSslEngineInteropTest.java index fa49fa20cb..121f6fe508 100644 --- a/handler/src/test/java/io/netty/handler/ssl/OpenSslConscryptSslEngineInteropTest.java +++ b/handler/src/test/java/io/netty/handler/ssl/OpenSslConscryptSslEngineInteropTest.java @@ -22,7 +22,6 @@ import org.junit.runner.RunWith; import org.junit.runners.Parameterized; import javax.net.ssl.SSLEngine; -import javax.net.ssl.SSLException; import java.security.Provider; import java.util.ArrayList; import java.util.Collection; @@ -109,20 +108,6 @@ public class OpenSslConscryptSslEngineInteropTest extends ConscryptSslEngineTest super.testMutualAuthInvalidIntermediateCAFailWithRequiredClientAuth(); } - @Override - @Test - public void testClientHostnameValidationSuccess() throws InterruptedException, SSLException { - assumeTrue(OpenSsl.supportsHostnameValidation()); - super.testClientHostnameValidationSuccess(); - } - - @Override - @Test - public void testClientHostnameValidationFail() throws InterruptedException, SSLException { - assumeTrue(OpenSsl.supportsHostnameValidation()); - super.testClientHostnameValidationFail(); - } - @Override @Test public void testSessionAfterHandshakeKeyManagerFactoryMutualAuth() throws Exception { diff --git a/handler/src/test/java/io/netty/handler/ssl/OpenSslEngineTest.java b/handler/src/test/java/io/netty/handler/ssl/OpenSslEngineTest.java index 37babf690c..b1daa880e8 100644 --- a/handler/src/test/java/io/netty/handler/ssl/OpenSslEngineTest.java +++ b/handler/src/test/java/io/netty/handler/ssl/OpenSslEngineTest.java @@ -147,20 +147,6 @@ public class OpenSslEngineTest extends SSLEngineTest { super.testMutualAuthValidClientCertChainTooLongFailRequireClientAuth(); } - @Override - @Test - public void testClientHostnameValidationSuccess() throws InterruptedException, SSLException { - assumeTrue(OpenSsl.supportsHostnameValidation()); - super.testClientHostnameValidationSuccess(); - } - - @Override - @Test - public void testClientHostnameValidationFail() throws InterruptedException, SSLException { - assumeTrue(OpenSsl.supportsHostnameValidation()); - super.testClientHostnameValidationFail(); - } - @Override public void testHandshakeSession() throws Exception { checkShouldUseKeyManagerFactory(); diff --git a/handler/src/test/java/io/netty/handler/ssl/OpenSslJdkSslEngineInteroptTest.java b/handler/src/test/java/io/netty/handler/ssl/OpenSslJdkSslEngineInteroptTest.java index 6b6a523f95..5c5f0b0a3f 100644 --- a/handler/src/test/java/io/netty/handler/ssl/OpenSslJdkSslEngineInteroptTest.java +++ b/handler/src/test/java/io/netty/handler/ssl/OpenSslJdkSslEngineInteroptTest.java @@ -100,20 +100,6 @@ public class OpenSslJdkSslEngineInteroptTest extends SSLEngineTest { super.testMutualAuthInvalidIntermediateCAFailWithRequiredClientAuth(); } - @Override - @Test - public void testClientHostnameValidationSuccess() throws InterruptedException, SSLException { - assumeTrue(OpenSsl.supportsHostnameValidation()); - super.testClientHostnameValidationSuccess(); - } - - @Override - @Test - public void testClientHostnameValidationFail() throws InterruptedException, SSLException { - assumeTrue(OpenSsl.supportsHostnameValidation()); - super.testClientHostnameValidationFail(); - } - @Override @Test public void testSessionAfterHandshakeKeyManagerFactoryMutualAuth() throws Exception { diff --git a/handler/src/test/java/io/netty/handler/ssl/SSLEngineTest.java b/handler/src/test/java/io/netty/handler/ssl/SSLEngineTest.java index 994b1437a7..6e25e8e513 100644 --- a/handler/src/test/java/io/netty/handler/ssl/SSLEngineTest.java +++ b/handler/src/test/java/io/netty/handler/ssl/SSLEngineTest.java @@ -2690,11 +2690,6 @@ public abstract class SSLEngineTest { @Test public void testUsingX509TrustManagerVerifiesHostname() throws Exception { SslProvider clientProvider = sslClientProvider(); - if (clientProvider == SslProvider.OPENSSL || clientProvider == SslProvider.OPENSSL_REFCNT) { - // Need to check if we support hostname validation in the current used OpenSSL version before running - // the test. - Assume.assumeTrue(OpenSsl.supportsHostnameValidation()); - } SelfSignedCertificate cert = new SelfSignedCertificate(); clientSslCtx = SslContextBuilder .forClient()