OpenSslSession.getLocalCertificates() and getLocalPrincipal() must r… (#10275)

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/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");

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
+        if (!context.sessionContext().useKeyManager()) {
-        // localCertificateChain will be set in setKeyMaterial(...).
+            // If we do not use the KeyManagerFactory we need to set localCertificateChain now.
-        localCertificateChain = context.keyCertChain;
+            // When we use a KeyManagerFactory it will be set during setKeyMaterial(...).
+            localCertificateChain = context.keyCertChain;
+        }
 
         this.jdkCompatibilityMode = jdkCompatibilityMode;
         Lock readerLock = context.ctxLock.readLock();

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);

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);

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);

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;

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);

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 {