From 7e76d02fe744dc95a81f8ec80762359e88540950 Mon Sep 17 00:00:00 2001 From: Norman Maurer Date: Fri, 8 Mar 2019 06:47:28 +0100 Subject: [PATCH] =?UTF-8?q?ReferenceCountedOpenSslEngines=20SSLSession=20m?= =?UTF-8?q?ust=20provide=20local=20certific=E2=80=A6=20(#8918)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Motivation: The SSLSession that is returned by SSLEngine.getHandshakeSession() must be able to provide the local certificates when the TrustManager is invoked on the server-side. Modifications: - Correctly return the local certificates - Add unit test Result: Be able to obtain local certificates from handshake SSLSession during verification on the server side. --- .../ssl/ReferenceCountedOpenSslEngine.java | 5 +- .../ssl/ConscryptJdkSslEngineInteropTest.java | 7 + .../handler/ssl/ConscryptSslEngineTest.java | 7 + .../ssl/JdkConscryptSslEngineInteropTest.java | 7 + .../ssl/JdkOpenSslEngineInteroptTest.java | 6 + .../netty/handler/ssl/OpenSslEngineTest.java | 6 + .../ssl/OpenSslJdkSslEngineInteroptTest.java | 6 + .../io/netty/handler/ssl/SSLEngineTest.java | 151 ++++++++++++++++++ 8 files changed, 192 insertions(+), 3 deletions(-) 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 dc7b1f600f..a45921c578 100644 --- a/handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslEngine.java +++ b/handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslEngine.java @@ -2002,7 +2002,6 @@ public class ReferenceCountedOpenSslEngine extends SSLEngine implements Referenc // thread. private X509Certificate[] x509PeerCerts; private Certificate[] peerCerts; - private Certificate[] localCerts; private String protocol; private String cipher; @@ -2149,7 +2148,6 @@ public class ReferenceCountedOpenSslEngine extends SSLEngine implements Referenc id = SSL.getSessionId(ssl); cipher = toJavaCipherSuite(SSL.getCipherForSSL(ssl)); protocol = SSL.getVersion(ssl); - localCerts = localCertificateChain; initPeerCerts(); selectApplicationProtocol(); @@ -2284,6 +2282,7 @@ public class ReferenceCountedOpenSslEngine extends SSLEngine implements Referenc @Override public Certificate[] getLocalCertificates() { + Certificate[] localCerts = ReferenceCountedOpenSslEngine.this.localCertificateChain; if (localCerts == null) { return null; } @@ -2310,7 +2309,7 @@ public class ReferenceCountedOpenSslEngine extends SSLEngine implements Referenc @Override public Principal getLocalPrincipal() { - Certificate[] local = localCerts; + Certificate[] local = ReferenceCountedOpenSslEngine.this.localCertificateChain; if (local == null || local.length == 0) { return null; } 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 3ad54a9310..a6768f1063 100644 --- a/handler/src/test/java/io/netty/handler/ssl/ConscryptJdkSslEngineInteropTest.java +++ b/handler/src/test/java/io/netty/handler/ssl/ConscryptJdkSslEngineInteropTest.java @@ -87,4 +87,11 @@ public class ConscryptJdkSslEngineInteropTest extends SSLEngineTest { // 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 3f3c8777ad..2dec32790b 100644 --- a/handler/src/test/java/io/netty/handler/ssl/ConscryptSslEngineTest.java +++ b/handler/src/test/java/io/netty/handler/ssl/ConscryptSslEngineTest.java @@ -85,4 +85,11 @@ public class ConscryptSslEngineTest extends SSLEngineTest { // 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/JdkConscryptSslEngineInteropTest.java b/handler/src/test/java/io/netty/handler/ssl/JdkConscryptSslEngineInteropTest.java index b3afb5a6e5..0b6202cd68 100644 --- a/handler/src/test/java/io/netty/handler/ssl/JdkConscryptSslEngineInteropTest.java +++ b/handler/src/test/java/io/netty/handler/ssl/JdkConscryptSslEngineInteropTest.java @@ -85,4 +85,11 @@ public class JdkConscryptSslEngineInteropTest extends SSLEngineTest { // TODO(scott): work around for a JDK issue. The exception should be SSLHandshakeException. return super.mySetupMutualAuthServerIsValidClientException(cause) || causedBySSLException(cause); } + + @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/JdkOpenSslEngineInteroptTest.java b/handler/src/test/java/io/netty/handler/ssl/JdkOpenSslEngineInteroptTest.java index 64add35a19..daccb8251b 100644 --- a/handler/src/test/java/io/netty/handler/ssl/JdkOpenSslEngineInteroptTest.java +++ b/handler/src/test/java/io/netty/handler/ssl/JdkOpenSslEngineInteroptTest.java @@ -128,6 +128,12 @@ public class JdkOpenSslEngineInteroptTest extends SSLEngineTest { return super.mySetupMutualAuthServerIsValidClientException(cause) || causedBySSLException(cause); } + @Override + public void testHandshakeSession() throws Exception { + checkShouldUseKeyManagerFactory(); + super.testHandshakeSession(); + } + @Override protected SSLEngine wrapEngine(SSLEngine engine) { return Java8SslTestUtils.wrapSSLEngineForTesting(engine); 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 824a11d8e9..37babf690c 100644 --- a/handler/src/test/java/io/netty/handler/ssl/OpenSslEngineTest.java +++ b/handler/src/test/java/io/netty/handler/ssl/OpenSslEngineTest.java @@ -161,6 +161,12 @@ public class OpenSslEngineTest extends SSLEngineTest { super.testClientHostnameValidationFail(); } + @Override + public void testHandshakeSession() throws Exception { + checkShouldUseKeyManagerFactory(); + super.testHandshakeSession(); + } + private static boolean isNpnSupported(String versionString) { String[] versionStringParts = versionString.split(" ", -1); if (versionStringParts.length == 2 && "LibreSSL".equals(versionStringParts[0])) { 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 df10fc731a..6b6a523f95 100644 --- a/handler/src/test/java/io/netty/handler/ssl/OpenSslJdkSslEngineInteroptTest.java +++ b/handler/src/test/java/io/netty/handler/ssl/OpenSslJdkSslEngineInteroptTest.java @@ -127,6 +127,12 @@ public class OpenSslJdkSslEngineInteroptTest extends SSLEngineTest { return super.mySetupMutualAuthServerIsValidServerException(cause) || causedBySSLException(cause); } + @Override + public void testHandshakeSession() throws Exception { + checkShouldUseKeyManagerFactory(); + super.testHandshakeSession(); + } + @Override protected SSLEngine wrapEngine(SSLEngine engine) { return Java8SslTestUtils.wrapSSLEngineForTesting(engine); 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 16630e9a2f..ec10f667fb 100644 --- a/handler/src/test/java/io/netty/handler/ssl/SSLEngineTest.java +++ b/handler/src/test/java/io/netty/handler/ssl/SSLEngineTest.java @@ -37,6 +37,7 @@ import io.netty.channel.socket.nio.NioSocketChannel; import io.netty.handler.ssl.ApplicationProtocolConfig.Protocol; import io.netty.handler.ssl.util.InsecureTrustManagerFactory; import io.netty.handler.ssl.util.SelfSignedCertificate; +import io.netty.handler.ssl.util.SimpleTrustManagerFactory; import io.netty.util.CharsetUtil; import io.netty.util.NetUtil; import io.netty.util.ReferenceCountUtil; @@ -71,18 +72,24 @@ import javax.net.ssl.SSLSocketFactory; import javax.net.ssl.TrustManager; import javax.net.ssl.TrustManagerFactory; import javax.net.ssl.TrustManagerFactorySpi; +import javax.net.ssl.X509ExtendedTrustManager; import javax.net.ssl.X509TrustManager; import javax.security.cert.X509Certificate; import java.io.ByteArrayInputStream; import java.io.File; import java.io.FileInputStream; +import java.io.IOException; import java.io.InputStream; import java.net.InetSocketAddress; +import java.net.Socket; import java.nio.ByteBuffer; import java.nio.channels.ClosedChannelException; import java.security.KeyStore; +import java.security.KeyStoreException; +import java.security.NoSuchAlgorithmException; import java.security.Principal; import java.security.Provider; +import java.security.UnrecoverableKeyException; import java.security.cert.Certificate; import java.security.cert.CertificateException; import java.util.ArrayList; @@ -107,6 +114,7 @@ import static io.netty.handler.ssl.SslUtils.PROTOCOL_TLS_V1_1; import static io.netty.handler.ssl.SslUtils.PROTOCOL_TLS_V1_2; import static io.netty.handler.ssl.SslUtils.PROTOCOL_TLS_V1_3; import static io.netty.handler.ssl.SslUtils.SSL_RECORD_HEADER_LENGTH; + import static org.junit.Assert.assertArrayEquals; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; @@ -3060,6 +3068,149 @@ public abstract class SSLEngineTest { } } + @Test + public void testHandshakeSession() throws Exception { + final SelfSignedCertificate ssc = new SelfSignedCertificate(); + + final TestTrustManagerFactory clientTmf = new TestTrustManagerFactory(ssc.cert()); + final TestTrustManagerFactory serverTmf = new TestTrustManagerFactory(ssc.cert()); + + clientSslCtx = SslContextBuilder.forClient() + .trustManager(new SimpleTrustManagerFactory() { + @Override + protected void engineInit(KeyStore keyStore) { + // NOOP + } + + @Override + protected void engineInit(ManagerFactoryParameters managerFactoryParameters) { + // NOOP + } + + @Override + protected TrustManager[] engineGetTrustManagers() { + return new TrustManager[] { clientTmf }; + } + }) + .keyManager(newKeyManagerFactory(ssc)) + .sslProvider(sslClientProvider()) + .sslContextProvider(clientSslContextProvider()) + .protocols(protocols()) + .ciphers(ciphers()) + .build(); + serverSslCtx = SslContextBuilder.forServer(newKeyManagerFactory(ssc)) + .trustManager(new SimpleTrustManagerFactory() { + @Override + protected void engineInit(KeyStore keyStore) { + // NOOP + } + + @Override + protected void engineInit(ManagerFactoryParameters managerFactoryParameters) { + // NOOP + } + + @Override + protected TrustManager[] engineGetTrustManagers() { + return new TrustManager[] { serverTmf }; + } + }) + .sslProvider(sslServerProvider()) + .sslContextProvider(serverSslContextProvider()) + .protocols(protocols()) + .ciphers(ciphers()) + .clientAuth(ClientAuth.REQUIRE) + .build(); + SSLEngine clientEngine = null; + SSLEngine serverEngine = null; + try { + clientEngine = wrapEngine(clientSslCtx.newEngine(UnpooledByteBufAllocator.DEFAULT)); + serverEngine = wrapEngine(serverSslCtx.newEngine(UnpooledByteBufAllocator.DEFAULT)); + handshake(clientEngine, serverEngine); + + assertTrue(clientTmf.isVerified()); + assertTrue(serverTmf.isVerified()); + } finally { + cleanupClientSslEngine(clientEngine); + cleanupServerSslEngine(serverEngine); + ssc.delete(); + } + } + + private KeyManagerFactory newKeyManagerFactory(SelfSignedCertificate ssc) + throws UnrecoverableKeyException, KeyStoreException, NoSuchAlgorithmException, + CertificateException, IOException { + return SslContext.buildKeyManagerFactory( + new java.security.cert.X509Certificate[] { ssc.cert() }, ssc.key(), null, null); + } + + private final class TestTrustManagerFactory extends X509ExtendedTrustManager { + private final Certificate localCert; + private volatile boolean verified; + + TestTrustManagerFactory(Certificate localCert) { + this.localCert = localCert; + } + + boolean isVerified() { + return verified; + } + + @Override + public void checkClientTrusted( + java.security.cert.X509Certificate[] x509Certificates, String s, Socket socket) { + fail(); + } + + @Override + public void checkServerTrusted( + java.security.cert.X509Certificate[] x509Certificates, String s, Socket socket) { + fail(); + } + + @Override + public void checkClientTrusted( + java.security.cert.X509Certificate[] x509Certificates, String s, SSLEngine sslEngine) { + verified = true; + assertFalse(sslEngine.getUseClientMode()); + SSLSession session = sslEngine.getHandshakeSession(); + assertNotNull(session); + Certificate[] localCertificates = session.getLocalCertificates(); + assertNotNull(localCertificates); + assertEquals(1, localCertificates.length); + assertEquals(localCert, localCertificates[0]); + assertNotNull(session.getLocalPrincipal()); + } + + @Override + public void checkServerTrusted( + java.security.cert.X509Certificate[] x509Certificates, String s, SSLEngine sslEngine) { + verified = true; + assertTrue(sslEngine.getUseClientMode()); + SSLSession session = sslEngine.getHandshakeSession(); + assertNotNull(session); + assertNull(session.getLocalCertificates()); + assertNull(session.getLocalPrincipal()); + } + + @Override + public void checkClientTrusted( + java.security.cert.X509Certificate[] x509Certificates, String s) { + fail(); + } + + @Override + public void checkServerTrusted( + java.security.cert.X509Certificate[] x509Certificates, String s) { + fail(); + } + + @Override + public java.security.cert.X509Certificate[] getAcceptedIssuers() { + return EmptyArrays.EMPTY_X509_CERTIFICATES; + } + } + protected SSLEngine wrapEngine(SSLEngine engine) { return engine; }