This is an automated email from the ASF dual-hosted git repository. gosullivan pushed a commit to branch develop in repository https://gitbox.apache.org/repos/asf/geode.git
The following commit(s) were added to refs/heads/develop by this push: new e423cd8 GEODE-3563 SSL socket handling problems in TCPConduit run (#1955) e423cd8 is described below commit e423cd8fa24329baf11fd6871a5ea6dc0f362b6c Author: Bruce Schuchardt <bschucha...@pivotal.io> AuthorDate: Mon May 14 14:17:02 2018 -0700 GEODE-3563 SSL socket handling problems in TCPConduit run (#1955) Modified the handshake method to establish a timeout and revert it when done. Added testing to ensure the timeout is being established and honored. --- .../distributed/internal/tcpserver/TcpServer.java | 3 +- .../internal/cache/tier/sockets/AcceptorImpl.java | 4 +-- .../apache/geode/internal/net/SocketCreator.java | 13 ++++--- .../org/apache/geode/internal/tcp/TCPConduit.java | 3 +- .../geode/internal/net/DummySocketCreator.java | 2 +- .../internal/net/SSLSocketIntegrationTest.java | 41 +++++++++++++++++----- .../geode/internal/net/SocketCreatorJUnitTest.java | 17 +++++++-- 7 files changed, 62 insertions(+), 21 deletions(-) diff --git a/geode-core/src/main/java/org/apache/geode/distributed/internal/tcpserver/TcpServer.java b/geode-core/src/main/java/org/apache/geode/distributed/internal/tcpserver/TcpServer.java index ae926e9..608c63d 100755 --- a/geode-core/src/main/java/org/apache/geode/distributed/internal/tcpserver/TcpServer.java +++ b/geode-core/src/main/java/org/apache/geode/distributed/internal/tcpserver/TcpServer.java @@ -359,7 +359,8 @@ public class TcpServer { long startTime = DistributionStats.getStatTime(); DataInputStream input = null; try { - getSocketCreator().startHandshakeIfSocketIsSSL(socket, READ_TIMEOUT); + socket.setSoTimeout(READ_TIMEOUT); + getSocketCreator().handshakeIfSocketIsSSL(socket, READ_TIMEOUT); try { input = new DataInputStream(socket.getInputStream()); diff --git a/geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/AcceptorImpl.java b/geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/AcceptorImpl.java index 83d3156..875ff6d 100755 --- a/geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/AcceptorImpl.java +++ b/geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/AcceptorImpl.java @@ -1549,12 +1549,12 @@ public class AcceptorImpl implements Acceptor, Runnable, CommBufferPool { } private CommunicationMode getCommunicationModeForNonSelector(Socket socket) throws IOException { - this.socketCreator.startHandshakeIfSocketIsSSL(socket, this.acceptTimeout); + socket.setSoTimeout(0); + this.socketCreator.handshakeIfSocketIsSSL(socket, this.acceptTimeout); byte communicationModeByte = (byte) socket.getInputStream().read(); if (communicationModeByte == -1) { throw new EOFException(); } - socket.setSoTimeout(0); return CommunicationMode.fromModeNumber(communicationModeByte); } diff --git a/geode-core/src/main/java/org/apache/geode/internal/net/SocketCreator.java b/geode-core/src/main/java/org/apache/geode/internal/net/SocketCreator.java index 21f48a7..7441fc9 100755 --- a/geode-core/src/main/java/org/apache/geode/internal/net/SocketCreator.java +++ b/geode-core/src/main/java/org/apache/geode/internal/net/SocketCreator.java @@ -957,14 +957,15 @@ public class SocketCreator { } /** - * Will be a server socket... this one simply registers the listeners. + * Use this method to perform the SSL handshake on a newly accepted socket. Non-SSL + * sockets are ignored by this method. * - * @param timeout the socket's timeout will be set to this (in milliseconds). + * @param timeout the number of milliseconds allowed for the handshake to complete */ - public void startHandshakeIfSocketIsSSL(Socket socket, int timeout) throws IOException { - socket.setSoTimeout(timeout); - + public void handshakeIfSocketIsSSL(Socket socket, int timeout) throws IOException { if (socket instanceof SSLSocket) { + int oldTimeout = socket.getSoTimeout(); + socket.setSoTimeout(timeout); SSLSocket sslSocket = (SSLSocket) socket; try { sslSocket.startHandshake(); @@ -985,6 +986,8 @@ public class SocketCreator { throw ex; } // else ignore + } finally { + socket.setSoTimeout(oldTimeout); } } } diff --git a/geode-core/src/main/java/org/apache/geode/internal/tcp/TCPConduit.java b/geode-core/src/main/java/org/apache/geode/internal/tcp/TCPConduit.java index 52ea508..6135c07 100644 --- a/geode-core/src/main/java/org/apache/geode/internal/tcp/TCPConduit.java +++ b/geode-core/src/main/java/org/apache/geode/internal/tcp/TCPConduit.java @@ -688,7 +688,8 @@ public class TCPConduit implements Runnable { ex); break; } - socketCreator.startHandshakeIfSocketIsSSL(othersock, idleConnectionTimeout); + othersock.setSoTimeout(0); + socketCreator.handshakeIfSocketIsSSL(othersock, idleConnectionTimeout); } if (stopped) { try { diff --git a/geode-core/src/test/java/org/apache/geode/internal/net/DummySocketCreator.java b/geode-core/src/test/java/org/apache/geode/internal/net/DummySocketCreator.java index e061f14..aa430b0 100644 --- a/geode-core/src/test/java/org/apache/geode/internal/net/DummySocketCreator.java +++ b/geode-core/src/test/java/org/apache/geode/internal/net/DummySocketCreator.java @@ -35,7 +35,7 @@ public class DummySocketCreator extends SocketCreator { } @Override - public void startHandshakeIfSocketIsSSL(Socket socket, int timeout) throws IOException { + public void handshakeIfSocketIsSSL(Socket socket, int timeout) throws IOException { this.socketSoTimeouts.add(timeout); throw new SSLException("This is a test SSLException"); } diff --git a/geode-core/src/test/java/org/apache/geode/internal/net/SSLSocketIntegrationTest.java b/geode-core/src/test/java/org/apache/geode/internal/net/SSLSocketIntegrationTest.java index 35b8f61..182c77f 100755 --- a/geode-core/src/test/java/org/apache/geode/internal/net/SSLSocketIntegrationTest.java +++ b/geode-core/src/test/java/org/apache/geode/internal/net/SSLSocketIntegrationTest.java @@ -22,7 +22,11 @@ import static org.apache.geode.distributed.ConfigurationProperties.MCAST_PORT; import static org.apache.geode.internal.security.SecurableCommunicationChannel.CLUSTER; import static org.assertj.core.api.Assertions.assertThat; import static org.awaitility.Awaitility.await; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertNotEquals; +import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertNull; import java.io.File; import java.io.IOException; @@ -93,6 +97,9 @@ public class SSLSocketIntegrationTest { @Rule public TestName testName = new TestName(); + + private Throwable serverException; + @Before public void setUp() throws Exception { File keystore = findTestKeystore(); @@ -152,7 +159,7 @@ public class SSLSocketIntegrationTest { @Test public void securedSocketTransmissionShouldWork() throws Exception { this.serverSocket = this.socketCreator.createServerSocket(0, 0, this.localHost); - this.serverThread = startServer(this.serverSocket); + this.serverThread = startServer(this.serverSocket, 15000); int serverPort = this.serverSocket.getLocalPort(); this.clientSocket = this.socketCreator.connectForServer(this.localHost, serverPort); @@ -163,8 +170,24 @@ public class SSLSocketIntegrationTest { output.flush(); // this is the real assertion of this test - await().atMost(1, TimeUnit.MINUTES) - .until(() -> assertThat(this.messageFromClient.get()).isEqualTo(MESSAGE)); + await().atMost(30, TimeUnit.SECONDS).until(() -> { + return !serverThread.isAlive(); + }); + assertNull(serverException); + assertThat(this.messageFromClient.get()).isEqualTo(MESSAGE); + } + + @Test(expected = SocketTimeoutException.class) + public void handshakeCanTimeoutOnServer() throws Throwable { + this.serverSocket = this.socketCreator.createServerSocket(0, 0, this.localHost); + this.serverThread = startServer(this.serverSocket, 1000); + + int serverPort = this.serverSocket.getLocalPort(); + Socket socket = new Socket(); + socket.connect(new InetSocketAddress(localHost, serverPort)); + await().atMost(5, TimeUnit.SECONDS).until(() -> assertFalse(serverThread.isAlive())); + assertNotNull(serverException); + throw serverException; } @Test @@ -249,16 +272,18 @@ public class SSLSocketIntegrationTest { return file; } - private Thread startServer(final ServerSocket serverSocket) throws Exception { + private Thread startServer(final ServerSocket serverSocket, int timeoutMillis) throws Exception { Thread serverThread = new Thread(new MyThreadGroup(this.testName.getMethodName()), () -> { + long startTime = System.currentTimeMillis(); try { Socket socket = serverSocket.accept(); - SocketCreatorFactory.getSocketCreatorForComponent(CLUSTER) - .startHandshakeIfSocketIsSSL(socket, 15000); + SocketCreatorFactory.getSocketCreatorForComponent(CLUSTER).handshakeIfSocketIsSSL(socket, + timeoutMillis); + assertEquals(0, socket.getSoTimeout()); ObjectInputStream ois = new ObjectInputStream(socket.getInputStream()); messageFromClient.set((String) ois.readObject()); - } catch (IOException | ClassNotFoundException e) { - throw new Error(e); + } catch (Throwable throwable) { + serverException = throwable; } }, this.testName.getMethodName() + "-server"); diff --git a/geode-core/src/test/java/org/apache/geode/internal/net/SocketCreatorJUnitTest.java b/geode-core/src/test/java/org/apache/geode/internal/net/SocketCreatorJUnitTest.java index e075b11..a0480b9 100644 --- a/geode-core/src/test/java/org/apache/geode/internal/net/SocketCreatorJUnitTest.java +++ b/geode-core/src/test/java/org/apache/geode/internal/net/SocketCreatorJUnitTest.java @@ -16,8 +16,13 @@ package org.apache.geode.internal.net; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; -import java.net.Socket; +import java.security.cert.Certificate; +import java.security.cert.X509Certificate; + +import javax.net.ssl.SSLSession; +import javax.net.ssl.SSLSocket; import org.junit.Test; import org.junit.experimental.categories.Category; @@ -46,11 +51,17 @@ public class SocketCreatorJUnitTest { @Test public void testConfigureServerSSLSocketSetsSoTimeout() throws Exception { final SocketCreator socketCreator = new SocketCreator(mock(SSLConfig.class)); - final Socket socket = mock(Socket.class); + final SSLSocket socket = mock(SSLSocket.class); + Certificate[] certs = new Certificate[] {mock(X509Certificate.class)}; + SSLSession session = mock(SSLSession.class); + when(session.getPeerCertificates()).thenReturn(certs); + when(socket.getSession()).thenReturn(session); final int timeout = 1938236; - socketCreator.startHandshakeIfSocketIsSSL(socket, timeout); + socketCreator.handshakeIfSocketIsSSL(socket, timeout); + verify(socket).getSession(); + verify(session).getPeerCertificates(); verify(socket).setSoTimeout(timeout); } -- To stop receiving notification emails like this one, please contact gosulli...@apache.org.