geode-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From gosulli...@apache.org
Subject [geode] branch develop updated: GEODE-3563 SSL socket handling problems in TCPConduit run (#1955)
Date Mon, 14 May 2018 21:17:10 GMT
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 <bschuchardt@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.

Mime
View raw message