Handle TLS 1.3 bad cert errors in proxy handler

Author: zoewangg

.changes/next-release/bugfix-NettyNIOHTTPClient-dbb46f2.json

@@ -0,0 +1,6 @@
+{
+    "category": "Netty NIO HTTP Client", 
+    "contributor": "", 
+    "type": "bugfix", 
+    "description": "Fixed the issue where BAD_CERTIFICATE issue manifested as acquire connection timeout error when using TLS1.3 and proxy."
+}

http-clients/netty-nio-client/src/main/java/software/amazon/awssdk/http/nio/netty/internal/NettyRequestExecutor.java

@@ -22,6 +22,7 @@
 import static software.amazon.awssdk.http.nio.netty.internal.ChannelAttributeKey.LAST_HTTP_CONTENT_RECEIVED_KEY;
 import static software.amazon.awssdk.http.nio.netty.internal.ChannelAttributeKey.REQUEST_CONTEXT_KEY;
 import static software.amazon.awssdk.http.nio.netty.internal.ChannelAttributeKey.RESPONSE_COMPLETE_KEY;
+import static software.amazon.awssdk.http.nio.netty.internal.utils.NettyUtils.CLOSED_CHANNEL_MESSAGE;
 
 import io.netty.buffer.ByteBuf;
 import io.netty.buffer.Unpooled;
@@ -310,7 +311,7 @@ private Throwable decorateException(Throwable originalCause) {
         } else if (originalCause instanceof WriteTimeoutException) {
             return new IOException("Write timed out", originalCause);
         } else if (originalCause instanceof ClosedChannelException) {
-            return new IOException(getMessageForClosedChannel(), originalCause);
+            return new IOException(CLOSED_CHANNEL_MESSAGE, originalCause);
         }
 
         return originalCause;
@@ -369,12 +370,6 @@ private String getMessageForTooManyAcquireOperationsError() {
                 + "AWS, or by increasing the number of hosts sending requests.";
     }
 
