Correctly produce ssl alert when certificate validation fails on the client-side when using native SSL implementation. (#8949)

Motivation:

When the verification of the server cert fails because of the used TrustManager on the client-side we need to ensure we produce the correct alert and send it to the remote peer before closing the connection.

Modifications:

- Use the correct verification mode on the client-side by default.
- Update tests

Result:

Fixes https://github.com/netty/netty/issues/8942.

handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslClientContext.java

@@ -131,7 +131,13 @@ public final class ReferenceCountedOpenSslClientContext extends ReferenceCounted
                 throw new SSLException("failed to set certificate and key", e);
             }
 
-            SSLContext.setVerify(ctx, SSL.SSL_CVERIFY_NONE, VERIFY_DEPTH);
+            // On the client side we always need to use SSL_CVERIFY_OPTIONAL (which will translate to SSL_VERIFY_PEER)
+            // to ensure that when the TrustManager throws we will produce the correct alert back to the server.
+            //
+            // See:
+            //   - https://www.openssl.org/docs/man1.0.2/man3/SSL_CTX_set_verify.html
+            //   - https://github.com/netty/netty/issues/8942
+            SSLContext.setVerify(ctx, SSL.SSL_CVERIFY_OPTIONAL, VERIFY_DEPTH);
 
             try {
                 if (trustCertCollection != null) {

handler/src/test/java/io/netty/handler/ssl/SslErrorTest.java

@@ -64,7 +64,8 @@ import java.util.Locale;
 @RunWith(Parameterized.class)
 public class SslErrorTest {
 
-    @Parameterized.Parameters(name = "{index}: serverProvider = {0}, clientProvider = {1}, exception = {2}")
+    @Parameterized.Parameters(
+            name = "{index}: serverProvider = {0}, clientProvider = {1}, exception = {2}, serverProduceError = {3}")
     public static Collection<Object[]> data() {
         List<SslProvider> serverProviders = new ArrayList<>(2);
         List<SslProvider> clientProviders = new ArrayList<>(3);
@@ -95,7 +96,8 @@ public class SslErrorTest {
         for (SslProvider serverProvider: serverProviders) {
             for (SslProvider clientProvider: clientProviders) {
                 for (CertificateException exception: exceptions) {
-                    params.add(new Object[] { serverProvider, clientProvider, exception});
+                    params.add(new Object[] { serverProvider, clientProvider, exception, true });
+                    params.add(new Object[] { serverProvider, clientProvider, exception, false });
                 }
             }
         }
@@ -110,11 +112,14 @@ public class SslErrorTest {
     private final SslProvider serverProvider;
     private final SslProvider clientProvider;
     private final CertificateException exception;
+    private final boolean serverProduceError;
 
-    public SslErrorTest(SslProvider serverProvider, SslProvider clientProvider, CertificateException exception) {
+    public SslErrorTest(SslProvider serverProvider, SslProvider clientProvider,
+                        CertificateException exception, boolean serverProduceError) {
         this.serverProvider = serverProvider;
         this.clientProvider = clientProvider;
         this.exception = exception;
+        this.serverProduceError = serverProduceError;
     }
 
     @Test(timeout = 30000)
@@ -124,56 +129,41 @@ public class SslErrorTest {
         Assume.assumeTrue(OpenSsl.isAvailable());
 
         SelfSignedCertificate ssc = new SelfSignedCertificate();
-        final SslContext sslServerCtx =
-                SslContextBuilder.forServer(ssc.certificate(), ssc.privateKey())
-                                 .sslProvider(serverProvider)
-                                 .trustManager(new SimpleTrustManagerFactory() {
-            @Override
-            protected void engineInit(KeyStore keyStore) { }
-            @Override
-            protected void engineInit(ManagerFactoryParameters managerFactoryParameters) { }
 
-            @Override
+        SslContextBuilder sslServerCtxBuilder = SslContextBuilder.forServer(ssc.certificate(), ssc.privateKey())
-            protected TrustManager[] engineGetTrustManagers() {
+                .sslProvider(serverProvider)
-                return new TrustManager[] { new X509TrustManager() {
+                .clientAuth(ClientAuth.REQUIRE);
-
+        SslContextBuilder sslClientCtxBuilder =  SslContextBuilder.forClient()
-                    @Override
-                    public void checkClientTrusted(X509Certificate[] x509Certificates, String s)
-                            throws CertificateException {
-                        throw exception;
-                    }
-
-                    @Override
-                    public void checkServerTrusted(X509Certificate[] x509Certificates, String s)
-                            throws CertificateException {
-                        // NOOP
-                    }
-
-                    @Override
-                    public X509Certificate[] getAcceptedIssuers() {
-                        return EmptyArrays.EMPTY_X509_CERTIFICATES;
-                    }
-                } };
-            }
-        }).clientAuth(ClientAuth.REQUIRE).build();
-
-        final SslContext sslClientCtx = SslContextBuilder.forClient()
-                .trustManager(InsecureTrustManagerFactory.INSTANCE)
                 .keyManager(new File(getClass().getResource("test.crt").getFile()),
                         new File(getClass().getResource("test_unencrypted.pem").getFile()))
-                .sslProvider(clientProvider).build();
+                .sslProvider(clientProvider);
+
+        if (serverProduceError) {
+            sslServerCtxBuilder.trustManager(new ExceptionTrustManagerFactory());
+            sslClientCtxBuilder.trustManager(InsecureTrustManagerFactory.INSTANCE);
+        } else {
+            sslServerCtxBuilder.trustManager(InsecureTrustManagerFactory.INSTANCE);
+            sslClientCtxBuilder.trustManager(new ExceptionTrustManagerFactory());
+        }
+
+        final SslContext sslServerCtx = sslServerCtxBuilder.build();
+        final SslContext sslClientCtx = sslClientCtxBuilder.build();
 
         Channel serverChannel = null;
         Channel clientChannel = null;
         EventLoopGroup group = new MultithreadEventLoopGroup(NioHandler.newFactory());
+        final Promise<Void> promise = group.next().newPromise();
         try {
             serverChannel = new ServerBootstrap().group(group)
                     .channel(NioServerSocketChannel.class)
                     .handler(new LoggingHandler(LogLevel.INFO))
                     .childHandler(new ChannelInitializer<Channel>() {
                         @Override
-                        protected void initChannel(Channel ch) throws Exception {
+                        protected void initChannel(Channel ch) {
                             ch.pipeline().addLast(sslServerCtx.newHandler(ch.alloc()));
+                            if (!serverProduceError) {
+                                ch.pipeline().addLast(new AlertValidationHandler(promise));
+                            }
                             ch.pipeline().addLast(new ChannelInboundHandler() {
 
                                 @Override
@@ -184,48 +174,21 @@ public class SslErrorTest {
                         }
                     }).bind(0).sync().channel();
 
-            final Promise<Void> promise = group.next().newPromise();
-
             clientChannel = new Bootstrap().group(group)
                     .channel(NioSocketChannel.class)
                     .handler(new ChannelInitializer<Channel>() {
                         @Override
-                        protected void initChannel(Channel ch) throws Exception {
+                        protected void initChannel(Channel ch) {
                             ch.pipeline().addLast(sslClientCtx.newHandler(ch.alloc()));
+
+                            if (serverProduceError) {
+                                ch.pipeline().addLast(new AlertValidationHandler(promise));
+                            }
                             ch.pipeline().addLast(new ChannelInboundHandler() {
+
                                 @Override
                                 public void exceptionCaught(ChannelHandlerContext ctx, Throwable cause) {
-                                    // Unwrap as its wrapped by a DecoderException
+                                    ctx.close();
-                                    Throwable unwrappedCause = cause.getCause();
-                                    if (unwrappedCause instanceof SSLException) {
-                                        if (exception instanceof TestCertificateException) {
-                                            CertPathValidatorException.Reason reason =
-                                                    ((CertPathValidatorException) exception.getCause()).getReason();
-                                            if (reason == CertPathValidatorException.BasicReason.EXPIRED) {
-                                                verifyException(unwrappedCause, "expired", promise);
-                                            } else if (reason == CertPathValidatorException.BasicReason.NOT_YET_VALID) {
-                                                // BoringSSL uses "expired" in this case while others use "bad"
-                                                if (OpenSsl.isBoringSSL()) {
-                                                    verifyException(unwrappedCause, "expired", promise);
-                                                } else {
-                                                    verifyException(unwrappedCause, "bad", promise);
-                                                }
-                                            } else if (reason == CertPathValidatorException.BasicReason.REVOKED) {
-                                                verifyException(unwrappedCause, "revoked", promise);
-                                            }
-                                        } else if (exception instanceof CertificateExpiredException) {
-                                            verifyException(unwrappedCause, "expired", promise);
-                                        } else if (exception instanceof CertificateNotYetValidException) {
-                                            // BoringSSL uses "expired" in this case while others use "bad"
-                                            if (OpenSsl.isBoringSSL()) {
-                                                verifyException(unwrappedCause, "expired", promise);
-                                            } else {
-                                                verifyException(unwrappedCause, "bad", promise);
-                                            }
-                                        } else if (exception instanceof CertificateRevokedException) {
-                                            verifyException(unwrappedCause, "revoked", promise);
-                                        }
-                                    }
                                 }
                             });
                         }
@@ -246,11 +209,88 @@ public class SslErrorTest {
         }
     }
 
+    private final class ExceptionTrustManagerFactory extends SimpleTrustManagerFactory {
+        @Override
+        protected void engineInit(KeyStore keyStore) { }
+        @Override
+        protected void engineInit(ManagerFactoryParameters managerFactoryParameters) { }
+
+        @Override
+        protected TrustManager[] engineGetTrustManagers() {
+            return new TrustManager[] { new X509TrustManager() {
+
+                @Override
+                public void checkClientTrusted(X509Certificate[] x509Certificates, String s)
+                        throws CertificateException {
+                    throw exception;
+                }
+
+                @Override
+                public void checkServerTrusted(X509Certificate[] x509Certificates, String s)
+                        throws CertificateException {
+                    throw exception;
+                }
+
+                @Override
+                public X509Certificate[] getAcceptedIssuers() {
+                    return EmptyArrays.EMPTY_X509_CERTIFICATES;
+                }
+            } };
+        }
+    }
+
+    private final class AlertValidationHandler implements ChannelInboundHandler {
+        private final Promise<Void> promise;
+
+        AlertValidationHandler(Promise<Void> promise) {
+            this.promise = promise;
+        }
+
+        @Override
+        public void exceptionCaught(ChannelHandlerContext ctx, Throwable cause) {
+            // Unwrap as its wrapped by a DecoderException
+            Throwable unwrappedCause = cause.getCause();
+            if (unwrappedCause instanceof SSLException) {
+                if (exception instanceof TestCertificateException) {
+                    CertPathValidatorException.Reason reason =
+                            ((CertPathValidatorException) exception.getCause()).getReason();
+                    if (reason == CertPathValidatorException.BasicReason.EXPIRED) {
+                        verifyException(unwrappedCause, "expired", promise);
+                    } else if (reason == CertPathValidatorException.BasicReason.NOT_YET_VALID) {
+                        // BoringSSL uses "expired" in this case while others use "bad"
+                        if (OpenSsl.isBoringSSL()) {
+                            verifyException(unwrappedCause, "expired", promise);
+                        } else {
+                            verifyException(unwrappedCause, "bad", promise);
+                        }
+                    } else if (reason == CertPathValidatorException.BasicReason.REVOKED) {
+                        verifyException(unwrappedCause, "revoked", promise);
+                    }
+                } else if (exception instanceof CertificateExpiredException) {
+                    verifyException(unwrappedCause, "expired", promise);
+                } else if (exception instanceof CertificateNotYetValidException) {
+                    // BoringSSL uses "expired" in this case while others use "bad"
+                    if (OpenSsl.isBoringSSL()) {
+                        verifyException(unwrappedCause, "expired", promise);
+                    } else {
+                        verifyException(unwrappedCause, "bad", promise);
+                    }
+                } else if (exception instanceof CertificateRevokedException) {
+                    verifyException(unwrappedCause, "revoked", promise);
+                }
+            }
+        }
+    }
+
     // Its a bit hacky to verify against the message that is part of the exception but there is no other way
     // at the moment as there are no different exceptions for the different alerts.
-    private static void verifyException(Throwable cause, String messagePart, Promise<Void> promise) {
+    private void verifyException(Throwable cause, String messagePart, Promise<Void> promise) {
         String message = cause.getMessage();
-        if (message.toLowerCase(Locale.UK).contains(messagePart.toLowerCase(Locale.UK))) {
+        if (message.toLowerCase(Locale.UK).contains(messagePart.toLowerCase(Locale.UK)) ||
+                // When the error is produced on the client side and the client side uses JDK as provider it will always
+                // use "certificate unknown".
+                !serverProduceError && clientProvider == SslProvider.JDK &&
+                        message.toLowerCase(Locale.UK).contains("certificate unknown")) {
             promise.setSuccess(null);
         } else {
             Throwable error = new AssertionError("message not contains '" + messagePart + "': " + message);