From 35bff1f16261264daebb01042ac689b55dc2f14e Mon Sep 17 00:00:00 2001 From: Tim Brooks Date: Tue, 17 Jul 2018 17:44:04 -0600 Subject: [PATCH 1/3] Fix all tests --- .../security/transport/nio/SSLDriver.java | 19 ++++++-- .../transport/nio/SSLDriverTests.java | 43 +++++++++++++++---- 2 files changed, 49 insertions(+), 13 deletions(-) diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/transport/nio/SSLDriver.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/transport/nio/SSLDriver.java index 382230684c77f..15782a8993f17 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/transport/nio/SSLDriver.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/transport/nio/SSLDriver.java @@ -349,8 +349,11 @@ private void handshake() throws SSLException { if (hasFlushPending() == false) { handshakeStatus = wrap(EMPTY_BUFFER_ARRAY).getHandshakeStatus(); } - continueHandshaking = false; - break; + // If we need NEED_TASK we should run the tasks immediately + if (handshakeStatus != SSLEngineResult.HandshakeStatus.NEED_TASK) { + continueHandshaking = false; + break; + } case NEED_TASK: runTasks(); handshakeStatus = engine.getHandshakeStatus(); @@ -433,7 +436,15 @@ private void runTasks() { private void maybeFinishHandshake() { // We only acknowledge that we are done handshaking if there are no bytes that need to be written - if (hasFlushPending() == false) { + if (engine.isOutboundDone() || engine.isInboundDone()) { + if (currentMode.isHandshake()) { + currentMode = new CloseMode(true); + } else { + String message = "Expected to be in handshaking mode. Instead in non-handshaking mode: " + currentMode; + throw new AssertionError(message); + } + + } else if (hasFlushPending() == false) { if (currentMode.isHandshake()) { currentMode = new ApplicationMode(); } else { @@ -510,7 +521,7 @@ private CloseMode(boolean isHandshaking) { if (isHandshaking && engine.isInboundDone() == false) { // If we attempt to close during a handshake either we are sending an alert and inbound // should already be closed or we are sending a close_notify. If we send a close_notify - // the peer will send an handshake error alert. If we attempt to receive the handshake alert, + // the peer might send an handshake error alert. If we attempt to receive the handshake alert, // the engine will throw an IllegalStateException as it is not in a proper state to receive // handshake message. Closing inbound immediately after close_notify is the cleanest option. needToReceiveClose = false; diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/transport/nio/SSLDriverTests.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/transport/nio/SSLDriverTests.java index e1e05032014ba..303ed92130aaf 100644 --- a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/transport/nio/SSLDriverTests.java +++ b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/transport/nio/SSLDriverTests.java @@ -57,8 +57,15 @@ public void testPingPongAndClose() throws Exception { public void testRenegotiate() throws Exception { SSLContext sslContext = getSSLContext(); - SSLDriver clientDriver = getDriver(sslContext.createSSLEngine(), true); - SSLDriver serverDriver = getDriver(sslContext.createSSLEngine(), false); + SSLEngine serverEngine = sslContext.createSSLEngine(); + SSLEngine clientEngine = sslContext.createSSLEngine(); + + String[] serverProtocols = {"TLSv1.2"}; + serverEngine.setEnabledProtocols(serverProtocols); + String[] clientProtocols = {"TLSv1.2"}; + clientEngine.setEnabledProtocols(clientProtocols); + SSLDriver clientDriver = getDriver(clientEngine, true); + SSLDriver serverDriver = getDriver(serverEngine, false); handshake(clientDriver, serverDriver); @@ -119,16 +126,27 @@ public void testHandshakeFailureBecauseProtocolMismatch() throws Exception { SSLContext sslContext = getSSLContext(); SSLEngine clientEngine = sslContext.createSSLEngine(); SSLEngine serverEngine = sslContext.createSSLEngine(); - String[] serverProtocols = {"TLSv1.1", "TLSv1.2"}; + String[] serverProtocols = {"TLSv1.2"}; serverEngine.setEnabledProtocols(serverProtocols); - String[] clientProtocols = {"TLSv1"}; + String[] clientProtocols = {"TLSv1.1"}; clientEngine.setEnabledProtocols(clientProtocols); SSLDriver clientDriver = getDriver(clientEngine, true); SSLDriver serverDriver = getDriver(serverEngine, false); SSLException sslException = expectThrows(SSLException.class, () -> handshake(clientDriver, serverDriver)); - assertEquals("Client requested protocol TLSv1 not enabled or not supported", sslException.getMessage()); - failedCloseAlert(serverDriver, clientDriver); + String oldExpected = "Client requested protocol TLSv1.1 not enabled or not supported"; + String jdk11Expected = "Received fatal alert: protocol_version"; + boolean expectedMessage = oldExpected.equals(sslException.getMessage()) || jdk11Expected.equals(sslException.getMessage()); + assertTrue("Unexpected exception message: " + sslException.getMessage(), expectedMessage); + + // In JDK11 we need an non-application write + if (serverDriver.needsNonApplicationWrite()) { + serverDriver.nonApplicationWrite(); + } + // Prior to JDK11 we still need to send a close alert + if (serverDriver.isClosed() == false) { + failedCloseAlert(serverDriver, clientDriver); + } } public void testHandshakeFailureBecauseNoCiphers() throws Exception { @@ -144,11 +162,18 @@ public void testHandshakeFailureBecauseNoCiphers() throws Exception { SSLDriver clientDriver = getDriver(clientEngine, true); SSLDriver serverDriver = getDriver(serverEngine, false); - SSLException sslException = expectThrows(SSLException.class, () -> handshake(clientDriver, serverDriver)); - assertEquals("no cipher suites in common", sslException.getMessage()); - failedCloseAlert(serverDriver, clientDriver); + expectThrows(SSLException.class, () -> handshake(clientDriver, serverDriver)); + // In JDK11 we need an non-application write + if (serverDriver.needsNonApplicationWrite()) { + serverDriver.nonApplicationWrite(); + } + // Prior to JDK11 we still need to send a close alert + if (serverDriver.isClosed() == false) { + failedCloseAlert(serverDriver, clientDriver); + } } + @AwaitsFix(bugUrl = "https://github.com/elastic/elasticsearch/issues/32144") public void testCloseDuringHandshake() throws Exception { SSLContext sslContext = getSSLContext(); SSLDriver clientDriver = getDriver(sslContext.createSSLEngine(), true); From 7cee171c9b3681903fd5f68ef30c866269020795 Mon Sep 17 00:00:00 2001 From: Tim Brooks Date: Tue, 17 Jul 2018 17:50:16 -0600 Subject: [PATCH 2/3] Fix comments --- .../elasticsearch/xpack/security/transport/nio/SSLDriver.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/transport/nio/SSLDriver.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/transport/nio/SSLDriver.java index 15782a8993f17..2f44daa850476 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/transport/nio/SSLDriver.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/transport/nio/SSLDriver.java @@ -435,16 +435,16 @@ private void runTasks() { } private void maybeFinishHandshake() { - // We only acknowledge that we are done handshaking if there are no bytes that need to be written if (engine.isOutboundDone() || engine.isInboundDone()) { + // If the engine is partially closed, immediate transition to close mode. if (currentMode.isHandshake()) { currentMode = new CloseMode(true); } else { String message = "Expected to be in handshaking mode. Instead in non-handshaking mode: " + currentMode; throw new AssertionError(message); } - } else if (hasFlushPending() == false) { + // We only acknowledge that we are done handshaking if there are no bytes that need to be written if (currentMode.isHandshake()) { currentMode = new ApplicationMode(); } else { From 3937942751227609c2da1258f2da311b1ce7d0e4 Mon Sep 17 00:00:00 2001 From: Tim Brooks Date: Tue, 17 Jul 2018 17:56:14 -0600 Subject: [PATCH 3/3] Remove fallthrough --- .../elasticsearch/xpack/security/transport/nio/SSLDriver.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/transport/nio/SSLDriver.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/transport/nio/SSLDriver.java index 2f44daa850476..fa7791689aaa1 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/transport/nio/SSLDriver.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/transport/nio/SSLDriver.java @@ -352,8 +352,8 @@ private void handshake() throws SSLException { // If we need NEED_TASK we should run the tasks immediately if (handshakeStatus != SSLEngineResult.HandshakeStatus.NEED_TASK) { continueHandshaking = false; - break; } + break; case NEED_TASK: runTasks(); handshakeStatus = engine.getHandshakeStatus();