Skip to content

Commit

Permalink
GEODE-8144: setting SNI server name is not needed if endpoint verific…
Browse files Browse the repository at this point in the history
…ation is disabled (apache#5250)

* GEODE-8144: endpoint identification in servers is not working

modified the fix for this issue to not set the SNI server name parameter
if endpoint verification is disabled.  We're doing this because setting
this parameter appears to decrease performance in large performance
tests.

* changed test to throw exceptions instead of asserting they don't exist

* replaced check for SNI server name in SSL parameters with a more in-depth check

* SSLParameters.getServerNames() may return a null value
  • Loading branch information
bschuchardt authored Jun 16, 2020
1 parent 9e52198 commit b1107d2
Show file tree
Hide file tree
Showing 2 changed files with 29 additions and 10 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
import static org.apache.geode.distributed.ConfigurationProperties.MCAST_PORT;
import static org.apache.geode.distributed.ConfigurationProperties.SSL_CIPHERS;
import static org.apache.geode.distributed.ConfigurationProperties.SSL_ENABLED_COMPONENTS;
import static org.apache.geode.distributed.ConfigurationProperties.SSL_ENDPOINT_IDENTIFICATION_ENABLED;
import static org.apache.geode.distributed.ConfigurationProperties.SSL_KEYSTORE;
import static org.apache.geode.distributed.ConfigurationProperties.SSL_KEYSTORE_PASSWORD;
import static org.apache.geode.distributed.ConfigurationProperties.SSL_PROTOCOLS;
Expand All @@ -32,7 +33,6 @@
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 static org.mockito.Mockito.mock;

import java.io.DataInputStream;
Expand All @@ -52,13 +52,17 @@
import java.nio.ByteBuffer;
import java.nio.channels.ServerSocketChannel;
import java.nio.channels.SocketChannel;
import java.util.List;
import java.util.Properties;
import java.util.concurrent.Semaphore;
import java.util.concurrent.TimeUnit;
import java.util.concurrent.atomic.AtomicReference;

import javax.net.ssl.SNIServerName;
import javax.net.ssl.SSLContext;
import javax.net.ssl.SSLEngine;
import javax.net.ssl.SSLException;
import javax.net.ssl.StandardConstants;

import org.apache.commons.io.FileUtils;
import org.junit.After;
Expand Down Expand Up @@ -130,6 +134,7 @@ public void setUp() throws Exception {
properties.setProperty(SSL_TRUSTSTORE, keystore.getCanonicalPath());
properties.setProperty(SSL_TRUSTSTORE_PASSWORD, "password");
properties.setProperty(SSL_REQUIRE_AUTHENTICATION, "true");
properties.setProperty(SSL_ENDPOINT_IDENTIFICATION_ENABLED, "false");
properties.setProperty(SSL_CIPHERS, "any");
properties.setProperty(SSL_PROTOCOLS, "TLSv1.2");

Expand Down Expand Up @@ -176,7 +181,7 @@ public void socketCreatorShouldUseSsl() throws Exception {
}

@Test
public void securedSocketTransmissionShouldWork() throws Exception {
public void securedSocketTransmissionShouldWork() throws Throwable {
this.serverSocket = this.socketCreator.forCluster().createServerSocket(0, 0, this.localHost);
this.serverThread = startServer(this.serverSocket, 15000);

Expand All @@ -194,12 +199,14 @@ public void securedSocketTransmissionShouldWork() throws Exception {
await().until(() -> {
return !serverThread.isAlive();
});
assertNull(serverException);
if (serverException != null) {
throw serverException;
}
assertThat(this.messageFromClient.get()).isEqualTo(MESSAGE);
}

@Test
public void testSecuredSocketTransmissionShouldWorkUsingNIO() throws Exception {
public void testSecuredSocketTransmissionShouldWorkUsingNIO() throws Throwable {
ServerSocketChannel serverChannel = ServerSocketChannel.open();
serverSocket = serverChannel.socket();

Expand Down Expand Up @@ -232,7 +239,9 @@ public void testSecuredSocketTransmissionShouldWorkUsingNIO() throws Exception {
await().until(() -> {
return !serverThread.isAlive();
});
assertNull(serverException);
if (serverException != null) {
throw serverException;
}
// assertThat(this.messageFromClient.get()).isEqualTo(MESSAGE);
}

Expand Down Expand Up @@ -264,12 +273,20 @@ private Thread startServerNIO(final ServerSocket serverSocket, int timeoutMillis

socket = serverSocket.accept();
SocketCreator sc = SocketCreatorFactory.getSocketCreatorForComponent(CLUSTER);
final SSLEngine sslEngine = sc.createSSLEngine("localhost", 1234);
engine =
sc.handshakeSSLSocketChannel(socket.getChannel(), sc.createSSLEngine("localhost", 1234),
sc.handshakeSSLSocketChannel(socket.getChannel(), sslEngine,
timeoutMillis,
false,
ByteBuffer.allocate(65535),
new BufferPool(mock(DMStats.class)));
final List<SNIServerName> serverNames = sslEngine.getSSLParameters().getServerNames();
if (serverNames != null && serverNames.stream()
.mapToInt(SNIServerName::getType)
.anyMatch(type -> type == StandardConstants.SNI_HOST_NAME)) {
serverException = new AssertionError("found SNI server name in SSL Parameters");
return;
}

readMessageFromNIOSSLClient(socket, buffer, engine);
readMessageFromNIOSSLClient(socket, buffer, engine);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -546,10 +546,12 @@ protected boolean useSSL() {
*/
public SSLEngine createSSLEngine(String hostName, int port) {
SSLEngine engine = getSslContext().createSSLEngine(hostName, port);
SSLParameters parameters = engine.getSSLParameters();
// set server-names so that endpoint identification algorithms can find what's expected
if (setServerNames(parameters, new HostAndPort(hostName, port))) {
engine.setSSLParameters(parameters);
if (sslConfig.doEndpointIdentification()) {
// set server-names so that endpoint identification algorithms can find what's expected
SSLParameters parameters = engine.getSSLParameters();
if (setServerNames(parameters, new HostAndPort(hostName, port))) {
engine.setSSLParameters(parameters);
}
}
return engine;
}
Expand Down

0 comments on commit b1107d2

Please sign in to comment.