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 fa7791689aa..7314eadf9ec 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 @@ -327,7 +327,7 @@ public class SSLDriver implements AutoCloseable { private void startHandshake() throws SSLException { handshakeStatus = engine.getHandshakeStatus(); if (handshakeStatus != SSLEngineResult.HandshakeStatus.NEED_UNWRAP && - handshakeStatus != SSLEngineResult.HandshakeStatus.NEED_WRAP) { + handshakeStatus != SSLEngineResult.HandshakeStatus.NEED_WRAP) { try { handshake(); } catch (SSLException e) { @@ -403,8 +403,8 @@ public class SSLDriver implements AutoCloseable { @Override public boolean needsNonApplicationWrite() { return handshakeStatus == SSLEngineResult.HandshakeStatus.NEED_WRAP - || handshakeStatus == SSLEngineResult.HandshakeStatus.NOT_HANDSHAKING - || handshakeStatus == SSLEngineResult.HandshakeStatus.FINISHED; + || handshakeStatus == SSLEngineResult.HandshakeStatus.NOT_HANDSHAKING + || handshakeStatus == SSLEngineResult.HandshakeStatus.FINISHED; } @Override @@ -465,7 +465,7 @@ public class SSLDriver implements AutoCloseable { networkReadBuffer.flip(); SSLEngineResult result = unwrap(buffer); boolean renegotiationRequested = result.getStatus() != SSLEngineResult.Status.CLOSED - && maybeRenegotiation(result.getHandshakeStatus()); + && maybeRenegotiation(result.getHandshakeStatus()); continueUnwrap = result.bytesProduced() > 0 && renegotiationRequested == false; } } @@ -533,17 +533,24 @@ public class SSLDriver implements AutoCloseable { } else { engine.closeOutbound(); } - } @Override public void read(InboundChannelBuffer buffer) throws SSLException { + if (needToReceiveClose == false) { + // There is an issue where receiving handshake messages after initiating the close process + // can place the SSLEngine back into handshaking mode. In order to handle this, if we + // initiate close during a handshake we do not wait to receive close. As we do not need to + // receive close, we will not handle reads. + return; + } + ensureApplicationBufferSize(buffer); boolean continueUnwrap = true; while (continueUnwrap && networkReadBuffer.position() > 0) { networkReadBuffer.flip(); SSLEngineResult result = unwrap(buffer); - continueUnwrap = result.bytesProduced() > 0; + continueUnwrap = result.bytesProduced() > 0 || result.bytesConsumed() > 0; } if (engine.isInboundDone()) { needToReceiveClose = false; @@ -598,7 +605,7 @@ public class SSLDriver implements AutoCloseable { try { engine.closeInbound(); } catch (SSLException e) { - if (e.getMessage().startsWith("Inbound closed before receiving peer's close_notify") == false) { + if (e.getMessage().contains("before receiving peer's close_notify") == false) { throw e; } } 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 5ff154d6562..b1d39ddc6ac 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 @@ -23,6 +23,7 @@ import java.nio.charset.StandardCharsets; import java.security.SecureRandom; import java.util.Arrays; import java.util.Collections; +import java.util.List; import java.util.function.Supplier; public class SSLDriverTests extends ESTestCase { @@ -96,7 +97,7 @@ public class SSLDriverTests extends ESTestCase { normalClose(clientDriver, serverDriver); } - public void testBigAppData() throws Exception { + public void testBigApplicationData() throws Exception { SSLContext sslContext = getSSLContext(); SSLDriver clientDriver = getDriver(sslContext.createSSLEngine(), true); @@ -124,8 +125,6 @@ public class SSLDriverTests extends ESTestCase { } public void testHandshakeFailureBecauseProtocolMismatch() throws Exception { - // See https://github.com/elastic/elasticsearch/issues/33751 - assumeTrue("test fails on JDK 11 >= ea28 currently", JavaVersion.current().compareTo(JavaVersion.parse("11")) < 0); SSLContext sslContext = getSSLContext(); SSLEngine clientEngine = sslContext.createSSLEngine(); SSLEngine serverEngine = sslContext.createSSLEngine(); @@ -138,7 +137,7 @@ public class SSLDriverTests extends ESTestCase { SSLException sslException = expectThrows(SSLException.class, () -> handshake(clientDriver, serverDriver)); String oldExpected = "Client requested protocol TLSv1.1 not enabled or not supported"; - String jdk11Expected = "Received fatal alert: protocol_version"; + String jdk11Expected = "The client supported protocol versions [TLSv1.1] are not accepted by server preferences [TLS12]"; boolean expectedMessage = oldExpected.equals(sslException.getMessage()) || jdk11Expected.equals(sslException.getMessage()); assertTrue("Unexpected exception message: " + sslException.getMessage(), expectedMessage); @@ -148,7 +147,8 @@ public class SSLDriverTests extends ESTestCase { } // Prior to JDK11 we still need to send a close alert if (serverDriver.isClosed() == false) { - failedCloseAlert(serverDriver, clientDriver); + failedCloseAlert(serverDriver, clientDriver, Arrays.asList("Received fatal alert: protocol_version", + "Received fatal alert: handshake_failure")); } } @@ -172,12 +172,14 @@ public class SSLDriverTests extends ESTestCase { } // Prior to JDK11 we still need to send a close alert if (serverDriver.isClosed() == false) { - failedCloseAlert(serverDriver, clientDriver); + List messages = Arrays.asList("Received fatal alert: handshake_failure", + "Received close_notify during handshake"); + failedCloseAlert(serverDriver, clientDriver, messages); } } - @AwaitsFix(bugUrl = "https://github.com/elastic/elasticsearch/issues/32144") - public void testCloseDuringHandshake() throws Exception { + public void testCloseDuringHandshakeJDK11() throws Exception { + assumeTrue("this tests ssl engine for JDK11", JavaVersion.current().compareTo(JavaVersion.parse("11")) >= 0); SSLContext sslContext = getSSLContext(); SSLDriver clientDriver = getDriver(sslContext.createSSLEngine(), true); SSLDriver serverDriver = getDriver(sslContext.createSSLEngine(), false); @@ -199,30 +201,66 @@ public class SSLDriverTests extends ESTestCase { serverDriver.initiateClose(); assertTrue(serverDriver.needsNonApplicationWrite()); assertFalse(serverDriver.isClosed()); - sendNeededWrites(serverDriver, clientDriver); + sendNonApplicationWrites(serverDriver, clientDriver); // We are immediately fully closed due to SSLEngine inconsistency assertTrue(serverDriver.isClosed()); // This should not throw exception yet as the SSLEngine will not UNWRAP data while attempting to WRAP clientDriver.read(clientBuffer); - sendNeededWrites(clientDriver, serverDriver); - SSLException sslException = expectThrows(SSLException.class, () -> clientDriver.read(clientBuffer)); - assertEquals("Received close_notify during handshake", sslException.getMessage()); - assertTrue(clientDriver.needsNonApplicationWrite()); - sendNeededWrites(clientDriver, serverDriver); + sendNonApplicationWrites(clientDriver, serverDriver); + clientDriver.read(clientBuffer); + sendNonApplicationWrites(clientDriver, serverDriver); serverDriver.read(serverBuffer); assertTrue(clientDriver.isClosed()); } - private void failedCloseAlert(SSLDriver sendDriver, SSLDriver receiveDriver) throws SSLException { + public void testCloseDuringHandshakePreJDK11() throws Exception { + assumeTrue("this tests ssl engine for pre-JDK11", JavaVersion.current().compareTo(JavaVersion.parse("11")) < 0); + SSLContext sslContext = getSSLContext(); + SSLDriver clientDriver = getDriver(sslContext.createSSLEngine(), true); + SSLDriver serverDriver = getDriver(sslContext.createSSLEngine(), false); + + clientDriver.init(); + serverDriver.init(); + + assertTrue(clientDriver.needsNonApplicationWrite()); + assertFalse(serverDriver.needsNonApplicationWrite()); + sendHandshakeMessages(clientDriver, serverDriver); + sendHandshakeMessages(serverDriver, clientDriver); + + sendData(clientDriver, serverDriver); + + assertTrue(clientDriver.isHandshaking()); + assertTrue(serverDriver.isHandshaking()); + + assertFalse(serverDriver.needsNonApplicationWrite()); + serverDriver.initiateClose(); + assertTrue(serverDriver.needsNonApplicationWrite()); + assertFalse(serverDriver.isClosed()); + sendNonApplicationWrites(serverDriver, clientDriver); + // We are immediately fully closed due to SSLEngine inconsistency + assertTrue(serverDriver.isClosed()); + // This should not throw exception yet as the SSLEngine will not UNWRAP data while attempting to WRAP + clientDriver.read(clientBuffer); + sendNonApplicationWrites(clientDriver, serverDriver); + SSLException sslException = expectThrows(SSLException.class, () -> clientDriver.read(clientBuffer)); + assertEquals("Received close_notify during handshake", sslException.getMessage()); + assertTrue(clientDriver.needsNonApplicationWrite()); + sendNonApplicationWrites(clientDriver, serverDriver); + serverDriver.read(serverBuffer); + assertTrue(clientDriver.isClosed()); + } + + private void failedCloseAlert(SSLDriver sendDriver, SSLDriver receiveDriver, List messages) throws SSLException { assertTrue(sendDriver.needsNonApplicationWrite()); assertFalse(sendDriver.isClosed()); - sendNeededWrites(sendDriver, receiveDriver); + sendNonApplicationWrites(sendDriver, receiveDriver); assertTrue(sendDriver.isClosed()); sendDriver.close(); SSLException sslException = expectThrows(SSLException.class, () -> receiveDriver.read(genericBuffer)); - assertEquals("Received fatal alert: handshake_failure", sslException.getMessage()); + assertTrue("Expected one of the following exception messages: " + messages + ". Found: " + sslException.getMessage(), + messages.stream().anyMatch(m -> sslException.getMessage().equals(m))); if (receiveDriver.needsNonApplicationWrite() == false) { assertTrue(receiveDriver.isClosed()); receiveDriver.close(); @@ -249,7 +287,7 @@ public class SSLDriverTests extends ESTestCase { sendDriver.initiateClose(); assertFalse(sendDriver.readyForApplicationWrites()); assertTrue(sendDriver.needsNonApplicationWrite()); - sendNeededWrites(sendDriver, receiveDriver); + sendNonApplicationWrites(sendDriver, receiveDriver); assertFalse(sendDriver.isClosed()); receiveDriver.read(genericBuffer); @@ -257,7 +295,7 @@ public class SSLDriverTests extends ESTestCase { assertFalse(receiveDriver.readyForApplicationWrites()); assertTrue(receiveDriver.needsNonApplicationWrite()); - sendNeededWrites(receiveDriver, sendDriver); + sendNonApplicationWrites(receiveDriver, sendDriver); assertTrue(receiveDriver.isClosed()); sendDriver.read(genericBuffer); @@ -267,7 +305,7 @@ public class SSLDriverTests extends ESTestCase { receiveDriver.close(); } - private void sendNeededWrites(SSLDriver sendDriver, SSLDriver receiveDriver) throws SSLException { + private void sendNonApplicationWrites(SSLDriver sendDriver, SSLDriver receiveDriver) throws SSLException { while (sendDriver.needsNonApplicationWrite() || sendDriver.hasFlushPending()) { if (sendDriver.hasFlushPending() == false) { sendDriver.nonApplicationWrite(); @@ -315,7 +353,6 @@ public class SSLDriverTests extends ESTestCase { assertTrue(sendDriver.needsNonApplicationWrite() || sendDriver.hasFlushPending()); while (sendDriver.needsNonApplicationWrite() || sendDriver.hasFlushPending()) { - assertFalse(receiveDriver.needsNonApplicationWrite()); if (sendDriver.hasFlushPending() == false) { sendDriver.nonApplicationWrite(); }