From ae95e9c3d6437ed63899ba9f2279903b5c1f5ae8 Mon Sep 17 00:00:00 2001 From: Norman Maurer Date: Tue, 28 Apr 2020 09:50:05 +0200 Subject: [PATCH] Update to latest Conscrypt release and add workarounds for bugs (#10211) Motivation: We are far behind with the version of Conscrypt we are using during testing. We should ensure we use the latest. Modifications: - Update conscrypt dependency - Ensure we use conscrypt provider in tests - Add workarounds for conscrypt bugs in testsuite Result: Use latest Conscrypt release --- .../ssl/ConscryptJdkSslEngineInteropTest.java | 14 ------ .../handler/ssl/ConscryptSslEngineTest.java | 14 ------ .../io/netty/handler/ssl/SSLEngineTest.java | 50 +++++++++++++++++-- pom.xml | 2 +- 4 files changed, 47 insertions(+), 33 deletions(-) diff --git a/handler/src/test/java/io/netty/handler/ssl/ConscryptJdkSslEngineInteropTest.java b/handler/src/test/java/io/netty/handler/ssl/ConscryptJdkSslEngineInteropTest.java index a6768f1063..c575b9bbcf 100644 --- a/handler/src/test/java/io/netty/handler/ssl/ConscryptJdkSslEngineInteropTest.java +++ b/handler/src/test/java/io/netty/handler/ssl/ConscryptJdkSslEngineInteropTest.java @@ -80,18 +80,4 @@ public class ConscryptJdkSslEngineInteropTest extends SSLEngineTest { // TODO(scott): work around for a JDK issue. The exception should be SSLHandshakeException. return super.mySetupMutualAuthServerIsValidServerException(cause) || causedBySSLException(cause); } - - @Ignore("Ignore due bug in Conscrypt") - @Override - public void testSessionBindingEvent() throws Exception { - // Ignore due bug in Conscrypt where the incorrect SSLSession object is used in the SSLSessionBindingEvent. - // See https://github.com/google/conscrypt/issues/593 - } - - @Ignore("Ignore due bug in Conscrypt") - @Override - public void testHandshakeSession() throws Exception { - // Ignore as Conscrypt does not correctly return the local certificates while the TrustManager is invoked. - // See https://github.com/google/conscrypt/issues/634 - } } diff --git a/handler/src/test/java/io/netty/handler/ssl/ConscryptSslEngineTest.java b/handler/src/test/java/io/netty/handler/ssl/ConscryptSslEngineTest.java index 2dec32790b..6aa0892572 100644 --- a/handler/src/test/java/io/netty/handler/ssl/ConscryptSslEngineTest.java +++ b/handler/src/test/java/io/netty/handler/ssl/ConscryptSslEngineTest.java @@ -78,18 +78,4 @@ public class ConscryptSslEngineTest extends SSLEngineTest { @Override public void testMutualAuthValidClientCertChainTooLongFailRequireClientAuth() { } - - @Ignore("Ignore due bug in Conscrypt") - @Override - public void testSessionBindingEvent() throws Exception { - // Ignore due bug in Conscrypt where the incorrect SSLSession object is used in the SSLSessionBindingEvent. - // See https://github.com/google/conscrypt/issues/593 - } - - @Ignore("Ignore due bug in Conscrypt") - @Override - public void testHandshakeSession() throws Exception { - // Ignore as Conscrypt does not correctly return the local certificates while the TrustManager is invoked. - // See https://github.com/google/conscrypt/issues/634 - } } 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 d8510b5093..315b5cd590 100644 --- a/handler/src/test/java/io/netty/handler/ssl/SSLEngineTest.java +++ b/handler/src/test/java/io/netty/handler/ssl/SSLEngineTest.java @@ -1959,12 +1959,14 @@ public abstract class SSLEngineTest { .trustManager(InsecureTrustManagerFactory.INSTANCE) .ciphers(Collections.singletonList(sharedCipher)) .protocols(PROTOCOL_TLS_V1_2, PROTOCOL_TLS_V1) + .sslContextProvider(clientSslContextProvider()) .sslProvider(sslClientProvider()) .build()); serverSslCtx = wrapContext(SslContextBuilder.forServer(ssc.certificate(), ssc.privateKey()) .ciphers(Collections.singletonList(sharedCipher)) .protocols(PROTOCOL_TLS_V1_2, PROTOCOL_TLS_V1) + .sslContextProvider(serverSslContextProvider()) .sslProvider(sslServerProvider()) .build()); SSLEngine clientEngine = null; @@ -1990,12 +1992,14 @@ public abstract class SSLEngineTest { .trustManager(InsecureTrustManagerFactory.INSTANCE) .ciphers(Collections.singletonList(sharedCipher), SupportedCipherSuiteFilter.INSTANCE) .protocols(PROTOCOL_TLS_V1_2, PROTOCOL_TLS_V1) + .sslContextProvider(clientSslContextProvider()) .sslProvider(sslClientProvider()) .build()); serverSslCtx = wrapContext(SslContextBuilder.forServer(ssc.certificate(), ssc.privateKey()) .ciphers(Collections.singletonList(sharedCipher), SupportedCipherSuiteFilter.INSTANCE) .protocols(PROTOCOL_TLS_V1_2, PROTOCOL_TLS_V1) + .sslContextProvider(serverSslContextProvider()) .sslProvider(sslServerProvider()) .build()); SSLEngine clientEngine = null; @@ -2018,6 +2022,7 @@ public abstract class SSLEngineTest { clientSslCtx = wrapContext(SslContextBuilder .forClient() .trustManager(cert.cert()) + .sslContextProvider(clientSslContextProvider()) .sslProvider(sslClientProvider()) .protocols(protocols()) .ciphers(ciphers()) @@ -2026,6 +2031,7 @@ public abstract class SSLEngineTest { serverSslCtx = wrapContext(SslContextBuilder .forServer(cert.certificate(), cert.privateKey()) + .sslContextProvider(serverSslContextProvider()) .sslProvider(sslServerProvider()) .protocols(protocols()) .ciphers(ciphers()) @@ -2061,6 +2067,7 @@ public abstract class SSLEngineTest { public void testSSLEngineUnwrapNoSslRecord() throws Exception { clientSslCtx = wrapContext(SslContextBuilder .forClient() + .sslContextProvider(clientSslContextProvider()) .sslProvider(sslClientProvider()) .protocols(protocols()) .ciphers(ciphers()) @@ -2091,6 +2098,7 @@ public abstract class SSLEngineTest { public void testBeginHandshakeAfterEngineClosed() throws SSLException { clientSslCtx = wrapContext(SslContextBuilder .forClient() + .sslContextProvider(clientSslContextProvider()) .sslProvider(sslClientProvider()) .protocols(protocols()) .ciphers(ciphers()) @@ -2105,6 +2113,12 @@ public abstract class SSLEngineTest { fail(); } catch (SSLException expected) { // expected + } catch (IllegalStateException e) { + if (!Conscrypt.isEngineSupported(client)) { + throw e; + } + // Workaround for conscrypt bug + // See https://github.com/google/conscrypt/issues/840 } } finally { cleanupClientSslEngine(client); @@ -2117,6 +2131,7 @@ public abstract class SSLEngineTest { clientSslCtx = wrapContext(SslContextBuilder .forClient() + .sslContextProvider(clientSslContextProvider()) .sslProvider(sslClientProvider()) .protocols(protocols()) .ciphers(ciphers()) @@ -2125,6 +2140,7 @@ public abstract class SSLEngineTest { serverSslCtx = wrapContext(SslContextBuilder .forServer(cert.certificate(), cert.privateKey()) + .sslContextProvider(serverSslContextProvider()) .sslProvider(sslServerProvider()) .protocols(protocols()) .ciphers(ciphers()) @@ -2168,6 +2184,7 @@ public abstract class SSLEngineTest { clientSslCtx = wrapContext(SslContextBuilder .forClient() + .sslContextProvider(clientSslContextProvider()) .sslProvider(sslClientProvider()) .protocols(protocols()) .ciphers(ciphers()) @@ -2176,6 +2193,7 @@ public abstract class SSLEngineTest { serverSslCtx = wrapContext(SslContextBuilder .forServer(cert.certificate(), cert.privateKey()) + .sslContextProvider(serverSslContextProvider()) .sslProvider(sslServerProvider()) .protocols(protocols()) .ciphers(ciphers()) @@ -2196,7 +2214,11 @@ public abstract class SSLEngineTest { engine.beginHandshake(); try { engine.closeInbound(); - fail(); + // Workaround for conscrypt bug + // See https://github.com/google/conscrypt/issues/839 + if (!Conscrypt.isEngineSupported(engine)) { + fail(); + } } catch (SSLException expected) { // expected } @@ -2209,6 +2231,7 @@ public abstract class SSLEngineTest { clientSslCtx = wrapContext(SslContextBuilder .forClient() .trustManager(cert.cert()) + .sslContextProvider(clientSslContextProvider()) .sslProvider(sslClientProvider()) // This test only works for non TLSv1.3 for now .protocols(PROTOCOL_TLS_V1_2) @@ -2217,6 +2240,7 @@ public abstract class SSLEngineTest { serverSslCtx = wrapContext(SslContextBuilder .forServer(cert.certificate(), cert.privateKey()) + .sslContextProvider(serverSslContextProvider()) .sslProvider(sslServerProvider()) // This test only works for non TLSv1.3 for now .protocols(PROTOCOL_TLS_V1_2) @@ -2246,7 +2270,8 @@ public abstract class SSLEngineTest { assertEquals(SSLEngineResult.Status.CLOSED, result.getStatus()); // Need an UNWRAP to read the response of the close_notify - if (PlatformDependent.javaVersion() >= 12 && sslClientProvider() == SslProvider.JDK) { + if ((PlatformDependent.javaVersion() >= 12 && sslClientProvider() == SslProvider.JDK) + || Conscrypt.isEngineSupported(client)) { // This is a workaround for a possible JDK12+ bug. // // See http://mail.openjdk.java.net/pipermail/security-dev/2019-February/019406.html. @@ -2364,6 +2389,7 @@ public abstract class SSLEngineTest { .forClient() .trustManager(cert.cert()) .sslProvider(sslClientProvider()) + .sslContextProvider(clientSslContextProvider()) .protocols(protocols()) .ciphers(ciphers()) .build()); @@ -2372,6 +2398,7 @@ public abstract class SSLEngineTest { serverSslCtx = wrapContext(SslContextBuilder .forServer(cert.certificate(), cert.privateKey()) .sslProvider(sslServerProvider()) + .sslContextProvider(serverSslContextProvider()) .protocols(protocols()) .ciphers(ciphers()) .build()); @@ -2406,6 +2433,7 @@ public abstract class SSLEngineTest { clientSslCtx = wrapContext(SslContextBuilder .forClient() .trustManager(cert.cert()) + .sslContextProvider(clientSslContextProvider()) .sslProvider(sslClientProvider()) .protocols(protocols()) .ciphers(ciphers()) @@ -2414,6 +2442,7 @@ public abstract class SSLEngineTest { serverSslCtx = wrapContext(SslContextBuilder .forServer(cert.certificate(), cert.privateKey()) + .sslContextProvider(serverSslContextProvider()) .sslProvider(sslServerProvider()) .protocols(protocols()) .ciphers(ciphers()) @@ -2485,6 +2514,7 @@ public abstract class SSLEngineTest { clientSslCtx = wrapContext(SslContextBuilder .forClient() .trustManager(cert.cert()) + .sslContextProvider(clientSslContextProvider()) .sslProvider(sslClientProvider()) .protocols(protocols()) .ciphers(ciphers()) @@ -2493,6 +2523,7 @@ public abstract class SSLEngineTest { serverSslCtx = wrapContext(SslContextBuilder .forServer(cert.certificate(), cert.privateKey()) + .sslContextProvider(serverSslContextProvider()) .sslProvider(sslServerProvider()) .protocols(protocols()) .ciphers(ciphers()) @@ -2553,6 +2584,7 @@ public abstract class SSLEngineTest { clientSslCtx = wrapContext(SslContextBuilder .forClient() .trustManager(cert.cert()) + .sslContextProvider(clientSslContextProvider()) .sslProvider(sslClientProvider()) .protocols(protocols()) .ciphers(ciphers()) @@ -2561,6 +2593,7 @@ public abstract class SSLEngineTest { serverSslCtx = wrapContext(SslContextBuilder .forServer(cert.certificate(), cert.privateKey()) + .sslContextProvider(serverSslContextProvider()) .sslProvider(sslServerProvider()) .protocols(protocols()) .ciphers(ciphers()) @@ -2628,6 +2661,7 @@ public abstract class SSLEngineTest { clientSslCtx = wrapContext(SslContextBuilder .forClient() .trustManager(cert.cert()) + .sslContextProvider(clientSslContextProvider()) .sslProvider(sslClientProvider()) .protocols(protocols()) .ciphers(ciphers()) @@ -2636,6 +2670,7 @@ public abstract class SSLEngineTest { serverSslCtx = wrapContext(SslContextBuilder .forServer(cert.certificate(), cert.privateKey()) + .sslContextProvider(serverSslContextProvider()) .sslProvider(sslServerProvider()) .protocols(protocols()) .ciphers(ciphers()) @@ -2684,6 +2719,7 @@ public abstract class SSLEngineTest { SslContext ctx = wrapContext(SslContextBuilder .forServer(cert.certificate(), cert.privateKey()) + .sslContextProvider(serverSslContextProvider()) .sslProvider(sslServerProvider()) .protocols(protocols()) .ciphers(ciphers()) @@ -2716,7 +2752,10 @@ public abstract class SSLEngineTest { @Test public void testUsingX509TrustManagerVerifiesHostname() throws Exception { - SslProvider clientProvider = sslClientProvider(); + if (clientSslContextProvider() != null) { + // Not supported when using conscrypt + return; + } SelfSignedCertificate cert = new SelfSignedCertificate(); clientSslCtx = wrapContext(SslContextBuilder .forClient() @@ -2755,6 +2794,7 @@ public abstract class SSLEngineTest { } }, null, TrustManagerFactory.getDefaultAlgorithm()) { }) + .sslContextProvider(clientSslContextProvider()) .sslProvider(sslClientProvider()) .build()); @@ -2765,6 +2805,7 @@ public abstract class SSLEngineTest { serverSslCtx = wrapContext(SslContextBuilder .forServer(cert.certificate(), cert.privateKey()) + .sslContextProvider(serverSslContextProvider()) .sslProvider(sslServerProvider()) .build()); @@ -2790,7 +2831,8 @@ public abstract class SSLEngineTest { SSLEngine server = null; try { serverSslCtx = wrapContext(SslContextBuilder.forServer(cert.key(), cert.cert()) - .sslProvider(sslClientProvider()) + .sslContextProvider(serverSslContextProvider()) + .sslProvider(sslServerProvider()) .ciphers(cipherList).build()); server = wrapEngine(serverSslCtx.newEngine(UnpooledByteBufAllocator.DEFAULT)); fail(); diff --git a/pom.xml b/pom.xml index 11d947ec32..0dcc7d36d4 100644 --- a/pom.xml +++ b/pom.xml @@ -350,7 +350,7 @@ ${os.detected.classifier} org.conscrypt conscrypt-openjdk-uber - 1.3.0 + 2.4.0 ${os.detected.name}-${os.detected.arch} ${project.basedir}/../common/src/test/resources/logback-test.xml