-    private String getMessageForClosedChannel() {
-        return "The channel was closed. This may have been done by the client (e.g. because the request was aborted), " +
-               "by the service (e.g. because the request took too long or the client tried to write on a read-only socket), " +
-               "or by an intermediary party (e.g. because the channel was idle for too long).";
-    }
-
     /**
      * Close and release the channel back to the pool.
      *

http-clients/netty-nio-client/src/main/java/software/amazon/awssdk/http/nio/netty/internal/ProxyTunnelInitHandler.java

@@ -65,9 +65,7 @@ public void handlerAdded(ChannelHandlerContext ctx) {
         HttpRequest connectRequest = connectRequest();
         ctx.channel().writeAndFlush(connectRequest).addListener(f -> {
             if (!f.isSuccess()) {
-                ctx.close();
-                sourcePool.release(ctx.channel());
-                initPromise.setFailure(new IOException("Unable to send CONNECT request to proxy", f.cause()));
+                handleConnectRequestFailure(ctx, f.cause());
             }
         });
     }
@@ -98,6 +96,31 @@ public void channelRead(ChannelHandlerContext ctx, Object msg) {
         initPromise.setFailure(new IOException("Could not connect to proxy"));
     }
 
+    @Override
+    public void channelInactive(ChannelHandlerContext ctx) {
+        if (!initPromise.isDone()) {
+            handleConnectRequestFailure(ctx, null);
+        }
+    }
+
+    @Override
+    public void exceptionCaught(ChannelHandlerContext ctx, Throwable cause) {
+        if (!initPromise.isDone()) {
+            handleConnectRequestFailure(ctx, cause);
+        } else {
+            ctx.fireExceptionCaught(cause);
+        }
+    }
+
+    private void handleConnectRequestFailure(ChannelHandlerContext ctx, Throwable cause) {
+        ctx.close();
+        sourcePool.release(ctx.channel());
+        String errorMsg = "Unable to send CONNECT request to proxy";
+        IOException ioException = cause == null ? new IOException(errorMsg) :
+                                  new IOException(errorMsg, cause);
+        initPromise.setFailure(ioException);
+    }
+
     private HttpRequest connectRequest() {
         String uri = getUri();
         HttpRequest request = new DefaultFullHttpRequest(HttpVersion.HTTP_1_1, HttpMethod.CONNECT, uri,

http-clients/netty-nio-client/src/main/java/software/amazon/awssdk/http/nio/netty/internal/ResponseHandler.java

@@ -24,6 +24,7 @@
 import static software.amazon.awssdk.http.nio.netty.internal.ChannelAttributeKey.RESPONSE_COMPLETE_KEY;
 import static software.amazon.awssdk.http.nio.netty.internal.utils.ExceptionHandlingUtils.tryCatch;
 import static software.amazon.awssdk.http.nio.netty.internal.utils.ExceptionHandlingUtils.tryCatchFinally;
+import static software.amazon.awssdk.http.nio.netty.internal.utils.NettyUtils.CLOSED_CHANNEL_MESSAGE;
 
 import io.netty.buffer.ByteBuf;
 import io.netty.channel.Channel;
@@ -397,7 +398,7 @@ private void notifyIfResponseNotCompleted(ChannelHandlerContext handlerCtx) {
         handlerCtx.channel().attr(KEEP_ALIVE).set(false);
 
         if (!Boolean.TRUE.equals(responseCompleted) && !Boolean.TRUE.equals(lastHttpContentReceived)) {
-            IOException err = new IOException("Server failed to send complete response");
+            IOException err = new IOException("Server failed to send complete response. " + CLOSED_CHANNEL_MESSAGE);
             runAndLogError("Fail to execute SdkAsyncHttpResponseHandler#onError", () -> requestCtx.handler().onError(err));
             executeFuture(handlerCtx).completeExceptionally(err);
             runAndLogError("Could not release channel", () -> closeAndRelease(handlerCtx));

http-clients/netty-nio-client/src/main/java/software/amazon/awssdk/http/nio/netty/internal/utils/NettyUtils.java

@@ -40,6 +40,14 @@ public final class NettyUtils {
      */
     public static final SucceededFuture<?> SUCCEEDED_FUTURE = new SucceededFuture<>(null, null);
 
