changeset 60165:38fc3785784a

8238990: java/net/httpclient/HandshakeFailureTest.java failed against TLSv1.3 on Windows Summary: The SSLTube and SSLFlowDelegate are improved to wrap any non-SSL exception that occur during the handshake in an SSLHandshakeException. Reviewed-by: chegar
author dfuchs
date Thu, 20 Feb 2020 20:04:15 +0000
parents eae299d8ada4
children e3bc1e0ec534
files src/java.net.http/share/classes/jdk/internal/net/http/common/SSLFlowDelegate.java src/java.net.http/share/classes/jdk/internal/net/http/common/SSLTube.java test/jdk/ProblemList.txt test/jdk/java/net/httpclient/HandshakeFailureTest.java
diffstat 4 files changed, 199 insertions(+), 64 deletions(-) [+]
line wrap: on
line diff
--- a/src/java.net.http/share/classes/jdk/internal/net/http/common/SSLFlowDelegate.java	Thu Feb 20 10:11:07 2020 -0800
+++ b/src/java.net.http/share/classes/jdk/internal/net/http/common/SSLFlowDelegate.java	Thu Feb 20 20:04:15 2020 +0000
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2017, 2019, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2017, 2020, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -229,6 +229,10 @@
         return SchedulingAction.CONTINUE;
     }
 
