You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@geode.apache.org by go...@apache.org on 2018/05/14 21:17:10 UTC
[geode] branch develop updated: GEODE-3563 SSL socket handling
problems in TCPConduit run (#1955)
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 <bs...@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
gosullivan@apache.org.