+    public static final String CLOSED_CHANNEL_MESSAGE = "The channel was closed. This may have been done by the client (e.g. "
+                                                        + "because the request was aborted), " +
+                                                        "by the service (e.g. because there was a handshake error[use -Djavax"
+                                                        + ".net.debug=ssl to enable SSL logs], the request "
+                                                        + "took too long, or the client tried to write on a read-only socket), " +
+                                                        "or by an intermediary party (e.g. because the channel was idle for too"
+                                                        + " long).";
+
     private static final Logger log = Logger.loggerFor(NettyUtils.class);
 
     private NettyUtils() {

http-clients/netty-nio-client/src/test/java/software/amazon/awssdk/http/nio/netty/NettyClientTlsAuthTest.java

@@ -26,16 +26,15 @@
 import com.github.tomakehurst.wiremock.WireMockServer;
 import com.github.tomakehurst.wiremock.core.WireMockConfiguration;
 import java.io.IOException;
-import org.hamcrest.CoreMatchers;
 import org.junit.After;
 import org.junit.AfterClass;
-import org.junit.Assume;
 import org.junit.BeforeClass;
 import org.junit.Rule;
 import org.junit.Test;
 import org.junit.rules.ExpectedException;
 import software.amazon.awssdk.http.EmptyPublisher;
 import software.amazon.awssdk.http.FileStoreTlsKeyManagersProvider;
+import software.amazon.awssdk.http.HttpTestUtils;
 import software.amazon.awssdk.http.SdkHttpFullRequest;
 import software.amazon.awssdk.http.SdkHttpMethod;
 import software.amazon.awssdk.http.TlsKeyManagersProvider;
@@ -139,8 +138,6 @@ public void proxyRequest_ableToAuthenticate() {
 
     @Test
     public void proxyRequest_noKeyManagerGiven_notAbleToSendConnect() throws Throwable {
-        // TODO: remove this and fix the issue with TLS1.3
-        Assume.assumeThat(System.getProperty("java.version"), CoreMatchers.startsWith("1.8"));
         thrown.expectCause(instanceOf(IOException.class));
         thrown.expectMessage("Unable to send CONNECT request to proxy");
 
@@ -173,6 +170,17 @@ public void proxyRequest_keyStoreSystemPropertiesConfigured_ableToAuthenticate()
         }
     }
 
+    @Test
+    public void nonProxy_noKeyManagerGiven_shouldThrowException() {
+        thrown.expectCause(instanceOf(IOException.class));
+        thrown.expectMessage("The channel was closed");
+
+        netty = NettyNioAsyncHttpClient.builder()
+                                       .buildWithDefaults(DEFAULTS);
+
+        HttpTestUtils.sendGetRequest(mockProxy.httpsPort(), netty).join();
+    }
+
     private void sendRequest(SdkAsyncHttpClient client, SdkAsyncHttpResponseHandler responseHandler) {
         AsyncExecuteRequest req = AsyncExecuteRequest.builder()
                 .request(testSdkRequest())

http-clients/netty-nio-client/src/test/java/software/amazon/awssdk/http/nio/netty/NettyNioAsyncHttpClientWireMockTest.java

@@ -395,15 +395,14 @@ public void builderUsesProvidedTrustManagersProvider() throws Exception {
 
             SdkHttpRequest request = createRequest(uri);
             RecordingResponseHandler recorder = new RecordingResponseHandler();
-            client.execute(AsyncExecuteRequest.builder().request(request).requestContentPublisher(createProvider("")).responseHandler(recorder).build());
+            netty.execute(AsyncExecuteRequest.builder().request(request).requestContentPublisher(createProvider("")).responseHandler(recorder).build());
 
             recorder.completeFuture.get(5, TimeUnit.SECONDS);
         } finally {
             selfSignedServer.stop();
         }
     }
 
-
     /**
      * Make a simple async request and wait for it to fiish.
      *

http-clients/netty-nio-client/src/test/java/software/amazon/awssdk/http/nio/netty/internal/ProxyTunnelInitHandlerTest.java

@@ -38,6 +38,7 @@
 import io.netty.handler.codec.http.HttpRequest;
 import io.netty.handler.codec.http.HttpResponseStatus;
 import io.netty.handler.codec.http.HttpVersion;
+import io.netty.handler.ssl.SslCloseCompletionEvent;
 import io.netty.handler.ssl.SslHandler;
 import io.netty.util.concurrent.Promise;
 import java.io.IOException;
@@ -161,6 +162,27 @@ public void requestWriteFails_failsPromise() {
         assertThat(promise.awaitUninterruptibly().isSuccess()).isFalse();
     }
 
+    @Test
+    public void channelInactive_shouldFailPromise() throws Exception {
+        Promise<Channel> promise = GROUP.next().newPromise();
+        ProxyTunnelInitHandler handler = new ProxyTunnelInitHandler(mockChannelPool, REMOTE_HOST, promise);
+        SslCloseCompletionEvent event = new SslCloseCompletionEvent(new RuntimeException(""));
+        handler.channelInactive(mockCtx);
+
+        assertThat(promise.awaitUninterruptibly().isSuccess()).isFalse();
+        verify(mockCtx).close();
+    }
+
+    @Test
+    public void unexpectedExceptionThrown_shouldFailPromise() throws Exception {
+        Promise<Channel> promise = GROUP.next().newPromise();
+        ProxyTunnelInitHandler handler = new ProxyTunnelInitHandler(mockChannelPool, REMOTE_HOST, promise);
+        handler.exceptionCaught(mockCtx, new RuntimeException("exception"));
+
+        assertThat(promise.awaitUninterruptibly().isSuccess()).isFalse();
+        verify(mockCtx).close();
+    }
+
     @Test
     public void handlerRemoved_removesCodec() {
         HttpClientCodec codec = new HttpClientCodec();