Mailing-List: contact commits-help@accumulo.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@accumulo.apache.org
Content-Type: text/plain; charset="us-ascii"
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
From: elserj@apache.org
To: commits@accumulo.apache.org
Date: Sat, 08 Nov 2014 00:19:42 -0000
Message-Id: <239755f662e34826b914ed971f63baa4@git.apache.org>
Subject: [1/3] accumulo git commit: ACCUMULO-3318 Update thrift RPC secure
 socket creation to default to TLS

Repository: accumulo
Updated Branches:
  refs/heads/1.6 9ea8f09a8 -> 520d802ec
  refs/heads/master 566839ef7 -> 7e0121d11


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/1.6
Commit: 520d802ec5ab33a8ce6a96672edaba58ab6aef37
Parents: 9ea8f09
Author: Josh Elser <elserj@apache.org>
Authored: Fri Nov 7 19:18:03 2014 -0500
Committer: Josh Elser <elserj@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");