+    protected Throwable checkForHandshake(Throwable t) {
+        return t;
+    }
+
 
     /**
      * Processing function for incoming data. Pass it thru SSLEngine.unwrap().
@@ -356,6 +360,12 @@
             }
         }
 
+        @Override
+        protected boolean errorCommon(Throwable throwable) {
+            throwable = SSLFlowDelegate.this.checkForHandshake(throwable);
+            return super.errorCommon(throwable);
+        }
+
         void schedule() {
             scheduler.runOrSchedule(exec);
         }
@@ -479,8 +489,9 @@
                             }
                         }
                     } catch (IOException ex) {
-                        errorCommon(ex);
-                        handleError(ex);
+                        Throwable cause = checkForHandshake(ex);
+                        errorCommon(cause);
+                        handleError(cause);
                         return;
                     }
                     if (handshaking && !complete) {
@@ -504,6 +515,7 @@
                     requestMoreDataIfNeeded();
                 }
             } catch (Throwable ex) {
+                ex = checkForHandshake(ex);
                 errorCommon(ex);
                 handleError(ex);
             }
@@ -823,6 +835,7 @@
                     writer.addData(HS_TRIGGER);
                 }
             } catch (Throwable ex) {
+                ex = checkForHandshake(ex);
                 errorCommon(ex);
                 handleError(ex);
             }
@@ -1127,7 +1140,7 @@
                 }
                 resumeActivity();
             } catch (Throwable t) {
-                handleError(t);
+                handleError(checkForHandshake(t));
             }
         });
     }
--- a/src/java.net.http/share/classes/jdk/internal/net/http/common/SSLTube.java	Thu Feb 20 10:11:07 2020 -0800
+++ b/src/java.net.http/share/classes/jdk/internal/net/http/common/SSLTube.java	Thu Feb 20 20:04:15 2020 +0000
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2017, 2019, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2017, 2020, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -34,6 +34,7 @@
 import java.util.concurrent.atomic.AtomicReference;
 import java.util.function.Consumer;
 import javax.net.ssl.SSLEngine;
+import javax.net.ssl.SSLException;
 import javax.net.ssl.SSLHandshakeException;
 import javax.net.ssl.SSLEngineResult.HandshakeStatus;
 import jdk.internal.net.http.common.SubscriberWrapper.SchedulingAction;
@@ -147,6 +148,13 @@
             // is called.
             upstreamWriter().onSubscribe(writeSubscription);
         }
+
+        // Check whether the given exception should be wrapped
+        // in SSLHandshakeFailure exception
+        @Override
+        protected Throwable checkForHandshake(Throwable t) {
+            return SSLTube.this.checkForHandshake(t);
+        }
     }
 
     public CompletableFuture<String> getALPN() {
@@ -430,10 +438,10 @@
         private void complete(DelegateWrapper subscriberImpl, Throwable t) {
             try {
                 if (t == null) subscriberImpl.onComplete();
-                else subscriberImpl.onError(t);
+                else subscriberImpl.onError(t = checkForHandshake(t));
                 if (debug.on()) {
-                    debug.log("subscriber completed %s"
-                            + ((t == null) ? "normally" : ("with error: " + t)));
+                    debug.log("subscriber completed %s",
+                            ((t == null) ? "normally" : ("with error: " + t)));
                 }
             } finally {
                 // Error or EOF while reading:
@@ -489,7 +497,7 @@
             // onError before onSubscribe. It also prevents race conditions
             // if onError is invoked concurrently with setDelegate.
             // See setDelegate.
-
+            throwable = checkForHandshake(throwable);
             errorRef.compareAndSet(null, throwable);
             Throwable failed = errorRef.get();
             finished = true;
@@ -517,29 +525,6 @@
             onErrorImpl(throwable);
         }
 
-        private boolean handshaking() {
-            HandshakeStatus hs = engine.getHandshakeStatus();
-            return !(hs == NOT_HANDSHAKING || hs == FINISHED);
-        }
-
-        private String handshakeFailed() {
-            // sslDelegate can be null if we reach here
-            // during the initial handshake, as that happens
-            // within the SSLFlowDelegate constructor.
-            // In that case we will want to raise an exception.
-            if (handshaking()
-                    && (sslDelegate == null
-                    || !sslDelegate.closeNotifyReceived())) {
-                return "Remote host terminated the handshake";
-            }
-            // The initial handshake may not have been started yet.
-            // In which case - if we are completed before the initial handshake
-            // is started, we consider this a handshake failure as well.
-            if ("SSL_NULL_WITH_NULL_NULL".equals(engine.getSession().getCipherSuite()))
-                return "Remote host closed the channel";
-            return null;
-        }
-
         @Override
         public void onComplete() {
             assert !finished && !onCompleteReceived;
@@ -548,15 +533,9 @@
                 subscriberImpl = subscribed;
             }
 
-            String handshakeFailed = handshakeFailed();
+            Throwable handshakeFailed = checkForHandshake(null);
             if (handshakeFailed != null) {
-                if (debug.on())
-                    debug.log("handshake: %s, inbound done: %s, outbound done: %s: %s",
-                              engine.getHandshakeStatus(),
-                              engine.isInboundDone(),
-                              engine.isOutboundDone(),
-                              handshakeFailed);
-                onErrorImpl(new SSLHandshakeException(handshakeFailed));
+                onErrorImpl(handshakeFailed);
             } else if (subscriberImpl != null) {
                 onCompleteReceived = finished = true;
                 complete(subscriberImpl, null);
@@ -569,6 +548,55 @@
         }
     }
 
+    private boolean handshaking() {
+        HandshakeStatus hs = engine.getHandshakeStatus();
+        return !(hs == NOT_HANDSHAKING || hs == FINISHED);
+    }
+
+    private String handshakeFailed() {
+        // sslDelegate can be null if we reach here
+        // during the initial handshake, as that happens
+        // within the SSLFlowDelegate constructor.
+        // In that case we will want to raise an exception.
+        if (handshaking()
+                && (sslDelegate == null
+                || !sslDelegate.closeNotifyReceived())) {
+            return "Remote host terminated the handshake";
+        }
+        // The initial handshake may not have been started yet.
+        // In which case - if we are completed before the initial handshake
+        // is started, we consider this a handshake failure as well.
+        if ("SSL_NULL_WITH_NULL_NULL".equals(engine.getSession().getCipherSuite()))
+            return "Remote host closed the channel";
+        return null;
+    }
+
+    /**
+     * If the stream is completed before the handshake is finished, we want
+     * to forward an SSLHandshakeException downstream.
+     * If t is not null an exception will always be returned. If t is null an
+     * exception will be returned if the engine is handshaking.
+     * @param t an exception from upstream, or null.
+     * @return t or an SSLHandshakeException wrapping t, or null.
+     */
+    Throwable checkForHandshake(Throwable t) {
+        if (t instanceof SSLException) {
+            return t;
+        }
+        String handshakeFailed = handshakeFailed();
+        if (handshakeFailed == null) return t;
+        if (debug.on())
+            debug.log("handshake: %s, inbound done: %s, outbound done: %s: %s",
+                    engine.getHandshakeStatus(),
+                    engine.isInboundDone(),
+                    engine.isOutboundDone(),
+                    handshakeFailed);
+
+        SSLHandshakeException e = new SSLHandshakeException(handshakeFailed);
+        if (t != null) e.initCause(t);
+        return e;
+    }
+
     @Override
     public void connectFlows(TubePublisher writePub,
                              TubeSubscriber readSub) {
--- a/test/jdk/ProblemList.txt	Thu Feb 20 10:11:07 2020 -0800
+++ b/test/jdk/ProblemList.txt	Thu Feb 20 20:04:15 2020 +0000
@@ -627,7 +627,6 @@
 
 java/net/ServerSocket/AcceptInheritHandle.java                  8211854 aix-ppc64
 
-java/net/httpclient/HandshakeFailureTest.java                   8238990 windows-all
 
 ############################################################################
 
--- a/test/jdk/java/net/httpclient/HandshakeFailureTest.java	Thu Feb 20 10:11:07 2020 -0800
+++ b/test/jdk/java/net/httpclient/HandshakeFailureTest.java	Thu Feb 20 20:04:15 2020 +0000
@@ -31,13 +31,13 @@
 import java.net.InetSocketAddress;
 import java.net.ServerSocket;
 import java.net.Socket;
-import java.net.SocketException;
 import java.net.URI;
 import java.net.http.HttpClient;
 import java.net.http.HttpClient.Version;
 import java.net.http.HttpRequest;
 import java.net.http.HttpResponse;
 import java.util.List;
+import java.util.Locale;
 import java.util.concurrent.CompletableFuture;
 import java.util.concurrent.CompletionException;
 
@@ -49,8 +49,8 @@
 
 /**
  * @test
- * @run main/othervm -Djdk.internal.httpclient.debug=false HandshakeFailureTest TLSv1.2
- * @run main/othervm -Djdk.internal.httpclient.debug=false HandshakeFailureTest TLSv1.3
+ * @run main/othervm -Djdk.internal.httpclient.debug=true HandshakeFailureTest TLSv1.2
+ * @run main/othervm -Djdk.internal.httpclient.debug=true HandshakeFailureTest TLSv1.3
  * @summary Verify SSLHandshakeException is received when the handshake fails,
  * either because the server closes (EOF) the connection during handshaking,
  * or no cipher suite can be negotiated (TLSv1.2) or no available authentication
@@ -65,9 +65,56 @@
     static final int TIMES = 10;
 
     private static String tlsProtocol;
+    private static int maxWsaeConnAborted;
+
+    // On Microsoft Windows, a WSAECONNABORTED error could be raised
+    // if the client side fails to retransmit a TCP packet.
+    // This could happen if for instance, the server stops reading and
+    // close the socket while the client is still trying to push
+    // data through.
+    // With TLSv1.3, and our dummy SSLServer implementation below,
+    // this can occur quite often.
+    // Our HTTP stack should automatically wrap such exceptions
+    // in SSLHandshakeException if they are raised while the handshake
+    // in progress. So it would be an error to receive WSAECONNABORTED
+    // here. This test has some special code to handle WSAECONNABORTED
+    // and fail if they reach the test code.
+    public static final String WSAECONNABORTED_MSG =
+            "An established connection was aborted by the software in your host machine";
+    public static final boolean isWindows = System.getProperty("os.name", "")
+            .toLowerCase(Locale.ROOT).contains("win");
+    public enum ExpectedExceptionType {
+        HANDSHAKE_FAILURE,
+        WSAECONNABORTED
+    }
+
+    // The exception checker is used to record how many WSAECONNABORTED
+    // have reached the test code. There should be none:
+    // (usually max should be 0)
+    static final class ExceptionChecker {
+        int count;
+        Throwable aborted = null;
+        public void check(Throwable expected) {
+            if (ExpectedExceptionType.WSAECONNABORTED == checkExceptionOrCause(expected)) {
+                count++;
+                aborted = expected;
+            }
+        }
+        public void check(int max) {
+            if (count > max) {
+                out.println("WSAECONNABORTED received too many times: " + count);
+                aborted.printStackTrace(out);
+                throw new AssertionError("WSAECONNABORTED received too many times: " + count, aborted);
+            }
+        }
+    }
 
     public static void main(String[] args) throws Exception {
         tlsProtocol = args[0];
+        // At this time, all WSAECONNABORTED exception raised during
+        // the handshake should have been wrapped in SSLHandshakeException,
+        // so we allow none to reach here.
+        maxWsaeConnAborted = 0;
 
         HandshakeFailureTest test = new HandshakeFailureTest();
         List<AbstractServer> servers = List.of(new PlainServer(), new SSLServer());
@@ -75,7 +122,7 @@
         for (AbstractServer server : servers) {
             try (server) {
                 out.format("%n%n------ Testing with server:%s ------%n", server);
-                URI uri = new URI("https://localhost:" + server.getPort() + "/");
+                URI uri = new URI("https://" + server.getAuthority() + "/");
 
                 test.testSyncSameClient(uri, Version.HTTP_1_1);
                 test.testSyncSameClient(uri, Version.HTTP_2);
@@ -101,6 +148,7 @@
     void testSyncSameClient(URI uri, Version version) throws Exception {
         out.printf("%n--- testSyncSameClient %s ---%n", version);
         HttpClient client = getClient();
+        ExceptionChecker exceptionChecker = new ExceptionChecker();
         for (int i = 0; i < TIMES; i++) {
             out.printf("iteration %d%n", i);
             HttpRequest request = HttpRequest.newBuilder(uri)
@@ -112,13 +160,15 @@
                 throw new RuntimeException(msg);
             } catch (IOException expected) {
                 out.printf("Client: caught expected exception: %s%n", expected);
-                checkExceptionOrCause(expected);
+                exceptionChecker.check(expected);
             }
         }
+        exceptionChecker.check(maxWsaeConnAborted);
     }
 
     void testSyncDiffClient(URI uri, Version version) throws Exception {
         out.printf("%n--- testSyncDiffClient %s ---%n", version);
+        ExceptionChecker exceptionChecker = new ExceptionChecker();
         for (int i = 0; i < TIMES; i++) {
             out.printf("iteration %d%n", i);
             // a new client each time
@@ -132,14 +182,16 @@
                 throw new RuntimeException(msg);
             } catch (IOException expected) {
                 out.printf("Client: caught expected exception: %s%n", expected);
-                checkExceptionOrCause(expected);
+                exceptionChecker.check(expected);
             }
         }
+        exceptionChecker.check(maxWsaeConnAborted);
     }
 
     void testAsyncSameClient(URI uri, Version version) throws Exception {
         out.printf("%n--- testAsyncSameClient %s ---%n", version);
         HttpClient client = getClient();
+        ExceptionChecker exceptionChecker = new ExceptionChecker();
         for (int i = 0; i < TIMES; i++) {
             out.printf("iteration %d%n", i);
             HttpRequest request = HttpRequest.newBuilder(uri)
@@ -154,13 +206,15 @@
             } catch (CompletionException ce) {
                 Throwable expected = ce.getCause();
                 out.printf("Client: caught expected exception: %s%n", expected);
-                checkExceptionOrCause(expected);
+                exceptionChecker.check(expected);
             }
         }
+        exceptionChecker.check(maxWsaeConnAborted);
     }
 
     void testAsyncDiffClient(URI uri, Version version) throws Exception {
         out.printf("%n--- testAsyncDiffClient %s ---%n", version);
+        ExceptionChecker exceptionChecker = new ExceptionChecker();
         for (int i = 0; i < TIMES; i++) {
             out.printf("iteration %d%n", i);
             // a new client each time
@@ -178,27 +232,39 @@
                 ce.printStackTrace(out);
                 Throwable expected = ce.getCause();
                 out.printf("Client: caught expected exception: %s%n", expected);
-                checkExceptionOrCause(expected);
+                exceptionChecker.check(expected);
             }
         }
+        exceptionChecker.check(maxWsaeConnAborted);
     }
 
-    static void checkExceptionOrCause(Throwable t) {
+    // Tells whether this exception was raised from a WSAECONNABORTED
+    // error raised in the native code.
+    static boolean isWsaeConnAborted(Throwable t) {
+        return t instanceof IOException && WSAECONNABORTED_MSG.equalsIgnoreCase(t.getMessage());
+    }
+
+    // We might allow some spurious WSAECONNABORTED exception.
+    // The decision whether to allow such errors or not is taken by
+    // the ExceptionChecker
+    static ExpectedExceptionType checkExceptionOrCause(Throwable t) {
         final Throwable original = t;
         do {
-            if (SSLHandshakeException.class.isInstance(t)
-                    // For TLSv1.3, possibly the server is (being) closed when
-                    // the client read the input alert. In this case, the client
-                    // just gets SocketException instead of SSLHandshakeException.
-                    || (tlsProtocol.equalsIgnoreCase("TLSv1.3")
-                            && SocketException.class.isInstance(t))) {
+            if (SSLHandshakeException.class.isInstance(t)) {
+                // For TLSv1.3, possibly the server is (being) closed when
+                // the client read the input alert. In this case, the client
+                // just gets SocketException instead of SSLHandshakeException.
                 System.out.println("Found expected exception/cause: " + t);
-                return; // found
+                return ExpectedExceptionType.HANDSHAKE_FAILURE;
+            }
+            if (isWindows && isWsaeConnAborted(t)) {
+                System.out.println("Found WSAECONNABORTED: " + t);
+                return ExpectedExceptionType.WSAECONNABORTED;
             }
         } while ((t = t.getCause()) != null);
         original.printStackTrace(System.out);
         throw new RuntimeException(
-                "Not found expected SSLHandshakeException or SocketException in "
+                "Not found expected SSLHandshakeException in "
                         + original);
     }
 
@@ -217,6 +283,12 @@
 
         int getPort() { return ss.getLocalPort(); }
 
+        String getAuthority() {
+            String address = ss.getInetAddress().getHostAddress();
+            if (address.contains(":")) address = "[" + address + "]";
+            return address + ":" + ss.getLocalPort();
+        }
+
         @Override
         public void close() {
             if (closed)
@@ -277,10 +349,24 @@
                     Thread.sleep(10 * (count % 10));
                     s.close(); // close without giving any reply
                 } catch (IOException e) {
-                    if (!closed)
-                        out.println("Unexpected" + e);
+                    if (!closed) {
+                        out.println("PlainServer: unexpected " + e);
+                        e.printStackTrace(out);
+                    }
                 } catch (InterruptedException e) {
-                    throw new RuntimeException(e);
+                    if (!closed) {
+                        out.println("PlainServer: unexpected " + e);
+                        e.printStackTrace(out);
+                        throw new RuntimeException(e);
+                    }
+                    break;
+                } catch (Error | RuntimeException e) {
+                    if (!closed) {
+                        out.println("PlainServer: unexpected " + e);
+                        e.printStackTrace(out);
+                        throw new RuntimeException(e);
+                    }
+                    break;
                 }
             }
         }
@@ -317,10 +403,19 @@
                 } catch (SSLHandshakeException expected) {
                     // Expected: SSLHandshakeException: no cipher suites in common (TLSv1.2)
                     // or no available authentication scheme (TLSv1.3)
-                    out.printf("Server: caught expected exception: %s%n", expected);
+                    out.printf("SSLServer: caught expected exception: %s%n", expected);
                 } catch (IOException e) {
-                    if (!closed)
-                        out.printf("UNEXPECTED %s", e);
+                    if (!closed) {
+                        out.println("SSLServer: unexpected " + e);
+                        e.printStackTrace(out);
+                    }
+                } catch (Error | RuntimeException e) {
+                    if (!closed) {
+                        out.println("SSLServer: unexpected " + e);
+                        e.printStackTrace(out);
+                        throw new RuntimeException(e);
+                    }
+                    break;
                 }
             }
         }