From 11b63f87441a9087d455b4d48f27b748359e63ee Mon Sep 17 00:00:00 2001 From: Norman Maurer Date: Wed, 13 May 2020 07:16:27 +0200 Subject: [PATCH] =?UTF-8?q?OpenSslSession.getLocalCertificates()=20and=20?= =?UTF-8?q?=20getLocalPrincipal()=20must=20r=E2=80=A6=20(#10275)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Motivation: OpenSslSession.getLocalCertificates() and getLocalPrincipal() must return null on client side if mTLS is not used as stated in the API documentation. At the moment this is not always the case Modifications: - Ensure we only return non-null if mTLS is used - Add unit tests Result: Follow SSLSession API contract --- .../handler/ssl/OpenSslSessionContext.java | 4 ++ .../ssl/ReferenceCountedOpenSslEngine.java | 8 ++- .../ConscryptOpenSslEngineInteropTest.java | 7 +++ .../ssl/JdkOpenSslEngineInteroptTest.java | 7 +++ .../OpenSslConscryptSslEngineInteropTest.java | 7 +++ .../netty/handler/ssl/OpenSslEngineTest.java | 7 +++ .../ssl/OpenSslJdkSslEngineInteroptTest.java | 7 +++ .../io/netty/handler/ssl/SSLEngineTest.java | 62 +++++++++++++++++++ 8 files changed, 106 insertions(+), 3 deletions(-) diff --git a/handler/src/main/java/io/netty/handler/ssl/OpenSslSessionContext.java b/handler/src/main/java/io/netty/handler/ssl/OpenSslSessionContext.java index 230c025c62..e22ad9ec4e 100644 --- a/handler/src/main/java/io/netty/handler/ssl/OpenSslSessionContext.java +++ b/handler/src/main/java/io/netty/handler/ssl/OpenSslSessionContext.java @@ -53,6 +53,10 @@ public abstract class OpenSslSessionContext implements SSLSessionContext { stats = new OpenSslSessionStats(context); } + final boolean useKeyManager() { + return provider != null; + } + @Override public SSLSession getSession(byte[] bytes) { requireNonNull(bytes, "bytes"); 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 f2d57a7033..6fa559c20b 100644 --- a/handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslEngine.java +++ b/handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslEngine.java @@ -307,9 +307,11 @@ public class ReferenceCountedOpenSslEngine extends SSLEngine implements Referenc }; engineMap = context.engineMap; enableOcsp = context.enableOcsp; - // context.keyCertChain will only be non-null if we do not use the KeyManagerFactory. In this case - // localCertificateChain will be set in setKeyMaterial(...). - localCertificateChain = context.keyCertChain; + if (!context.sessionContext().useKeyManager()) { + // If we do not use the KeyManagerFactory we need to set localCertificateChain now. + // When we use a KeyManagerFactory it will be set during setKeyMaterial(...). + localCertificateChain = context.keyCertChain; + } this.jdkCompatibilityMode = jdkCompatibilityMode; Lock readerLock = context.ctxLock.readLock(); 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 2744ac5b70..e1ca54e672 100644 --- a/handler/src/test/java/io/netty/handler/ssl/ConscryptOpenSslEngineInteropTest.java +++ b/handler/src/test/java/io/netty/handler/ssl/ConscryptOpenSslEngineInteropTest.java @@ -143,6 +143,13 @@ public class ConscryptOpenSslEngineInteropTest extends ConscryptSslEngineTest { return super.mySetupMutualAuthServerIsValidServerException(cause) || causedBySSLException(cause); } + @Override + @Test + public void testSessionLocalWhenNonMutualWithKeyManager() throws Exception { + checkShouldUseKeyManagerFactory(); + super.testSessionLocalWhenNonMutualWithKeyManager(); + } + @Override protected SSLEngine wrapEngine(SSLEngine engine) { return Java8SslTestUtils.wrapSSLEngineForTesting(engine); 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 0dd042f1a6..29720fd772 100644 --- a/handler/src/test/java/io/netty/handler/ssl/JdkOpenSslEngineInteroptTest.java +++ b/handler/src/test/java/io/netty/handler/ssl/JdkOpenSslEngineInteroptTest.java @@ -151,6 +151,13 @@ public class JdkOpenSslEngineInteroptTest extends SSLEngineTest { super.testSupportedSignatureAlgorithms(); } + @Override + @Test + public void testSessionLocalWhenNonMutualWithKeyManager() throws Exception { + checkShouldUseKeyManagerFactory(); + super.testSessionLocalWhenNonMutualWithKeyManager(); + } + @Override protected SSLEngine wrapEngine(SSLEngine engine) { return Java8SslTestUtils.wrapSSLEngineForTesting(engine); 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 16f224c49e..5b20566c27 100644 --- a/handler/src/test/java/io/netty/handler/ssl/OpenSslConscryptSslEngineInteropTest.java +++ b/handler/src/test/java/io/netty/handler/ssl/OpenSslConscryptSslEngineInteropTest.java @@ -135,6 +135,13 @@ public class OpenSslConscryptSslEngineInteropTest extends ConscryptSslEngineTest return super.mySetupMutualAuthServerIsValidServerException(cause) || causedBySSLException(cause); } + @Override + @Test + public void testSessionLocalWhenNonMutualWithKeyManager() throws Exception { + checkShouldUseKeyManagerFactory(); + super.testSessionLocalWhenNonMutualWithKeyManager(); + } + @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 075bddce02..8c57925fe3 100644 --- a/handler/src/test/java/io/netty/handler/ssl/OpenSslEngineTest.java +++ b/handler/src/test/java/io/netty/handler/ssl/OpenSslEngineTest.java @@ -1303,6 +1303,13 @@ public class OpenSslEngineTest extends SSLEngineTest { } } + @Override + @Test + public void testSessionLocalWhenNonMutualWithKeyManager() throws Exception { + checkShouldUseKeyManagerFactory(); + super.testSessionLocalWhenNonMutualWithKeyManager(); + } + @Override protected SslProvider sslClientProvider() { return SslProvider.OPENSSL; 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 96e7060d2e..52767bc992 100644 --- a/handler/src/test/java/io/netty/handler/ssl/OpenSslJdkSslEngineInteroptTest.java +++ b/handler/src/test/java/io/netty/handler/ssl/OpenSslJdkSslEngineInteroptTest.java @@ -136,6 +136,13 @@ public class OpenSslJdkSslEngineInteroptTest extends SSLEngineTest { super.testSupportedSignatureAlgorithms(); } + @Override + @Test + public void testSessionLocalWhenNonMutualWithKeyManager() throws Exception { + checkShouldUseKeyManagerFactory(); + super.testSessionLocalWhenNonMutualWithKeyManager(); + } + @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 315b5cd590..4bad6fd944 100644 --- a/handler/src/test/java/io/netty/handler/ssl/SSLEngineTest.java +++ b/handler/src/test/java/io/netty/handler/ssl/SSLEngineTest.java @@ -3346,6 +3346,68 @@ public abstract class SSLEngineTest { } } + @Test + public void testSessionLocalWhenNonMutualWithKeyManager() throws Exception { + testSessionLocalWhenNonMutual(true); + } + + @Test + public void testSessionLocalWhenNonMutualWithoutKeyManager() throws Exception { + testSessionLocalWhenNonMutual(false); + } + + private void testSessionLocalWhenNonMutual(boolean useKeyManager) throws Exception { + final SelfSignedCertificate ssc = new SelfSignedCertificate(); + + SslContextBuilder clientSslCtxBuilder = SslContextBuilder.forClient() + .trustManager(InsecureTrustManagerFactory.INSTANCE) + .sslProvider(sslClientProvider()) + .sslContextProvider(clientSslContextProvider()) + .protocols(protocols()) + .ciphers(ciphers()); + + if (useKeyManager) { + clientSslCtxBuilder.keyManager(newKeyManagerFactory(ssc)); + } else { + clientSslCtxBuilder.keyManager(ssc.certificate(), ssc.privateKey()); + } + clientSslCtx = wrapContext(clientSslCtxBuilder.build()); + + final SslContextBuilder serverSslCtxBuilder; + if (useKeyManager) { + serverSslCtxBuilder = SslContextBuilder.forServer(newKeyManagerFactory(ssc)); + } else { + serverSslCtxBuilder = SslContextBuilder.forServer(ssc.certificate(), ssc.privateKey()); + } + serverSslCtxBuilder.trustManager(InsecureTrustManagerFactory.INSTANCE) + .sslProvider(sslServerProvider()) + .sslContextProvider(serverSslContextProvider()) + .protocols(protocols()) + .ciphers(ciphers()) + .clientAuth(ClientAuth.NONE); + + serverSslCtx = wrapContext(serverSslCtxBuilder.build()); + SSLEngine clientEngine = null; + SSLEngine serverEngine = null; + try { + clientEngine = wrapEngine(clientSslCtx.newEngine(UnpooledByteBufAllocator.DEFAULT)); + serverEngine = wrapEngine(serverSslCtx.newEngine(UnpooledByteBufAllocator.DEFAULT)); + handshake(clientEngine, serverEngine); + + SSLSession clientSession = clientEngine.getSession(); + assertNull(clientSession.getLocalCertificates()); + assertNull(clientSession.getLocalPrincipal()); + + SSLSession serverSession = serverEngine.getSession(); + assertNotNull(serverSession.getLocalCertificates()); + assertNotNull(serverSession.getLocalPrincipal()); + } finally { + cleanupClientSslEngine(clientEngine); + cleanupServerSslEngine(serverEngine); + ssc.delete(); + } + } + @Test public void testMasterKeyLogging() throws Exception {