From f8c89e2e055aeb3070d05e6b9425bcff6a8013fd 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 c697335f30..5d1dd2b7a0 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); @@ -342,7 +334,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); @@ -385,7 +376,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; @@ -544,11 +534,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 a6c4e2f912..5526fed753 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! @@ -1961,18 +1957,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 c90a69fd7f..572284c0b9 100644 --- a/handler/src/test/java/io/netty/handler/ssl/OpenSslEngineTest.java +++ b/handler/src/test/java/io/netty/handler/ssl/OpenSslEngineTest.java @@ -146,20 +146,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 440c34c25b..5d92f8a884 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 9074b39dcf..c0bac5f94e 100644 --- a/handler/src/test/java/io/netty/handler/ssl/SSLEngineTest.java +++ b/handler/src/test/java/io/netty/handler/ssl/SSLEngineTest.java @@ -2678,11 +2678,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()