This is an automated email from the ASF dual-hosted git repository.
mhanson pushed a commit to branch release/1.11.0
in repository https://gitbox.apache.org/repos/asf/geode.git
The following commit(s) were added to refs/heads/release/1.11.0 by this push:
new 1ffe53b Feature/geode 6661 (#4284)
1ffe53b is described below
commit 1ffe53b9eb53f2d55cf1c3ef681d8545b54116b3
Author: Bruce Schuchardt <[email protected]>
AuthorDate: Wed Nov 6 07:54:54 2019 -0800
Feature/geode 6661 (#4284)
* GEODE-6661 NioSslEngine has some problems in its ByteBuffer management
Reverting the change to use a temporary byte buffer for SSL handshakes.
At the end of a handshake the buffer may contain application data that
must be available for subsequent decryption. In the case of TCPConduit
this is usually the "handshake" bytes transmitted for that package's
communications protocol.
Since we really need those bytes I've removed the option of expanding
the handshake buffer if it's smaller than the SSL session's required
packet size. TCPConduit uses that figure to allocate the buffer so this
should be safe. I've added a test for this.
* reverting investigative changes to test
* fix failing unit tests - adjusted buffer sizes
* reverted another set of investigative test changes
---
...LSocketHostNameVerificationIntegrationTest.java | 8 +-
.../internal/net/SSLSocketIntegrationTest.java | 2 +-
.../apache/geode/internal/net/NioSslEngine.java | 161 ++++++++++-----------
.../geode/internal/net/NioSslEngineTest.java | 32 +++-
4 files changed, 117 insertions(+), 86 deletions(-)
diff --git
a/geode-core/src/integrationTest/java/org/apache/geode/internal/net/SSLSocketHostNameVerificationIntegrationTest.java
b/geode-core/src/integrationTest/java/org/apache/geode/internal/net/SSLSocketHostNameVerificationIntegrationTest.java
index 2715c24..a1b0226 100755
---
a/geode-core/src/integrationTest/java/org/apache/geode/internal/net/SSLSocketHostNameVerificationIntegrationTest.java
+++
b/geode-core/src/integrationTest/java/org/apache/geode/internal/net/SSLSocketHostNameVerificationIntegrationTest.java
@@ -172,7 +172,8 @@ public class SSLSocketHostNameVerificationIntegrationTest {
try {
this.socketCreator.handshakeSSLSocketChannel(clientSocket.getChannel(),
sslEngine, 0, true,
- ByteBuffer.allocate(500), new BufferPool(mock(DMStats.class)));
+ ByteBuffer.allocate(sslEngine.getSession().getPacketBufferSize()),
+ new BufferPool(mock(DMStats.class)));
if (doEndPointIdentification && !addCertificateSAN) {
fail("Failed to validate hostname in certificate SAN");
@@ -198,12 +199,13 @@ public class SSLSocketHostNameVerificationIntegrationTest
{
try {
socket = serverSocket.accept();
SocketCreator sc =
SocketCreatorFactory.getSocketCreatorForComponent(CLUSTER);
+ final SSLEngine sslEngine =
sc.createSSLEngine(this.localHost.getHostName(), 1234);
engine =
sc.handshakeSSLSocketChannel(socket.getChannel(),
- sc.createSSLEngine(this.localHost.getHostName(), 1234),
+ sslEngine,
timeoutMillis,
false,
- ByteBuffer.allocate(500),
+
ByteBuffer.allocate(sslEngine.getSession().getPacketBufferSize()),
new BufferPool(mock(DMStats.class)));
} catch (Throwable throwable) {
serverException = throwable;
diff --git
a/geode-core/src/integrationTest/java/org/apache/geode/internal/net/SSLSocketIntegrationTest.java
b/geode-core/src/integrationTest/java/org/apache/geode/internal/net/SSLSocketIntegrationTest.java
index 0ae31a5..8659c67 100755
---
a/geode-core/src/integrationTest/java/org/apache/geode/internal/net/SSLSocketIntegrationTest.java
+++
b/geode-core/src/integrationTest/java/org/apache/geode/internal/net/SSLSocketIntegrationTest.java
@@ -263,7 +263,7 @@ public class SSLSocketIntegrationTest {
sc.handshakeSSLSocketChannel(socket.getChannel(),
sc.createSSLEngine("localhost", 1234),
timeoutMillis,
false,
- ByteBuffer.allocate(500),
+ ByteBuffer.allocate(65535),
new BufferPool(mock(DMStats.class)));
readMessageFromNIOSSLClient(socket, buffer, engine);
diff --git
a/geode-core/src/main/java/org/apache/geode/internal/net/NioSslEngine.java
b/geode-core/src/main/java/org/apache/geode/internal/net/NioSslEngine.java
index ba5fa16..e914847 100644
--- a/geode-core/src/main/java/org/apache/geode/internal/net/NioSslEngine.java
+++ b/geode-core/src/main/java/org/apache/geode/internal/net/NioSslEngine.java
@@ -86,11 +86,14 @@ public class NioSslEngine implements NioFilter {
ByteBuffer peerNetData)
throws IOException, InterruptedException {
- if (logger.isDebugEnabled()) {
- logger.debug("Allocating new buffer for SSL handshake");
+ if (peerNetData.capacity() < engine.getSession().getPacketBufferSize()) {
+ throw new IllegalArgumentException(String.format("Provided buffer is too
small to perform "
+ + "SSL handshake. Buffer capacity is %s but need %s",
+ peerNetData.capacity(), engine.getSession().getPacketBufferSize()));
}
- ByteBuffer handshakeBuffer =
-
bufferPool.acquireDirectReceiveBuffer(engine.getSession().getPacketBufferSize());
+
+ ByteBuffer handshakeBuffer = peerNetData;
+ handshakeBuffer.clear();
ByteBuffer myAppData = ByteBuffer.wrap(new byte[0]);
@@ -109,88 +112,84 @@ public class NioSslEngine implements NioFilter {
SSLEngineResult.HandshakeStatus status = engine.getHandshakeStatus();
SSLEngineResult engineResult = null;
- try {
- // Process handshaking message
- while (status != FINISHED &&
- status != SSLEngineResult.HandshakeStatus.NOT_HANDSHAKING) {
- if (socketChannel.socket().isClosed()) {
- logger.info("Handshake terminated because socket is closed");
- throw new SocketException("handshake terminated - socket is closed");
+ // Process handshaking message
+ while (status != FINISHED &&
+ status != SSLEngineResult.HandshakeStatus.NOT_HANDSHAKING) {
+ if (socketChannel.socket().isClosed()) {
+ logger.info("Handshake terminated because socket is closed");
+ throw new SocketException("handshake terminated - socket is closed");
+ }
+
+ if (timeoutNanos > 0) {
+ if (timeoutNanos < System.nanoTime()) {
+ logger.info("TLS handshake is timing out");
+ throw new SocketTimeoutException("handshake timed out");
}
+ }
- if (timeoutNanos > 0) {
- if (timeoutNanos < System.nanoTime()) {
- logger.info("TLS handshake is timing out");
- throw new SocketTimeoutException("handshake timed out");
+ switch (status) {
+ case NEED_UNWRAP:
+ // Receive handshaking data from peer
+ int dataRead = socketChannel.read(handshakeBuffer);
+
+ // Process incoming handshaking data
+ handshakeBuffer.flip();
+ engineResult = engine.unwrap(handshakeBuffer, peerAppData);
+ handshakeBuffer.compact();
+ status = engineResult.getHandshakeStatus();
+
+ // if we're not finished, there's nothing to process and no data was
read let's hang out
+ // for a little
+ if (peerAppData.remaining() == 0 && dataRead == 0 && status ==
NEED_UNWRAP) {
+ Thread.sleep(10);
}
- }
- switch (status) {
- case NEED_UNWRAP:
- // Receive handshaking data from peer
- int dataRead = socketChannel.read(handshakeBuffer);
-
- // Process incoming handshaking data
- handshakeBuffer.flip();
- engineResult = engine.unwrap(handshakeBuffer, peerAppData);
- handshakeBuffer.compact();
- status = engineResult.getHandshakeStatus();
-
- // if we're not finished, there's nothing to process and no data
was read let's hang out
- // for a little
- if (peerAppData.remaining() == 0 && dataRead == 0 && status ==
NEED_UNWRAP) {
- Thread.sleep(10);
- }
-
- if (engineResult.getStatus() == BUFFER_OVERFLOW) {
- peerAppData =
- expandWriteBuffer(TRACKED_RECEIVER, peerAppData,
peerAppData.capacity() * 2);
- }
- break;
-
- case NEED_WRAP:
- // Empty the local network packet buffer.
- myNetData.clear();
-
- // Generate handshaking data
- engineResult = engine.wrap(myAppData, myNetData);
- status = engineResult.getHandshakeStatus();
-
- // Check status
- switch (engineResult.getStatus()) {
- case BUFFER_OVERFLOW:
- myNetData =
- expandWriteBuffer(TRACKED_SENDER, myNetData,
- myNetData.capacity() * 2);
- break;
- case OK:
- myNetData.flip();
- // Send the handshaking data to peer
- while (myNetData.hasRemaining()) {
- socketChannel.write(myNetData);
- }
- break;
- case CLOSED:
- break;
- default:
- logger.info("handshake terminated with illegal state due to
{}", status);
- throw new IllegalStateException(
- "Unknown SSLEngineResult status: " +
engineResult.getStatus());
- }
- break;
- case NEED_TASK:
- // Handle blocking tasks
- handleBlockingTasks();
- status = engine.getHandshakeStatus();
- break;
- default:
- logger.info("handshake terminated with illegal state due to {}",
status);
- throw new IllegalStateException("Unknown SSL Handshake state: " +
status);
- }
- Thread.sleep(10);
+ if (engineResult.getStatus() == BUFFER_OVERFLOW) {
+ peerAppData =
+ expandWriteBuffer(TRACKED_RECEIVER, peerAppData,
peerAppData.capacity() * 2);
+ }
+ break;
+
+ case NEED_WRAP:
+ // Empty the local network packet buffer.
+ myNetData.clear();
+
+ // Generate handshaking data
+ engineResult = engine.wrap(myAppData, myNetData);
+ status = engineResult.getHandshakeStatus();
+
+ // Check status
+ switch (engineResult.getStatus()) {
+ case BUFFER_OVERFLOW:
+ myNetData =
+ expandWriteBuffer(TRACKED_SENDER, myNetData,
+ myNetData.capacity() * 2);
+ break;
+ case OK:
+ myNetData.flip();
+ // Send the handshaking data to peer
+ while (myNetData.hasRemaining()) {
+ socketChannel.write(myNetData);
+ }
+ break;
+ case CLOSED:
+ break;
+ default:
+ logger.info("handshake terminated with illegal state due to {}",
status);
+ throw new IllegalStateException(
+ "Unknown SSLEngineResult status: " +
engineResult.getStatus());
+ }
+ break;
+ case NEED_TASK:
+ // Handle blocking tasks
+ handleBlockingTasks();
+ status = engine.getHandshakeStatus();
+ break;
+ default:
+ logger.info("handshake terminated with illegal state due to {}",
status);
+ throw new IllegalStateException("Unknown SSL Handshake state: " +
status);
}
- } finally {
- bufferPool.releaseReceiveBuffer(handshakeBuffer);
+ Thread.sleep(10);
}
if (status != FINISHED) {
logger.info("handshake terminated with exception due to {}", status);
diff --git
a/geode-core/src/test/java/org/apache/geode/internal/net/NioSslEngineTest.java
b/geode-core/src/test/java/org/apache/geode/internal/net/NioSslEngineTest.java
index e5a4963..e50b878 100644
---
a/geode-core/src/test/java/org/apache/geode/internal/net/NioSslEngineTest.java
+++
b/geode-core/src/test/java/org/apache/geode/internal/net/NioSslEngineTest.java
@@ -109,7 +109,7 @@ public class NioSslEngineTest {
new SSLEngineResult(BUFFER_OVERFLOW, NEED_WRAP, 0, 0),
new SSLEngineResult(CLOSED, FINISHED, 100, 0));
- spyNioSslEngine.handshake(mockChannel, 10000,
ByteBuffer.allocate(netBufferSize / 2));
+ spyNioSslEngine.handshake(mockChannel, 10000,
ByteBuffer.allocate(netBufferSize));
verify(mockEngine, atLeast(2)).getHandshakeStatus();
verify(mockEngine, times(3)).wrap(any(ByteBuffer.class),
any(ByteBuffer.class));
verify(mockEngine, times(3)).unwrap(any(ByteBuffer.class),
any(ByteBuffer.class));
@@ -120,6 +120,36 @@ public class NioSslEngineTest {
}
@Test
+ public void handshakeWithInsufficientBufferSize() throws Exception {
+ SocketChannel mockChannel = mock(SocketChannel.class);
+ when(mockChannel.read(any(ByteBuffer.class))).thenReturn(100, 100, 100, 0);
+ Socket mockSocket = mock(Socket.class);
+ when(mockChannel.socket()).thenReturn(mockSocket);
+ when(mockSocket.isClosed()).thenReturn(false);
+
+ // // initial read of handshake status followed by read of handshake
status after task execution
+ // when(mockEngine.getHandshakeStatus()).thenReturn(NEED_UNWRAP,
NEED_WRAP);
+ //
+ // // interleaved wraps/unwraps/task-execution
+ // when(mockEngine.unwrap(any(ByteBuffer.class),
any(ByteBuffer.class))).thenReturn(
+ // new SSLEngineResult(OK, NEED_WRAP, 100, 100),
+ // new SSLEngineResult(BUFFER_OVERFLOW, NEED_UNWRAP, 0, 0),
+ // new SSLEngineResult(OK, NEED_TASK, 100, 0));
+ //
+ // when(mockEngine.getDelegatedTask()).thenReturn(() -> {
+ // }, (Runnable) null);
+ //
+ // when(mockEngine.wrap(any(ByteBuffer.class),
any(ByteBuffer.class))).thenReturn(
+ // new SSLEngineResult(OK, NEED_UNWRAP, 100, 100),
+ // new SSLEngineResult(BUFFER_OVERFLOW, NEED_WRAP, 0, 0),
+ // new SSLEngineResult(CLOSED, FINISHED, 100, 0));
+ //
+ assertThatThrownBy(() -> spyNioSslEngine.handshake(mockChannel, 10000,
+ ByteBuffer.allocate(netBufferSize /
2))).isExactlyInstanceOf(IllegalArgumentException.class)
+ .hasMessageContaining("Provided buffer is too small");
+ }
+
+ @Test
public void handshakeDetectsClosedSocket() throws Exception {
SocketChannel mockChannel = mock(SocketChannel.class);
when(mockChannel.read(any(ByteBuffer.class))).thenReturn(100, 100, 100, 0);
