ACCUMULO-3318 Update thrift RPC secure socket creation to default to TLS
Project: http://git-wip-us.apache.org/repos/asf/accumulo/repo Commit: http://git-wip-us.apache.org/repos/asf/accumulo/commit/520d802e Tree: http://git-wip-us.apache.org/repos/asf/accumulo/tree/520d802e Diff: http://git-wip-us.apache.org/repos/asf/accumulo/diff/520d802e Branch: refs/heads/master Commit: 520d802ec5ab33a8ce6a96672edaba58ab6aef37 Parents: 9ea8f09 Author: Josh Elser <els...@apache.org> Authored: Fri Nov 7 19:18:03 2014 -0500 Committer: Josh Elser <els...@apache.org> Committed: Fri Nov 7 19:18:03 2014 -0500 ---------------------------------------------------------------------- .../org/apache/accumulo/core/conf/Property.java | 5 + .../accumulo/core/util/SslConnectionParams.java | 104 +++++++--- .../apache/accumulo/core/util/ThriftUtil.java | 200 ++++++++++++++++++- .../org/apache/accumulo/server/Accumulo.java | 2 +- 4 files changed, 282 insertions(+), 29 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/accumulo/blob/520d802e/core/src/main/java/org/apache/accumulo/core/conf/Property.java ---------------------------------------------------------------------- diff --git a/core/src/main/java/org/apache/accumulo/core/conf/Property.java b/core/src/main/java/org/apache/accumulo/core/conf/Property.java index 45ef384..a03b210 100644 --- a/core/src/main/java/org/apache/accumulo/core/conf/Property.java +++ b/core/src/main/java/org/apache/accumulo/core/conf/Property.java @@ -99,6 +99,11 @@ public enum Property { RPC_USE_JSSE("rpc.useJsse", "false", PropertyType.BOOLEAN, "Use JSSE system properties to configure SSL rather than the " + RPC_PREFIX.getKey() + "javax.net.ssl.* Accumulo properties"), RPC_SSL_CIPHER_SUITES("rpc.ssl.cipher.suites", "", PropertyType.STRING, "Comma separated list of cipher suites that can be used by accepted connections"), + RPC_SSL_ENABLED_PROTOCOLS("rpc.ssl.server.enabled.protocols", "TLSv1,TLSv1.1,TLSv1.2", PropertyType.STRING, + "Comma separated list of protocols that can be used to accept connections"), + // TLSv1.2 should be used as the default when JDK6 support is dropped + RPC_SSL_CLIENT_PROTOCOL("rpc.ssl.client.protocol", "TLSv1", PropertyType.STRING, + "The protocol used to connect to a secure server, must be in the list of enabled protocols on the server side (rpc.ssl.server.enabled.protocols)"), // instance properties (must be the same for every node in an instance) INSTANCE_PREFIX("instance.", null, PropertyType.PREFIX, http://git-wip-us.apache.org/repos/asf/accumulo/blob/520d802e/core/src/main/java/org/apache/accumulo/core/util/SslConnectionParams.java ---------------------------------------------------------------------- diff --git a/core/src/main/java/org/apache/accumulo/core/util/SslConnectionParams.java b/core/src/main/java/org/apache/accumulo/core/util/SslConnectionParams.java index d3b1ae9..cd433c6 100644 --- a/core/src/main/java/org/apache/accumulo/core/util/SslConnectionParams.java +++ b/core/src/main/java/org/apache/accumulo/core/util/SslConnectionParams.java @@ -19,6 +19,7 @@ package org.apache.accumulo.core.util; import java.io.File; import java.io.FileNotFoundException; import java.net.URL; +import java.util.Arrays; import org.apache.accumulo.core.conf.AccumuloConfiguration; import org.apache.accumulo.core.conf.Property; @@ -26,7 +27,7 @@ import org.apache.commons.lang.StringUtils; import org.apache.log4j.Logger; import org.apache.thrift.transport.TSSLTransportFactory.TSSLTransportParameters; -public class SslConnectionParams { +public class SslConnectionParams { private static final Logger log = Logger.getLogger(SslConnectionParams.class); private boolean useJsse = false; @@ -43,6 +44,11 @@ public class SslConnectionParams { private String trustStoreType; private String[] cipherSuites; + private String[] serverProtocols; + private String clientProtocol; + + // Use the static construction methods + private SslConnectionParams() {} public static SslConnectionParams forConfig(AccumuloConfiguration conf, boolean server) { if (!conf.getBoolean(Property.INSTANCE_RPC_SSL_ENABLED)) @@ -74,6 +80,11 @@ public class SslConnectionParams { result.cipherSuites = StringUtils.split(ciphers, ','); } + String enabledProtocols = conf.get(Property.RPC_SSL_ENABLED_PROTOCOLS); + result.serverProtocols = StringUtils.split(enabledProtocols, ','); + + result.clientProtocol = conf.get(Property.RPC_SSL_CLIENT_PROTOCOL); + return result; } @@ -124,9 +135,9 @@ public class SslConnectionParams { // try classpath URL url = SslConnectionParams.class.getClassLoader().getResource(keystorePath); if (url != null) { - file = new File(url.toURI()); - if (file.exists()) - return file.getAbsolutePath(); + file = new File(url.toURI()); + if (file.exists()) + return file.getAbsolutePath(); } } } catch (Exception e) { @@ -151,16 +162,58 @@ public class SslConnectionParams { return clientAuth; } + public String[] getServerProtocols() { + return serverProtocols; + } + + public String getClientProtocol() { + return clientProtocol; + } + + public boolean isKeyStoreSet() { + return keyStoreSet; + } + + public String getKeyStorePath() { + return keyStorePath; + } + + /** + * @return the keyStorePass + */ + public String getKeyStorePass() { + return keyStorePass; + } + + public String getKeyStoreType() { + return keyStoreType; + } + + public boolean isTrustStoreSet() { + return trustStoreSet; + } + + public String getTrustStorePath() { + return trustStorePath; + } + + public String getTrustStorePass() { + return trustStorePass; + } + + /** + * @return the trustStoreType + */ + public String getTrustStoreType() { + return trustStoreType; + } + public TSSLTransportParameters getTTransportParams() { if (useJsse) throw new IllegalStateException("Cannot get TTransportParams for JSEE configuration."); - TSSLTransportParameters params; - if (null != cipherSuites) { - // TLS is the default value used in thrift 0.9.1 - params = new TSSLTransportParameters("TLS", cipherSuites); - } else { - params = new TSSLTransportParameters(); - } + + // Null cipherSuites is implicitly handled + TSSLTransportParameters params = new TSSLTransportParameters(clientProtocol, cipherSuites); params.requireClientAuth(clientAuth); if (keyStoreSet) { @@ -175,18 +228,20 @@ public class SslConnectionParams { @Override public int hashCode() { int hash = 0; - hash = 31*hash + (clientAuth?0:1); - hash = 31*hash + (useJsse?0:1); + hash = 31 * hash + (clientAuth ? 0 : 1); + hash = 31 * hash + (useJsse ? 0 : 1); if (useJsse) return hash; - hash = 31*hash + (keyStoreSet?0:1); - hash = 31*hash + (trustStoreSet?0:1); + hash = 31 * hash + (keyStoreSet ? 0 : 1); + hash = 31 * hash + (trustStoreSet ? 0 : 1); if (keyStoreSet) { - hash = 31*hash + keyStorePath.hashCode(); + hash = 31 * hash + keyStorePath.hashCode(); } if (trustStoreSet) { - hash = 31*hash + trustStorePath.hashCode(); + hash = 31 * hash + trustStorePath.hashCode(); } + hash = 31 * hash + clientProtocol.hashCode(); + hash = 31 * hash + Arrays.hashCode(serverProtocols); return super.hashCode(); } @@ -195,7 +250,7 @@ public class SslConnectionParams { if (!(obj instanceof SslConnectionParams)) return false; - SslConnectionParams other = (SslConnectionParams)obj; + SslConnectionParams other = (SslConnectionParams) obj; if (clientAuth != other.clientAuth) return false; if (useJsse) @@ -203,19 +258,18 @@ public class SslConnectionParams { if (keyStoreSet) { if (!other.keyStoreSet) return false; - if (!keyStorePath.equals(other.keyStorePath) || - !keyStorePass.equals(other.keyStorePass) || - !keyStoreType.equals(other.keyStoreType)) + if (!keyStorePath.equals(other.keyStorePath) || !keyStorePass.equals(other.keyStorePass) || !keyStoreType.equals(other.keyStoreType)) return false; } if (trustStoreSet) { if (!other.trustStoreSet) return false; - if (!trustStorePath.equals(other.trustStorePath) || - !trustStorePass.equals(other.trustStorePass) || - !trustStoreType.equals(other.trustStoreType)) + if (!trustStorePath.equals(other.trustStorePath) || !trustStorePass.equals(other.trustStorePass) || !trustStoreType.equals(other.trustStoreType)) return false; } - return true; + if (!Arrays.equals(serverProtocols, other.serverProtocols)) { + return false; + } + return clientProtocol.equals(other.clientProtocol); } } http://git-wip-us.apache.org/repos/asf/accumulo/blob/520d802e/core/src/main/java/org/apache/accumulo/core/util/ThriftUtil.java ---------------------------------------------------------------------- diff --git a/core/src/main/java/org/apache/accumulo/core/util/ThriftUtil.java b/core/src/main/java/org/apache/accumulo/core/util/ThriftUtil.java index da4e567..69cc242 100644 --- a/core/src/main/java/org/apache/accumulo/core/util/ThriftUtil.java +++ b/core/src/main/java/org/apache/accumulo/core/util/ThriftUtil.java @@ -16,10 +16,25 @@ */ package org.apache.accumulo.core.util; +import java.io.FileInputStream; import java.io.IOException; import java.net.InetAddress; +import java.net.ServerSocket; +import java.net.Socket; +import java.net.UnknownHostException; +import java.security.KeyStore; +import java.util.Arrays; import java.util.HashMap; +import java.util.HashSet; import java.util.Map; +import java.util.Set; + +import javax.net.ssl.KeyManagerFactory; +import javax.net.ssl.SSLContext; +import javax.net.ssl.SSLServerSocket; +import javax.net.ssl.SSLSocket; +import javax.net.ssl.SSLSocketFactory; +import javax.net.ssl.TrustManagerFactory; import org.apache.accumulo.core.client.AccumuloException; import org.apache.accumulo.core.client.AccumuloSecurityException; @@ -48,6 +63,7 @@ import org.apache.thrift.transport.TTransport; import org.apache.thrift.transport.TTransportException; import org.apache.thrift.transport.TTransportFactory; +import com.google.common.base.Preconditions; import com.google.common.net.HostAndPort; public class ThriftUtil { @@ -201,11 +217,34 @@ public class ThriftUtil { } public static TServerSocket getServerSocket(int port, int timeout, InetAddress address, SslConnectionParams params) throws TTransportException { + TServerSocket tServerSock; if (params.useJsse()) { - return TSSLTransportFactory.getServerSocket(port, timeout, params.isClientAuth(), address); + tServerSock = TSSLTransportFactory.getServerSocket(port, timeout, params.isClientAuth(), address); } else { - return TSSLTransportFactory.getServerSocket(port, timeout, address, params.getTTransportParams()); + tServerSock = TSSLTransportFactory.getServerSocket(port, timeout, address, params.getTTransportParams()); + } + + ServerSocket serverSock = tServerSock.getServerSocket(); + if (serverSock instanceof SSLServerSocket) { + SSLServerSocket sslServerSock = (SSLServerSocket) serverSock; + String[] protocols = params.getServerProtocols(); + + // Be nice for the user and automatically remove protocols that might not exist in their JVM. Keeps us from forcing config alterations too + // e.g. TLSv1.1 and TLSv1.2 don't exist in JDK6 + Set<String> socketEnabledProtocols = new HashSet<String>(Arrays.asList(sslServerSock.getEnabledProtocols())); + // Keep only the enabled protocols that were specified by the configuration + socketEnabledProtocols.retainAll(Arrays.asList(protocols)); + if (socketEnabledProtocols.isEmpty()) { + // Bad configuration... + throw new RuntimeException("No available protocols available for secure socket. Availaable protocols: " + + Arrays.toString(sslServerSock.getEnabledProtocols()) + ", allowed protocols: " + Arrays.toString(protocols)); + } + + // Set the protocol(s) on the server socket + sslServerSock.setEnabledProtocols(socketEnabledProtocols.toArray(new String[0])); } + + return tServerSock; } public static TTransport createClientTransport(HostAndPort address, int timeout, SslConnectionParams sslParams) throws TTransportException { @@ -217,7 +256,21 @@ public class ThriftUtil { if (sslParams.useJsse()) { transport = TSSLTransportFactory.getClientSocket(address.getHostText(), address.getPort(), timeout); } else { - transport = TSSLTransportFactory.getClientSocket(address.getHostText(), address.getPort(), timeout, sslParams.getTTransportParams()); + // JDK6's factory doesn't appear to pass the protocol onto the Socket properly so we have + // to do some magic to make sure that happens. Not an issue in JDK7 + + // Taken from thrift-0.9.1 to make the SSLContext + SSLContext sslContext = createSSLContext(sslParams); + + // Create the factory from it + SSLSocketFactory sslSockFactory = sslContext.getSocketFactory(); + + // Wrap the real factory with our own that will set the protocol on the Socket before returning it + ProtocolOverridingSSLSocketFactory wrappingSslSockFactory = new ProtocolOverridingSSLSocketFactory(sslSockFactory, + new String[] {sslParams.getClientProtocol()}); + + // Create the TSocket from that + transport = createClient(wrappingSslSockFactory, address.getHostText(), address.getPort(), timeout); } // TSSLTransportFactory leaves transports open, so no need to open here } else if (timeout == 0) { @@ -240,4 +293,145 @@ public class ThriftUtil { } return transport; } + + /** + * Lifted from TSSLTransportFactory in Thrift-0.9.1. The method to create a client socket with an SSLContextFactory object is not visibile to us. Have to use + * SslConnectionParams instead of TSSLTransportParameters because no getters exist on TSSLTransportParameters. + * + * @param params + * Parameters to use to create the SSLContext + */ + private static SSLContext createSSLContext(SslConnectionParams params) throws TTransportException { + SSLContext ctx; + try { + ctx = SSLContext.getInstance(params.getClientProtocol()); + TrustManagerFactory tmf = null; + KeyManagerFactory kmf = null; + + if (params.isTrustStoreSet()) { + tmf = TrustManagerFactory.getInstance(TrustManagerFactory.getDefaultAlgorithm()); + KeyStore ts = KeyStore.getInstance(params.getTrustStoreType()); + ts.load(new FileInputStream(params.getTrustStorePath()), params.getTrustStorePass().toCharArray()); + tmf.init(ts); + } + + if (params.isKeyStoreSet()) { + kmf = KeyManagerFactory.getInstance(KeyManagerFactory.getDefaultAlgorithm()); + KeyStore ks = KeyStore.getInstance(params.getKeyStoreType()); + ks.load(new FileInputStream(params.getKeyStorePath()), params.getKeyStorePass().toCharArray()); + kmf.init(ks, params.getKeyStorePass().toCharArray()); + } + + if (params.isKeyStoreSet() && params.isTrustStoreSet()) { + ctx.init(kmf.getKeyManagers(), tmf.getTrustManagers(), null); + } else if (params.isKeyStoreSet()) { + ctx.init(kmf.getKeyManagers(), null, null); + } else { + ctx.init(null, tmf.getTrustManagers(), null); + } + + } catch (Exception e) { + throw new TTransportException("Error creating the transport", e); + } + return ctx; + } + + /** + * Lifted from Thrift-0.9.1 because it was private. Create an SSLSocket with the given factory, host:port, and timeout. + * + * @param factory + * Factory to create the socket from + * @param host + * Destination host + * @param port + * Destination port + * @param timeout + * Socket timeout + */ + private static TSocket createClient(SSLSocketFactory factory, String host, int port, int timeout) throws TTransportException { + try { + SSLSocket socket = (SSLSocket) factory.createSocket(host, port); + socket.setSoTimeout(timeout); + return new TSocket(socket); + } catch (Exception e) { + throw new TTransportException("Could not connect to " + host + " on port " + port, e); + } + } + + /** + * JDK6's SSLSocketFactory doesn't seem to properly set the protocols on the Sockets that it creates which causes an SSLv2 client hello message during + * handshake, even when only TLSv1 is enabled. This only appears to be an issue on the client sockets, not the server sockets. + * + * This class wraps the SSLSocketFactory ensuring that the Socket is properly configured. + * http://www.coderanch.com/t/637177/Security/Disabling-handshake-message-Java + * + * This class can be removed when JDK6 support is officially unsupported by Accumulo + */ + private static class ProtocolOverridingSSLSocketFactory extends SSLSocketFactory { + + private final SSLSocketFactory delegate; + private final String[] enabledProtocols; + + public ProtocolOverridingSSLSocketFactory(final SSLSocketFactory delegate, final String[] enabledProtocols) { + Preconditions.checkNotNull(enabledProtocols); + Preconditions.checkArgument(0 != enabledProtocols.length, "Expected at least one protocol"); + this.delegate = delegate; + this.enabledProtocols = enabledProtocols; + } + + @Override + public String[] getDefaultCipherSuites() { + return delegate.getDefaultCipherSuites(); + } + + @Override + public String[] getSupportedCipherSuites() { + return delegate.getSupportedCipherSuites(); + } + + @Override + public Socket createSocket(final Socket socket, final String host, final int port, final boolean autoClose) throws IOException { + final Socket underlyingSocket = delegate.createSocket(socket, host, port, autoClose); + return overrideProtocol(underlyingSocket); + } + + @Override + public Socket createSocket(final String host, final int port) throws IOException, UnknownHostException { + final Socket underlyingSocket = delegate.createSocket(host, port); + return overrideProtocol(underlyingSocket); + } + + @Override + public Socket createSocket(final String host, final int port, final InetAddress localAddress, final int localPort) throws IOException, UnknownHostException { + final Socket underlyingSocket = delegate.createSocket(host, port, localAddress, localPort); + return overrideProtocol(underlyingSocket); + } + + @Override + public Socket createSocket(final InetAddress host, final int port) throws IOException { + final Socket underlyingSocket = delegate.createSocket(host, port); + return overrideProtocol(underlyingSocket); + } + + @Override + public Socket createSocket(final InetAddress host, final int port, final InetAddress localAddress, final int localPort) throws IOException { + final Socket underlyingSocket = delegate.createSocket(host, port, localAddress, localPort); + return overrideProtocol(underlyingSocket); + } + + /** + * Set the {@link javax.net.ssl.SSLSocket#getEnabledProtocols() enabled protocols} to {@link #enabledProtocols} if the <code>socket</code> is a + * {@link SSLSocket} + * + * @param socket + * The Socket + * @return + */ + private Socket overrideProtocol(final Socket socket) { + if (socket instanceof SSLSocket) { + ((SSLSocket) socket).setEnabledProtocols(enabledProtocols); + } + return socket; + } + } } http://git-wip-us.apache.org/repos/asf/accumulo/blob/520d802e/server/base/src/main/java/org/apache/accumulo/server/Accumulo.java ---------------------------------------------------------------------- diff --git a/server/base/src/main/java/org/apache/accumulo/server/Accumulo.java b/server/base/src/main/java/org/apache/accumulo/server/Accumulo.java index a1581c4..8fc2327 100644 --- a/server/base/src/main/java/org/apache/accumulo/server/Accumulo.java +++ b/server/base/src/main/java/org/apache/accumulo/server/Accumulo.java @@ -212,7 +212,7 @@ public class Accumulo { // Encourage users to configure TLS final String SSL = "SSL"; - for (Property sslProtocolProperty : Arrays.asList(Property.MONITOR_SSL_INCLUDE_PROTOCOLS)) { + for (Property sslProtocolProperty : Arrays.asList(Property.RPC_SSL_CLIENT_PROTOCOL, Property.RPC_SSL_ENABLED_PROTOCOLS, Property.MONITOR_SSL_INCLUDE_PROTOCOLS)) { String value = conf.get(sslProtocolProperty); if (value.contains(SSL)) { log.warn("It is recommended that " + sslProtocolProperty + " only allow TLS");
