Move the hostname verification to after the SSL handshake has completed (Netty Only), back port #525

Author: Stephane Landelle

src/main/java/com/ning/http/client/providers/netty/NettyConnectListener.java

@@ -21,7 +21,7 @@
 import com.ning.http.client.ProxyServer;
 import com.ning.http.client.Request;
 import com.ning.http.client.uri.UriComponents;
-import com.ning.http.util.AllowAllHostnameVerifier;
+import com.ning.http.util.Base64;
 import com.ning.http.util.ProxyUtils;
 
 import org.jboss.netty.buffer.ChannelBuffer;
@@ -34,22 +34,21 @@
 import org.slf4j.LoggerFactory;
 
 import javax.net.ssl.HostnameVerifier;
+import javax.net.ssl.SSLEngine;
+import javax.net.ssl.SSLSession;
 
 import java.io.IOException;
 import java.net.ConnectException;
-import java.net.InetSocketAddress;
 import java.nio.channels.ClosedChannelException;
-import java.util.concurrent.atomic.AtomicBoolean;
 
 /**
  * Non Blocking connect.
  */
 final class NettyConnectListener<T> implements ChannelFutureListener {
-    private final static Logger logger = LoggerFactory.getLogger(NettyConnectListener.class);
+    private static final Logger LOGGER = LoggerFactory.getLogger(NettyConnectListener.class);
     private final AsyncHttpClientConfig config;
     private final NettyResponseFuture<T> future;
     private final HttpRequest nettyRequest;
-    private final AtomicBoolean handshakeDone = new AtomicBoolean(false);
 
     private NettyConnectListener(AsyncHttpClientConfig config,
                                  NettyResponseFuture<T> future) {
@@ -66,38 +65,51 @@ public final void operationComplete(ChannelFuture f) throws Exception {
         if (f.isSuccess()) {
             Channel channel = f.getChannel();
             channel.getPipeline().getContext(NettyAsyncHttpProvider.class).setAttachment(future);
-            SslHandler sslHandler = (SslHandler) channel.getPipeline().get(NettyAsyncHttpProvider.SSL_HANDLER);
-            if (!handshakeDone.getAndSet(true) && (sslHandler != null)) {
-                ((SslHandler) channel.getPipeline().get(NettyAsyncHttpProvider.SSL_HANDLER)).handshake().addListener(this);
-                return;
-            }
-
-            HostnameVerifier v = config.getHostnameVerifier();
-            if (sslHandler != null && !(v instanceof AllowAllHostnameVerifier)) {
-                // TODO: channel.getRemoteAddress()).getHostName() is very expensive. Should cache the result.
-                if (!v.verify(InetSocketAddress.class.cast(channel.getRemoteAddress()).getHostName(),
-                        sslHandler.getEngine().getSession())) {
-                    throw new ConnectException("HostnameVerifier exception.");
-                }
+            final SslHandler sslHandler = (SslHandler) channel.getPipeline().get(NettyAsyncHttpProvider.SSL_HANDLER);
+            
+            final HostnameVerifier hostnameVerifier = config.getHostnameVerifier();
+            if (hostnameVerifier != null && sslHandler != null) {
+                final String host = future.getURI().getHost();
+                sslHandler.handshake().addListener(new ChannelFutureListener() {
+                    @Override
+                    public void operationComplete(ChannelFuture handshakeFuture) throws Exception {
+                        if (handshakeFuture.isSuccess()) {
+                            Channel channel = (Channel) handshakeFuture.getChannel();
+                            SSLEngine engine = sslHandler.getEngine();
+                            SSLSession session = engine.getSession();
+
+                            LOGGER.debug("onFutureSuccess: session = {}, id = {}, isValid = {}, host = {}", session.toString(),
+                                    Base64.encode(session.getId()), session.isValid(), host);
+                            if (!hostnameVerifier.verify(host, session)) {
+                                ConnectException exception = new ConnectException("HostnameVerifier exception");
+                                future.abort(exception);
+                                throw exception;
+                            } else {
+                                future.provider().writeRequest(channel, config, future);
+                            }
+                        }
+                    }
+                });
+            } else {
+                future.provider().writeRequest(f.getChannel(), config, future);
             }
 
-            future.provider().writeRequest(f.getChannel(), config, future);
         } else {
             Throwable cause = f.getCause();
 
             boolean canRetry = future.canRetry();
-            logger.debug("Trying to recover a dead cached channel {} with a retry value of {} ", f.getChannel(), canRetry);
+            LOGGER.debug("Trying to recover a dead cached channel {} with a retry value of {} ", f.getChannel(), canRetry);
             if (canRetry && cause != null && (NettyAsyncHttpProvider.abortOnDisconnectException(cause)
                     || cause instanceof ClosedChannelException
                     || future.getState() != NettyResponseFuture.STATE.NEW)) {
 
-                logger.debug("Retrying {} ", nettyRequest);
+                LOGGER.debug("Retrying {} ", nettyRequest);
                 if (future.provider().remotelyClosed(f.getChannel(), future)) {
                     return;
                 }
             }
 
-            logger.debug("Failed to recover from exception: {} with channel {}", cause, f.getChannel());
+            LOGGER.debug("Failed to recover from exception: {} with channel {}", cause, f.getChannel());
 
             boolean printCause = f.getCause() != null && cause.getMessage() != null;
             ConnectException e = new ConnectException(printCause ? cause.getMessage() + " to " + future.getURI().toString() : future.getURI().toString());

src/test/java/com/ning/http/client/async/BasicHttpsTest.java

@@ -18,6 +18,7 @@
 import com.ning.http.client.AsyncHttpClient;
 import com.ning.http.client.AsyncHttpClientConfig.Builder;
 import com.ning.http.client.Response;
+
 import org.eclipse.jetty.server.Request;
 import org.eclipse.jetty.server.Server;
 import org.eclipse.jetty.server.handler.AbstractHandler;
@@ -34,16 +35,19 @@
 import javax.net.ssl.SSLContext;
 import javax.net.ssl.SSLHandshakeException;
 import javax.net.ssl.TrustManager;
+import javax.net.ssl.TrustManagerFactory;
 import javax.net.ssl.X509TrustManager;
 import javax.servlet.ServletException;
 import javax.servlet.http.HttpServletRequest;
 import javax.servlet.http.HttpServletResponse;
+
 import java.io.File;
 import java.io.IOException;
 import java.io.InputStream;
 import java.net.ConnectException;
 import java.net.ServerSocket;
 import java.net.URL;
+import java.security.GeneralSecurityException;
 import java.security.KeyStore;
 import java.security.SecureRandom;
 import java.security.cert.CertificateException;
@@ -207,7 +211,7 @@ public void setUpGlobal() throws Exception {
 
     @Test(groups = { "standalone", "default_provider" })
     public void zeroCopyPostTest() throws Throwable {
-        final AsyncHttpClient client = getAsyncHttpClient(new Builder().setSSLContext(createSSLContext()).build());
+        final AsyncHttpClient client = getAsyncHttpClient(new Builder().setSSLContext(createSSLContext(new AtomicBoolean(true))).build());
         try {
             ClassLoader cl = getClass().getClassLoader();
             // override system properties
@@ -226,7 +230,7 @@ public void zeroCopyPostTest() throws Throwable {
 
     @Test(groups = { "standalone", "default_provider" })
     public void multipleSSLRequestsTest() throws Throwable {
-        final AsyncHttpClient c = getAsyncHttpClient(new Builder().setSSLContext(createSSLContext()).build());
+        final AsyncHttpClient c = getAsyncHttpClient(new Builder().setSSLContext(createSSLContext(new AtomicBoolean(true))).build());
         try {
             String body = "hello there";
 
@@ -246,7 +250,7 @@ public void multipleSSLRequestsTest() throws Throwable {
 
     @Test(groups = { "standalone", "default_provider" })
     public void multipleSSLWithoutCacheTest() throws Throwable {
-        final AsyncHttpClient c = getAsyncHttpClient(new Builder().setSSLContext(createSSLContext()).setAllowSslConnectionPool(false).build());
+        final AsyncHttpClient c = getAsyncHttpClient(new Builder().setSSLContext(createSSLContext(new AtomicBoolean(true))).setAllowSslConnectionPool(false).build());
         try {
             String body = "hello there";
             c.preparePost(getTargetUrl()).setBody(body).setHeader("Content-Type", "text/html").execute();
@@ -262,55 +266,72 @@ public void multipleSSLWithoutCacheTest() throws Throwable {
     }
 
     @Test(groups = { "standalone", "default_provider" })
-    public void reconnectsAfterFailedCertificationPath() throws Throwable {
-        final AsyncHttpClient c = getAsyncHttpClient(new Builder().setSSLContext(createSSLContext()).build());
+    public void reconnectsAfterFailedCertificationPath() throws Exception {
+        
+        AtomicBoolean trust = new AtomicBoolean(false);
+        AsyncHttpClient c = getAsyncHttpClient(new Builder().setSSLContext(createSSLContext(trust)).build());
         try {
-            final String body = "hello there";
+            String body = "hello there";
 
-            TRUST_SERVER_CERT.set(false);
+            // first request fails because server certificate is rejected
+            Throwable cause = null;
             try {
-                // first request fails because server certificate is rejected
-                try {
-                    c.preparePost(getTargetUrl()).setBody(body).setHeader("Content-Type", "text/html").execute().get(TIMEOUT, TimeUnit.SECONDS);
-                } catch (final ExecutionException e) {
-                    Throwable cause = e.getCause();
-                    if (cause instanceof ConnectException) {
-                        assertNotNull(cause.getCause());
-                        assertTrue(cause.getCause() instanceof SSLHandshakeException);
-                    } else {
-                        assertTrue(cause instanceof SSLHandshakeException);
-                    }
+                c.preparePost(getTargetUrl()).setBody(body).setHeader("Content-Type", "text/html").execute().get(TIMEOUT, TimeUnit.SECONDS);
+            } catch (final ExecutionException e) {
+                cause = e.getCause();
+                if (cause instanceof ConnectException) {
+                    //assertNotNull(cause.getCause());
+                    assertTrue(cause.getCause() instanceof SSLHandshakeException, "Expected an SSLHandshakeException, got a " + cause.getCause());
+                } else {
+                   assertTrue(cause instanceof IOException, "Expected an IOException, got a " + cause);
                 }
+            } catch (Exception e) {
+                System.err.println("WTF"+ e.getMessage());
+            }
+            assertNotNull(cause);
 
-                TRUST_SERVER_CERT.set(true);
-
-                // second request should succeed
-                final Response response = c.preparePost(getTargetUrl()).setBody(body).setHeader("Content-Type", "text/html").execute().get(TIMEOUT, TimeUnit.SECONDS);
+            // second request should succeed
+            trust.set(true);
+            Response response = c.preparePost(getTargetUrl()).setBody(body).setHeader("Content-Type", "text/html").execute().get(TIMEOUT, TimeUnit.SECONDS);
 
-                assertEquals(response.getResponseBody(), body);
-            } finally {
-                TRUST_SERVER_CERT.set(true);
-            }
+            assertEquals(response.getResponseBody(), body);
         } finally {
             c.close();
         }
     }
 
-    private static SSLContext createSSLContext() {
+    private static KeyManager[] createKeyManagers() throws GeneralSecurityException, IOException {
+        InputStream keyStoreStream = Thread.currentThread().getContextClassLoader().getResourceAsStream("ssltest-cacerts.jks");
+        char[] keyStorePassword = "changeit".toCharArray();
+        KeyStore ks = KeyStore.getInstance("JKS");
+        ks.load(keyStoreStream, keyStorePassword);
+        assert(ks.size() > 0);
+
+        // Set up key manager factory to use our key store
+        char[] certificatePassword = "changeit".toCharArray();
+        KeyManagerFactory kmf = KeyManagerFactory.getInstance("SunX509");
+        kmf.init(ks, certificatePassword);
+
+        // Initialize the SSLContext to work with our key managers.
+        return kmf.getKeyManagers();
+    }
+
+    private static TrustManager[] createTrustManagers() throws GeneralSecurityException, IOException {
+        InputStream keyStoreStream = Thread.currentThread().getContextClassLoader().getResourceAsStream("ssltest-keystore.jks");
+        char[] keyStorePassword = "changeit".toCharArray();
+        KeyStore ks = KeyStore.getInstance("JKS");
+        ks.load(keyStoreStream, keyStorePassword);
+        assert(ks.size() > 0);
+
+        TrustManagerFactory tmf = TrustManagerFactory.getInstance(TrustManagerFactory.getDefaultAlgorithm());
+        tmf.init(ks);
+        return tmf.getTrustManagers();
+    }
+
+    public static SSLContext createSSLContext(AtomicBoolean trust) {
         try {
-            InputStream keyStoreStream = BasicHttpsTest.class.getResourceAsStream("ssltest-cacerts.jks");
-            char[] keyStorePassword = "changeit".toCharArray();
-            KeyStore ks = KeyStore.getInstance("JKS");
-            ks.load(keyStoreStream, keyStorePassword);
-
-            // Set up key manager factory to use our key store
-            char[] certificatePassword = "changeit".toCharArray();
-            KeyManagerFactory kmf = KeyManagerFactory.getInstance("SunX509");
-            kmf.init(ks, certificatePassword);
-
-            // Initialize the SSLContext to work with our key managers.
-            KeyManager[] keyManagers = kmf.getKeyManagers();
-            TrustManager[] trustManagers = new TrustManager[] { DUMMY_TRUST_MANAGER };
+            KeyManager[] keyManagers = createKeyManagers();
+            TrustManager[] trustManagers = new TrustManager[] { dummyTrustManager(trust, (X509TrustManager) createTrustManagers()[0]) };
             SecureRandom secureRandom = new SecureRandom();
 
             SSLContext sslContext = SSLContext.getInstance("TLS");
@@ -322,20 +343,39 @@ private static SSLContext createSSLContext() {
         }
     }
 
-    private static final AtomicBoolean TRUST_SERVER_CERT = new AtomicBoolean(true);
-    private static final TrustManager DUMMY_TRUST_MANAGER = new X509TrustManager() {
-        public X509Certificate[] getAcceptedIssuers() {
-            return new X509Certificate[0];
+    public static class DummyTrustManager implements X509TrustManager {
+
+        private final X509TrustManager tm;
+        private final AtomicBoolean trust;
+
+        public DummyTrustManager(final AtomicBoolean trust, final X509TrustManager tm) {
+            this.trust = trust;
+            this.tm = tm;
         }
 
-        public void checkClientTrusted(X509Certificate[] chain, String authType) throws CertificateException {
+        @Override
+        public void checkClientTrusted(X509Certificate[] chain, String authType)
+                throws CertificateException {
+            tm.checkClientTrusted(chain, authType);
         }
 
-        public void checkServerTrusted(X509Certificate[] chain, String authType) throws CertificateException {
-            if (!TRUST_SERVER_CERT.get()) {
+        @Override
+        public void checkServerTrusted(X509Certificate[] chain, String authType)
+                throws CertificateException {
+            if (!trust.get()) {
                 throw new CertificateException("Server certificate not trusted.");
             }
+            tm.checkServerTrusted(chain, authType);
         }
-    };
 
+        @Override
+        public X509Certificate[] getAcceptedIssuers() {
+            return tm.getAcceptedIssuers();
+        }
+    }
+
+    private static TrustManager dummyTrustManager(final AtomicBoolean trust, final X509TrustManager tm) {
+        return new DummyTrustManager(trust, tm);
+
+    }
 }

src/test/java/com/ning/http/client/async/grizzly/GrizzlyBasicHttpsTest.java

@@ -24,9 +24,4 @@ public class GrizzlyBasicHttpsTest extends BasicHttpsTest {
     public AsyncHttpClient getAsyncHttpClient(AsyncHttpClientConfig config) {
         return ProviderUtil.grizzlyProvider(config);
     }
-
-    @Override
-    public void zeroCopyPostTest() throws Throwable {
-        super.zeroCopyPostTest(); // To change body of overridden methods use File | Settings | File Templates.
-    }
 }