410246 - HttpClient with proxy does not tunnel HTTPS requests.

Modified HttpClient to tunnel properly requests, and ported tests from Jetty 7 to test this behavior.

jetty-client/src/main/java/org/eclipse/jetty/client/HttpClient.java

@@ -937,6 +937,33 @@ public class HttpClient extends ContainerLifeCycle
         dump(out, indent, getBeans(), destinations.values());
     }
 
+    protected Connection tunnel(Connection connection)
+    {
+        HttpConnection httpConnection = (HttpConnection)connection;
+        HttpDestination destination = httpConnection.getDestination();
+        SslConnection sslConnection = createSslConnection(destination, httpConnection.getEndPoint());
+        Connection result = (Connection)sslConnection.getDecryptedEndPoint().getConnection();
+        selectorManager.connectionClosed(httpConnection);
+        selectorManager.connectionOpened(sslConnection);
+        LOG.debug("Tunnelled {} over {}", connection, result);
+        return result;
+    }
+
+    private SslConnection createSslConnection(HttpDestination destination, EndPoint endPoint)
+    {
+        SSLEngine engine = sslContextFactory.newSSLEngine(destination.getHost(), destination.getPort());
+        engine.setUseClientMode(true);
+
+        SslConnection sslConnection = newSslConnection(HttpClient.this, endPoint, engine);
+        sslConnection.setRenegotiationAllowed(sslContextFactory.isRenegotiationAllowed());
+        endPoint.setConnection(sslConnection);
+        EndPoint appEndPoint = sslConnection.getDecryptedEndPoint();
+        HttpConnection connection = newHttpConnection(this, appEndPoint, destination);
+        appEndPoint.setConnection(connection);
+
+        return sslConnection;
+    }
+
     protected class ClientSelectorManager extends SelectorManager
     {
         public ClientSelectorManager(Executor executor, Scheduler scheduler)
@@ -962,7 +989,7 @@ public class HttpClient extends ContainerLifeCycle
             HttpDestination destination = callback.destination;
 
             SslContextFactory sslContextFactory = getSslContextFactory();
-            if (HttpScheme.HTTPS.is(destination.getScheme()))
+            if (!destination.isProxied() && HttpScheme.HTTPS.is(destination.getScheme()))
             {
                 if (sslContextFactory == null)
                 {
@@ -972,17 +999,8 @@ public class HttpClient extends ContainerLifeCycle
                 }
                 else
                 {
-                    SSLEngine engine = sslContextFactory.newSSLEngine(destination.getHost(), destination.getPort());
+                    SslConnection sslConnection = createSslConnection(destination, endPoint);
-                    engine.setUseClientMode(true);
+                    callback.succeeded((Connection)sslConnection.getDecryptedEndPoint().getConnection());
-
-                    SslConnection sslConnection = newSslConnection(HttpClient.this, endPoint, engine);
-                    sslConnection.setRenegotiationAllowed(sslContextFactory.isRenegotiationAllowed());
-                    EndPoint appEndPoint = sslConnection.getDecryptedEndPoint();
-                    HttpConnection connection = newHttpConnection(HttpClient.this, appEndPoint, destination);
-
-                    appEndPoint.setConnection(connection);
-                    callback.succeeded(connection);
-
                     return sslConnection;
                 }
             }

jetty-client/src/main/java/org/eclipse/jetty/client/HttpConnection.java

@@ -53,6 +53,7 @@ public class HttpConnection extends AbstractConnection implements Connection
     private final HttpSender sender;
     private final HttpReceiver receiver;
     private long idleTimeout;
+    private volatile boolean closed;
 
     public HttpConnection(HttpClient client, EndPoint endPoint, HttpDestination destination)
     {
@@ -80,6 +81,24 @@ public class HttpConnection extends AbstractConnection implements Connection
         fillInterested();
     }
 
+    @Override
+    public void onClose()
+    {
+        closed = true;
+        super.onClose();
+    }
+
+    @Override
+    public void fillInterested()
+    {
+        // This is necessary when "upgrading" the connection for example after proxied
+        // CONNECT requests, because the old connection will read the CONNECT response
+        // and then set the read interest, while the new connection attached to the same
+        // EndPoint also will set the read interest, causing a ReadPendingException.
+        if (!closed)
+            super.fillInterested();
+    }
+
     @Override
     protected boolean onReadTimeout()
     {

jetty-client/src/main/java/org/eclipse/jetty/client/HttpDestination.java

@@ -46,6 +46,7 @@ import org.eclipse.jetty.util.component.ContainerLifeCycle;
 import org.eclipse.jetty.util.component.Dumpable;
 import org.eclipse.jetty.util.log.Log;
 import org.eclipse.jetty.util.log.Logger;
+import org.eclipse.jetty.util.ssl.SslContextFactory;
 
 public class HttpDestination implements Destination, Closeable, Dumpable
 {
@@ -474,13 +475,23 @@ public class HttpDestination implements Destination, Closeable, Dumpable
         @Override
         public void succeeded(Connection connection)
         {
-            boolean tunnel = isProxied() &&
+            if (isProxied() && HttpScheme.HTTPS.is(getScheme()))
-                    HttpScheme.HTTPS.is(getScheme()) &&
+            {
-                    client.getSslContextFactory() != null;
+                if (client.getSslContextFactory() != null)
-            if (tunnel)
+                {
-                tunnel(connection);
+                    tunnel(connection);
+                }
+                else
+                {
+                    String message = String.format("Cannot perform requests over SSL, no %s in %s",
+                            SslContextFactory.class.getSimpleName(), HttpClient.class.getSimpleName());
+                    delegate.failed(new IllegalStateException(message));
+                }
+            }
             else
+            {
                 delegate.succeeded(connection);
+            }
         }
 
         @Override
@@ -513,7 +524,9 @@ public class HttpDestination implements Destination, Closeable, Dumpable
                         Response response = result.getResponse();
                         if (response.getStatus() == 200)
                         {
-                            delegate.succeeded(connection);
+                            // Wrap the connection with TLS
+                            Connection tunnel = client.tunnel(connection);
+                            delegate.succeeded(tunnel);
                         }
                         else
                         {

jetty-client/src/main/java/org/eclipse/jetty/client/HttpRequest.java

@@ -147,7 +147,12 @@ public class HttpRequest implements Request
     public Request path(String path)
     {
         URI uri = URI.create(path);
-        this.path = uri.getRawPath();
+        String rawPath = uri.getRawPath();
+        if (uri.isOpaque())
+            rawPath = path;
+        if (rawPath == null)
+            rawPath = "";
+        this.path = rawPath;
         String query = uri.getRawQuery();
         if (query != null)
         {
@@ -561,7 +566,7 @@ public class HttpRequest implements Request
         if (query != null && withQuery)
             path += "?" + query;
         URI result = URI.create(path);
-        if (!result.isAbsolute())
+        if (!result.isAbsolute() && !result.isOpaque())
             result = URI.create(client.address(getScheme(), getHost(), getPort()) + path);
         return result;
     }

jetty-client/src/test/java/org/eclipse/jetty/client/HttpClientAuthenticationTest.java

@@ -316,7 +316,7 @@ public class HttpClientAuthenticationTest extends AbstractHttpClientServerTest
         authenticationStore.addAuthentication(authentication);
 
         Request request = client.newRequest("localhost", connector.getLocalPort()).scheme(scheme).path("/secure");
-        ContentResponse response = request.timeout(555, TimeUnit.SECONDS).send();
+        ContentResponse response = request.timeout(5, TimeUnit.SECONDS).send();
         Assert.assertNotNull(response);
         Assert.assertEquals(401, response.getStatus());
     }

jetty-client/src/test/java/org/eclipse/jetty/client/HttpClientProxyTest.java

@@ -119,7 +119,7 @@ public class HttpClientProxyTest extends AbstractHttpClientServerTest
 
         ContentResponse response1 = client.newRequest(serverHost, serverPort)
                 .scheme(scheme)
-                .timeout(555, TimeUnit.SECONDS)
+                .timeout(5, TimeUnit.SECONDS)
                 .send();
 
         // No Authentication available => 407
@@ -140,7 +140,7 @@ public class HttpClientProxyTest extends AbstractHttpClientServerTest
         // ...and perform the request again => 407 + 204
         ContentResponse response2 = client.newRequest(serverHost, serverPort)
                 .scheme(scheme)
-                .timeout(555, TimeUnit.SECONDS)
+                .timeout(5, TimeUnit.SECONDS)
                 .send();
 
         Assert.assertEquals(status, response2.getStatus());
@@ -150,7 +150,7 @@ public class HttpClientProxyTest extends AbstractHttpClientServerTest
         requests.set(0);
         ContentResponse response3 = client.newRequest(serverHost, serverPort)
                 .scheme(scheme)
-                .timeout(555, TimeUnit.SECONDS)
+                .timeout(5, TimeUnit.SECONDS)
                 .send();
 
         Assert.assertEquals(status, response3.getStatus());

jetty-client/src/test/java/org/eclipse/jetty/client/HttpClientTest.java

@@ -444,6 +444,7 @@ public class HttpClientTest extends AbstractHttpClientServerTest
         final CountDownLatch latch = new CountDownLatch(3);
         client.newRequest("localhost", connector.getLocalPort())
                 .scheme(scheme)
+                .path("/one")
                 .listener(new Request.Listener.Empty()
                 {
                     @Override
@@ -477,6 +478,7 @@ public class HttpClientTest extends AbstractHttpClientServerTest
 
         client.newRequest("localhost", connector.getLocalPort())
                 .scheme(scheme)
+                .path("/two")
                 .onResponseSuccess(new Response.SuccessListener()
                 {
                     @Override

jetty-proxy/src/test/java/org/eclipse/jetty/proxy/ProxyTunnellingTest.java

@@ -0,0 +1,307 @@
+//
+//  ========================================================================
+//  Copyright (c) 1995-2013 Mort Bay Consulting Pty. Ltd.
+//  ------------------------------------------------------------------------
+//  All rights reserved. This program and the accompanying materials
+//  are made available under the terms of the Eclipse Public License v1.0
+//  and Apache License v2.0 which accompanies this distribution.
+//
+//      The Eclipse Public License is available at
+//      http://www.eclipse.org/legal/epl-v10.html
+//
+//      The Apache License v2.0 is available at
+//      http://www.opensource.org/licenses/apache2.0.php
+//
+//  You may elect to redistribute this code under either of these licenses.
+//  ========================================================================
+//
+
+package org.eclipse.jetty.proxy;
+
+import java.io.IOException;
+import java.net.ConnectException;
+import java.net.URLEncoder;
+import java.util.concurrent.ExecutionException;
+import javax.servlet.ServletException;
+import javax.servlet.ServletOutputStream;
+import javax.servlet.http.HttpServletRequest;
+import javax.servlet.http.HttpServletResponse;
+
+import org.eclipse.jetty.client.HttpClient;
+import org.eclipse.jetty.client.api.ContentResponse;
+import org.eclipse.jetty.client.api.ProxyConfiguration;
+import org.eclipse.jetty.client.util.StringContentProvider;
+import org.eclipse.jetty.http.HttpHeader;
+import org.eclipse.jetty.http.HttpMethod;
+import org.eclipse.jetty.http.HttpScheme;
+import org.eclipse.jetty.http.MimeTypes;
+import org.eclipse.jetty.server.Handler;
+import org.eclipse.jetty.server.HttpConnection;
+import org.eclipse.jetty.server.Request;
+import org.eclipse.jetty.server.Server;
+import org.eclipse.jetty.server.ServerConnector;
+import org.eclipse.jetty.server.handler.AbstractHandler;
+import org.eclipse.jetty.toolchain.test.MavenTestingUtils;
+import org.eclipse.jetty.toolchain.test.TestTracker;
+import org.eclipse.jetty.util.ssl.SslContextFactory;
+import org.hamcrest.Matchers;
+import org.junit.After;
+import org.junit.Assert;
+import org.junit.Rule;
+import org.junit.Test;
+
+import static org.junit.Assert.assertEquals;
+
+public class ProxyTunnellingTest
+{
+    @Rule
+    public TestTracker tracker = new TestTracker();
+
+    private SslContextFactory sslContextFactory;
+    private Server server;
+    private ServerConnector serverConnector;
+    private Server proxy;
+    private ServerConnector proxyConnector;
+
+    protected int proxyPort()
+    {
+        return proxyConnector.getLocalPort();
+    }
+
+    protected void startSSLServer(Handler handler) throws Exception
+    {
+        sslContextFactory = new SslContextFactory();
+        String keyStorePath = MavenTestingUtils.getTestResourceFile("keystore").getAbsolutePath();
+        sslContextFactory.setKeyStorePath(keyStorePath);
+        sslContextFactory.setKeyStorePassword("storepwd");
+        sslContextFactory.setKeyManagerPassword("keypwd");
+        server = new Server();
+        serverConnector = new ServerConnector(server, sslContextFactory);
+        server.addConnector(serverConnector);
+        server.setHandler(handler);
+        server.start();
+    }
+
+    protected void startProxy() throws Exception
+    {
+        startProxy(new ConnectHandler());
+    }
+
+    protected void startProxy(ConnectHandler connectHandler) throws Exception
+    {
+        proxy = new Server();
+        proxyConnector = new ServerConnector(proxy);
+        proxy.addConnector(proxyConnector);
+        // Under Windows, it takes a while to detect that a connection
+        // attempt fails, so use an explicit timeout
+        connectHandler.setConnectTimeout(1000);
+        proxy.setHandler(connectHandler);
+        proxy.start();
+    }
+
+    @After
+    public void stop() throws Exception
+    {
+        stopProxy();
+        stopServer();
+    }
+
+    protected void stopServer() throws Exception
+    {
+        server.stop();
+        server.join();
+    }
+
+    protected void stopProxy() throws Exception
+    {
+        proxy.stop();
+        proxy.join();
+    }
+
+    @Test
+    public void testOneMessageSSL() throws Exception
+    {
+        startSSLServer(new ServerHandler());
+        startProxy();
+
+        HttpClient httpClient = new HttpClient(sslContextFactory);
+        httpClient.setProxyConfiguration(new ProxyConfiguration("localhost", proxyPort()));
+        httpClient.start();
+
+        try
+        {
+            String body = "BODY";
+            ContentResponse response = httpClient.newRequest("localhost", serverConnector.getLocalPort())
+                    .scheme(HttpScheme.HTTPS.asString())
+                    .method(HttpMethod.GET)
+                    .path("/echo?body=" + URLEncoder.encode(body, "UTF-8"))
+                    .send();
+
+            String content = response.getContentAsString();
+            assertEquals(body, content);
+        }
+        finally
+        {
+            httpClient.stop();
+        }
+    }
+
+    @Test
+    public void testTwoMessagesSSL() throws Exception
+    {
+        startSSLServer(new ServerHandler());
+        startProxy();
+
+        HttpClient httpClient = new HttpClient(sslContextFactory);
+        httpClient.setProxyConfiguration(new ProxyConfiguration("localhost", proxyPort()));
+        httpClient.start();
+
+        try
+        {
+            String body = "BODY";
+            ContentResponse response1 = httpClient.newRequest("localhost", serverConnector.getLocalPort())
+                    .scheme(HttpScheme.HTTPS.asString())
+                    .method(HttpMethod.GET)
+                    .path("/echo?body=" + URLEncoder.encode(body, "UTF-8"))
+                    .send();
+
+            String content = response1.getContentAsString();
+            assertEquals(body, content);
+
+            content = "body=" + body;
+            ContentResponse response2 = httpClient.newRequest("localhost", serverConnector.getLocalPort())
+                    .scheme(HttpScheme.HTTPS.asString())
+                    .method(HttpMethod.POST)
+                    .path("/echo")
+                    .header(HttpHeader.CONTENT_TYPE, MimeTypes.Type.FORM_ENCODED.asString())
+                    .header(HttpHeader.CONTENT_LENGTH, String.valueOf(content.length()))
+                    .content(new StringContentProvider(content))
+                    .send();
+
+            content = response2.getContentAsString();
+            assertEquals(body, content);
+        }
+        finally
+        {
+            httpClient.stop();
+        }
+    }
+
+    @Test
+    public void testProxyDown() throws Exception
+    {
+        startSSLServer(new ServerHandler());
+        startProxy();
+        int proxyPort = proxyPort();
+        stopProxy();
+
+        HttpClient httpClient = new HttpClient(sslContextFactory);
+        httpClient.setProxyConfiguration(new ProxyConfiguration("localhost", proxyPort));
+        httpClient.start();
+
+        try
+        {
+            String body = "BODY";
+            httpClient.newRequest("localhost", serverConnector.getLocalPort())
+                    .scheme(HttpScheme.HTTPS.asString())
+                    .method(HttpMethod.GET)
+                    .path("/echo?body=" + URLEncoder.encode(body, "UTF-8"))
+                    .send();
+            Assert.fail();
+        }
+        catch (ExecutionException x)
+        {
+            Assert.assertThat(x.getCause(), Matchers.instanceOf(ConnectException.class));
+        }
+        finally
+        {
+            httpClient.stop();
+        }
+    }
+
+    @Test
+    public void testServerDown() throws Exception
+    {
+        startSSLServer(new ServerHandler());
+        int serverPort = serverConnector.getLocalPort();
+        stopServer();
+        startProxy();
+
+        HttpClient httpClient = new HttpClient(sslContextFactory);
+        httpClient.setProxyConfiguration(new ProxyConfiguration("localhost", proxyPort()));
+        httpClient.start();
+
+        try
+        {
+            String body = "BODY";
+            httpClient.newRequest("localhost", serverPort)
+                    .scheme(HttpScheme.HTTPS.asString())
+                    .method(HttpMethod.GET)
+                    .path("/echo?body=" + URLEncoder.encode(body, "UTF-8"))
+                    .send();
+            Assert.fail();
+        }
+        catch (ExecutionException x)
+        {
+            // Expected
+        }
+        finally
+        {
+            httpClient.stop();
+        }
+    }
+
+    @Test
+    public void testProxyClosesConnection() throws Exception
+    {
+        startSSLServer(new ServerHandler());
+        startProxy(new ConnectHandler()
+        {
+            @Override
+            protected void handleConnect(Request jettyRequest, HttpServletRequest request, HttpServletResponse response, String serverAddress)
+            {
+                HttpConnection.getCurrentConnection().close();
+            }
+        });
+
+        HttpClient httpClient = new HttpClient(sslContextFactory);
+        httpClient.setProxyConfiguration(new ProxyConfiguration("localhost", proxyPort()));
+        httpClient.start();
+
+        try
+        {
+            httpClient.newRequest("localhost", serverConnector.getLocalPort())
+                    .scheme(HttpScheme.HTTPS.asString())
+                    .send();
+            Assert.fail();
+        }
+        catch (ExecutionException x)
+        {
+            // Expected
+        }
+        finally
+        {
+            httpClient.stop();
+        }
+    }
+
+    private static class ServerHandler extends AbstractHandler
+    {
+        public void handle(String target, Request request, HttpServletRequest httpRequest, HttpServletResponse httpResponse) throws IOException, ServletException
+        {
+            request.setHandled(true);
+
+            String uri = httpRequest.getRequestURI();
+            if ("/echo".equals(uri))
+            {
+                String body = httpRequest.getParameter("body");
+                ServletOutputStream output = httpResponse.getOutputStream();
+                output.print(body);
+            }
+            else
+            {
+                throw new ServletException();
+            }
+        }
+    }
+}

jetty-proxy/src/test/resources/jetty-logging.properties

@@ -0,0 +1,3 @@
+org.eclipse.jetty.util.log.class=org.eclipse.jetty.util.log.StdErrLog
+#org.eclipse.jetty.LEVEL=DEBUG
+#org.eclipse.jetty.client.LEVEL=DEBUG