[GitHub] drill pull request #950: DRILL-5431: SSL Support

[asfgit]

Github user asfgit closed the pull request at:

https://github.com/apache/drill/pull/950


---

[GitHub] drill pull request #950: DRILL-5431: SSL Support

[parthchandra]

Github user parthchandra commented on a diff in the pull request:

https://github.com/apache/drill/pull/950#discussion_r143246761
  
--- Diff: 
exec/java-exec/src/test/java/org/apache/drill/exec/rpc/user/security/TestUserBitSSL.java
 ---
@@ -201,6 +202,7 @@ public void testSSLQuery() throws Exception {
 test("SELECT * FROM cp.`region.json`");
   }
 
+  @Ignore
--- End diff --

Will do.


---

[GitHub] drill pull request #950: DRILL-5431: SSL Support

[arina-ielchiieva]

Github user arina-ielchiieva commented on a diff in the pull request:

https://github.com/apache/drill/pull/950#discussion_r143152156
  
--- Diff: 
exec/java-exec/src/test/java/org/apache/drill/exec/TestSSLConfig.java ---
@@ -76,7 +76,7 @@ public void testMissingKeystorePassword() throws 
Exception {
   fail();
   //Expected
 } catch (Exception e) {
-
+  assertTrue(e instanceof DrillException);
--- End diff --

Thanks, for adding `assertTrue`.


---

[GitHub] drill pull request #950: DRILL-5431: SSL Support

[arina-ielchiieva]

Github user arina-ielchiieva commented on a diff in the pull request:

https://github.com/apache/drill/pull/950#discussion_r143151998
  
--- Diff: 
exec/java-exec/src/test/java/org/apache/drill/exec/rpc/user/security/TestUserBitSSL.java
 ---
@@ -201,6 +202,7 @@ public void testSSLQuery() throws Exception {
 test("SELECT * FROM cp.`region.json`");
   }
 
+  @Ignore
--- End diff --

Parth, can you please add explanation (`@Ignore("some reason")`) why test 
is ignored (it would be really helpful in future).


---

[GitHub] drill pull request #950: DRILL-5431: SSL Support

[parthchandra]

Github user parthchandra commented on a diff in the pull request:

https://github.com/apache/drill/pull/950#discussion_r143042574
  
--- Diff: contrib/native/client/src/clientlib/wincert.ipp ---
@@ -0,0 +1,98 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#if defined(IS_SSL_ENABLED)
+
+#include 
+#include 
+
+#if defined _WIN32  || defined _WIN64
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+
+#pragma comment (lib, "crypt32.lib")
+#pragma comment (lib, "cryptui.lib")
+
+#define MY_ENCODING_TYPE  (PKCS_7_ASN_ENCODING | X509_ASN_ENCODING)
+
+inline
+int loadSystemTrustStore(const SSL *ssl, std::string& msg) {
--- End diff --

I'm actually already using these methods (see `SSLStreamChannel::init()`). 
The verification callback implements validating the certificate. In our case we 
are using the boost provided rfc2818 verification method. The load verify file 
should point to the truststore containing the certificates in pem format. 
OpenSSL will read this file and load the certificate into its in-memory X509 
certificate store.
The `loadSystemTrustStore` method reads the certificates from the Windows 
store (probably the registry) converts from the native store format into X509 
and then loads it into the in-memory store. After that OpenSSL takes over and 
does the verification. 
For Keychain, we will have to do something similar. 
Writing our own certificate verification is going to be error prone, 
especially if you want to do rfc2818 verification. Not sure I'm up to it :(.


---

[GitHub] drill pull request #950: DRILL-5431: SSL Support

[laurentgo]

Github user laurentgo commented on a diff in the pull request:

https://github.com/apache/drill/pull/950#discussion_r142994068
  
--- Diff: contrib/native/client/src/clientlib/channel.hpp ---
@@ -0,0 +1,236 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#ifndef CHANNEL_HPP
+#define CHANNEL_HPP
+
+#include "drill/common.hpp"
+#include "drill/drillClient.hpp"
+#include "streamSocket.hpp"
+
+namespace Drill {
+
+class UserProperties;
+
+class ConnectionEndpoint{
+public:
+ConnectionEndpoint(const char* connStr);
+ConnectionEndpoint(const char* host, const char* port);
+~ConnectionEndpoint();
+
+//parse the connection string and set up the host and port to 
connect to
+connectionStatus_t getDrillbitEndpoint();
+
+std::string& getProtocol(){return m_protocol;}
+std::string& getHost(){return m_host;}
+std::string& getPort(){return m_port;}
+DrillClientError* getError(){ return m_pError;};
+
+private:
+void parseConnectString();
+bool isDirectConnection();
+bool isZookeeperConnection();
+connectionStatus_t getDrillbitEndpointFromZk();
+connectionStatus_t handleError(connectionStatus_t status, 
std::string msg);
+
+std::string m_connectString;
+std::string m_pathToDrill;
+std::string m_protocol; 
+std::string m_hostPortStr;
+std::string m_host;
+std::string m_port;
+
+DrillClientError* m_pError;
+
+};
+
+class ChannelContext{
+public:
+ChannelContext(DrillUserProperties* 
props):m_properties(props){};
+virtual ~ChannelContext(){};
+const DrillUserProperties* getUserProperties() const { return 
m_properties;}
+protected:
+DrillUserProperties* m_properties;
+};
+
+class SSLChannelContext: public ChannelContext{
+public:
+static boost::asio::ssl::context::method 
getTlsVersion(std::string version){
--- End diff --

sorry, it's abbreviation for const ref (I believe std::cref was introduced 
in C++11, which we are not using sadly, although there's a boost equivalent of 
course).


---

[GitHub] drill pull request #950: DRILL-5431: SSL Support

[laurentgo]

Github user laurentgo commented on a diff in the pull request:

https://github.com/apache/drill/pull/950#discussion_r142993025
  
--- Diff: contrib/native/client/src/clientlib/wincert.ipp ---
@@ -0,0 +1,98 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#if defined(IS_SSL_ENABLED)
+
+#include 
+#include 
+
+#if defined _WIN32  || defined _WIN64
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+
+#pragma comment (lib, "crypt32.lib")
+#pragma comment (lib, "cryptui.lib")
+
+#define MY_ENCODING_TYPE  (PKCS_7_ASN_ENCODING | X509_ASN_ENCODING)
+
+inline
+int loadSystemTrustStore(const SSL *ssl, std::string& msg) {
--- End diff --

it looks like boost::asio support both loading a file and/or a verify 
callback:
- 
http://www.boost.org/doc/libs/1_47_0/doc/html/boost_asio/reference/ssl__context/set_verify_callback/overload1.html
- 
http://www.boost.org/doc/libs/1_47_0/doc/html/boost_asio/reference/ssl__context/load_verify_file.html

It seems you wouldn't even need to access the native handle when using 
these functions


---

[GitHub] drill pull request #950: DRILL-5431: SSL Support

[arina-ielchiieva]

Github user arina-ielchiieva commented on a diff in the pull request:

https://github.com/apache/drill/pull/950#discussion_r142862510
  
--- Diff: 
exec/java-exec/src/test/java/org/apache/drill/exec/TestSSLConfig.java ---
@@ -49,12 +64,19 @@ public void testMissingKeystorePassword() throws 
Exception {
 ConfigBuilder config = new ConfigBuilder();
 config.put(ExecConstants.HTTP_KEYSTORE_PATH, "/root");
 config.put(ExecConstants.HTTP_KEYSTORE_PASSWORD, "");
+config.put(ExecConstants.SSL_USE_HADOOP_CONF, false);
+config.put(ExecConstants.USER_SSL_ENABLED, true);
 try {
-  SSLConfig sslv = new SSLConfig(config.build());
+  SSLConfig sslv = new SSLConfigBuilder()
+  .config(config.build())
+  .mode(SSLFactory.Mode.SERVER)
+  .initializeSSLContext(false)
+  .validateKeyStore(true)
+  .build();
   fail();
   //Expected
 } catch (Exception e) {
-  assertTrue(e instanceof DrillException);
+
--- End diff --

But the assert `assertTrue(e instanceof DrillException);` was removed and 
catch block is empty, test will never fail...


---

[GitHub] drill pull request #950: DRILL-5431: SSL Support

[arina-ielchiieva]

Github user arina-ielchiieva commented on a diff in the pull request:

https://github.com/apache/drill/pull/950#discussion_r142861999
  
--- Diff: 
exec/java-exec/src/test/java/org/apache/drill/exec/rpc/user/security/TestUserBitSSL.java
 ---
@@ -223,9 +223,12 @@ public void testClientConfigHostnameVerification() {
   ts.load(null, password.toCharArray());
   ts.setCertificateEntry("drillTest", certificate.cert());
   // Store away the truststore.
-  FileOutputStream fos1 = new FileOutputStream(tempFile1);
-  ts.store(fos1, password.toCharArray());
-  fos1.close();
+  try (FileOutputStream fos1 = new FileOutputStream(tempFile1);) {
+ts.store(fos1, password.toCharArray());
+fos1.close();
--- End diff --

No need to close stream. It will be closed automatically.


---

[GitHub] drill pull request #950: DRILL-5431: SSL Support

[parthchandra]

Github user parthchandra commented on a diff in the pull request:

https://github.com/apache/drill/pull/950#discussion_r142830326
  
--- Diff: contrib/native/client/src/clientlib/drillClientImpl.cpp ---
@@ -250,7 +205,15 @@ void DrillClientImpl::doWriteToSocket(const char* 
dataPtr, size_t bytesToWrite,
 // Write all the bytes to socket. In case of error when all bytes are 
not successfully written
 // proper errorCode will be set.
 while(1) {
-size_t bytesWritten = 
m_socket.write_some(boost::asio::buffer(dataPtr, bytesToWrite), errorCode);
+size_t bytesWritten;
+{
+boost::lock_guard lock(m_channelMutex);
--- End diff --

Oh this was found by Rob Wu. The problem occurs when the heartbeat timer 
has gone off and we are in the handler which is about to send off a heartbeat. 
Before the heartbeat is sent if the caller deletes DrillClientImpl (via 
DrillClient) then the channel may be closed and the pointer to the channel may 
be set to null causing the heartbeat send to crash. This did not occur 
previously because the socket was not a pointer and/or boost was able to handle 
it quite nicely. 


---

[GitHub] drill pull request #950: DRILL-5431: SSL Support

[parthchandra]

Github user parthchandra commented on a diff in the pull request:

https://github.com/apache/drill/pull/950#discussion_r142827719
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/ssl/SSLConfigServer.java ---
@@ -0,0 +1,331 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.drill.exec.ssl;
+
+import com.google.common.base.Preconditions;
+import io.netty.handler.ssl.SslContext;
+import io.netty.handler.ssl.SslContextBuilder;
+import io.netty.handler.ssl.SslProvider;
+import org.apache.drill.common.config.DrillConfig;
+import org.apache.drill.common.exceptions.DrillException;
+import org.apache.drill.exec.ExecConstants;
+import org.apache.drill.exec.memory.BufferAllocator;
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.security.ssl.SSLFactory;
+
+import javax.net.ssl.KeyManagerFactory;
+import javax.net.ssl.SSLContext;
+import javax.net.ssl.SSLEngine;
+import javax.net.ssl.TrustManagerFactory;
+import java.text.MessageFormat;
+
+public class SSLConfigServer extends SSLConfig {
+
+  private static final org.slf4j.Logger logger = 
org.slf4j.LoggerFactory.getLogger(SSLConfigServer.class);
+
+  private final DrillConfig config;
+  private final Configuration hadoopConfig;
+  private final boolean userSslEnabled;
+  private final boolean httpsEnabled;
+  private final String keyStoreType;
+  private final String keyStorePath;
+  private final String keyStorePassword;
+  private final String keyPassword;
+  private final String trustStoreType;
+  private final String trustStorePath;
+  private final String trustStorePassword;
+  private final String protocol;
+  private final String provider;
+
+  public SSLConfigServer(DrillConfig config, Configuration hadoopConfig) 
throws DrillException {
+this.config = config;
+SSLFactory.Mode mode = SSLFactory.Mode.SERVER;
+httpsEnabled =
+config.hasPath(ExecConstants.HTTP_ENABLE_SSL) && 
config.getBoolean(ExecConstants.HTTP_ENABLE_SSL);
+// For testing we will mock up a hadoop configuration, however for 
regular use, we find the actual hadoop config.
+boolean enableHadoopConfig = 
config.getBoolean(ExecConstants.SSL_USE_HADOOP_CONF);
+if (enableHadoopConfig) {
+  if (hadoopConfig == null) {
+this.hadoopConfig = new Configuration(); // get hadoop 
configuration
+  } else {
+this.hadoopConfig = hadoopConfig;
+  }
+  String hadoopSSLConfigFile =
+  
this.hadoopConfig.get(resolveHadoopPropertyName(HADOOP_SSL_CONF_TPL_KEY, 
getMode()));
+  logger.debug("Using Hadoop configuration for SSL");
+  logger.debug("Hadoop SSL configuration file: {}", 
hadoopSSLConfigFile);
+  this.hadoopConfig.addResource(hadoopSSLConfigFile);
+} else {
+  this.hadoopConfig = null;
+}
+userSslEnabled =
+config.hasPath(ExecConstants.USER_SSL_ENABLED) && 
config.getBoolean(ExecConstants.USER_SSL_ENABLED);
+trustStoreType = getConfigParam(ExecConstants.SSL_TRUSTSTORE_TYPE,
+resolveHadoopPropertyName(HADOOP_SSL_TRUSTSTORE_TYPE_TPL_KEY, 
mode));
+trustStorePath = getConfigParam(ExecConstants.SSL_TRUSTSTORE_PATH,
+resolveHadoopPropertyName(HADOOP_SSL_TRUSTSTORE_LOCATION_TPL_KEY, 
mode));
+trustStorePassword = 
getConfigParam(ExecConstants.SSL_TRUSTSTORE_PASSWORD,
+resolveHadoopPropertyName(HADOOP_SSL_TRUSTSTORE_PASSWORD_TPL_KEY, 
mode));
+keyStoreType = getConfigParam(ExecConstants.SSL_KEYSTORE_TYPE,
+resolveHadoopPropertyName(HADOOP_SSL_KEYSTORE_TYPE_TPL_KEY, mode));
+keyStorePath = getConfigParam(ExecConstants.SSL_KEYSTORE_PATH,
+resolveHadoopPropertyName(HADOOP_SSL_KEYSTORE_LOCATION_TPL_KEY, 
mode));
+keyStorePassword = getConfigParam(ExecConstants.SSL_KEYSTORE_PASSWORD,
+resolveHadoopPropertyName(HADOOP_SSL_KEYSTORE_PASSWORD_TPL_KEY, 
mode));
+

[GitHub] drill pull request #950: DRILL-5431: SSL Support

[parthchandra]

Github user parthchandra commented on a diff in the pull request:

https://github.com/apache/drill/pull/950#discussion_r142827417
  
--- Diff: exec/java-exec/pom.xml ---
@@ -589,6 +669,17 @@
 
   
 
+
+  
+  
+kr.motd.maven
+os-maven-plugin
+1.4.0.Final
--- End diff --

I can build easily on Centos. But I took your tip and updated it to 
1.5.0.Final. Hopefully that fixes it for you as well.


---

[GitHub] drill pull request #950: DRILL-5431: SSL Support

[parthchandra]

Github user parthchandra commented on a diff in the pull request:

https://github.com/apache/drill/pull/950#discussion_r142827859
  
--- Diff: 
exec/java-exec/src/test/java/org/apache/drill/exec/TestSSLConfig.java ---
@@ -49,12 +64,19 @@ public void testMissingKeystorePassword() throws 
Exception {
 ConfigBuilder config = new ConfigBuilder();
 config.put(ExecConstants.HTTP_KEYSTORE_PATH, "/root");
 config.put(ExecConstants.HTTP_KEYSTORE_PASSWORD, "");
+config.put(ExecConstants.SSL_USE_HADOOP_CONF, false);
+config.put(ExecConstants.USER_SSL_ENABLED, true);
 try {
-  SSLConfig sslv = new SSLConfig(config.build());
+  SSLConfig sslv = new SSLConfigBuilder()
+  .config(config.build())
+  .mode(SSLFactory.Mode.SERVER)
+  .initializeSSLContext(false)
+  .validateKeyStore(true)
+  .build();
   fail();
   //Expected
 } catch (Exception e) {
-  assertTrue(e instanceof DrillException);
+
--- End diff --

If the hadoop config is invalid the build method will throw an exception. 
So we are looking to make sure the build method does, in fact, throw an 
exception. If it does the test passes, otherwise it fails.


---

[GitHub] drill pull request #950: DRILL-5431: SSL Support

[parthchandra]

Github user parthchandra commented on a diff in the pull request:

https://github.com/apache/drill/pull/950#discussion_r142827572
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/ssl/SSLConfig.java ---
@@ -0,0 +1,265 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.drill.exec.ssl;
+
+import io.netty.handler.ssl.SslContext;
+import io.netty.handler.ssl.SslProvider;
+import io.netty.handler.ssl.util.InsecureTrustManagerFactory;
+import org.apache.drill.common.exceptions.DrillException;
+import org.apache.drill.exec.memory.BufferAllocator;
+import org.apache.hadoop.security.ssl.SSLFactory;
+
+import javax.net.ssl.KeyManagerFactory;
+import javax.net.ssl.SSLContext;
+import javax.net.ssl.SSLEngine;
+import javax.net.ssl.TrustManagerFactory;
+import java.io.FileInputStream;
+import java.io.InputStream;
+import java.security.KeyStore;
+
+public abstract class SSLConfig {
+
+  private static final org.slf4j.Logger logger = 
org.slf4j.LoggerFactory.getLogger(SSLConfig.class);
+
+  public static final String DEFAULT_SSL_PROVIDER = "JDK"; // JDK or 
OPENSSL
+  public static final String DEFAULT_SSL_PROTOCOL = "TLSv1.2";
+  public static final int DEFAULT_SSL_HANDSHAKE_TIMEOUT_MS = 10 * 1000; // 
10 seconds
+
+  // Either the Netty SSL context or the JDK SSL context will be 
initialized
+  // The JDK SSL context is use iff the useSystemTrustStore setting is 
enabled.
+  protected SslContext nettySslContext;
+  protected SSLContext jdkSSlContext;
+
+  private static final boolean isWindows = 
System.getProperty("os.name").toLowerCase().indexOf("win") >= 0;
+  private static final boolean isMacOs = 
System.getProperty("os.name").toLowerCase().indexOf("mac") >= 0;
+
+  public static final String HADOOP_SSL_CONF_TPL_KEY = 
"hadoop.ssl.{0}.conf";
+  public static final String HADOOP_SSL_KEYSTORE_LOCATION_TPL_KEY = 
"ssl.{0}.keystore.location";
+  public static final String HADOOP_SSL_KEYSTORE_PASSWORD_TPL_KEY = 
"ssl.{0}.keystore.password";
+  public static final String HADOOP_SSL_KEYSTORE_TYPE_TPL_KEY = 
"ssl.{0}.keystore.type";
+  public static final String HADOOP_SSL_KEYSTORE_KEYPASSWORD_TPL_KEY =
+  "ssl.{0}.keystore.keypassword";
+  public static final String HADOOP_SSL_TRUSTSTORE_LOCATION_TPL_KEY = 
"ssl.{0}.truststore.location";
+  public static final String HADOOP_SSL_TRUSTSTORE_PASSWORD_TPL_KEY = 
"ssl.{0}.truststore.password";
+  public static final String HADOOP_SSL_TRUSTSTORE_TYPE_TPL_KEY = 
"ssl.{0}.truststore.type";
+
+  public SSLConfig() {
+  }
+
+  public abstract void validateKeyStore() throws DrillException;
+
+  // We need to use different SSLContext objects depending on what the 
user has chosen
+  // For most uses we will use the Netty SslContext class. This allows us 
to use either
+  // the JDK implementation or the OpenSSL implementation. However if the 
user wants to
+  // use the system trust store, then the only way to access it is via the 
JDK's
+  // SSLContext class. (See the createSSLEngine method below).
+
+  public abstract SslContext initNettySslContext() throws DrillException;
+
+  public abstract SSLContext initJDKSSLContext() throws DrillException;
+
+  public abstract boolean isUserSslEnabled();
+
+  public abstract boolean isHttpsEnabled();
+
+  public abstract String getKeyStoreType();
+
+  public abstract String getKeyStorePath();
+
+  public abstract String getKeyStorePassword();
+
+  public abstract String getKeyPassword();
+
+  public abstract String getTrustStoreType();
+
+  public abstract boolean hasTrustStorePath();
+
+  public abstract String getTrustStorePath();
+
+  public abstract boolean hasTrustStorePassword();
+
+  public abstract String getTrustStorePassword();
+
+  public abstract String getProtocol();
+
+  public abstract SslProvider getProvider();
+
+  

[GitHub] drill pull request #950: DRILL-5431: SSL Support

[parthchandra]

Github user parthchandra commented on a diff in the pull request:

https://github.com/apache/drill/pull/950#discussion_r142827694
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/ssl/SSLConfigClient.java ---
@@ -0,0 +1,273 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.drill.exec.ssl;
+
+import io.netty.handler.ssl.SslContext;
+import io.netty.handler.ssl.SslContextBuilder;
+import io.netty.handler.ssl.SslProvider;
+import org.apache.drill.common.config.DrillConfig;
+import org.apache.drill.common.config.DrillProperties;
+import org.apache.drill.common.exceptions.DrillException;
+import org.apache.drill.exec.memory.BufferAllocator;
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.security.ssl.SSLFactory;
+
+import javax.net.ssl.SSLContext;
+import javax.net.ssl.SSLEngine;
+import javax.net.ssl.SSLParameters;
+import javax.net.ssl.TrustManagerFactory;
+import java.util.Properties;
+
+public class SSLConfigClient extends SSLConfig {
+
+  private static final org.slf4j.Logger logger = 
org.slf4j.LoggerFactory.getLogger(SSLConfigClient.class);
+
+  Properties properties;
--- End diff --

OK


---

[GitHub] drill pull request #950: DRILL-5431: SSL Support

[parthchandra]

Github user parthchandra commented on a diff in the pull request:

https://github.com/apache/drill/pull/950#discussion_r142827342
  
--- Diff: distribution/pom.xml ---
@@ -97,8 +97,41 @@
 
   org.apache.hbase
   hbase-client
+  
+   
+   io.netty
+   netty
+   
+  
+
+
--- End diff --

I did it this way because the assembly plugin includes jars with test scope 
and the parent pom needs to match what's in the assembly plugin. My maven isn't 
fluent enough to explain why that should be so, or why we ask the assembly 
plugin to package the jars in test scope. If you know how/why I can/should 
change this, I'll be happy to do so (probably as a separate JIRA).


---

[GitHub] drill pull request #950: DRILL-5431: SSL Support

[parthchandra]

Github user parthchandra commented on a diff in the pull request:

https://github.com/apache/drill/pull/950#discussion_r142822245
  
--- Diff: contrib/native/client/readme.ssl ---
@@ -0,0 +1,58 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+Installing OpenSSL - 
+On Mac: 
+brew install openssl
+On Linux :
+
+Set up the certificate
+Generate a private key
+
+openssl genrsa -des3 -out drillTestServerKey.pem 1024
+
+Generate Certificate signing request
+
+openssl req -new -key drillTestServerKey.pem -out 
drillTestServer.csr
+
+Sign certificate with private key
+
+openssl x509 -req -days 3650 -in drillTestServer.csr -signkey 
drillTestServerKey.pem -out drillTestCert.pem
+
+Remove password requirement (needed for example)
+
+cp drillTestServerKey.pem drillTestServerKey.safe.pem
+openssl rsa -in drillTestServerKey.safe.pem -out 
drillTestServerKey.pem
+
+Generate dhparam file
+
+openssl dhparam -out dh512.pem 512
--- End diff --

The server side of the boost example uses this file to initialize 
diffie-helman key exchange. It is not needed.


---

[GitHub] drill pull request #950: DRILL-5431: SSL Support

[parthchandra]

Github user parthchandra commented on a diff in the pull request:

https://github.com/apache/drill/pull/950#discussion_r142822091
  
--- Diff: contrib/native/client/readme.linux ---
@@ -84,6 +84,21 @@ OR
 ln -svf libboost_filesystem.a libboost_filesystem-mt.a
 ln -svf libboost_date_time.a libboost_date_time-mt.a
 
+5) Install or  build Cyrus SASL 
+   To Install 
+   yum install cyrus-sasl-devel
+   yum install cyrus-sasl-gssapi
+   libs are installed in /usr/lib64/sasl2
+   includes are installed in /usr/include
+
+   To build your own 
+   See readme.sasl for instructions
+
+6) Install OpenSSL
+   yum install openssl
--- End diff --

Yes, probably not needed. But I left it in there, just in case. Yum can 
easily figure it out :)


---

[GitHub] drill pull request #950: DRILL-5431: SSL Support

[parthchandra]

Github user parthchandra commented on a diff in the pull request:

https://github.com/apache/drill/pull/950#discussion_r142822382
  
--- Diff: contrib/native/client/src/clientlib/channel.hpp ---
@@ -0,0 +1,236 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#ifndef CHANNEL_HPP
+#define CHANNEL_HPP
+
+#include "drill/common.hpp"
+#include "drill/drillClient.hpp"
+#include "streamSocket.hpp"
+
+namespace Drill {
+
+class UserProperties;
+
+class ConnectionEndpoint{
+public:
+ConnectionEndpoint(const char* connStr);
+ConnectionEndpoint(const char* host, const char* port);
--- End diff --

Well the tcp resolver uses a string and the parse method gets a string from 
the connect string, so why convert?


---

[GitHub] drill pull request #950: DRILL-5431: SSL Support

[parthchandra]

Github user parthchandra commented on a diff in the pull request:

https://github.com/apache/drill/pull/950#discussion_r142821870
  
--- Diff: contrib/native/client/CMakeLists.txt ---
@@ -93,7 +94,7 @@ else()
 #of boost. Arbirtarily, we choose the new namspace to be 
drill_boost.
 #See the instructions in the readme for linux/macos and rebuild 
boost. Then
 #uncomment the line below to build
-set(Boost_NAMESPACE drill_boost)
+#set(Boost_NAMESPACE drill_boost)
--- End diff --

Not really. I undid what was probably a temporary change. The default has 
always been to not have to use a custom boost build.


---

[GitHub] drill pull request #950: DRILL-5431: SSL Support

[parthchandra]

Github user parthchandra commented on a diff in the pull request:

https://github.com/apache/drill/pull/950#discussion_r142821976
  
--- Diff: contrib/native/client/example/querySubmitter.cpp ---
@@ -544,4 +573,4 @@ int main(int argc, char* argv[]) {
 }
 
 return 0;
-}
+}
--- End diff --

added newline


---

[GitHub] drill pull request #950: DRILL-5431: SSL Support

[parthchandra]

Github user parthchandra commented on a diff in the pull request:

https://github.com/apache/drill/pull/950#discussion_r142825441
  
--- Diff: contrib/native/client/src/include/drill/drillConfig.hpp ---
@@ -0,0 +1,153 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+
+#ifndef DRILL_CONFIG_H
+#define DRILL_CONFIG_H
+
+#include "drill/common.hpp"
+#include 
--- End diff --

Done


---

[GitHub] drill pull request #950: DRILL-5431: SSL Support

[parthchandra]

Github user parthchandra commented on a diff in the pull request:

https://github.com/apache/drill/pull/950#discussion_r142822299
  
--- Diff: contrib/native/client/src/clientlib/CMakeLists.txt ---
@@ -36,6 +40,7 @@ include_directories(${CMAKE_CURRENT_SOURCE_DIR} 
${CMAKE_CURRENT_SOURCE_DIR}/../i
 include_directories(${PROTOBUF_INCLUDE_DIR})
 include_directories(${Zookeeper_INCLUDE_DIRS})
 include_directories(${SASL_INCLUDE_DIRS})
+include_directories("${OPENSSL_INCLUDE_DIR}")
--- End diff --

Yeah. I wonder why I did that ...


---

[GitHub] drill pull request #950: DRILL-5431: SSL Support

[parthchandra]

Github user parthchandra commented on a diff in the pull request:

https://github.com/apache/drill/pull/950#discussion_r142825478
  
--- Diff: contrib/native/client/src/include/drill/drillConfig.hpp ---
@@ -0,0 +1,153 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+
+#ifndef DRILL_CONFIG_H
+#define DRILL_CONFIG_H
+
+#include "drill/common.hpp"
+#include 
+
+
+
+#if defined _WIN32 || defined __CYGWIN__
--- End diff --

Done. It was already in common.hpp


---

[GitHub] drill pull request #950: DRILL-5431: SSL Support

[parthchandra]

Github user parthchandra commented on a diff in the pull request:

https://github.com/apache/drill/pull/950#discussion_r142821887
  
--- Diff: contrib/native/client/example/querySubmitter.cpp ---
@@ -50,7 +49,14 @@ struct Option{
 {"service_host", "Service host for Kerberos", false},
 {"service_name", "Service name for Kerberos", false},
 {"auth", "Authentication mechanism to use", false},
-{"sasl_encrypt", "Negotiate for encrypted connection", false}
+{"sasl_encrypt", "Negotiate for encrypted connection", false},
+{"enableSSL", "Enable SSL", false},
+{"TLSProtocol", "TLS protocol version", false},
+{"certFilePath", "Path to SSL certificate file", false},
+{"disableHostnameVerification", "disable host name verification", 
false},
+{"disableCertVerification", "disable certificate verification", false},
+   {"useSystemTrustStore", "[Windows only]. Use the system truststore.", 
false }
--- End diff --

done


---

[GitHub] drill pull request #950: DRILL-5431: SSL Support

[parthchandra]

Github user parthchandra commented on a diff in the pull request:

https://github.com/apache/drill/pull/950#discussion_r142824514
  
--- Diff: contrib/native/client/src/clientlib/channel.hpp ---
@@ -0,0 +1,236 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#ifndef CHANNEL_HPP
+#define CHANNEL_HPP
+
+#include "drill/common.hpp"
+#include "drill/drillClient.hpp"
+#include "streamSocket.hpp"
+
+namespace Drill {
+
+class UserProperties;
+
+class ConnectionEndpoint{
+public:
+ConnectionEndpoint(const char* connStr);
+ConnectionEndpoint(const char* host, const char* port);
+~ConnectionEndpoint();
+
+//parse the connection string and set up the host and port to 
connect to
+connectionStatus_t getDrillbitEndpoint();
+
+std::string& getProtocol(){return m_protocol;}
+std::string& getHost(){return m_host;}
+std::string& getPort(){return m_port;}
+DrillClientError* getError(){ return m_pError;};
+
+private:
+void parseConnectString();
+bool isDirectConnection();
+bool isZookeeperConnection();
+connectionStatus_t getDrillbitEndpointFromZk();
+connectionStatus_t handleError(connectionStatus_t status, 
std::string msg);
+
+std::string m_connectString;
+std::string m_pathToDrill;
+std::string m_protocol; 
+std::string m_hostPortStr;
+std::string m_host;
+std::string m_port;
+
+DrillClientError* m_pError;
+
+};
+
+class ChannelContext{
+public:
+ChannelContext(DrillUserProperties* 
props):m_properties(props){};
+virtual ~ChannelContext(){};
+const DrillUserProperties* getUserProperties() const { return 
m_properties;}
+protected:
+DrillUserProperties* m_properties;
+};
+
+class SSLChannelContext: public ChannelContext{
+public:
+static boost::asio::ssl::context::method 
getTlsVersion(std::string version){
+if(version.empty()){
--- End diff --

Done


---

[GitHub] drill pull request #950: DRILL-5431: SSL Support

[parthchandra]

Github user parthchandra commented on a diff in the pull request:

https://github.com/apache/drill/pull/950#discussion_r142822406
  
--- Diff: contrib/native/client/src/clientlib/channel.hpp ---
@@ -0,0 +1,236 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#ifndef CHANNEL_HPP
+#define CHANNEL_HPP
+
+#include "drill/common.hpp"
+#include "drill/drillClient.hpp"
+#include "streamSocket.hpp"
+
+namespace Drill {
+
+class UserProperties;
+
+class ConnectionEndpoint{
+public:
+ConnectionEndpoint(const char* connStr);
+ConnectionEndpoint(const char* host, const char* port);
+~ConnectionEndpoint();
+
+//parse the connection string and set up the host and port to 
connect to
+connectionStatus_t getDrillbitEndpoint();
+
+std::string& getProtocol(){return m_protocol;}
--- End diff --

Done


---

[GitHub] drill pull request #950: DRILL-5431: SSL Support

[parthchandra]

Github user parthchandra commented on a diff in the pull request:

https://github.com/apache/drill/pull/950#discussion_r142824494
  
--- Diff: contrib/native/client/src/clientlib/channel.hpp ---
@@ -0,0 +1,236 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#ifndef CHANNEL_HPP
+#define CHANNEL_HPP
+
+#include "drill/common.hpp"
+#include "drill/drillClient.hpp"
+#include "streamSocket.hpp"
+
+namespace Drill {
+
+class UserProperties;
+
+class ConnectionEndpoint{
+public:
+ConnectionEndpoint(const char* connStr);
+ConnectionEndpoint(const char* host, const char* port);
+~ConnectionEndpoint();
+
+//parse the connection string and set up the host and port to 
connect to
+connectionStatus_t getDrillbitEndpoint();
+
+std::string& getProtocol(){return m_protocol;}
+std::string& getHost(){return m_host;}
+std::string& getPort(){return m_port;}
+DrillClientError* getError(){ return m_pError;};
+
+private:
+void parseConnectString();
+bool isDirectConnection();
+bool isZookeeperConnection();
+connectionStatus_t getDrillbitEndpointFromZk();
+connectionStatus_t handleError(connectionStatus_t status, 
std::string msg);
+
+std::string m_connectString;
+std::string m_pathToDrill;
+std::string m_protocol; 
+std::string m_hostPortStr;
+std::string m_host;
+std::string m_port;
+
+DrillClientError* m_pError;
+
+};
+
+class ChannelContext{
+public:
+ChannelContext(DrillUserProperties* 
props):m_properties(props){};
+virtual ~ChannelContext(){};
+const DrillUserProperties* getUserProperties() const { return 
m_properties;}
+protected:
+DrillUserProperties* m_properties;
+};
+
+class SSLChannelContext: public ChannelContext{
+public:
+static boost::asio::ssl::context::method 
getTlsVersion(std::string version){
--- End diff --

I made it a const reference (or were you suggesting std::cref?)


---

[GitHub] drill pull request #950: DRILL-5431: SSL Support

[parthchandra]

Github user parthchandra commented on a diff in the pull request:

https://github.com/apache/drill/pull/950#discussion_r142821955
  
--- Diff: contrib/native/client/example/querySubmitter.cpp ---
@@ -390,11 +404,26 @@ int main(int argc, char* argv[]) {
 if(auth.length()>0){
 props.setProperty(USERPROP_AUTH_MECHANISM, auth);
 }
+if(enableSSL.length()>0){
--- End diff --

Fixed indent. I left the check as before so that it is consistent with the 
previous code.


---

[GitHub] drill pull request #950: DRILL-5431: SSL Support

[parthchandra]

Github user parthchandra commented on a diff in the pull request:

https://github.com/apache/drill/pull/950#discussion_r142824662
  
--- Diff: contrib/native/client/src/clientlib/channel.hpp ---
@@ -0,0 +1,236 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#ifndef CHANNEL_HPP
+#define CHANNEL_HPP
+
+#include "drill/common.hpp"
+#include "drill/drillClient.hpp"
+#include "streamSocket.hpp"
+
+namespace Drill {
+
+class UserProperties;
+
+class ConnectionEndpoint{
+public:
+ConnectionEndpoint(const char* connStr);
+ConnectionEndpoint(const char* host, const char* port);
+~ConnectionEndpoint();
+
+//parse the connection string and set up the host and port to 
connect to
+connectionStatus_t getDrillbitEndpoint();
+
+std::string& getProtocol(){return m_protocol;}
+std::string& getHost(){return m_host;}
+std::string& getPort(){return m_port;}
+DrillClientError* getError(){ return m_pError;};
+
+private:
+void parseConnectString();
+bool isDirectConnection();
+bool isZookeeperConnection();
+connectionStatus_t getDrillbitEndpointFromZk();
+connectionStatus_t handleError(connectionStatus_t status, 
std::string msg);
+
+std::string m_connectString;
+std::string m_pathToDrill;
+std::string m_protocol; 
+std::string m_hostPortStr;
+std::string m_host;
+std::string m_port;
+
+DrillClientError* m_pError;
+
+};
+
+class ChannelContext{
+public:
+ChannelContext(DrillUserProperties* 
props):m_properties(props){};
+virtual ~ChannelContext(){};
+const DrillUserProperties* getUserProperties() const { return 
m_properties;}
+protected:
+DrillUserProperties* m_properties;
+};
+
+class SSLChannelContext: public ChannelContext{
+public:
+static boost::asio::ssl::context::method 
getTlsVersion(std::string version){
+if(version.empty()){
+return boost::asio::ssl::context::tlsv12;
+} else if (version == "tlsv12") {
+return boost::asio::ssl::context::tlsv12;
+} else if (version == "tlsv11") {
+return boost::asio::ssl::context::tlsv11;
+} else if (version == "sslv23") {
--- End diff --

Agree that the outdated, insecure versions should not be supported. The 
Java version documents that we will support TLS 1, TLS 1.2, and TLS 1.2,  so I 
left those three in there with TLS1.2 as the default.


---

[GitHub] drill pull request #950: DRILL-5431: SSL Support

[parthchandra]

Github user parthchandra commented on a diff in the pull request:

https://github.com/apache/drill/pull/950#discussion_r142825337
  
--- Diff: contrib/native/client/src/clientlib/wincert.ipp ---
@@ -0,0 +1,98 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#if defined(IS_SSL_ENABLED)
+
+#include 
+#include 
+
+#if defined _WIN32  || defined _WIN64
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+
+#pragma comment (lib, "crypt32.lib")
+#pragma comment (lib, "cryptui.lib")
+
+#define MY_ENCODING_TYPE  (PKCS_7_ASN_ENCODING | X509_ASN_ENCODING)
+
+inline
+int loadSystemTrustStore(const SSL *ssl, std::string& msg) {
--- End diff --

And that is how boost does it too. But then checking for the certificate is 
complicated stuff, so it is easier to just load the certificates from the 
system store into openssl and let openssl deal with it.
Anyway, I think that this counts as an enhancement. I can take care of this 
when I add support for Mac Keychain.


---

[GitHub] drill pull request #950: DRILL-5431: SSL Support

[parthchandra]

Github user parthchandra commented on a diff in the pull request:

https://github.com/apache/drill/pull/950#discussion_r142822133
  
--- Diff: contrib/native/client/readme.ssl ---
@@ -0,0 +1,58 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+Installing OpenSSL - 
+On Mac: 
+brew install openssl
+On Linux :
+
+Set up the certificate
--- End diff --

Done


---

[GitHub] drill pull request #950: DRILL-5431: SSL Support

[parthchandra]

Github user parthchandra commented on a diff in the pull request:

https://github.com/apache/drill/pull/950#discussion_r142821994
  
--- Diff: contrib/native/client/readme.linux ---
@@ -84,6 +84,21 @@ OR
 ln -svf libboost_filesystem.a libboost_filesystem-mt.a
 ln -svf libboost_date_time.a libboost_date_time-mt.a
 
+5) Install or  build Cyrus SASL 
+   To Install 
+   yum install cyrus-sasl-devel
--- End diff --

done


---

[GitHub] drill pull request #950: DRILL-5431: SSL Support

[parthchandra]

Github user parthchandra commented on a diff in the pull request:

https://github.com/apache/drill/pull/950#discussion_r142821900
  
--- Diff: contrib/native/client/example/querySubmitter.cpp ---
@@ -302,6 +310,12 @@ int main(int argc, char* argv[]) {
 std::string serviceHost=qsOptionValues["service_host"];
 std::string serviceName=qsOptionValues["service_name"];
 std::string auth=qsOptionValues["auth"];
+std::string enableSSL=qsOptionValues["enableSSL"];
+std::string tlsProtocol=qsOptionValues["TLSProtocol"];
+std::string certFilePath=qsOptionValues["certFilePath"];
+std::string 
disableHostnameVerification=qsOptionValues["disableHostnameVerification"];
+std::string 
disableCertVerification=qsOptionValues["disableCertVerification"];
+   std::string useSystemTrustStore = 
qsOptionValues["useSystemTrustStore"];
--- End diff --

done


---

[GitHub] drill pull request #950: DRILL-5431: SSL Support

[parthchandra]

Github user parthchandra commented on a diff in the pull request:

https://github.com/apache/drill/pull/950#discussion_r142824715
  
--- Diff: contrib/native/client/src/clientlib/channel.hpp ---
@@ -0,0 +1,236 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#ifndef CHANNEL_HPP
+#define CHANNEL_HPP
+
+#include "drill/common.hpp"
+#include "drill/drillClient.hpp"
+#include "streamSocket.hpp"
+
+namespace Drill {
+
+class UserProperties;
+
+class ConnectionEndpoint{
+public:
+ConnectionEndpoint(const char* connStr);
+ConnectionEndpoint(const char* host, const char* port);
+~ConnectionEndpoint();
+
+//parse the connection string and set up the host and port to 
connect to
+connectionStatus_t getDrillbitEndpoint();
+
+std::string& getProtocol(){return m_protocol;}
+std::string& getHost(){return m_host;}
+std::string& getPort(){return m_port;}
+DrillClientError* getError(){ return m_pError;};
+
+private:
+void parseConnectString();
+bool isDirectConnection();
+bool isZookeeperConnection();
+connectionStatus_t getDrillbitEndpointFromZk();
+connectionStatus_t handleError(connectionStatus_t status, 
std::string msg);
+
+std::string m_connectString;
+std::string m_pathToDrill;
+std::string m_protocol; 
+std::string m_hostPortStr;
+std::string m_host;
+std::string m_port;
+
+DrillClientError* m_pError;
+
+};
+
+class ChannelContext{
+public:
+ChannelContext(DrillUserProperties* 
props):m_properties(props){};
+virtual ~ChannelContext(){};
+const DrillUserProperties* getUserProperties() const { return 
m_properties;}
+protected:
+DrillUserProperties* m_properties;
+};
+
+class SSLChannelContext: public ChannelContext{
+public:
+static boost::asio::ssl::context::method 
getTlsVersion(std::string version){
+if(version.empty()){
+return boost::asio::ssl::context::tlsv12;
+} else if (version == "tlsv12") {
+return boost::asio::ssl::context::tlsv12;
+} else if (version == "tlsv11") {
+return boost::asio::ssl::context::tlsv11;
+} else if (version == "sslv23") {
+return boost::asio::ssl::context::sslv23;
+} else if (version == "tlsv1") {
+return boost::asio::ssl::context::tlsv1;
+} else if (version == "sslv3") {
+return boost::asio::ssl::context::sslv3;
+} else {
+return boost::asio::ssl::context::tlsv12;
+}
+}
+
+SSLChannelContext(DrillUserProperties *props, 
boost::asio::ssl::context::method tlsVersion, boost::asio::ssl::verify_mode 
verifyMode) :
+ChannelContext(props),
+m_SSLContext(tlsVersion) {
+m_SSLContext.set_default_verify_paths();
+m_SSLContext.set_options(
+boost::asio::ssl::context::default_workarounds
+| boost::asio::ssl::context::no_sslv2
+| boost::asio::ssl::context::single_dh_use
+);
+m_SSLContext.set_verify_mode(verifyMode);
+};
+~SSLChannelContext(){};
+boost::asio::ssl::context& getSslContext(){ return 
m_SSLContext;}
+private:
+boost::asio::ssl::context m_SSLContext;
+};
+
+typedef ChannelContext ChannelContext_t; 
+

[GitHub] drill pull request #950: DRILL-5431: SSL Support

[laurentgo]

Github user laurentgo commented on a diff in the pull request:

https://github.com/apache/drill/pull/950#discussion_r142718970
  
--- Diff: contrib/native/client/readme.linux ---
@@ -84,6 +84,21 @@ OR
 ln -svf libboost_filesystem.a libboost_filesystem-mt.a
 ln -svf libboost_date_time.a libboost_date_time-mt.a
 
+5) Install or  build Cyrus SASL 
+   To Install 
+   yum install cyrus-sasl-devel
--- End diff --

you can install multiple packages using one command: `yum install 
cyrus-sasl-devel cyrus-sasl-gssapi`


---

[GitHub] drill pull request #950: DRILL-5431: SSL Support

[laurentgo]

Github user laurentgo commented on a diff in the pull request:

https://github.com/apache/drill/pull/950#discussion_r142731277
  
--- Diff: contrib/native/client/src/clientlib/channel.cpp ---
@@ -0,0 +1,448 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#include 
+#include 
+#include "drill/drillConfig.hpp"
+#include "drill/drillError.hpp"
+#include "drill/userProperties.hpp"
+#include "channel.hpp"
+#include "errmsgs.hpp"
+#include "logger.hpp"
+#include "utils.hpp"
+#include "zookeeperClient.hpp"
+
+#include "GeneralRPC.pb.h"
+
+namespace Drill{
+
+ConnectionEndpoint::ConnectionEndpoint(const char* connStr){
+m_connectString=connStr;
+m_pError=NULL;
+}
+
+ConnectionEndpoint::ConnectionEndpoint(const char* host, const char* port){
+m_host=host;
+m_port=port;
+m_protocol="drillbit"; // direct connection
+m_pError=NULL;
+}
+
+ConnectionEndpoint::~ConnectionEndpoint(){
+if(m_pError!=NULL){
+delete m_pError; m_pError=NULL;
+}
+}
+
+connectionStatus_t ConnectionEndpoint::getDrillbitEndpoint(){
+connectionStatus_t ret=CONN_SUCCESS;
+if(!m_connectString.empty()){
+parseConnectString();
+if(m_protocol.empty()){
+return handleError(CONN_INVALID_INPUT, 
getMessage(ERR_CONN_UNKPROTO, ""));
+}
+if(isZookeeperConnection()){
+if((ret=getDrillbitEndpointFromZk())!=CONN_SUCCESS){
+DRILL_LOG(LOG_INFO) << "Failed to get endpoint from zk" << 
std::endl;
+return ret;
+}
+}else if(!this->isDirectConnection()){
+return handleError(CONN_INVALID_INPUT, 
getMessage(ERR_CONN_UNKPROTO, this->getProtocol().c_str()));
+}
+}else{
+if(m_host.empty() || m_port.empty()){
+return handleError(CONN_INVALID_INPUT, 
getMessage(ERR_CONN_NOCONNSTR));
+}
+}
+return ret;
+}
+
+void ConnectionEndpoint::parseConnectString(){
+boost::regex connStrExpr("(.*)=(((.*):([0-9]+),?)+)(/.+)?");
+boost::cmatch matched;
+
+if(boost::regex_match(m_connectString.c_str(), matched, connStrExpr)){
+m_protocol.assign(matched[1].first, matched[1].second);
+if(isDirectConnection()){
+m_host.assign(matched[4].first, matched[4].second);
--- End diff --

looks like you can also do `m_host = matched[4].str()`


---

[GitHub] drill pull request #950: DRILL-5431: SSL Support

[laurentgo]

Github user laurentgo commented on a diff in the pull request:

https://github.com/apache/drill/pull/950#discussion_r142739285
  
--- Diff: contrib/native/client/src/clientlib/wincert.ipp ---
@@ -0,0 +1,98 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#if defined(IS_SSL_ENABLED)
+
+#include 
+#include 
+
+#if defined _WIN32  || defined _WIN64
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+
+#pragma comment (lib, "crypt32.lib")
+#pragma comment (lib, "cryptui.lib")
+
+#define MY_ENCODING_TYPE  (PKCS_7_ASN_ENCODING | X509_ASN_ENCODING)
+
+inline
+int loadSystemTrustStore(const SSL *ssl, std::string& msg) {
--- End diff --

maybe it should be a configurable callback where the drill client provides 
the certificate, and the callback returns if the certificate is valid or not? 
this way it would be easier to add support for system keystore support on mac 
or linux too...


---

[GitHub] drill pull request #950: DRILL-5431: SSL Support

[laurentgo]

Github user laurentgo commented on a diff in the pull request:

https://github.com/apache/drill/pull/950#discussion_r142719061
  
--- Diff: contrib/native/client/readme.linux ---
@@ -84,6 +84,21 @@ OR
 ln -svf libboost_filesystem.a libboost_filesystem-mt.a
 ln -svf libboost_date_time.a libboost_date_time-mt.a
 
+5) Install or  build Cyrus SASL 
+   To Install 
+   yum install cyrus-sasl-devel
+   yum install cyrus-sasl-gssapi
+   libs are installed in /usr/lib64/sasl2
+   includes are installed in /usr/include
+
+   To build your own 
+   See readme.sasl for instructions
+
+6) Install OpenSSL
+   yum install openssl
--- End diff --

might not be necessary as it is a dependency of openssl-devel


---

[GitHub] drill pull request #950: DRILL-5431: SSL Support

[laurentgo]

Github user laurentgo commented on a diff in the pull request:

https://github.com/apache/drill/pull/950#discussion_r142721373
  
--- Diff: contrib/native/client/src/clientlib/CMakeLists.txt ---
@@ -36,6 +40,7 @@ include_directories(${CMAKE_CURRENT_SOURCE_DIR} 
${CMAKE_CURRENT_SOURCE_DIR}/../i
 include_directories(${PROTOBUF_INCLUDE_DIR})
 include_directories(${Zookeeper_INCLUDE_DIRS})
 include_directories(${SASL_INCLUDE_DIRS})
+include_directories("${OPENSSL_INCLUDE_DIR}")
--- End diff --

not sure why this variable is quoted (but others aren't)


---

[GitHub] drill pull request #950: DRILL-5431: SSL Support

[laurentgo]

Github user laurentgo commented on a diff in the pull request:

https://github.com/apache/drill/pull/950#discussion_r142734246
  
--- Diff: contrib/native/client/src/clientlib/channel.hpp ---
@@ -0,0 +1,236 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#ifndef CHANNEL_HPP
+#define CHANNEL_HPP
+
+#include "drill/common.hpp"
+#include "drill/drillClient.hpp"
+#include "streamSocket.hpp"
+
+namespace Drill {
+
+class UserProperties;
+
+class ConnectionEndpoint{
+public:
+ConnectionEndpoint(const char* connStr);
+ConnectionEndpoint(const char* host, const char* port);
+~ConnectionEndpoint();
+
+//parse the connection string and set up the host and port to 
connect to
+connectionStatus_t getDrillbitEndpoint();
+
+std::string& getProtocol(){return m_protocol;}
+std::string& getHost(){return m_host;}
+std::string& getPort(){return m_port;}
+DrillClientError* getError(){ return m_pError;};
+
+private:
+void parseConnectString();
+bool isDirectConnection();
+bool isZookeeperConnection();
+connectionStatus_t getDrillbitEndpointFromZk();
+connectionStatus_t handleError(connectionStatus_t status, 
std::string msg);
+
+std::string m_connectString;
+std::string m_pathToDrill;
+std::string m_protocol; 
+std::string m_hostPortStr;
+std::string m_host;
+std::string m_port;
+
+DrillClientError* m_pError;
+
+};
+
+class ChannelContext{
+public:
+ChannelContext(DrillUserProperties* 
props):m_properties(props){};
+virtual ~ChannelContext(){};
+const DrillUserProperties* getUserProperties() const { return 
m_properties;}
+protected:
+DrillUserProperties* m_properties;
+};
+
+class SSLChannelContext: public ChannelContext{
+public:
+static boost::asio::ssl::context::method 
getTlsVersion(std::string version){
+if(version.empty()){
+return boost::asio::ssl::context::tlsv12;
+} else if (version == "tlsv12") {
+return boost::asio::ssl::context::tlsv12;
+} else if (version == "tlsv11") {
+return boost::asio::ssl::context::tlsv11;
+} else if (version == "sslv23") {
+return boost::asio::ssl::context::sslv23;
+} else if (version == "tlsv1") {
+return boost::asio::ssl::context::tlsv1;
+} else if (version == "sslv3") {
+return boost::asio::ssl::context::sslv3;
+} else {
+return boost::asio::ssl::context::tlsv12;
+}
+}
+
+SSLChannelContext(DrillUserProperties *props, 
boost::asio::ssl::context::method tlsVersion, boost::asio::ssl::verify_mode 
verifyMode) :
+ChannelContext(props),
+m_SSLContext(tlsVersion) {
+m_SSLContext.set_default_verify_paths();
+m_SSLContext.set_options(
+boost::asio::ssl::context::default_workarounds
+| boost::asio::ssl::context::no_sslv2
+| boost::asio::ssl::context::single_dh_use
+);
+m_SSLContext.set_verify_mode(verifyMode);
+};
+~SSLChannelContext(){};
+boost::asio::ssl::context& getSslContext(){ return 
m_SSLContext;}
+private:
+boost::asio::ssl::context m_SSLContext;
+};
+
+typedef ChannelContext ChannelContext_t; 
+

[GitHub] drill pull request #950: DRILL-5431: SSL Support

[laurentgo]

Github user laurentgo commented on a diff in the pull request:

https://github.com/apache/drill/pull/950#discussion_r142718781
  
--- Diff: contrib/native/client/example/querySubmitter.cpp ---
@@ -544,4 +573,4 @@ int main(int argc, char* argv[]) {
 }
 
 return 0;
-}
+}
--- End diff --

(style) I know some tools complain when removing the last newline


---

[GitHub] drill pull request #950: DRILL-5431: SSL Support

[laurentgo]

Github user laurentgo commented on a diff in the pull request:

https://github.com/apache/drill/pull/950#discussion_r142739836
  
--- Diff: contrib/native/client/src/include/drill/drillConfig.hpp ---
@@ -0,0 +1,153 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+
+#ifndef DRILL_CONFIG_H
+#define DRILL_CONFIG_H
+
+#include "drill/common.hpp"
+#include 
+
+
+
+#if defined _WIN32 || defined __CYGWIN__
--- End diff --

maybe should be in a common header since used in multiple places?


---

[GitHub] drill pull request #950: DRILL-5431: SSL Support

[laurentgo]

Github user laurentgo commented on a diff in the pull request:

https://github.com/apache/drill/pull/950#discussion_r142719411
  
--- Diff: contrib/native/client/readme.ssl ---
@@ -0,0 +1,58 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+Installing OpenSSL - 
+On Mac: 
+brew install openssl
+On Linux :
+
+Set up the certificate
--- End diff --

maybe should be mentionned for testing purposes only...


---

[GitHub] drill pull request #950: DRILL-5431: SSL Support

[laurentgo]

Github user laurentgo commented on a diff in the pull request:

https://github.com/apache/drill/pull/950#discussion_r142740210
  
--- Diff: contrib/native/client/src/include/drill/drillConfig.hpp ---
@@ -0,0 +1,153 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+
+#ifndef DRILL_CONFIG_H
+#define DRILL_CONFIG_H
+
+#include "drill/common.hpp"
+#include 
--- End diff --

only using mutex I believe (defined in ``)


---

[GitHub] drill pull request #950: DRILL-5431: SSL Support

[laurentgo]

Github user laurentgo commented on a diff in the pull request:

https://github.com/apache/drill/pull/950#discussion_r142732709
  
--- Diff: contrib/native/client/src/clientlib/channel.hpp ---
@@ -0,0 +1,236 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#ifndef CHANNEL_HPP
+#define CHANNEL_HPP
+
+#include "drill/common.hpp"
+#include "drill/drillClient.hpp"
+#include "streamSocket.hpp"
+
+namespace Drill {
+
+class UserProperties;
+
+class ConnectionEndpoint{
+public:
+ConnectionEndpoint(const char* connStr);
+ConnectionEndpoint(const char* host, const char* port);
+~ConnectionEndpoint();
+
+//parse the connection string and set up the host and port to 
connect to
+connectionStatus_t getDrillbitEndpoint();
+
+std::string& getProtocol(){return m_protocol;}
+std::string& getHost(){return m_host;}
+std::string& getPort(){return m_port;}
+DrillClientError* getError(){ return m_pError;};
+
+private:
+void parseConnectString();
+bool isDirectConnection();
+bool isZookeeperConnection();
+connectionStatus_t getDrillbitEndpointFromZk();
+connectionStatus_t handleError(connectionStatus_t status, 
std::string msg);
+
+std::string m_connectString;
+std::string m_pathToDrill;
+std::string m_protocol; 
+std::string m_hostPortStr;
+std::string m_host;
+std::string m_port;
+
+DrillClientError* m_pError;
+
+};
+
+class ChannelContext{
+public:
+ChannelContext(DrillUserProperties* 
props):m_properties(props){};
+virtual ~ChannelContext(){};
+const DrillUserProperties* getUserProperties() const { return 
m_properties;}
+protected:
+DrillUserProperties* m_properties;
+};
+
+class SSLChannelContext: public ChannelContext{
+public:
+static boost::asio::ssl::context::method 
getTlsVersion(std::string version){
--- End diff --

version should be a cref


---

[GitHub] drill pull request #950: DRILL-5431: SSL Support

[laurentgo]

Github user laurentgo commented on a diff in the pull request:

https://github.com/apache/drill/pull/950#discussion_r142729342
  
--- Diff: contrib/native/client/src/clientlib/channel.cpp ---
@@ -0,0 +1,448 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#include 
+#include 
+#include "drill/drillConfig.hpp"
+#include "drill/drillError.hpp"
+#include "drill/userProperties.hpp"
+#include "channel.hpp"
+#include "errmsgs.hpp"
+#include "logger.hpp"
+#include "utils.hpp"
+#include "zookeeperClient.hpp"
+
+#include "GeneralRPC.pb.h"
+
+namespace Drill{
+
+ConnectionEndpoint::ConnectionEndpoint(const char* connStr){
+m_connectString=connStr;
+m_pError=NULL;
+}
+
+ConnectionEndpoint::ConnectionEndpoint(const char* host, const char* port){
+m_host=host;
+m_port=port;
+m_protocol="drillbit"; // direct connection
--- End diff --

maybe there should be some constants for the protocol types?


---

[GitHub] drill pull request #950: DRILL-5431: SSL Support

[laurentgo]

Github user laurentgo commented on a diff in the pull request:

https://github.com/apache/drill/pull/950#discussion_r142729078
  
--- Diff: contrib/native/client/src/clientlib/channel.cpp ---
@@ -0,0 +1,448 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#include 
+#include 
+#include "drill/drillConfig.hpp"
+#include "drill/drillError.hpp"
+#include "drill/userProperties.hpp"
+#include "channel.hpp"
+#include "errmsgs.hpp"
+#include "logger.hpp"
+#include "utils.hpp"
+#include "zookeeperClient.hpp"
+
+#include "GeneralRPC.pb.h"
+
+namespace Drill{
+
+ConnectionEndpoint::ConnectionEndpoint(const char* connStr){
+m_connectString=connStr;
+m_pError=NULL;
+}
+
+ConnectionEndpoint::ConnectionEndpoint(const char* host, const char* port){
+m_host=host;
+m_port=port;
+m_protocol="drillbit"; // direct connection
+m_pError=NULL;
+}
+
+ConnectionEndpoint::~ConnectionEndpoint(){
+if(m_pError!=NULL){
+delete m_pError; m_pError=NULL;
+}
+}
+
+connectionStatus_t ConnectionEndpoint::getDrillbitEndpoint(){
+connectionStatus_t ret=CONN_SUCCESS;
+if(!m_connectString.empty()){
+parseConnectString();
+if(m_protocol.empty()){
+return handleError(CONN_INVALID_INPUT, 
getMessage(ERR_CONN_UNKPROTO, ""));
+}
+if(isZookeeperConnection()){
+if((ret=getDrillbitEndpointFromZk())!=CONN_SUCCESS){
+DRILL_LOG(LOG_INFO) << "Failed to get endpoint from zk" << 
std::endl;
+return ret;
+}
+}else if(!this->isDirectConnection()){
+return handleError(CONN_INVALID_INPUT, 
getMessage(ERR_CONN_UNKPROTO, this->getProtocol().c_str()));
+}
+}else{
+if(m_host.empty() || m_port.empty()){
+return handleError(CONN_INVALID_INPUT, 
getMessage(ERR_CONN_NOCONNSTR));
+}
+}
+return ret;
+}
+
+void ConnectionEndpoint::parseConnectString(){
+boost::regex connStrExpr("(.*)=(((.*):([0-9]+),?)+)(/.+)?");
+boost::cmatch matched;
+
+if(boost::regex_match(m_connectString.c_str(), matched, connStrExpr)){
--- End diff --

I'm surprised a conversion to a C char* is required by boost...


---

[GitHub] drill pull request #950: DRILL-5431: SSL Support

[laurentgo]

Github user laurentgo commented on a diff in the pull request:

https://github.com/apache/drill/pull/950#discussion_r142735461
  
--- Diff: contrib/native/client/src/clientlib/drillClientImpl.cpp ---
@@ -65,108 +56,72 @@ struct ToRpcType: public 
std::unary_functionm_bIsConnected) {
-if(std::strcmp(connStr, m_connectStr.c_str())){ // trying to 
connect to a different address is not allowed if already connected
+if(!std::strcmp(connStr, m_connectStr.c_str())){
+// trying to connect to a different address is not allowed if 
already connected
 return handleConnError(CONN_ALREADYCONNECTED, 
getMessage(ERR_CONN_ALREADYCONN));
 }
 return CONN_SUCCESS;
 }
+std::string val;
+channelType_t type = ( props->isPropSet(USERPROP_USESSL) &&
+props->getProp(USERPROP_USESSL, val) =="true") ?
+CHANNEL_TYPE_SSLSTREAM :
+CHANNEL_TYPE_SOCKET;
 
-m_connectStr=connStr;
-Utils::parseConnectStr(connStr, pathToDrill, protocol, hostPortStr);
-if(protocol == "zk"){
-ZookeeperClient zook(pathToDrill);
-std::vector drillbits;
-int err = zook.getAllDrillbits(hostPortStr, drillbits);
-if(!err){
-if (drillbits.empty()){
-return handleConnError(CONN_FAILURE, 
getMessage(ERR_CONN_ZKNODBIT));
-}
-Utils::shuffle(drillbits);
-exec::DrillbitEndpoint endpoint;
-err = zook.getEndPoint(drillbits[drillbits.size() -1], 
endpoint);// get the last one in the list
-if(!err){
-host=boost::lexical_cast(endpoint.address());
-
port=boost::lexical_cast(endpoint.user_port());
-}
-DRILL_MT_LOG(DRILL_LOG(LOG_TRACE) << "Choosing drillbit <" << 
(drillbits.size() - 1)  << ">. Selected " << endpoint.DebugString() << 
std::endl;)
-
-}
-if(err){
-return handleConnError(CONN_ZOOKEEPER_ERROR, 
getMessage(ERR_CONN_ZOOKEEPER, zook.getError().c_str()));
-}
-zook.close();
-m_bIsDirectConnection=true;
-}else if(protocol == "local"){
-boost::lock_guard lock(m_dcMutex);//strtok is not 
reentrant
-char tempStr[MAX_CONNECT_STR+1];
-strncpy(tempStr, hostPortStr.c_str(), MAX_CONNECT_STR); 
tempStr[MAX_CONNECT_STR]=0;
-host=strtok(tempStr, ":");
-port=strtok(NULL, "");
-m_bIsDirectConnection=false;
-}else{
-return handleConnError(CONN_INVALID_INPUT, 
getMessage(ERR_CONN_UNKPROTO, protocol.c_str()));
-}
-DRILL_MT_LOG(DRILL_LOG(LOG_TRACE) << "Connecting to endpoint: " << 
host << ":" << port << std::endl;)
-std::string serviceHost;
-for (size_t i = 0; i < props->size(); i++) {
-if (props->keyAt(i) == USERPROP_SERVICE_HOST) {
-serviceHost = props->valueAt(i);
-}
+connectionStatus_t ret = CONN_SUCCESS;
+m_pChannelContext = ChannelContextFactory::getChannelContext(type, 
props);
+m_pChannel= ChannelFactory::getChannel(type, m_io_service, connStr);
+ret=m_pChannel->init(m_pChannelContext);
--- End diff --

I guess it could be passed at construct time (or even better, the channel 
creates its own context?)


---

[GitHub] drill pull request #950: DRILL-5431: SSL Support

[laurentgo]

Github user laurentgo commented on a diff in the pull request:

https://github.com/apache/drill/pull/950#discussion_r142727129
  
--- Diff: contrib/native/client/src/clientlib/channel.hpp ---
@@ -0,0 +1,236 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#ifndef CHANNEL_HPP
+#define CHANNEL_HPP
+
+#include "drill/common.hpp"
+#include "drill/drillClient.hpp"
+#include "streamSocket.hpp"
+
+namespace Drill {
+
+class UserProperties;
+
+class ConnectionEndpoint{
+public:
+ConnectionEndpoint(const char* connStr);
--- End diff --

maybe we should stop using C char* and use std::string more


---

[GitHub] drill pull request #950: DRILL-5431: SSL Support

[laurentgo]

Github user laurentgo commented on a diff in the pull request:

https://github.com/apache/drill/pull/950#discussion_r142731548
  
--- Diff: contrib/native/client/src/clientlib/channel.cpp ---
@@ -0,0 +1,448 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#include 
+#include 
+#include "drill/drillConfig.hpp"
+#include "drill/drillError.hpp"
+#include "drill/userProperties.hpp"
+#include "channel.hpp"
+#include "errmsgs.hpp"
+#include "logger.hpp"
+#include "utils.hpp"
+#include "zookeeperClient.hpp"
+
+#include "GeneralRPC.pb.h"
+
+namespace Drill{
+
+ConnectionEndpoint::ConnectionEndpoint(const char* connStr){
+m_connectString=connStr;
+m_pError=NULL;
+}
+
+ConnectionEndpoint::ConnectionEndpoint(const char* host, const char* port){
+m_host=host;
+m_port=port;
+m_protocol="drillbit"; // direct connection
+m_pError=NULL;
+}
+
+ConnectionEndpoint::~ConnectionEndpoint(){
+if(m_pError!=NULL){
+delete m_pError; m_pError=NULL;
+}
+}
+
+connectionStatus_t ConnectionEndpoint::getDrillbitEndpoint(){
+connectionStatus_t ret=CONN_SUCCESS;
+if(!m_connectString.empty()){
+parseConnectString();
+if(m_protocol.empty()){
+return handleError(CONN_INVALID_INPUT, 
getMessage(ERR_CONN_UNKPROTO, ""));
+}
+if(isZookeeperConnection()){
+if((ret=getDrillbitEndpointFromZk())!=CONN_SUCCESS){
+DRILL_LOG(LOG_INFO) << "Failed to get endpoint from zk" << 
std::endl;
+return ret;
+}
+}else if(!this->isDirectConnection()){
+return handleError(CONN_INVALID_INPUT, 
getMessage(ERR_CONN_UNKPROTO, this->getProtocol().c_str()));
+}
+}else{
+if(m_host.empty() || m_port.empty()){
+return handleError(CONN_INVALID_INPUT, 
getMessage(ERR_CONN_NOCONNSTR));
+}
+}
+return ret;
+}
+
+void ConnectionEndpoint::parseConnectString(){
+boost::regex connStrExpr("(.*)=(((.*):([0-9]+),?)+)(/.+)?");
+boost::cmatch matched;
+
+if(boost::regex_match(m_connectString.c_str(), matched, connStrExpr)){
+m_protocol.assign(matched[1].first, matched[1].second);
+if(isDirectConnection()){
+m_host.assign(matched[4].first, matched[4].second);
+m_port.assign(matched[5].first, matched[5].second);
+}else {
+// if the connection is to a zookeeper,
+// we will get the host and the port only after connecting to 
the Zookeeper
+m_host = "";
+m_port = "";
+}
+m_hostPortStr.assign(matched[2].first, matched[2].second);
+if(matched[6].matched) {
+m_pathToDrill.assign(matched[6].first, matched[6].second);
+}
+DRILL_MT_LOG(DRILL_LOG(LOG_DEBUG)
+ << "Conn str: "<< m_connectString
+ << ";  protocol: " << m_protocol
+ << ";  host: " << m_host
+ << ";  port: " << m_port
+ << ";  path to drill: " << m_pathToDrill
+ << std::endl;)
+} else {
+DRILL_MT_LOG(DRILL_LOG(LOG_DEBUG) << "Invalid connect string. 
Regexp did not match" << std::endl;)
+}
+return;
+}
+
+bool ConnectionEndpoint::isDirectConnection(){
+assert(!m_protocol.empty());
+return (!strcmp(m_protocol.c_str(), "local") || 
!strcmp(m_protocol.c_str(), "drillbit"));
--- End diff --

let's not use strcmp if we can avoid it. `mprotocol != "local"` should work 
fine.


---

[GitHub] drill pull request #950: DRILL-5431: SSL Support

[laurentgo]

Github user laurentgo commented on a diff in the pull request:

https://github.com/apache/drill/pull/950#discussion_r142733695
  
--- Diff: contrib/native/client/src/clientlib/channel.hpp ---
@@ -0,0 +1,236 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#ifndef CHANNEL_HPP
+#define CHANNEL_HPP
+
+#include "drill/common.hpp"
+#include "drill/drillClient.hpp"
+#include "streamSocket.hpp"
+
+namespace Drill {
+
+class UserProperties;
+
+class ConnectionEndpoint{
+public:
+ConnectionEndpoint(const char* connStr);
+ConnectionEndpoint(const char* host, const char* port);
+~ConnectionEndpoint();
+
+//parse the connection string and set up the host and port to 
connect to
+connectionStatus_t getDrillbitEndpoint();
+
+std::string& getProtocol(){return m_protocol;}
+std::string& getHost(){return m_host;}
+std::string& getPort(){return m_port;}
+DrillClientError* getError(){ return m_pError;};
+
+private:
+void parseConnectString();
+bool isDirectConnection();
+bool isZookeeperConnection();
+connectionStatus_t getDrillbitEndpointFromZk();
+connectionStatus_t handleError(connectionStatus_t status, 
std::string msg);
+
+std::string m_connectString;
+std::string m_pathToDrill;
+std::string m_protocol; 
+std::string m_hostPortStr;
+std::string m_host;
+std::string m_port;
+
+DrillClientError* m_pError;
+
+};
+
+class ChannelContext{
+public:
+ChannelContext(DrillUserProperties* 
props):m_properties(props){};
+virtual ~ChannelContext(){};
+const DrillUserProperties* getUserProperties() const { return 
m_properties;}
+protected:
+DrillUserProperties* m_properties;
+};
+
+class SSLChannelContext: public ChannelContext{
+public:
+static boost::asio::ssl::context::method 
getTlsVersion(std::string version){
+if(version.empty()){
+return boost::asio::ssl::context::tlsv12;
+} else if (version == "tlsv12") {
+return boost::asio::ssl::context::tlsv12;
+} else if (version == "tlsv11") {
+return boost::asio::ssl::context::tlsv11;
+} else if (version == "sslv23") {
--- End diff --

spec didn't mention support for ssl protocol. Shouldn't we disable support 
for those (since there are pretty insecure)? consensus is that tls1.2 should be 
used, so maybe it should be set as the mininum?


---

[GitHub] drill pull request #950: DRILL-5431: SSL Support

[laurentgo]

Github user laurentgo commented on a diff in the pull request:

https://github.com/apache/drill/pull/950#discussion_r142717986
  
--- Diff: contrib/native/client/example/querySubmitter.cpp ---
@@ -50,7 +49,14 @@ struct Option{
 {"service_host", "Service host for Kerberos", false},
 {"service_name", "Service name for Kerberos", false},
 {"auth", "Authentication mechanism to use", false},
-{"sasl_encrypt", "Negotiate for encrypted connection", false}
+{"sasl_encrypt", "Negotiate for encrypted connection", false},
+{"enableSSL", "Enable SSL", false},
+{"TLSProtocol", "TLS protocol version", false},
+{"certFilePath", "Path to SSL certificate file", false},
+{"disableHostnameVerification", "disable host name verification", 
false},
+{"disableCertVerification", "disable certificate verification", false},
+   {"useSystemTrustStore", "[Windows only]. Use the system truststore.", 
false }
--- End diff --

(style) indentation issue


---

[GitHub] drill pull request #950: DRILL-5431: SSL Support

[laurentgo]

Github user laurentgo commented on a diff in the pull request:

https://github.com/apache/drill/pull/950#discussion_r142736318
  
--- Diff: contrib/native/client/src/clientlib/drillClientImpl.cpp ---
@@ -250,7 +205,15 @@ void DrillClientImpl::doWriteToSocket(const char* 
dataPtr, size_t bytesToWrite,
 // Write all the bytes to socket. In case of error when all bytes are 
not successfully written
 // proper errorCode will be set.
 while(1) {
-size_t bytesWritten = 
m_socket.write_some(boost::asio::buffer(dataPtr, bytesToWrite), errorCode);
+size_t bytesWritten;
+{
+boost::lock_guard lock(m_channelMutex);
--- End diff --

it seems weird that a mutex is now required. Before, the socket was 
existing, why it would not be the case for the channel?


---

[GitHub] drill pull request #950: DRILL-5431: SSL Support

[laurentgo]

Github user laurentgo commented on a diff in the pull request:

https://github.com/apache/drill/pull/950#discussion_r142727180
  
--- Diff: contrib/native/client/src/clientlib/channel.hpp ---
@@ -0,0 +1,236 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#ifndef CHANNEL_HPP
+#define CHANNEL_HPP
+
+#include "drill/common.hpp"
+#include "drill/drillClient.hpp"
+#include "streamSocket.hpp"
+
+namespace Drill {
+
+class UserProperties;
+
+class ConnectionEndpoint{
+public:
+ConnectionEndpoint(const char* connStr);
+ConnectionEndpoint(const char* host, const char* port);
--- End diff --

shouldn't port be an int?


---

[GitHub] drill pull request #950: DRILL-5431: SSL Support

[laurentgo]

Github user laurentgo commented on a diff in the pull request:

https://github.com/apache/drill/pull/950#discussion_r142727549
  
--- Diff: contrib/native/client/src/clientlib/channel.hpp ---
@@ -0,0 +1,236 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#ifndef CHANNEL_HPP
+#define CHANNEL_HPP
+
+#include "drill/common.hpp"
+#include "drill/drillClient.hpp"
+#include "streamSocket.hpp"
+
+namespace Drill {
+
+class UserProperties;
+
+class ConnectionEndpoint{
+public:
+ConnectionEndpoint(const char* connStr);
+ConnectionEndpoint(const char* host, const char* port);
+~ConnectionEndpoint();
+
+//parse the connection string and set up the host and port to 
connect to
+connectionStatus_t getDrillbitEndpoint();
+
+std::string& getProtocol(){return m_protocol;}
--- End diff --

should either return a new string, or a const ref to the internal string. 
In both cases, this method should probably be const too (same for other methods)


---

[GitHub] drill pull request #950: DRILL-5431: SSL Support

[laurentgo]

Github user laurentgo commented on a diff in the pull request:

https://github.com/apache/drill/pull/950#discussion_r142719553
  
--- Diff: contrib/native/client/readme.ssl ---
@@ -0,0 +1,58 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+Installing OpenSSL - 
+On Mac: 
+brew install openssl
+On Linux :
+
+Set up the certificate
+Generate a private key
+
+openssl genrsa -des3 -out drillTestServerKey.pem 1024
+
+Generate Certificate signing request
+
+openssl req -new -key drillTestServerKey.pem -out 
drillTestServer.csr
+
+Sign certificate with private key
+
+openssl x509 -req -days 3650 -in drillTestServer.csr -signkey 
drillTestServerKey.pem -out drillTestCert.pem
+
+Remove password requirement (needed for example)
+
+cp drillTestServerKey.pem drillTestServerKey.safe.pem
+openssl rsa -in drillTestServerKey.safe.pem -out 
drillTestServerKey.pem
+
+Generate dhparam file
+
+openssl dhparam -out dh512.pem 512
--- End diff --

what this file is for?


---

[GitHub] drill pull request #950: DRILL-5431: SSL Support

[laurentgo]

Github user laurentgo commented on a diff in the pull request:

https://github.com/apache/drill/pull/950#discussion_r142728092
  
--- Diff: contrib/native/client/src/clientlib/channel.cpp ---
@@ -0,0 +1,448 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#include 
+#include 
+#include "drill/drillConfig.hpp"
+#include "drill/drillError.hpp"
+#include "drill/userProperties.hpp"
+#include "channel.hpp"
+#include "errmsgs.hpp"
+#include "logger.hpp"
+#include "utils.hpp"
+#include "zookeeperClient.hpp"
+
+#include "GeneralRPC.pb.h"
+
+namespace Drill{
+
+ConnectionEndpoint::ConnectionEndpoint(const char* connStr){
+m_connectString=connStr;
--- End diff --

we should probably copy it for safety, no? but maybe better would be to 
have factory method to do the parsing and fill all the fields instead of 
leaving some unassigned (with random values since there's no automatic 
initialization in C++?)


---

[GitHub] drill pull request #950: DRILL-5431: SSL Support

[laurentgo]

Github user laurentgo commented on a diff in the pull request:

https://github.com/apache/drill/pull/950#discussion_r142717783
  
--- Diff: contrib/native/client/CMakeLists.txt ---
@@ -93,7 +94,7 @@ else()
 #of boost. Arbirtarily, we choose the new namspace to be 
drill_boost.
 #See the instructions in the readme for linux/macos and rebuild 
boost. Then
 #uncomment the line below to build
-set(Boost_NAMESPACE drill_boost)
+#set(Boost_NAMESPACE drill_boost)
--- End diff --

temporary change?


---

[GitHub] drill pull request #950: DRILL-5431: SSL Support

[laurentgo]

Github user laurentgo commented on a diff in the pull request:

https://github.com/apache/drill/pull/950#discussion_r142718085
  
--- Diff: contrib/native/client/example/querySubmitter.cpp ---
@@ -302,6 +310,12 @@ int main(int argc, char* argv[]) {
 std::string serviceHost=qsOptionValues["service_host"];
 std::string serviceName=qsOptionValues["service_name"];
 std::string auth=qsOptionValues["auth"];
+std::string enableSSL=qsOptionValues["enableSSL"];
+std::string tlsProtocol=qsOptionValues["TLSProtocol"];
+std::string certFilePath=qsOptionValues["certFilePath"];
+std::string 
disableHostnameVerification=qsOptionValues["disableHostnameVerification"];
+std::string 
disableCertVerification=qsOptionValues["disableCertVerification"];
+   std::string useSystemTrustStore = 
qsOptionValues["useSystemTrustStore"];
--- End diff --

(style) indentation issue


---

[GitHub] drill pull request #950: DRILL-5431: SSL Support

[laurentgo]

Github user laurentgo commented on a diff in the pull request:

https://github.com/apache/drill/pull/950#discussion_r142718173
  
--- Diff: contrib/native/client/example/querySubmitter.cpp ---
@@ -390,11 +404,26 @@ int main(int argc, char* argv[]) {
 if(auth.length()>0){
 props.setProperty(USERPROP_AUTH_MECHANISM, auth);
 }
+if(enableSSL.length()>0){
--- End diff --

(style) indentation issue
you can also use `!enableSSL.empty()` instead of checking the length


---

[GitHub] drill pull request #950: DRILL-5431: SSL Support

[laurentgo]

Github user laurentgo commented on a diff in the pull request:

https://github.com/apache/drill/pull/950#discussion_r142728722
  
--- Diff: contrib/native/client/src/clientlib/channel.cpp ---
@@ -0,0 +1,448 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#include 
+#include 
+#include "drill/drillConfig.hpp"
+#include "drill/drillError.hpp"
+#include "drill/userProperties.hpp"
+#include "channel.hpp"
+#include "errmsgs.hpp"
+#include "logger.hpp"
+#include "utils.hpp"
+#include "zookeeperClient.hpp"
+
+#include "GeneralRPC.pb.h"
+
+namespace Drill{
+
+ConnectionEndpoint::ConnectionEndpoint(const char* connStr){
+m_connectString=connStr;
+m_pError=NULL;
+}
+
+ConnectionEndpoint::ConnectionEndpoint(const char* host, const char* port){
+m_host=host;
+m_port=port;
+m_protocol="drillbit"; // direct connection
+m_pError=NULL;
+}
+
+ConnectionEndpoint::~ConnectionEndpoint(){
+if(m_pError!=NULL){
+delete m_pError; m_pError=NULL;
+}
+}
+
+connectionStatus_t ConnectionEndpoint::getDrillbitEndpoint(){
+connectionStatus_t ret=CONN_SUCCESS;
+if(!m_connectString.empty()){
+parseConnectString();
+if(m_protocol.empty()){
+return handleError(CONN_INVALID_INPUT, 
getMessage(ERR_CONN_UNKPROTO, ""));
+}
+if(isZookeeperConnection()){
+if((ret=getDrillbitEndpointFromZk())!=CONN_SUCCESS){
+DRILL_LOG(LOG_INFO) << "Failed to get endpoint from zk" << 
std::endl;
+return ret;
+}
+}else if(!this->isDirectConnection()){
--- End diff --

style: not sure why `this` is used here but not in the previous test 
statement


---

[GitHub] drill pull request #950: DRILL-5431: SSL Support

[arina-ielchiieva]

Github user arina-ielchiieva commented on a diff in the pull request:

https://github.com/apache/drill/pull/950#discussion_r142667318
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/UserClient.java ---
@@ -132,38 +173,47 @@ public void submitQuery(UserResultsListener 
resultsListener, RunQuery query) {
   /**
* Connects, and if required, authenticates. This method blocks until 
both operations are complete.
*
-   * @param endpoint endpoint to connect to
-   * @param properties properties
+   * @param endpointendpoint to connect to
+   * @param properties  properties
* @param credentials credentials
* @throws RpcException if either connection or authentication fails
*/
   public void connect(final DrillbitEndpoint endpoint, final 
DrillProperties properties,
-  final UserCredentials credentials) throws 
RpcException {
-final UserToBitHandshake.Builder hsBuilder = 
UserToBitHandshake.newBuilder()
-.setRpcVersion(UserRpcConfig.RPC_VERSION)
-.setSupportListening(true)
-.setSupportComplexTypes(supportComplexTypes)
-.setSupportTimeout(true)
-.setCredentials(credentials)
-.setClientInfos(UserRpcUtils.getRpcEndpointInfos(clientName))
-.setSaslSupport(SaslSupport.SASL_PRIVACY)
-.setProperties(properties.serializeForServer());
+  final UserCredentials credentials) throws RpcException {
+final UserToBitHandshake.Builder hsBuilder =
+
UserToBitHandshake.newBuilder().setRpcVersion(UserRpcConfig.RPC_VERSION).setSupportListening(true)
--- End diff --

Having each parameter assignment on separate line was more readable...


---

[GitHub] drill pull request #950: DRILL-5431: SSL Support

[arina-ielchiieva]

Github user arina-ielchiieva commented on a diff in the pull request:

https://github.com/apache/drill/pull/950#discussion_r142679480
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/ssl/SSLConfigServer.java ---
@@ -0,0 +1,331 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.drill.exec.ssl;
+
+import com.google.common.base.Preconditions;
+import io.netty.handler.ssl.SslContext;
+import io.netty.handler.ssl.SslContextBuilder;
+import io.netty.handler.ssl.SslProvider;
+import org.apache.drill.common.config.DrillConfig;
+import org.apache.drill.common.exceptions.DrillException;
+import org.apache.drill.exec.ExecConstants;
+import org.apache.drill.exec.memory.BufferAllocator;
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.security.ssl.SSLFactory;
+
+import javax.net.ssl.KeyManagerFactory;
+import javax.net.ssl.SSLContext;
+import javax.net.ssl.SSLEngine;
+import javax.net.ssl.TrustManagerFactory;
+import java.text.MessageFormat;
+
+public class SSLConfigServer extends SSLConfig {
+
+  private static final org.slf4j.Logger logger = 
org.slf4j.LoggerFactory.getLogger(SSLConfigServer.class);
+
+  private final DrillConfig config;
+  private final Configuration hadoopConfig;
+  private final boolean userSslEnabled;
+  private final boolean httpsEnabled;
+  private final String keyStoreType;
+  private final String keyStorePath;
+  private final String keyStorePassword;
+  private final String keyPassword;
+  private final String trustStoreType;
+  private final String trustStorePath;
+  private final String trustStorePassword;
+  private final String protocol;
+  private final String provider;
+
+  public SSLConfigServer(DrillConfig config, Configuration hadoopConfig) 
throws DrillException {
+this.config = config;
+SSLFactory.Mode mode = SSLFactory.Mode.SERVER;
+httpsEnabled =
+config.hasPath(ExecConstants.HTTP_ENABLE_SSL) && 
config.getBoolean(ExecConstants.HTTP_ENABLE_SSL);
+// For testing we will mock up a hadoop configuration, however for 
regular use, we find the actual hadoop config.
+boolean enableHadoopConfig = 
config.getBoolean(ExecConstants.SSL_USE_HADOOP_CONF);
+if (enableHadoopConfig) {
+  if (hadoopConfig == null) {
+this.hadoopConfig = new Configuration(); // get hadoop 
configuration
+  } else {
+this.hadoopConfig = hadoopConfig;
+  }
+  String hadoopSSLConfigFile =
+  
this.hadoopConfig.get(resolveHadoopPropertyName(HADOOP_SSL_CONF_TPL_KEY, 
getMode()));
+  logger.debug("Using Hadoop configuration for SSL");
+  logger.debug("Hadoop SSL configuration file: {}", 
hadoopSSLConfigFile);
+  this.hadoopConfig.addResource(hadoopSSLConfigFile);
+} else {
+  this.hadoopConfig = null;
+}
+userSslEnabled =
+config.hasPath(ExecConstants.USER_SSL_ENABLED) && 
config.getBoolean(ExecConstants.USER_SSL_ENABLED);
+trustStoreType = getConfigParam(ExecConstants.SSL_TRUSTSTORE_TYPE,
+resolveHadoopPropertyName(HADOOP_SSL_TRUSTSTORE_TYPE_TPL_KEY, 
mode));
+trustStorePath = getConfigParam(ExecConstants.SSL_TRUSTSTORE_PATH,
+resolveHadoopPropertyName(HADOOP_SSL_TRUSTSTORE_LOCATION_TPL_KEY, 
mode));
+trustStorePassword = 
getConfigParam(ExecConstants.SSL_TRUSTSTORE_PASSWORD,
+resolveHadoopPropertyName(HADOOP_SSL_TRUSTSTORE_PASSWORD_TPL_KEY, 
mode));
+keyStoreType = getConfigParam(ExecConstants.SSL_KEYSTORE_TYPE,
+resolveHadoopPropertyName(HADOOP_SSL_KEYSTORE_TYPE_TPL_KEY, mode));
+keyStorePath = getConfigParam(ExecConstants.SSL_KEYSTORE_PATH,
+resolveHadoopPropertyName(HADOOP_SSL_KEYSTORE_LOCATION_TPL_KEY, 
mode));
+keyStorePassword = getConfigParam(ExecConstants.SSL_KEYSTORE_PASSWORD,
+resolveHadoopPropertyName(HADOOP_SSL_KEYSTORE_PASSWORD_TPL_KEY, 
mode));
+  

[GitHub] drill pull request #950: DRILL-5431: SSL Support

[arina-ielchiieva]

Github user arina-ielchiieva commented on a diff in the pull request:

https://github.com/apache/drill/pull/950#discussion_r142683194
  
--- Diff: 
exec/java-exec/src/test/java/org/apache/drill/exec/rpc/user/security/TestUserBitSSL.java
 ---
@@ -0,0 +1,338 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.drill.exec.rpc.user.security;
+
+import com.typesafe.config.ConfigValueFactory;
+import io.netty.handler.ssl.util.SelfSignedCertificate;
+import junit.framework.TestCase;
+import org.apache.drill.BaseTestQuery;
+import org.apache.drill.common.config.DrillConfig;
+import org.apache.drill.common.config.DrillProperties;
+import org.apache.drill.exec.ExecConstants;
+import org.junit.AfterClass;
+import org.junit.BeforeClass;
+import org.junit.Test;
+
+import java.io.File;
+import java.io.FileOutputStream;
+import java.net.InetAddress;
+import java.security.KeyStore;
+import java.util.Properties;
+
+import static junit.framework.TestCase.fail;
+import static org.junit.Assert.assertEquals;
+
+public class TestUserBitSSL extends BaseTestQuery {
+  private static final org.slf4j.Logger logger =
+  org.slf4j.LoggerFactory.getLogger(TestUserBitSSL.class);
+
+  private static DrillConfig newConfig;
+  private static Properties initProps; // initial client properties
+  private static ClassLoader classLoader;
+  private static String ksPath;
+  private static String tsPath;
+  private static String emptyTSPath;
+  private static String unknownKsPath;
+
+  @BeforeClass
+  public static void setupTest() throws Exception {
+
+// Create a new DrillConfig
+classLoader = TestUserBitSSL.class.getClassLoader();
+ksPath = new 
File(classLoader.getResource("ssl/keystore.ks").getFile()).getAbsolutePath();
+unknownKsPath = new 
File(classLoader.getResource("ssl/unknownkeystore.ks").getFile()).getAbsolutePath();
+tsPath = new 
File(classLoader.getResource("ssl/truststore.ks").getFile()).getAbsolutePath();
+emptyTSPath = new 
File(classLoader.getResource("ssl/emptytruststore.ks").getFile()).getAbsolutePath();
+newConfig = new 
DrillConfig(DrillConfig.create(cloneDefaultTestConfigProperties())
+.withValue(ExecConstants.SSL_USE_HADOOP_CONF,
+ConfigValueFactory.fromAnyRef(false))
+.withValue(ExecConstants.USER_SSL_ENABLED,
+ConfigValueFactory.fromAnyRef(true))
+.withValue(ExecConstants.SSL_KEYSTORE_TYPE,
+ConfigValueFactory.fromAnyRef("JKS"))
+.withValue(ExecConstants.SSL_KEYSTORE_PATH,
+ConfigValueFactory.fromAnyRef(ksPath))
+.withValue(ExecConstants.SSL_KEYSTORE_PASSWORD,
+ConfigValueFactory.fromAnyRef("drill123"))
+.withValue(ExecConstants.SSL_KEY_PASSWORD,
+ConfigValueFactory.fromAnyRef("drill123"))
+.withValue(ExecConstants.SSL_TRUSTSTORE_TYPE,
+ConfigValueFactory.fromAnyRef("JKS"))
+.withValue(ExecConstants.SSL_TRUSTSTORE_PATH,
+ConfigValueFactory.fromAnyRef(tsPath))
+.withValue(ExecConstants.SSL_TRUSTSTORE_PASSWORD,
+ConfigValueFactory.fromAnyRef("drill123"))
+.withValue(ExecConstants.SSL_PROTOCOL,
+ConfigValueFactory.fromAnyRef("TLSv1.2")),
+  false);
+
+initProps = new Properties();
+initProps.setProperty(DrillProperties.ENABLE_TLS, "true");
+initProps.setProperty(DrillProperties.TRUSTSTORE_PATH, tsPath);
+initProps.setProperty(DrillProperties.TRUSTSTORE_PASSWORD, "drill123");
+initProps.setProperty(DrillProperties.DISABLE_HOST_VERIFICATION, 
"true");
+
+// Start an SSL enabled cluster
+updateTestCluster(1, newConfig, initProps);
+  }
+
+  @AfterClass
+  public static void cleanTest() throws Exception {
+DrillConfig restoreConfig =
+new 

[GitHub] drill pull request #950: DRILL-5431: SSL Support

[arina-ielchiieva]

Github user arina-ielchiieva commented on a diff in the pull request:

https://github.com/apache/drill/pull/950#discussion_r142681811
  
--- Diff: 
exec/java-exec/src/test/java/org/apache/drill/exec/TestSSLConfig.java ---
@@ -49,12 +64,19 @@ public void testMissingKeystorePassword() throws 
Exception {
 ConfigBuilder config = new ConfigBuilder();
 config.put(ExecConstants.HTTP_KEYSTORE_PATH, "/root");
 config.put(ExecConstants.HTTP_KEYSTORE_PASSWORD, "");
+config.put(ExecConstants.SSL_USE_HADOOP_CONF, false);
+config.put(ExecConstants.USER_SSL_ENABLED, true);
 try {
-  SSLConfig sslv = new SSLConfig(config.build());
+  SSLConfig sslv = new SSLConfigBuilder()
+  .config(config.build())
+  .mode(SSLFactory.Mode.SERVER)
+  .initializeSSLContext(false)
+  .validateKeyStore(true)
+  .build();
   fail();
   //Expected
 } catch (Exception e) {
-  assertTrue(e instanceof DrillException);
+
--- End diff --

So what is test is actually testing? Since we just fail and ignore the 
exception.


---

[GitHub] drill pull request #950: DRILL-5431: SSL Support

[arina-ielchiieva]

Github user arina-ielchiieva commented on a diff in the pull request:

https://github.com/apache/drill/pull/950#discussion_r142678376
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/ssl/SSLConfigClient.java ---
@@ -0,0 +1,273 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.drill.exec.ssl;
+
+import io.netty.handler.ssl.SslContext;
+import io.netty.handler.ssl.SslContextBuilder;
+import io.netty.handler.ssl.SslProvider;
+import org.apache.drill.common.config.DrillConfig;
+import org.apache.drill.common.config.DrillProperties;
+import org.apache.drill.common.exceptions.DrillException;
+import org.apache.drill.exec.memory.BufferAllocator;
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.security.ssl.SSLFactory;
+
+import javax.net.ssl.SSLContext;
+import javax.net.ssl.SSLEngine;
+import javax.net.ssl.SSLParameters;
+import javax.net.ssl.TrustManagerFactory;
+import java.util.Properties;
+
+public class SSLConfigClient extends SSLConfig {
+
+  private static final org.slf4j.Logger logger = 
org.slf4j.LoggerFactory.getLogger(SSLConfigClient.class);
+
+  Properties properties;
--- End diff --

private final?


---

[GitHub] drill pull request #950: DRILL-5431: SSL Support

[arina-ielchiieva]

Github user arina-ielchiieva commented on a diff in the pull request:

https://github.com/apache/drill/pull/950#discussion_r142687176
  
--- Diff: distribution/pom.xml ---
@@ -97,8 +97,41 @@
 
   org.apache.hbase
   hbase-client
+  
+   
+   io.netty
+   netty
+   
+  
+
+
--- End diff --

Not sure why we need to netty dependency with `test` scope in distribution 
pom... 


---

[GitHub] drill pull request #950: DRILL-5431: SSL Support

[arina-ielchiieva]

Github user arina-ielchiieva commented on a diff in the pull request:

https://github.com/apache/drill/pull/950#discussion_r142674202
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/ssl/SSLConfig.java ---
@@ -0,0 +1,265 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.drill.exec.ssl;
+
+import io.netty.handler.ssl.SslContext;
+import io.netty.handler.ssl.SslProvider;
+import io.netty.handler.ssl.util.InsecureTrustManagerFactory;
+import org.apache.drill.common.exceptions.DrillException;
+import org.apache.drill.exec.memory.BufferAllocator;
+import org.apache.hadoop.security.ssl.SSLFactory;
+
+import javax.net.ssl.KeyManagerFactory;
+import javax.net.ssl.SSLContext;
+import javax.net.ssl.SSLEngine;
+import javax.net.ssl.TrustManagerFactory;
+import java.io.FileInputStream;
+import java.io.InputStream;
+import java.security.KeyStore;
+
+public abstract class SSLConfig {
+
+  private static final org.slf4j.Logger logger = 
org.slf4j.LoggerFactory.getLogger(SSLConfig.class);
+
+  public static final String DEFAULT_SSL_PROVIDER = "JDK"; // JDK or 
OPENSSL
+  public static final String DEFAULT_SSL_PROTOCOL = "TLSv1.2";
+  public static final int DEFAULT_SSL_HANDSHAKE_TIMEOUT_MS = 10 * 1000; // 
10 seconds
+
+  // Either the Netty SSL context or the JDK SSL context will be 
initialized
+  // The JDK SSL context is use iff the useSystemTrustStore setting is 
enabled.
+  protected SslContext nettySslContext;
+  protected SSLContext jdkSSlContext;
+
+  private static final boolean isWindows = 
System.getProperty("os.name").toLowerCase().indexOf("win") >= 0;
+  private static final boolean isMacOs = 
System.getProperty("os.name").toLowerCase().indexOf("mac") >= 0;
+
+  public static final String HADOOP_SSL_CONF_TPL_KEY = 
"hadoop.ssl.{0}.conf";
+  public static final String HADOOP_SSL_KEYSTORE_LOCATION_TPL_KEY = 
"ssl.{0}.keystore.location";
+  public static final String HADOOP_SSL_KEYSTORE_PASSWORD_TPL_KEY = 
"ssl.{0}.keystore.password";
+  public static final String HADOOP_SSL_KEYSTORE_TYPE_TPL_KEY = 
"ssl.{0}.keystore.type";
+  public static final String HADOOP_SSL_KEYSTORE_KEYPASSWORD_TPL_KEY =
+  "ssl.{0}.keystore.keypassword";
+  public static final String HADOOP_SSL_TRUSTSTORE_LOCATION_TPL_KEY = 
"ssl.{0}.truststore.location";
+  public static final String HADOOP_SSL_TRUSTSTORE_PASSWORD_TPL_KEY = 
"ssl.{0}.truststore.password";
+  public static final String HADOOP_SSL_TRUSTSTORE_TYPE_TPL_KEY = 
"ssl.{0}.truststore.type";
+
+  public SSLConfig() {
+  }
+
+  public abstract void validateKeyStore() throws DrillException;
+
+  // We need to use different SSLContext objects depending on what the 
user has chosen
+  // For most uses we will use the Netty SslContext class. This allows us 
to use either
+  // the JDK implementation or the OpenSSL implementation. However if the 
user wants to
+  // use the system trust store, then the only way to access it is via the 
JDK's
+  // SSLContext class. (See the createSSLEngine method below).
+
+  public abstract SslContext initNettySslContext() throws DrillException;
+
+  public abstract SSLContext initJDKSSLContext() throws DrillException;
+
+  public abstract boolean isUserSslEnabled();
+
+  public abstract boolean isHttpsEnabled();
+
+  public abstract String getKeyStoreType();
+
+  public abstract String getKeyStorePath();
+
+  public abstract String getKeyStorePassword();
+
+  public abstract String getKeyPassword();
+
+  public abstract String getTrustStoreType();
+
+  public abstract boolean hasTrustStorePath();
+
+  public abstract String getTrustStorePath();
+
+  public abstract boolean hasTrustStorePassword();
+
+  public abstract String getTrustStorePassword();
+
+  public abstract String getProtocol();
+
+  public abstract SslProvider getProvider();
+

[GitHub] drill pull request #950: DRILL-5431: SSL Support

[arina-ielchiieva]

Github user arina-ielchiieva commented on a diff in the pull request:

https://github.com/apache/drill/pull/950#discussion_r142691920
  
--- Diff: exec/java-exec/pom.xml ---
@@ -589,6 +669,17 @@
 
   
 
+
+  
+  
+kr.motd.maven
+os-maven-plugin
+1.4.0.Final
--- End diff --

Could not build Drill project on centos. Failed with exception:
`[ERROR] Failed to execute goal 
org.apache.maven.plugins:maven-shade-plugin:2.4.1:shade (default) on project 
drill-jdbc-all: Error creating shaded jar: The name 
"os.detected.release.like."centos"" is not legal for JDOM/XML elements: XML 
names cannot contain the character """. -> [Help 1]`

It seems we need to use `1.5.0.Final` version (example of similar problem - 
https://github.com/apache/beam/pull/2391).



---

[GitHub] drill pull request #950: DRILL-5431: SSL Support

[parthchandra]

Github user parthchandra commented on a diff in the pull request:

https://github.com/apache/drill/pull/950#discussion_r141728836
  
--- Diff: contrib/native/client/src/clientlib/channel.cpp ---
@@ -0,0 +1,452 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#include 
+#include 
+
+#include "drill/drillConfig.hpp"
+#include "drill/drillError.hpp"
+#include "drill/userProperties.hpp"
+#include "channel.hpp"
+#include "errmsgs.hpp"
+#include "logger.hpp"
+#include "utils.hpp"
+#include "zookeeperClient.hpp"
+
+#include "GeneralRPC.pb.h"
+
+namespace Drill{
+
+ConnectionEndpoint::ConnectionEndpoint(const char* connStr){
+m_connectString=connStr;
+m_pError=NULL;
+}
+
+ConnectionEndpoint::ConnectionEndpoint(const char* host, const char* port){
+m_host=host;
+m_port=port;
+m_protocol="drillbit"; // direct connection
+m_pError=NULL;
+}
+
+ConnectionEndpoint::~ConnectionEndpoint(){
+if(m_pError!=NULL){
+delete m_pError; m_pError=NULL;
+}
+}
+
+connectionStatus_t ConnectionEndpoint::getDrillbitEndpoint(){
+connectionStatus_t ret=CONN_SUCCESS;
+if(!m_connectString.empty()){
+parseConnectString();
+if(m_protocol.empty()){
+return handleError(CONN_INVALID_INPUT, 
getMessage(ERR_CONN_UNKPROTO, ""));
+}
+if(isZookeeperConnection()){
+if((ret=getDrillbitEndpointFromZk())!=CONN_SUCCESS){
+return ret;
+}
+}else if(!this->isDirectConnection()){
+return handleError(CONN_INVALID_INPUT, 
getMessage(ERR_CONN_UNKPROTO, this->getProtocol().c_str()));
+}
+}else{
+if(m_host.empty() || m_port.empty()){
+return handleError(CONN_INVALID_INPUT, 
getMessage(ERR_CONN_NOCONNSTR));
+}
+}
+return ret;
+}
+
+void ConnectionEndpoint::parseConnectString(){
+boost::regex connStrExpr("(.*)=(.*):([0-9]+)(?:/(.+))?");
+boost::cmatch matched;
+
+if(boost::regex_match(m_connectString.c_str(), matched, connStrExpr)){
+m_protocol.assign(matched[1].first, matched[1].second);
+std::string host, port;
+host.assign(matched[2].first, matched[2].second);
+port.assign(matched[3].first, matched[3].second);
+if(isDirectConnection()){
+// if the connection is to a zookeeper, 
+// we will get the host and the port only after connecting to 
the Zookeeper
+m_host=host;
+m_port=port;
+}
+m_hostPortStr=host+std::string(":")+port;
+std::string pathToDrill;
+if(matched.size()==5){
+pathToDrill.assign(matched[4].first, matched[4].second);
+if(!pathToDrill.empty()){
+m_pathToDrill=std::string("/")+pathToDrill;
+}
+}
+DRILL_MT_LOG(DRILL_LOG(LOG_DEBUG) 
+<< "Conn str: "<< m_connectString 
+<< ";  protocol: " << m_protocol 
+<< ";  host: " << host 
+<< "; port: " << port 
+<< ";  path to drill: " << m_pathToDrill 
+<< std::endl;)
+} else {
+DRILL_MT_LOG(DRILL_LOG(LOG_DEBUG) << "Invalid connect string. 
Regexp did not match" << std::endl;)
+}
+
+return;
+}
+
+bool ConnectionEndpoint::isDirectConnection(){
+assert(!m_protocol.empty());
+return (!strcmp(m_protocol.c_str(), "local") || 
!strcmp(m_protocol.c_str(), "drillbit"));
+}
+
+bool ConnectionEndpoint::isZookeeperConnection(){
+assert(!m_protocol.empty());
+return (!strcmp(m_protocol.c_str(), "zk"));
+}
+
+connectionStatus_t ConnectionEndpoint::getDrillbitEndpointFromZk(){
+ZookeeperClient zook(m_pathToDrill);
+

[GitHub] drill pull request #950: DRILL-5431: SSL Support

[parthchandra]

Github user parthchandra commented on a diff in the pull request:

https://github.com/apache/drill/pull/950#discussion_r141740253
  
--- Diff: contrib/native/client/src/clientlib/channel.hpp ---
@@ -0,0 +1,237 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#ifndef CHANNEL_HPP
+#define CHANNEL_HPP
+
+#include "drill/common.hpp"
+#include "drill/drillClient.hpp"
+#include "streamSocket.hpp"
+
+namespace Drill {
+
+class UserProperties;
+
+class ConnectionEndpoint{
+public:
+ConnectionEndpoint(const char* connStr);
+ConnectionEndpoint(const char* host, const char* port);
+~ConnectionEndpoint();
+
+//parse the connection string and set up the host and port to 
connect to
+connectionStatus_t getDrillbitEndpoint();
+
+std::string& getProtocol(){return m_protocol;}
+std::string& getHost(){return m_host;}
+std::string& getPort(){return m_port;}
+DrillClientError* getError(){ return m_pError;};
+
+private:
+void parseConnectString();
+connectionStatus_t validateConnectionString();
+bool isDirectConnection();
+bool isZookeeperConnection();
+connectionStatus_t getDrillbitEndpointFromZk();
+connectionStatus_t handleError(connectionStatus_t status, 
std::string msg);
+
+std::string m_connectString;
+std::string m_pathToDrill;
+std::string m_protocol; 
+std::string m_hostPortStr;
+std::string m_host;
+std::string m_port;
+
+DrillClientError* m_pError;
+
+};
+
+class ChannelContext{
+public:
+ChannelContext(DrillUserProperties* 
props):m_properties(props){};
+virtual ~ChannelContext(){};
+const DrillUserProperties* getUserProperties() const { return 
m_properties;}
+protected:
+DrillUserProperties* m_properties;
+};
+
+class SSLChannelContext: public ChannelContext{
+public:
+static boost::asio::ssl::context::method 
getTlsVersion(std::string version){
+if(version.empty()){
+return boost::asio::ssl::context::tlsv12;
+} else if (version == "tlsv12") {
+return boost::asio::ssl::context::tlsv12;
+} else if (version == "tlsv11") {
+return boost::asio::ssl::context::tlsv11;
+} else if (version == "sslv23") {
+return boost::asio::ssl::context::sslv23;
+} else if (version == "tlsv1") {
+return boost::asio::ssl::context::tlsv1;
+} else if (version == "sslv3") {
+return boost::asio::ssl::context::sslv3;
+} else {
+return boost::asio::ssl::context::tlsv12;
+}
+}
+
+SSLChannelContext(DrillUserProperties *props, 
boost::asio::ssl::context::method tlsVersion, boost::asio::ssl::verify_mode 
verifyMode) :
+ChannelContext(props),
+m_SSLContext(tlsVersion) {
+m_SSLContext.set_default_verify_paths();
+m_SSLContext.set_options(
+boost::asio::ssl::context::default_workarounds
+| boost::asio::ssl::context::no_sslv2
+| boost::asio::ssl::context::single_dh_use
+);
+m_SSLContext.set_verify_mode(verifyMode);
+};
+~SSLChannelContext(){};
+boost::asio::ssl::context& getSslContext(){ return 
m_SSLContext;}
+private:
+boost::asio::ssl::context m_SSLContext;
+};
   

[GitHub] drill pull request #950: DRILL-5431: SSL Support

[parthchandra]

Github user parthchandra commented on a diff in the pull request:

https://github.com/apache/drill/pull/950#discussion_r141726875
  
--- Diff: contrib/native/client/src/clientlib/channel.cpp ---
@@ -0,0 +1,452 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#include 
+#include 
+
+#include "drill/drillConfig.hpp"
+#include "drill/drillError.hpp"
+#include "drill/userProperties.hpp"
+#include "channel.hpp"
+#include "errmsgs.hpp"
+#include "logger.hpp"
+#include "utils.hpp"
+#include "zookeeperClient.hpp"
+
+#include "GeneralRPC.pb.h"
+
+namespace Drill{
+
+ConnectionEndpoint::ConnectionEndpoint(const char* connStr){
+m_connectString=connStr;
+m_pError=NULL;
+}
+
+ConnectionEndpoint::ConnectionEndpoint(const char* host, const char* port){
+m_host=host;
+m_port=port;
+m_protocol="drillbit"; // direct connection
+m_pError=NULL;
+}
+
+ConnectionEndpoint::~ConnectionEndpoint(){
+if(m_pError!=NULL){
+delete m_pError; m_pError=NULL;
+}
+}
+
+connectionStatus_t ConnectionEndpoint::getDrillbitEndpoint(){
+connectionStatus_t ret=CONN_SUCCESS;
+if(!m_connectString.empty()){
+parseConnectString();
+if(m_protocol.empty()){
+return handleError(CONN_INVALID_INPUT, 
getMessage(ERR_CONN_UNKPROTO, ""));
+}
+if(isZookeeperConnection()){
+if((ret=getDrillbitEndpointFromZk())!=CONN_SUCCESS){
+return ret;
+}
+}else if(!this->isDirectConnection()){
+return handleError(CONN_INVALID_INPUT, 
getMessage(ERR_CONN_UNKPROTO, this->getProtocol().c_str()));
+}
+}else{
+if(m_host.empty() || m_port.empty()){
+return handleError(CONN_INVALID_INPUT, 
getMessage(ERR_CONN_NOCONNSTR));
+}
+}
+return ret;
+}
+
+void ConnectionEndpoint::parseConnectString(){
+boost::regex connStrExpr("(.*)=(.*):([0-9]+)(?:/(.+))?");
+boost::cmatch matched;
+
+if(boost::regex_match(m_connectString.c_str(), matched, connStrExpr)){
+m_protocol.assign(matched[1].first, matched[1].second);
+std::string host, port;
+host.assign(matched[2].first, matched[2].second);
+port.assign(matched[3].first, matched[3].second);
+if(isDirectConnection()){
+// if the connection is to a zookeeper, 
+// we will get the host and the port only after connecting to 
the Zookeeper
+m_host=host;
+m_port=port;
+}
+m_hostPortStr=host+std::string(":")+port;
+std::string pathToDrill;
+if(matched.size()==5){
+pathToDrill.assign(matched[4].first, matched[4].second);
+if(!pathToDrill.empty()){
+m_pathToDrill=std::string("/")+pathToDrill;
+}
+}
+DRILL_MT_LOG(DRILL_LOG(LOG_DEBUG) 
+<< "Conn str: "<< m_connectString 
+<< ";  protocol: " << m_protocol 
+<< ";  host: " << host 
+<< "; port: " << port 
+<< ";  path to drill: " << m_pathToDrill 
+<< std::endl;)
+} else {
+DRILL_MT_LOG(DRILL_LOG(LOG_DEBUG) << "Invalid connect string. 
Regexp did not match" << std::endl;)
+}
+
+return;
+}
+
+bool ConnectionEndpoint::isDirectConnection(){
+assert(!m_protocol.empty());
+return (!strcmp(m_protocol.c_str(), "local") || 
!strcmp(m_protocol.c_str(), "drillbit"));
+}
+
+bool ConnectionEndpoint::isZookeeperConnection(){
+assert(!m_protocol.empty());
+return (!strcmp(m_protocol.c_str(), "zk"));
+}
+
+connectionStatus_t ConnectionEndpoint::getDrillbitEndpointFromZk(){
+ZookeeperClient zook(m_pathToDrill);
+

[GitHub] drill pull request #950: DRILL-5431: SSL Support

[parthchandra]

Github user parthchandra commented on a diff in the pull request:

https://github.com/apache/drill/pull/950#discussion_r141732868
  
--- Diff: contrib/native/client/src/clientlib/channel.hpp ---
@@ -0,0 +1,237 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#ifndef CHANNEL_HPP
+#define CHANNEL_HPP
+
+#include "drill/common.hpp"
+#include "drill/drillClient.hpp"
+#include "streamSocket.hpp"
+
+namespace Drill {
+
+class UserProperties;
+
+class ConnectionEndpoint{
+public:
+ConnectionEndpoint(const char* connStr);
+ConnectionEndpoint(const char* host, const char* port);
+~ConnectionEndpoint();
+
+//parse the connection string and set up the host and port to 
connect to
+connectionStatus_t getDrillbitEndpoint();
+
+std::string& getProtocol(){return m_protocol;}
+std::string& getHost(){return m_host;}
+std::string& getPort(){return m_port;}
+DrillClientError* getError(){ return m_pError;};
+
+private:
+void parseConnectString();
+connectionStatus_t validateConnectionString();
+bool isDirectConnection();
+bool isZookeeperConnection();
+connectionStatus_t getDrillbitEndpointFromZk();
+connectionStatus_t handleError(connectionStatus_t status, 
std::string msg);
+
+std::string m_connectString;
+std::string m_pathToDrill;
+std::string m_protocol; 
+std::string m_hostPortStr;
+std::string m_host;
+std::string m_port;
+
+DrillClientError* m_pError;
+
+};
+
+class ChannelContext{
+public:
+ChannelContext(DrillUserProperties* 
props):m_properties(props){};
+virtual ~ChannelContext(){};
+const DrillUserProperties* getUserProperties() const { return 
m_properties;}
+protected:
+DrillUserProperties* m_properties;
+};
+
+class SSLChannelContext: public ChannelContext{
+public:
+static boost::asio::ssl::context::method 
getTlsVersion(std::string version){
+if(version.empty()){
+return boost::asio::ssl::context::tlsv12;
+} else if (version == "tlsv12") {
+return boost::asio::ssl::context::tlsv12;
+} else if (version == "tlsv11") {
+return boost::asio::ssl::context::tlsv11;
+} else if (version == "sslv23") {
+return boost::asio::ssl::context::sslv23;
+} else if (version == "tlsv1") {
+return boost::asio::ssl::context::tlsv1;
+} else if (version == "sslv3") {
+return boost::asio::ssl::context::sslv3;
+} else {
+return boost::asio::ssl::context::tlsv12;
+}
+}
+
+SSLChannelContext(DrillUserProperties *props, 
boost::asio::ssl::context::method tlsVersion, boost::asio::ssl::verify_mode 
verifyMode) :
+ChannelContext(props),
+m_SSLContext(tlsVersion) {
+m_SSLContext.set_default_verify_paths();
+m_SSLContext.set_options(
+boost::asio::ssl::context::default_workarounds
+| boost::asio::ssl::context::no_sslv2
+| boost::asio::ssl::context::single_dh_use
+);
+m_SSLContext.set_verify_mode(verifyMode);
+};
+~SSLChannelContext(){};
+boost::asio::ssl::context& getSslContext(){ return 
m_SSLContext;}
+private:
+boost::asio::ssl::context m_SSLContext;
+};
   

[GitHub] drill pull request #950: DRILL-5431: SSL Support

[parthchandra]

Github user parthchandra commented on a diff in the pull request:

https://github.com/apache/drill/pull/950#discussion_r141676885
  
--- Diff: contrib/native/client/src/clientlib/wincert.ipp ---
@@ -0,0 +1,91 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#if defined(IS_SSL_ENABLED)
+
+#include 
+#include 
+
+#if defined _WIN32  || defined _WIN64
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+
+#pragma comment (lib, "crypt32.lib")
+#pragma comment (lib, "cryptui.lib")
+
+#define MY_ENCODING_TYPE  (PKCS_7_ASN_ENCODING | X509_ASN_ENCODING)
+
+inline
+int loadSystemTrustStore(const SSL *ssl) {
--- End diff --

Good idea. I'll make it a generic error message string. Store name is 
windows specific and later we might enhance this to support other native system 
certificate stores


---

[GitHub] drill pull request #950: DRILL-5431: SSL Support

[parthchandra]

Github user parthchandra commented on a diff in the pull request:

https://github.com/apache/drill/pull/950#discussion_r141676469
  
--- Diff: contrib/native/client/src/clientlib/zkCluster.cpp ---
@@ -0,0 +1,175 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#include "drill/common.hpp"
+#include 
+#ifdef _WIN32
+#include 
+#else
+#include 
+#endif
+#include "drill/drillConfig.hpp"
+#include "drill/drillClient.hpp"
+#include "errmsgs.hpp"
+#include "logger.hpp"
+#include "zkCluster.hpp"
+
+namespace Drill{
+
+char ZkCluster::s_drillRoot[]="/drill/";
+char ZkCluster::s_defaultCluster[]="drillbits1";
+
+ZkCluster::ZkCluster(){
+m_pDrillbits=new String_vector;
+srand (time(NULL));
+m_bConnecting=true;
+memset(_id, 0, sizeof(m_id));
+}
+
+ZkCluster::~ZkCluster(){
+delete m_pDrillbits;
+}
+
+ZooLogLevel ZkCluster::getZkLogLevel(){
+//typedef enum {ZOO_LOG_LEVEL_ERROR=1,
+//ZOO_LOG_LEVEL_WARN=2,
+//ZOO_LOG_LEVEL_INFO=3,
+//ZOO_LOG_LEVEL_DEBUG=4
+//} ZooLogLevel;
+switch(DrillClientConfig::getLogLevel()){
+case LOG_TRACE:
+case LOG_DEBUG:
+return ZOO_LOG_LEVEL_DEBUG;
+case LOG_INFO:
+return ZOO_LOG_LEVEL_INFO;
+case LOG_WARNING:
+return ZOO_LOG_LEVEL_WARN;
+case LOG_ERROR:
+case LOG_FATAL:
+default:
+return ZOO_LOG_LEVEL_ERROR;
+}
+return ZOO_LOG_LEVEL_ERROR;
+}
+
+int ZkCluster::connectToZookeeper(const char* connectStr, const char* 
pathToDrill){
+uint32_t waitTime=3; // 10 seconds
--- End diff --

Done


---

[GitHub] drill pull request #950: DRILL-5431: SSL Support

[parthchandra]

Github user parthchandra commented on a diff in the pull request:

https://github.com/apache/drill/pull/950#discussion_r141677219
  
--- Diff: contrib/native/client/src/clientlib/wincert.ipp ---
@@ -0,0 +1,91 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#if defined(IS_SSL_ENABLED)
+
+#include 
+#include 
+
+#if defined _WIN32  || defined _WIN64
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+
+#pragma comment (lib, "crypt32.lib")
+#pragma comment (lib, "cryptui.lib")
+
+#define MY_ENCODING_TYPE  (PKCS_7_ASN_ENCODING | X509_ASN_ENCODING)
+
+inline
+int loadSystemTrustStore(const SSL *ssl) {
+HCERTSTORE hStore;
+PCCERT_CONTEXT pContext = NULL;
+X509 *x509;
+   char* stores[] = {
+   "CA",
+   "MY",
+   "ROOT",
+   "SPC"
+   };
+ 
+SSL_CTX * ctx = SSL_get_SSL_CTX(ssl);
+X509_STORE *store = SSL_CTX_get_cert_store(ctx);
+
+   for(int i=0; i<4; i++){
+hStore = CertOpenSystemStore(NULL, stores[i]);
+
+if (!hStore)
+return 1;
--- End diff --

BTW, if not certificates exist in the store (unlikely), then the 
certificate validation itself will fail. 


---

[GitHub] drill pull request #950: DRILL-5431: SSL Support

[parthchandra]

Github user parthchandra commented on a diff in the pull request:

https://github.com/apache/drill/pull/950#discussion_r141725904
  
--- Diff: contrib/native/client/src/clientlib/channel.cpp ---
@@ -0,0 +1,452 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#include 
+#include 
+
+#include "drill/drillConfig.hpp"
+#include "drill/drillError.hpp"
+#include "drill/userProperties.hpp"
+#include "channel.hpp"
+#include "errmsgs.hpp"
+#include "logger.hpp"
+#include "utils.hpp"
+#include "zookeeperClient.hpp"
+
+#include "GeneralRPC.pb.h"
+
+namespace Drill{
+
+ConnectionEndpoint::ConnectionEndpoint(const char* connStr){
+m_connectString=connStr;
+m_pError=NULL;
+}
+
+ConnectionEndpoint::ConnectionEndpoint(const char* host, const char* port){
+m_host=host;
+m_port=port;
+m_protocol="drillbit"; // direct connection
+m_pError=NULL;
+}
+
+ConnectionEndpoint::~ConnectionEndpoint(){
+if(m_pError!=NULL){
+delete m_pError; m_pError=NULL;
+}
+}
+
+connectionStatus_t ConnectionEndpoint::getDrillbitEndpoint(){
+connectionStatus_t ret=CONN_SUCCESS;
+if(!m_connectString.empty()){
+parseConnectString();
+if(m_protocol.empty()){
+return handleError(CONN_INVALID_INPUT, 
getMessage(ERR_CONN_UNKPROTO, ""));
+}
+if(isZookeeperConnection()){
+if((ret=getDrillbitEndpointFromZk())!=CONN_SUCCESS){
+return ret;
+}
+}else if(!this->isDirectConnection()){
+return handleError(CONN_INVALID_INPUT, 
getMessage(ERR_CONN_UNKPROTO, this->getProtocol().c_str()));
+}
+}else{
+if(m_host.empty() || m_port.empty()){
+return handleError(CONN_INVALID_INPUT, 
getMessage(ERR_CONN_NOCONNSTR));
+}
+}
+return ret;
+}
+
+void ConnectionEndpoint::parseConnectString(){
+boost::regex connStrExpr("(.*)=(.*):([0-9]+)(?:/(.+))?");
--- End diff --

Updated the regexp.



---

[GitHub] drill pull request #950: DRILL-5431: SSL Support

[parthchandra]

Github user parthchandra commented on a diff in the pull request:

https://github.com/apache/drill/pull/950#discussion_r141725643
  
--- Diff: contrib/native/client/src/clientlib/streamSocket.hpp ---
@@ -0,0 +1,212 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+
+#ifndef STREAMSOCKET_HPP
+#define STREAMSOCKET_HPP
+
+#include "drill/common.hpp"
+#include "env.h"
+#include "wincert.ipp"
+
+#include 
+#include 
+
+namespace Drill {
+
+typedef boost::asio::ip::tcp::socket::lowest_layer_type streamSocket_t;
+typedef boost::asio::ssl::stream 
sslTCPSocket_t;
+typedef boost::asio::ip::tcp::socket basicTCPSocket_t;
+
+
+// Some helper typedefs to define the highly templatized boost::asio 
methods
+typedef boost::asio::const_buffers_1 ConstBufferSequence; 
+typedef boost::asio::mutable_buffers_1 MutableBufferSequence;
+
+// ReadHandlers have different possible signatures.
+//
+// As a standard C-type callback
+//typedef void (*ReadHandler)(const boost::system::error_code& ec, 
std::size_t bytes_transferred);
+//
+// Or as a C++ functor
+//struct ReadHandler {
+//virtual void operator()(
+//const boost::system::error_code& ec,
+//std::size_t bytes_transferred) = 0;
+//};
+//
+// We need a different signature though, since we need to pass in a member 
function of a drill client 
+// class (which is C++), as a functor generated by boost::bind as a 
ReadHandler
+// 
+typedef boost::function ReadHandler;
+
+class AsioStreamSocket{
+public:
+virtual ~AsioStreamSocket(){};
+virtual streamSocket_t& getInnerSocket() = 0;
+
+virtual std::size_t writeSome(
+const ConstBufferSequence& buffers,
+boost::system::error_code & ec) = 0;
+
+virtual std::size_t readSome(
+const MutableBufferSequence& buffers,
+boost::system::error_code & ec) = 0;
+
+//
+// boost::asio::async_read has the signature 
+// template<
+// typename AsyncReadStream,
+// typename MutableBufferSequence,
+// typename ReadHandler>
+// void-or-deduced async_read(
+// AsyncReadStream & s,
+// const MutableBufferSequence & buffers,
+// ReadHandler handler);
+//
+// For our use case, the derived class will have an instance of a 
concrete type for AsyncReadStream which 
+// will implement the requirements for the AsyncReadStream type. 
We need not pass that in as a parameter 
+// since the class already has the value
+// The method is templatized since the ReadHandler type is 
dependent on the class implementing the read 
+// handler (basically the class using the asio stream)
+//
+virtual void asyncRead( const MutableBufferSequence & buffers, 
ReadHandler handler) = 0;
+
+// call the underlying protocol's handshake method.
+// if the useSystemConfig flag is true, then use properties read
+// from the underlying operating system
+virtual void protocolHandshake(bool useSystemConfig) = 0;
+virtual void protocolClose() = 0;
+};
+
+class Socket: 
+public AsioStreamSocket, 
+public basicTCPSocket_t{
+
+public:
+Socket(boost::asio::io_service& ioService) : 
basicTCPSocket_t(ioService) {
+}
+
+~Socket(){
+boost::system::error_code ignorederr;
+this->shutdown(boost::asio::ip::tcp::socket::shutdown_both, 
ignorederr);
+this->close();
+};
+
+basicTCPSocket_t& getSocketStream(){ return *this;}
+
+streamSocket_t& getInnerSocket(){ return this->lowest_layer();}
+
+std::size_t writeSome(
+

[GitHub] drill pull request #950: DRILL-5431: SSL Support

[parthchandra]

Github user parthchandra commented on a diff in the pull request:

https://github.com/apache/drill/pull/950#discussion_r141742537
  
--- Diff: contrib/native/client/src/clientlib/drillClientImpl.cpp ---
@@ -65,108 +66,70 @@ struct ToRpcType: public 
std::unary_functionm_bIsConnected) {
-if(std::strcmp(connStr, m_connectStr.c_str())){ // trying to 
connect to a different address is not allowed if already connected
+connectionStatus_t DrillClientImpl::connect(const char* connStr, 
DrillUserProperties* props){
+if (this->m_bIsConnected || (this->m_pChannelContext!=NULL && 
this->m_pChannel!=NULL)) {
+if(!std::strcmp(connStr, m_connectStr.c_str())){
+// trying to connect to a different address is not allowed if 
already connected
 return handleConnError(CONN_ALREADYCONNECTED, 
getMessage(ERR_CONN_ALREADYCONN));
 }
 return CONN_SUCCESS;
 }
+std::string val;
+channelType_t type = ( props->isPropSet(USERPROP_USESSL) &&
+props->getProp(USERPROP_USESSL, val) =="true") ?
+CHANNEL_TYPE_SSLSTREAM :
+CHANNEL_TYPE_SOCKET;
 
-m_connectStr=connStr;
-Utils::parseConnectStr(connStr, pathToDrill, protocol, hostPortStr);
-if(protocol == "zk"){
-ZookeeperClient zook(pathToDrill);
-std::vector drillbits;
-int err = zook.getAllDrillbits(hostPortStr, drillbits);
-if(!err){
-if (drillbits.empty()){
-return handleConnError(CONN_FAILURE, 
getMessage(ERR_CONN_ZKNODBIT));
-}
-Utils::shuffle(drillbits);
-exec::DrillbitEndpoint endpoint;
-err = zook.getEndPoint(drillbits[drillbits.size() -1], 
endpoint);// get the last one in the list
-if(!err){
-host=boost::lexical_cast(endpoint.address());
-
port=boost::lexical_cast(endpoint.user_port());
-}
-DRILL_MT_LOG(DRILL_LOG(LOG_TRACE) << "Choosing drillbit <" << 
(drillbits.size() - 1)  << ">. Selected " << endpoint.DebugString() << 
std::endl;)
-
-}
-if(err){
-return handleConnError(CONN_ZOOKEEPER_ERROR, 
getMessage(ERR_CONN_ZOOKEEPER, zook.getError().c_str()));
-}
-zook.close();
-m_bIsDirectConnection=true;
-}else if(protocol == "local"){
-boost::lock_guard lock(m_dcMutex);//strtok is not 
reentrant
-char tempStr[MAX_CONNECT_STR+1];
-strncpy(tempStr, hostPortStr.c_str(), MAX_CONNECT_STR); 
tempStr[MAX_CONNECT_STR]=0;
-host=strtok(tempStr, ":");
-port=strtok(NULL, "");
-m_bIsDirectConnection=false;
-}else{
-return handleConnError(CONN_INVALID_INPUT, 
getMessage(ERR_CONN_UNKPROTO, protocol.c_str()));
-}
-DRILL_MT_LOG(DRILL_LOG(LOG_TRACE) << "Connecting to endpoint: " << 
host << ":" << port << std::endl;)
-std::string serviceHost;
-for (size_t i = 0; i < props->size(); i++) {
-if (props->keyAt(i) == USERPROP_SERVICE_HOST) {
-serviceHost = props->valueAt(i);
-}
+connectionStatus_t ret = CONN_SUCCESS;
+m_pChannelContext = ChannelContextFactory::getChannelContext(type, 
props);
+m_pChannel= ChannelFactory::getChannel(type, m_io_service, connStr);
+ret=m_pChannel->init(m_pChannelContext);
+if(ret!=CONN_SUCCESS){
+handleConnError(m_pChannel->getError());
+return ret;
 }
-if (serviceHost.empty()) {
-props->setProperty(USERPROP_SERVICE_HOST, host);
+ret= m_pChannel->connect();
+if(ret!=CONN_SUCCESS){
+handleConnError(m_pChannel->getError());
+return ret;
 }
-connectionStatus_t ret = this->connect(host.c_str(), port.c_str());
+props->setProperty(USERPROP_SERVICE_HOST, 
m_pChannel->getEndpoint()->getHost());
 return ret;
 }
 
-connectionStatus_t DrillClientImpl::connect(const char* host, const char* 
port){
-using boost::asio::ip::tcp;
-tcp::endpoint endpoint;
-try{
-tcp::resolver resolver(m_io_service);
-tcp::resolver::query query(tcp::v4(), host, port);
-tcp::resolver::iterator iter = resolver.resolve(query);
-tcp::resolver::iterator end;
-while (iter != end){
-endpoint = *iter++;
-

[GitHub] drill pull request #950: DRILL-5431: SSL Support

[parthchandra]

Github user parthchandra commented on a diff in the pull request:

https://github.com/apache/drill/pull/950#discussion_r141730279
  
--- Diff: contrib/native/client/src/clientlib/channel.hpp ---
@@ -0,0 +1,237 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#ifndef CHANNEL_HPP
+#define CHANNEL_HPP
+
+#include "drill/common.hpp"
+#include "drill/drillClient.hpp"
+#include "streamSocket.hpp"
+
+namespace Drill {
+
+class UserProperties;
+
+class ConnectionEndpoint{
+public:
+ConnectionEndpoint(const char* connStr);
+ConnectionEndpoint(const char* host, const char* port);
+~ConnectionEndpoint();
+
+//parse the connection string and set up the host and port to 
connect to
+connectionStatus_t getDrillbitEndpoint();
+
+std::string& getProtocol(){return m_protocol;}
+std::string& getHost(){return m_host;}
+std::string& getPort(){return m_port;}
+DrillClientError* getError(){ return m_pError;};
+
+private:
+void parseConnectString();
+connectionStatus_t validateConnectionString();
--- End diff --

Cause there isn't one. Removed the declaration.


---

[GitHub] drill pull request #950: DRILL-5431: SSL Support

[parthchandra]

Github user parthchandra commented on a diff in the pull request:

https://github.com/apache/drill/pull/950#discussion_r141742427
  
--- Diff: contrib/native/client/src/clientlib/drillClientImpl.cpp ---
@@ -65,108 +66,70 @@ struct ToRpcType: public 
std::unary_functionm_bIsConnected) {
-if(std::strcmp(connStr, m_connectStr.c_str())){ // trying to 
connect to a different address is not allowed if already connected
+connectionStatus_t DrillClientImpl::connect(const char* connStr, 
DrillUserProperties* props){
+if (this->m_bIsConnected || (this->m_pChannelContext!=NULL && 
this->m_pChannel!=NULL)) {
+if(!std::strcmp(connStr, m_connectStr.c_str())){
+// trying to connect to a different address is not allowed if 
already connected
 return handleConnError(CONN_ALREADYCONNECTED, 
getMessage(ERR_CONN_ALREADYCONN));
 }
 return CONN_SUCCESS;
 }
+std::string val;
+channelType_t type = ( props->isPropSet(USERPROP_USESSL) &&
+props->getProp(USERPROP_USESSL, val) =="true") ?
+CHANNEL_TYPE_SSLSTREAM :
+CHANNEL_TYPE_SOCKET;
 
-m_connectStr=connStr;
-Utils::parseConnectStr(connStr, pathToDrill, protocol, hostPortStr);
-if(protocol == "zk"){
-ZookeeperClient zook(pathToDrill);
-std::vector drillbits;
-int err = zook.getAllDrillbits(hostPortStr, drillbits);
-if(!err){
-if (drillbits.empty()){
-return handleConnError(CONN_FAILURE, 
getMessage(ERR_CONN_ZKNODBIT));
-}
-Utils::shuffle(drillbits);
-exec::DrillbitEndpoint endpoint;
-err = zook.getEndPoint(drillbits[drillbits.size() -1], 
endpoint);// get the last one in the list
-if(!err){
-host=boost::lexical_cast(endpoint.address());
-
port=boost::lexical_cast(endpoint.user_port());
-}
-DRILL_MT_LOG(DRILL_LOG(LOG_TRACE) << "Choosing drillbit <" << 
(drillbits.size() - 1)  << ">. Selected " << endpoint.DebugString() << 
std::endl;)
-
-}
-if(err){
-return handleConnError(CONN_ZOOKEEPER_ERROR, 
getMessage(ERR_CONN_ZOOKEEPER, zook.getError().c_str()));
-}
-zook.close();
-m_bIsDirectConnection=true;
-}else if(protocol == "local"){
-boost::lock_guard lock(m_dcMutex);//strtok is not 
reentrant
-char tempStr[MAX_CONNECT_STR+1];
-strncpy(tempStr, hostPortStr.c_str(), MAX_CONNECT_STR); 
tempStr[MAX_CONNECT_STR]=0;
-host=strtok(tempStr, ":");
-port=strtok(NULL, "");
-m_bIsDirectConnection=false;
-}else{
-return handleConnError(CONN_INVALID_INPUT, 
getMessage(ERR_CONN_UNKPROTO, protocol.c_str()));
-}
-DRILL_MT_LOG(DRILL_LOG(LOG_TRACE) << "Connecting to endpoint: " << 
host << ":" << port << std::endl;)
-std::string serviceHost;
-for (size_t i = 0; i < props->size(); i++) {
-if (props->keyAt(i) == USERPROP_SERVICE_HOST) {
-serviceHost = props->valueAt(i);
-}
+connectionStatus_t ret = CONN_SUCCESS;
+m_pChannelContext = ChannelContextFactory::getChannelContext(type, 
props);
+m_pChannel= ChannelFactory::getChannel(type, m_io_service, connStr);
+ret=m_pChannel->init(m_pChannelContext);
+if(ret!=CONN_SUCCESS){
+handleConnError(m_pChannel->getError());
+return ret;
 }
-if (serviceHost.empty()) {
-props->setProperty(USERPROP_SERVICE_HOST, host);
+ret= m_pChannel->connect();
+if(ret!=CONN_SUCCESS){
+handleConnError(m_pChannel->getError());
+return ret;
 }
-connectionStatus_t ret = this->connect(host.c_str(), port.c_str());
+props->setProperty(USERPROP_SERVICE_HOST, 
m_pChannel->getEndpoint()->getHost());
 return ret;
 }
 
-connectionStatus_t DrillClientImpl::connect(const char* host, const char* 
port){
-using boost::asio::ip::tcp;
-tcp::endpoint endpoint;
-try{
-tcp::resolver resolver(m_io_service);
-tcp::resolver::query query(tcp::v4(), host, port);
-tcp::resolver::iterator iter = resolver.resolve(query);
-tcp::resolver::iterator end;
-while (iter != end){
-endpoint = *iter++;
-

[GitHub] drill pull request #950: DRILL-5431: SSL Support

[parthchandra]

Github user parthchandra commented on a diff in the pull request:

https://github.com/apache/drill/pull/950#discussion_r141676935
  
--- Diff: contrib/native/client/src/clientlib/wincert.ipp ---
@@ -0,0 +1,91 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#if defined(IS_SSL_ENABLED)
+
+#include 
+#include 
+
+#if defined _WIN32  || defined _WIN64
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+
+#pragma comment (lib, "crypt32.lib")
+#pragma comment (lib, "cryptui.lib")
+
+#define MY_ENCODING_TYPE  (PKCS_7_ASN_ENCODING | X509_ASN_ENCODING)
+
+inline
+int loadSystemTrustStore(const SSL *ssl) {
+HCERTSTORE hStore;
+PCCERT_CONTEXT pContext = NULL;
+X509 *x509;
+   char* stores[] = {
+   "CA",
+   "MY",
+   "ROOT",
+   "SPC"
+   };
+ 
+SSL_CTX * ctx = SSL_get_SSL_CTX(ssl);
+X509_STORE *store = SSL_CTX_get_cert_store(ctx);
+
+   for(int i=0; i<4; i++){
+hStore = CertOpenSystemStore(NULL, stores[i]);
+
+if (!hStore)
+return 1;
--- End diff --

agreed.


---

[GitHub] drill pull request #950: DRILL-5431: SSL Support

[parthchandra]

Github user parthchandra commented on a diff in the pull request:

https://github.com/apache/drill/pull/950#discussion_r141474850
  
--- Diff: 
exec/java-exec/src/test/java/org/apache/drill/exec/rpc/user/security/TestUserBitSSLServer.java
 ---
@@ -0,0 +1,126 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.drill.exec.rpc.user.security;
+
+import com.typesafe.config.ConfigValueFactory;
+import junit.framework.TestCase;
+import org.apache.drill.BaseTestQuery;
+import org.apache.drill.common.config.DrillConfig;
+import org.apache.drill.common.config.DrillProperties;
+import org.apache.drill.exec.ExecConstants;
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.security.ssl.SSLFactory;
+import org.junit.AfterClass;
+import org.junit.BeforeClass;
+import org.junit.Test;
+
+import java.io.File;
+import java.text.MessageFormat;
+import java.util.Properties;
+
+import static org.apache.drill.exec.ssl.SSLConfig.HADOOP_SSL_CONF_TPL_KEY;
+import static org.junit.Assert.assertEquals;
+
+public class TestUserBitSSLServer extends BaseTestQuery {
+  private static final org.slf4j.Logger logger =
+  org.slf4j.LoggerFactory.getLogger(TestUserBitSSLServer.class);
+
+  private static DrillConfig sslConfig;
+  private static Properties initProps; // initial client properties
+  private static ClassLoader classLoader;
+  private static String ksPath;
+  private static String tsPath;
+  private static String emptyTSPath;
+
+  @BeforeClass
+  public static void setupTest() throws Exception {
+
+classLoader = TestUserBitSSLServer.class.getClassLoader();
+ksPath = new 
File(classLoader.getResource("ssl/keystore.ks").getFile()).getAbsolutePath();
+tsPath = new 
File(classLoader.getResource("ssl/truststore.ks").getFile()).getAbsolutePath();
+emptyTSPath = new 
File(classLoader.getResource("ssl/emptytruststore.ks").getFile()).getAbsolutePath();
+sslConfig = new 
DrillConfig(DrillConfig.create(cloneDefaultTestConfigProperties())
+.withValue(ExecConstants.USER_SSL_ENABLED, 
ConfigValueFactory.fromAnyRef(true))
+.withValue(ExecConstants.SSL_KEYSTORE_TYPE, 
ConfigValueFactory.fromAnyRef("JKS"))
+.withValue(ExecConstants.SSL_KEYSTORE_PATH, 
ConfigValueFactory.fromAnyRef(ksPath))
+.withValue(ExecConstants.SSL_KEYSTORE_PASSWORD, 
ConfigValueFactory.fromAnyRef("drill123"))
+.withValue(ExecConstants.SSL_KEY_PASSWORD, 
ConfigValueFactory.fromAnyRef("drill123"))
+.withValue(ExecConstants.SSL_PROTOCOL, 
ConfigValueFactory.fromAnyRef("TLSv1.2")), false);
+initProps = new Properties();
+initProps.setProperty(DrillProperties.ENABLE_TLS, "true");
+initProps.setProperty(DrillProperties.TRUSTSTORE_PATH, tsPath);
+initProps.setProperty(DrillProperties.TRUSTSTORE_PASSWORD, "drill123");
+initProps.setProperty(DrillProperties.DISABLE_HOST_VERIFICATION, 
"true");
+  }
+
+  @AfterClass
+  public static void cleanTest() throws Exception {
+DrillConfig restoreConfig =
+new 
DrillConfig(DrillConfig.create(cloneDefaultTestConfigProperties()), false);
+updateTestCluster(1, restoreConfig);
+  }
+
+  @Test
+  public void testInvalidKeystorePath() throws Exception {
+DrillConfig testConfig = new DrillConfig(DrillConfig.create(sslConfig)
+.withValue(ExecConstants.SSL_KEYSTORE_PATH, 
ConfigValueFactory.fromAnyRef("/bad/path")),
+false);
+
+// Start an SSL enabled cluster
+boolean failureCaught = false;
+try {
+  updateTestCluster(1, testConfig, initProps);
+} catch (Exception e) {
+  failureCaught = true;
+}
+assertEquals(failureCaught, true);
+  }
+
+  @Test
+  public void testInvalidKeystorePassword() throws Exception {
+DrillConfig testConfig = new DrillConfig(DrillConfig.create(sslConfig)
+

[GitHub] drill pull request #950: DRILL-5431: SSL Support

[parthchandra]

Github user parthchandra commented on a diff in the pull request:

https://github.com/apache/drill/pull/950#discussion_r141670440
  
--- Diff: contrib/native/client/src/include/drill/userProperties.hpp ---
@@ -0,0 +1,73 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+#ifndef USER_PROPERTIES_H
+#define USER_PROPERTIES_H
+
+#include 
+#include "drill/common.hpp"
+
+namespace Drill{
+
+class DECLSPEC_DRILL_CLIENT DrillUserProperties{
+public:
+static const std::map USER_PROPERTIES;
+
+DrillUserProperties(){};
+
+void setProperty( const std::string& propName, const std::string& 
propValue){
--- End diff --

Not a bad idea, but we might have to stay with this for the moment. The 
implementation currently does not own the string that's passed in, leaving it 
to the caller to free the memory when appropriate. To convert always to 
lowercase during the set method, will require me to change the behavior, make a 
copy and then remember to free the memory in case it is overwritten. If we're 
doing that, then we might as well enhance the implementation to get/set type 
specific options (setBoolean, setInt, etc) and that would become so much work 
for not much gain. 


---

[GitHub] drill pull request #950: DRILL-5431: SSL Support

[parthchandra]

Github user parthchandra commented on a diff in the pull request:

https://github.com/apache/drill/pull/950#discussion_r141498641
  
--- Diff: 
exec/rpc/src/main/java/org/apache/drill/exec/rpc/ConnectionMultiListener.java 
---
@@ -0,0 +1,240 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.drill.exec.rpc;
+
+import com.google.protobuf.MessageLite;
+import io.netty.buffer.ByteBuf;
+import io.netty.channel.Channel;
+import io.netty.channel.ChannelFuture;
+import io.netty.util.concurrent.Future;
+import io.netty.util.concurrent.GenericFutureListener;
+import org.apache.drill.common.exceptions.DrillException;
+import org.slf4j.Logger;
+
+import java.net.SocketAddress;
+import java.util.concurrent.TimeUnit;
+
+/**
+ * @param  Client Connection Listener
+ * @param  Outbound handshake message type
+ * @param  Inbound handshake message type
+ * @param  BasicClient type
+ * 
+ * Implements a wrapper class that allows a client connection 
to associate different behaviours after
+ * establishing a connection with the server. The client can 
choose to send an application handshake, or
+ * in the case of SSL, wait for a SSL handshake completion and 
then send an application handshake.
+ */
+
+public class ConnectionMultiListener {
+
+  private static final Logger logger = 
org.slf4j.LoggerFactory.getLogger(ConnectionMultiListener.class);
+
+  private final RpcConnectionHandler connectionListener;
+  private final HS handshakeValue;
+  private final BC parent;
+
+  private ConnectionMultiListener(RpcConnectionHandler 
connectionListener, HS handshakeValue,
+  BC basicClient) {
+assert connectionListener != null;
+assert handshakeValue != null;
+
+this.connectionListener = connectionListener;
+this.handshakeValue = handshakeValue;
+this.parent = basicClient;
+  }
+
+  @SuppressWarnings("unchecked")
+  public static  Builder
+  newBuilder(RpcConnectionHandler connectionListener, HS 
handshakeValue,
+  BC basicClient) {
+return new Builder(connectionListener, handshakeValue, basicClient);
+  }
+
+  public ConnectionHandler connectionHandler = null;
+  public HandshakeSendHandler handshakeSendHandler = null;
+  public SSLConnectionHandler sslConnectionHandler = null;
+
+  /**
+   * Manages connection establishment outcomes.
+   */
+  private class ConnectionHandler implements 
GenericFutureListener {
+
+@Override public void operationComplete(ChannelFuture future) throws 
Exception {
+  boolean isInterrupted = false;
+
+  // We want to wait for at least 120 secs when interrupts occur. 
Establishing a connection fails/succeeds quickly,
+  // So there is no point propagating the interruption as failure 
immediately.
+  long remainingWaitTimeMills = 12;
+  long startTime = System.currentTimeMillis();
+  // logger.debug("Connection operation finished.  Success: {}", 
future.isSuccess());
+  while (true) {
+try {
+  future.get(remainingWaitTimeMills, TimeUnit.MILLISECONDS);
+  if (future.isSuccess()) {
+SocketAddress remote = future.channel().remoteAddress();
+SocketAddress local = future.channel().localAddress();
+parent.setAddresses(remote, local);
+// if SSL is enabled send the handshake after the ssl 
handshake is completed, otherwise send it
+// now
+if(!parent.isSslEnabled()) {
+  // send a handshake on the current thread. This is the only 
time we will send from within the event thread.
+  // We can do this because the connection will not be backed 
up.
+  parent.send(handshakeSendHandler, handshakeValue, true);
+}
+  } else {
+

[GitHub] drill pull request #950: DRILL-5431: SSL Support

[parthchandra]

Github user parthchandra commented on a diff in the pull request:

https://github.com/apache/drill/pull/950#discussion_r141471056
  
--- Diff: 
exec/java-exec/src/test/java/org/apache/drill/exec/TestSSLConfig.java ---
@@ -91,10 +123,35 @@ public void testForTrustStore() throws Exception {
 ConfigBuilder config = new ConfigBuilder();
 config.put(ExecConstants.HTTP_TRUSTSTORE_PATH, "/root");
 config.put(ExecConstants.HTTP_TRUSTSTORE_PASSWORD, "root");
-SSLConfig sslv = new SSLConfig(config.build());
+config.put(ExecConstants.SSL_USE_HADOOP_CONF, false);
+SSLConfig sslv = new SSLConfigBuilder()
+.config(config.build())
+.mode(SSLFactory.Mode.SERVER)
+.initializeSSLContext(false)
+.validateKeyStore(true)
+.build();
 assertEquals(true, sslv.hasTrustStorePath());
 assertEquals(true,sslv.hasTrustStorePassword());
 assertEquals("/root",sslv.getTrustStorePath());
 assertEquals("root",sslv.getTrustStorePassword());
   }
-}
\ No newline at end of file
+
+  @Test
+  public void testInvalidHadoopKeystore() throws Exception {
+Configuration hadoopConfig = new Configuration();
+String hadoopSSLFileProp = MessageFormat
+.format(HADOOP_SSL_CONF_TPL_KEY, 
SSLFactory.Mode.SERVER.toString().toLowerCase());
+hadoopConfig.set(hadoopSSLFileProp, "ssl-server-invalid.xml");
+ConfigBuilder config = new ConfigBuilder();
+config.put(ExecConstants.SSL_USE_HADOOP_CONF, true);
+SSLConfig sslv = new SSLConfigBuilder()
+.config(config.build())
+.mode(SSLFactory.Mode.SERVER)
+.initializeSSLContext(false)
+.validateKeyStore(true)
+.hadoopConfig(hadoopConfig)
+.build();
+assertEquals("FAIL", sslv.getKeyStorePassword());
--- End diff --

Yes. Buggy test. Fixed it. Previous test was passing because the SSL was 
not enabled in the config. This caused the validation of keystore to be skipped.


---

[GitHub] drill pull request #950: DRILL-5431: SSL Support

[sohami]

Github user sohami commented on a diff in the pull request:

https://github.com/apache/drill/pull/950#discussion_r140914589
  
--- Diff: contrib/native/client/src/clientlib/zkCluster.cpp ---
@@ -0,0 +1,175 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#include "drill/common.hpp"
+#include 
+#ifdef _WIN32
+#include 
+#else
+#include 
+#endif
+#include "drill/drillConfig.hpp"
+#include "drill/drillClient.hpp"
+#include "errmsgs.hpp"
+#include "logger.hpp"
+#include "zkCluster.hpp"
+
+namespace Drill{
+
+char ZkCluster::s_drillRoot[]="/drill/";
+char ZkCluster::s_defaultCluster[]="drillbits1";
+
+ZkCluster::ZkCluster(){
+m_pDrillbits=new String_vector;
+srand (time(NULL));
+m_bConnecting=true;
+memset(_id, 0, sizeof(m_id));
+}
+
+ZkCluster::~ZkCluster(){
+delete m_pDrillbits;
+}
+
+ZooLogLevel ZkCluster::getZkLogLevel(){
+//typedef enum {ZOO_LOG_LEVEL_ERROR=1,
+//ZOO_LOG_LEVEL_WARN=2,
+//ZOO_LOG_LEVEL_INFO=3,
+//ZOO_LOG_LEVEL_DEBUG=4
+//} ZooLogLevel;
+switch(DrillClientConfig::getLogLevel()){
+case LOG_TRACE:
+case LOG_DEBUG:
+return ZOO_LOG_LEVEL_DEBUG;
+case LOG_INFO:
+return ZOO_LOG_LEVEL_INFO;
+case LOG_WARNING:
+return ZOO_LOG_LEVEL_WARN;
+case LOG_ERROR:
+case LOG_FATAL:
+default:
+return ZOO_LOG_LEVEL_ERROR;
+}
+return ZOO_LOG_LEVEL_ERROR;
+}
+
+int ZkCluster::connectToZookeeper(const char* connectStr, const char* 
pathToDrill){
+uint32_t waitTime=3; // 10 seconds
--- End diff --

10 seconds ?
As discussed in person please remove these files as per your plan: 
`zkCluster.cpp and zkCluster.hpp`


---

[GitHub] drill pull request #950: DRILL-5431: SSL Support

[sohami]

Github user sohami commented on a diff in the pull request:

https://github.com/apache/drill/pull/950#discussion_r140893808
  
--- Diff: contrib/native/client/src/include/drill/userProperties.hpp ---
@@ -0,0 +1,73 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+#ifndef USER_PROPERTIES_H
+#define USER_PROPERTIES_H
+
+#include 
+#include "drill/common.hpp"
+
+namespace Drill{
+
+class DECLSPEC_DRILL_CLIENT DrillUserProperties{
+public:
+static const std::map USER_PROPERTIES;
+
+DrillUserProperties(){};
+
+void setProperty( const std::string& propName, const std::string& 
propValue){
+std::pair< std::string, std::string> in = make_pair(propName, 
propValue);
+m_properties.insert(in);
+}
+
+size_t size() const { return m_properties.size(); }
+
+const bool  isPropSet(const std::string& key) const{
+bool isSet=true;
+std::map::const_iterator 
f=m_properties.find(key);
+if(f==m_properties.end()){
+isSet=false;
+}
+return isSet;
+}
+
+const std::string&  getProp(const std::string& key, std::string& 
value) const{
--- End diff --

this method is little confusing since it's returning value both in input 
parameter and as a return value. I think we should choose either of it NOT both.


---

[GitHub] drill pull request #950: DRILL-5431: SSL Support

[sohami]

Github user sohami commented on a diff in the pull request:

https://github.com/apache/drill/pull/950#discussion_r141215252
  
--- Diff: contrib/native/client/src/clientlib/channel.cpp ---
@@ -0,0 +1,452 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#include 
+#include 
+
+#include "drill/drillConfig.hpp"
+#include "drill/drillError.hpp"
+#include "drill/userProperties.hpp"
+#include "channel.hpp"
+#include "errmsgs.hpp"
+#include "logger.hpp"
+#include "utils.hpp"
+#include "zookeeperClient.hpp"
+
+#include "GeneralRPC.pb.h"
+
+namespace Drill{
+
+ConnectionEndpoint::ConnectionEndpoint(const char* connStr){
+m_connectString=connStr;
+m_pError=NULL;
+}
+
+ConnectionEndpoint::ConnectionEndpoint(const char* host, const char* port){
+m_host=host;
+m_port=port;
+m_protocol="drillbit"; // direct connection
+m_pError=NULL;
+}
+
+ConnectionEndpoint::~ConnectionEndpoint(){
+if(m_pError!=NULL){
+delete m_pError; m_pError=NULL;
+}
+}
+
+connectionStatus_t ConnectionEndpoint::getDrillbitEndpoint(){
+connectionStatus_t ret=CONN_SUCCESS;
+if(!m_connectString.empty()){
+parseConnectString();
+if(m_protocol.empty()){
+return handleError(CONN_INVALID_INPUT, 
getMessage(ERR_CONN_UNKPROTO, ""));
+}
+if(isZookeeperConnection()){
+if((ret=getDrillbitEndpointFromZk())!=CONN_SUCCESS){
+return ret;
+}
+}else if(!this->isDirectConnection()){
+return handleError(CONN_INVALID_INPUT, 
getMessage(ERR_CONN_UNKPROTO, this->getProtocol().c_str()));
+}
+}else{
+if(m_host.empty() || m_port.empty()){
+return handleError(CONN_INVALID_INPUT, 
getMessage(ERR_CONN_NOCONNSTR));
+}
+}
+return ret;
+}
+
+void ConnectionEndpoint::parseConnectString(){
+boost::regex connStrExpr("(.*)=(.*):([0-9]+)(?:/(.+))?");
--- End diff --

Haven't reviewed this change based on regex change discussed in person.


---

[GitHub] drill pull request #950: DRILL-5431: SSL Support

[sohami]

Github user sohami commented on a diff in the pull request:

https://github.com/apache/drill/pull/950#discussion_r140621499
  
--- Diff: 
exec/rpc/src/main/java/org/apache/drill/exec/rpc/ConnectionMultiListener.java 
---
@@ -0,0 +1,240 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.drill.exec.rpc;
+
+import com.google.protobuf.MessageLite;
+import io.netty.buffer.ByteBuf;
+import io.netty.channel.Channel;
+import io.netty.channel.ChannelFuture;
+import io.netty.util.concurrent.Future;
+import io.netty.util.concurrent.GenericFutureListener;
+import org.apache.drill.common.exceptions.DrillException;
+import org.slf4j.Logger;
+
+import java.net.SocketAddress;
+import java.util.concurrent.TimeUnit;
+
+/**
+ * @param  Client Connection Listener
+ * @param  Outbound handshake message type
+ * @param  Inbound handshake message type
+ * @param  BasicClient type
+ * 
+ * Implements a wrapper class that allows a client connection 
to associate different behaviours after
+ * establishing a connection with the server. The client can 
choose to send an application handshake, or
+ * in the case of SSL, wait for a SSL handshake completion and 
then send an application handshake.
+ */
+
+public class ConnectionMultiListener {
--- End diff --

I am seeing lots of unchecked warning for this file. Please fix those.


---

[GitHub] drill pull request #950: DRILL-5431: SSL Support

[sohami]

Github user sohami commented on a diff in the pull request:

https://github.com/apache/drill/pull/950#discussion_r140621635
  
--- Diff: exec/rpc/src/main/java/org/apache/drill/exec/rpc/BasicClient.java 
---
@@ -179,6 +203,13 @@ void send(RpcOutcomeListener listener, T 
rpcType, SEND protobufBody,
 return super.send(connection, rpcType, protobufBody, clazz, 
dataBodies);
   }
 
+  public  void send(
+  RpcOutcomeListener listener, SEND protobufBody, boolean 
allowInEventLoop,
+  ByteBuf... dataBodies) {
+super.send(listener, connection, handshakeType, protobufBody, 
(Class) responseClass,
+allowInEventLoop, dataBodies);
--- End diff --

Seeing unchecked warning for this function. Please resolve this.


---

[GitHub] drill pull request #950: DRILL-5431: SSL Support

[sohami]

Github user sohami commented on a diff in the pull request:

https://github.com/apache/drill/pull/950#discussion_r141215109
  
--- Diff: contrib/native/client/src/clientlib/channel.cpp ---
@@ -0,0 +1,452 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#include 
+#include 
+
+#include "drill/drillConfig.hpp"
+#include "drill/drillError.hpp"
+#include "drill/userProperties.hpp"
+#include "channel.hpp"
+#include "errmsgs.hpp"
+#include "logger.hpp"
+#include "utils.hpp"
+#include "zookeeperClient.hpp"
+
+#include "GeneralRPC.pb.h"
+
+namespace Drill{
+
+ConnectionEndpoint::ConnectionEndpoint(const char* connStr){
+m_connectString=connStr;
+m_pError=NULL;
+}
+
+ConnectionEndpoint::ConnectionEndpoint(const char* host, const char* port){
+m_host=host;
+m_port=port;
+m_protocol="drillbit"; // direct connection
+m_pError=NULL;
+}
+
+ConnectionEndpoint::~ConnectionEndpoint(){
+if(m_pError!=NULL){
+delete m_pError; m_pError=NULL;
+}
+}
+
+connectionStatus_t ConnectionEndpoint::getDrillbitEndpoint(){
+connectionStatus_t ret=CONN_SUCCESS;
+if(!m_connectString.empty()){
+parseConnectString();
+if(m_protocol.empty()){
+return handleError(CONN_INVALID_INPUT, 
getMessage(ERR_CONN_UNKPROTO, ""));
+}
+if(isZookeeperConnection()){
+if((ret=getDrillbitEndpointFromZk())!=CONN_SUCCESS){
+return ret;
+}
+}else if(!this->isDirectConnection()){
+return handleError(CONN_INVALID_INPUT, 
getMessage(ERR_CONN_UNKPROTO, this->getProtocol().c_str()));
+}
+}else{
+if(m_host.empty() || m_port.empty()){
+return handleError(CONN_INVALID_INPUT, 
getMessage(ERR_CONN_NOCONNSTR));
+}
+}
+return ret;
+}
+
+void ConnectionEndpoint::parseConnectString(){
+boost::regex connStrExpr("(.*)=(.*):([0-9]+)(?:/(.+))?");
+boost::cmatch matched;
+
+if(boost::regex_match(m_connectString.c_str(), matched, connStrExpr)){
+m_protocol.assign(matched[1].first, matched[1].second);
+std::string host, port;
+host.assign(matched[2].first, matched[2].second);
+port.assign(matched[3].first, matched[3].second);
+if(isDirectConnection()){
+// if the connection is to a zookeeper, 
+// we will get the host and the port only after connecting to 
the Zookeeper
+m_host=host;
+m_port=port;
+}
+m_hostPortStr=host+std::string(":")+port;
+std::string pathToDrill;
+if(matched.size()==5){
+pathToDrill.assign(matched[4].first, matched[4].second);
+if(!pathToDrill.empty()){
+m_pathToDrill=std::string("/")+pathToDrill;
+}
+}
+DRILL_MT_LOG(DRILL_LOG(LOG_DEBUG) 
+<< "Conn str: "<< m_connectString 
+<< ";  protocol: " << m_protocol 
+<< ";  host: " << host 
+<< "; port: " << port 
+<< ";  path to drill: " << m_pathToDrill 
+<< std::endl;)
+} else {
+DRILL_MT_LOG(DRILL_LOG(LOG_DEBUG) << "Invalid connect string. 
Regexp did not match" << std::endl;)
+}
+
+return;
+}
+
+bool ConnectionEndpoint::isDirectConnection(){
+assert(!m_protocol.empty());
+return (!strcmp(m_protocol.c_str(), "local") || 
!strcmp(m_protocol.c_str(), "drillbit"));
+}
+
+bool ConnectionEndpoint::isZookeeperConnection(){
+assert(!m_protocol.empty());
+return (!strcmp(m_protocol.c_str(), "zk"));
+}
+
+connectionStatus_t ConnectionEndpoint::getDrillbitEndpointFromZk(){
+ZookeeperClient zook(m_pathToDrill);
+

[GitHub] drill pull request #950: DRILL-5431: SSL Support

[sohami]

Github user sohami commented on a diff in the pull request:

https://github.com/apache/drill/pull/950#discussion_r140623048
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/UserClient.java ---
@@ -102,19 +115,78 @@
   // these are used for authentication
   private volatile List serverAuthMechanisms = null;
   private volatile boolean authComplete = true;
+  private SSLConfig sslConfig;
+  private Channel sslChannel;
--- End diff --

I don't think you have to store the sslChannel reference explicitly here to 
make sure it's closed. The connection wrapper like AbstractRemoteConnection 
will already have reference to channel object and will take care of closing it.
Also that path is taking care of channel close both in graceful (explicitly 
close being called on client) and failure scenario (in which case Netty 
channelClosedHandler will be invoked).

Same for server side.


---

[GitHub] drill pull request #950: DRILL-5431: SSL Support

[sohami]

Github user sohami commented on a diff in the pull request:

https://github.com/apache/drill/pull/950#discussion_r141245539
  
--- Diff: contrib/native/client/src/clientlib/drillClientImpl.cpp ---
@@ -65,108 +66,70 @@ struct ToRpcType: public 
std::unary_functionm_bIsConnected) {
-if(std::strcmp(connStr, m_connectStr.c_str())){ // trying to 
connect to a different address is not allowed if already connected
+connectionStatus_t DrillClientImpl::connect(const char* connStr, 
DrillUserProperties* props){
+if (this->m_bIsConnected || (this->m_pChannelContext!=NULL && 
this->m_pChannel!=NULL)) {
+if(!std::strcmp(connStr, m_connectStr.c_str())){
+// trying to connect to a different address is not allowed if 
already connected
 return handleConnError(CONN_ALREADYCONNECTED, 
getMessage(ERR_CONN_ALREADYCONN));
 }
 return CONN_SUCCESS;
 }
+std::string val;
+channelType_t type = ( props->isPropSet(USERPROP_USESSL) &&
+props->getProp(USERPROP_USESSL, val) =="true") ?
+CHANNEL_TYPE_SSLSTREAM :
+CHANNEL_TYPE_SOCKET;
 
-m_connectStr=connStr;
-Utils::parseConnectStr(connStr, pathToDrill, protocol, hostPortStr);
-if(protocol == "zk"){
-ZookeeperClient zook(pathToDrill);
-std::vector drillbits;
-int err = zook.getAllDrillbits(hostPortStr, drillbits);
-if(!err){
-if (drillbits.empty()){
-return handleConnError(CONN_FAILURE, 
getMessage(ERR_CONN_ZKNODBIT));
-}
-Utils::shuffle(drillbits);
-exec::DrillbitEndpoint endpoint;
-err = zook.getEndPoint(drillbits[drillbits.size() -1], 
endpoint);// get the last one in the list
-if(!err){
-host=boost::lexical_cast(endpoint.address());
-
port=boost::lexical_cast(endpoint.user_port());
-}
-DRILL_MT_LOG(DRILL_LOG(LOG_TRACE) << "Choosing drillbit <" << 
(drillbits.size() - 1)  << ">. Selected " << endpoint.DebugString() << 
std::endl;)
-
-}
-if(err){
-return handleConnError(CONN_ZOOKEEPER_ERROR, 
getMessage(ERR_CONN_ZOOKEEPER, zook.getError().c_str()));
-}
-zook.close();
-m_bIsDirectConnection=true;
-}else if(protocol == "local"){
-boost::lock_guard lock(m_dcMutex);//strtok is not 
reentrant
-char tempStr[MAX_CONNECT_STR+1];
-strncpy(tempStr, hostPortStr.c_str(), MAX_CONNECT_STR); 
tempStr[MAX_CONNECT_STR]=0;
-host=strtok(tempStr, ":");
-port=strtok(NULL, "");
-m_bIsDirectConnection=false;
-}else{
-return handleConnError(CONN_INVALID_INPUT, 
getMessage(ERR_CONN_UNKPROTO, protocol.c_str()));
-}
-DRILL_MT_LOG(DRILL_LOG(LOG_TRACE) << "Connecting to endpoint: " << 
host << ":" << port << std::endl;)
-std::string serviceHost;
-for (size_t i = 0; i < props->size(); i++) {
-if (props->keyAt(i) == USERPROP_SERVICE_HOST) {
-serviceHost = props->valueAt(i);
-}
+connectionStatus_t ret = CONN_SUCCESS;
+m_pChannelContext = ChannelContextFactory::getChannelContext(type, 
props);
+m_pChannel= ChannelFactory::getChannel(type, m_io_service, connStr);
+ret=m_pChannel->init(m_pChannelContext);
+if(ret!=CONN_SUCCESS){
+handleConnError(m_pChannel->getError());
+return ret;
 }
-if (serviceHost.empty()) {
-props->setProperty(USERPROP_SERVICE_HOST, host);
+ret= m_pChannel->connect();
+if(ret!=CONN_SUCCESS){
+handleConnError(m_pChannel->getError());
+return ret;
 }
-connectionStatus_t ret = this->connect(host.c_str(), port.c_str());
+props->setProperty(USERPROP_SERVICE_HOST, 
m_pChannel->getEndpoint()->getHost());
 return ret;
 }
 
-connectionStatus_t DrillClientImpl::connect(const char* host, const char* 
port){
-using boost::asio::ip::tcp;
-tcp::endpoint endpoint;
-try{
-tcp::resolver resolver(m_io_service);
-tcp::resolver::query query(tcp::v4(), host, port);
-tcp::resolver::iterator iter = resolver.resolve(query);
-tcp::resolver::iterator end;
-while (iter != end){
-endpoint = *iter++;
-

[GitHub] drill pull request #950: DRILL-5431: SSL Support

[sohami]

Github user sohami commented on a diff in the pull request:

https://github.com/apache/drill/pull/950#discussion_r140973654
  
--- Diff: contrib/native/client/src/clientlib/streamSocket.hpp ---
@@ -0,0 +1,212 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+
+#ifndef STREAMSOCKET_HPP
+#define STREAMSOCKET_HPP
+
+#include "drill/common.hpp"
+#include "env.h"
+#include "wincert.ipp"
+
+#include 
+#include 
+
+namespace Drill {
+
+typedef boost::asio::ip::tcp::socket::lowest_layer_type streamSocket_t;
+typedef boost::asio::ssl::stream 
sslTCPSocket_t;
+typedef boost::asio::ip::tcp::socket basicTCPSocket_t;
+
+
+// Some helper typedefs to define the highly templatized boost::asio 
methods
+typedef boost::asio::const_buffers_1 ConstBufferSequence; 
+typedef boost::asio::mutable_buffers_1 MutableBufferSequence;
+
+// ReadHandlers have different possible signatures.
+//
+// As a standard C-type callback
+//typedef void (*ReadHandler)(const boost::system::error_code& ec, 
std::size_t bytes_transferred);
+//
+// Or as a C++ functor
+//struct ReadHandler {
+//virtual void operator()(
+//const boost::system::error_code& ec,
+//std::size_t bytes_transferred) = 0;
+//};
+//
+// We need a different signature though, since we need to pass in a member 
function of a drill client 
+// class (which is C++), as a functor generated by boost::bind as a 
ReadHandler
+// 
+typedef boost::function ReadHandler;
+
+class AsioStreamSocket{
+public:
+virtual ~AsioStreamSocket(){};
+virtual streamSocket_t& getInnerSocket() = 0;
+
+virtual std::size_t writeSome(
+const ConstBufferSequence& buffers,
+boost::system::error_code & ec) = 0;
+
+virtual std::size_t readSome(
+const MutableBufferSequence& buffers,
+boost::system::error_code & ec) = 0;
+
+//
+// boost::asio::async_read has the signature 
+// template<
+// typename AsyncReadStream,
+// typename MutableBufferSequence,
+// typename ReadHandler>
+// void-or-deduced async_read(
+// AsyncReadStream & s,
+// const MutableBufferSequence & buffers,
+// ReadHandler handler);
+//
+// For our use case, the derived class will have an instance of a 
concrete type for AsyncReadStream which 
+// will implement the requirements for the AsyncReadStream type. 
We need not pass that in as a parameter 
+// since the class already has the value
+// The method is templatized since the ReadHandler type is 
dependent on the class implementing the read 
+// handler (basically the class using the asio stream)
+//
+virtual void asyncRead( const MutableBufferSequence & buffers, 
ReadHandler handler) = 0;
+
+// call the underlying protocol's handshake method.
+// if the useSystemConfig flag is true, then use properties read
+// from the underlying operating system
+virtual void protocolHandshake(bool useSystemConfig) = 0;
+virtual void protocolClose() = 0;
+};
+
+class Socket: 
+public AsioStreamSocket, 
+public basicTCPSocket_t{
+
+public:
+Socket(boost::asio::io_service& ioService) : 
basicTCPSocket_t(ioService) {
+}
+
+~Socket(){
+boost::system::error_code ignorederr;
+this->shutdown(boost::asio::ip::tcp::socket::shutdown_both, 
ignorederr);
+this->close();
+};
+
+basicTCPSocket_t& getSocketStream(){ return *this;}
+
+streamSocket_t& getInnerSocket(){ return this->lowest_layer();}
+
+std::size_t writeSome(
+  

[GitHub] drill pull request #950: DRILL-5431: SSL Support

[sohami]

Github user sohami commented on a diff in the pull request:

https://github.com/apache/drill/pull/950#discussion_r140971420
  
--- Diff: contrib/native/client/src/clientlib/streamSocket.hpp ---
@@ -0,0 +1,212 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+
+#ifndef STREAMSOCKET_HPP
+#define STREAMSOCKET_HPP
+
+#include "drill/common.hpp"
+#include "env.h"
+#include "wincert.ipp"
+
+#include 
+#include 
+
+namespace Drill {
+
+typedef boost::asio::ip::tcp::socket::lowest_layer_type streamSocket_t;
+typedef boost::asio::ssl::stream 
sslTCPSocket_t;
+typedef boost::asio::ip::tcp::socket basicTCPSocket_t;
+
+
+// Some helper typedefs to define the highly templatized boost::asio 
methods
+typedef boost::asio::const_buffers_1 ConstBufferSequence; 
+typedef boost::asio::mutable_buffers_1 MutableBufferSequence;
+
+// ReadHandlers have different possible signatures.
+//
+// As a standard C-type callback
+//typedef void (*ReadHandler)(const boost::system::error_code& ec, 
std::size_t bytes_transferred);
+//
+// Or as a C++ functor
+//struct ReadHandler {
+//virtual void operator()(
+//const boost::system::error_code& ec,
+//std::size_t bytes_transferred) = 0;
+//};
+//
+// We need a different signature though, since we need to pass in a member 
function of a drill client 
+// class (which is C++), as a functor generated by boost::bind as a 
ReadHandler
+// 
+typedef boost::function ReadHandler;
+
+class AsioStreamSocket{
+public:
+virtual ~AsioStreamSocket(){};
+virtual streamSocket_t& getInnerSocket() = 0;
+
+virtual std::size_t writeSome(
+const ConstBufferSequence& buffers,
+boost::system::error_code & ec) = 0;
+
+virtual std::size_t readSome(
+const MutableBufferSequence& buffers,
+boost::system::error_code & ec) = 0;
+
+//
+// boost::asio::async_read has the signature 
+// template<
+// typename AsyncReadStream,
+// typename MutableBufferSequence,
+// typename ReadHandler>
+// void-or-deduced async_read(
+// AsyncReadStream & s,
+// const MutableBufferSequence & buffers,
+// ReadHandler handler);
+//
+// For our use case, the derived class will have an instance of a 
concrete type for AsyncReadStream which 
+// will implement the requirements for the AsyncReadStream type. 
We need not pass that in as a parameter 
+// since the class already has the value
+// The method is templatized since the ReadHandler type is 
dependent on the class implementing the read 
+// handler (basically the class using the asio stream)
+//
+virtual void asyncRead( const MutableBufferSequence & buffers, 
ReadHandler handler) = 0;
+
+// call the underlying protocol's handshake method.
+// if the useSystemConfig flag is true, then use properties read
+// from the underlying operating system
+virtual void protocolHandshake(bool useSystemConfig) = 0;
+virtual void protocolClose() = 0;
+};
+
+class Socket: 
+public AsioStreamSocket, 
+public basicTCPSocket_t{
+
+public:
+Socket(boost::asio::io_service& ioService) : 
basicTCPSocket_t(ioService) {
+}
+
+~Socket(){
+boost::system::error_code ignorederr;
+this->shutdown(boost::asio::ip::tcp::socket::shutdown_both, 
ignorederr);
+this->close();
+};
+
+basicTCPSocket_t& getSocketStream(){ return *this;}
+
+streamSocket_t& getInnerSocket(){ return this->lowest_layer();}
+
+std::size_t writeSome(
+  

[GitHub] drill pull request #950: DRILL-5431: SSL Support

[sohami]

Github user sohami commented on a diff in the pull request:

https://github.com/apache/drill/pull/950#discussion_r140970043
  
--- Diff: contrib/native/client/src/clientlib/wincert.ipp ---
@@ -0,0 +1,91 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#if defined(IS_SSL_ENABLED)
+
+#include 
+#include 
+
+#if defined _WIN32  || defined _WIN64
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+
+#pragma comment (lib, "crypt32.lib")
+#pragma comment (lib, "cryptui.lib")
+
+#define MY_ENCODING_TYPE  (PKCS_7_ASN_ENCODING | X509_ASN_ENCODING)
+
+inline
+int loadSystemTrustStore(const SSL *ssl) {
--- End diff --

Can we update this method to take a second parameter like `string& 
store_name` which we can set in case of error while opening store. so the 
caller in case of error can also print the store name which caused the error.


---

[GitHub] drill pull request #950: DRILL-5431: SSL Support

[sohami]

Github user sohami commented on a diff in the pull request:

https://github.com/apache/drill/pull/950#discussion_r140621394
  
--- Diff: 
exec/rpc/src/main/java/org/apache/drill/exec/rpc/ConnectionMultiListener.java 
---
@@ -0,0 +1,240 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.drill.exec.rpc;
+
+import com.google.protobuf.MessageLite;
+import io.netty.buffer.ByteBuf;
+import io.netty.channel.Channel;
+import io.netty.channel.ChannelFuture;
+import io.netty.util.concurrent.Future;
+import io.netty.util.concurrent.GenericFutureListener;
+import org.apache.drill.common.exceptions.DrillException;
+import org.slf4j.Logger;
+
+import java.net.SocketAddress;
+import java.util.concurrent.TimeUnit;
+
+/**
+ * @param  Client Connection Listener
+ * @param  Outbound handshake message type
+ * @param  Inbound handshake message type
+ * @param  BasicClient type
+ * 
+ * Implements a wrapper class that allows a client connection 
to associate different behaviours after
+ * establishing a connection with the server. The client can 
choose to send an application handshake, or
+ * in the case of SSL, wait for a SSL handshake completion and 
then send an application handshake.
+ */
+
+public class ConnectionMultiListener {
+
+  private static final Logger logger = 
org.slf4j.LoggerFactory.getLogger(ConnectionMultiListener.class);
+
+  private final RpcConnectionHandler connectionListener;
+  private final HS handshakeValue;
+  private final BC parent;
+
+  private ConnectionMultiListener(RpcConnectionHandler 
connectionListener, HS handshakeValue,
+  BC basicClient) {
+assert connectionListener != null;
+assert handshakeValue != null;
+
+this.connectionListener = connectionListener;
+this.handshakeValue = handshakeValue;
+this.parent = basicClient;
+  }
+
+  @SuppressWarnings("unchecked")
+  public static  Builder
+  newBuilder(RpcConnectionHandler connectionListener, HS 
handshakeValue,
+  BC basicClient) {
+return new Builder(connectionListener, handshakeValue, basicClient);
+  }
+
+  public ConnectionHandler connectionHandler = null;
+  public HandshakeSendHandler handshakeSendHandler = null;
+  public SSLConnectionHandler sslConnectionHandler = null;
+
+  /**
+   * Manages connection establishment outcomes.
+   */
+  private class ConnectionHandler implements 
GenericFutureListener {
+
+@Override public void operationComplete(ChannelFuture future) throws 
Exception {
+  boolean isInterrupted = false;
+
+  // We want to wait for at least 120 secs when interrupts occur. 
Establishing a connection fails/succeeds quickly,
+  // So there is no point propagating the interruption as failure 
immediately.
+  long remainingWaitTimeMills = 12;
+  long startTime = System.currentTimeMillis();
+  // logger.debug("Connection operation finished.  Success: {}", 
future.isSuccess());
+  while (true) {
+try {
+  future.get(remainingWaitTimeMills, TimeUnit.MILLISECONDS);
+  if (future.isSuccess()) {
+SocketAddress remote = future.channel().remoteAddress();
+SocketAddress local = future.channel().localAddress();
+parent.setAddresses(remote, local);
+// if SSL is enabled send the handshake after the ssl 
handshake is completed, otherwise send it
+// now
+if(!parent.isSslEnabled()) {
+  // send a handshake on the current thread. This is the only 
time we will send from within the event thread.
+  // We can do this because the connection will not be backed 
up.
+  parent.send(handshakeSendHandler, handshakeValue, true);
+}
+  } else {
+

[GitHub] drill pull request #950: DRILL-5431: SSL Support

[sohami]

Github user sohami commented on a diff in the pull request:

https://github.com/apache/drill/pull/950#discussion_r140587424
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/ssl/SSLConfig.java ---
@@ -0,0 +1,325 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.drill.exec.ssl;
+
+import com.google.common.base.Preconditions;
+import io.netty.handler.ssl.SslContext;
+import io.netty.handler.ssl.SslProvider;
+import io.netty.handler.ssl.util.InsecureTrustManagerFactory;
+import org.apache.drill.common.config.DrillConfig;
+import org.apache.drill.common.exceptions.DrillException;
+import org.apache.drill.exec.ExecConstants;
+import org.apache.drill.exec.memory.BufferAllocator;
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.security.ssl.SSLFactory;
+
+import javax.net.ssl.KeyManagerFactory;
+import javax.net.ssl.SSLContext;
+import javax.net.ssl.SSLEngine;
+import javax.net.ssl.TrustManagerFactory;
+import java.io.FileInputStream;
+import java.io.InputStream;
+import java.security.KeyStore;
+import java.text.MessageFormat;
+
+public abstract class SSLConfig {
+
+  private static final org.slf4j.Logger logger = 
org.slf4j.LoggerFactory.getLogger(SSLConfig.class);
+
+  public static final String DEFAULT_SSL_PROVIDER = "JDK"; // JDK or 
OPENSSL
+  public static final String DEFAULT_SSL_PROTOCOL = "TLSv1.2";
+  public static final int DEFAULT_SSL_HANDSHAKE_TIMEOUT_MS = 10 * 1000; // 
10 seconds
+
+  protected final boolean httpsEnabled;
+  protected final DrillConfig config;
+  protected final Configuration hadoopConfig;
+
+  // Either the Netty SSL context or the JDK SSL context will be 
initialized
+  // The JDK SSL context is use iff the useSystemTrustStore setting is 
enabled.
+  protected SslContext nettySslContext;
+  protected SSLContext jdkSSlContext;
+
+  private static final boolean isWindows = 
System.getProperty("os.name").toLowerCase().indexOf("win") >= 0;
+  private static final boolean isMacOs = 
System.getProperty("os.name").toLowerCase().indexOf("mac") >= 0;
+
+  public static final String HADOOP_SSL_CONF_TPL_KEY = 
"hadoop.ssl.{0}.conf";
+  public static final String HADOOP_SSL_KEYSTORE_LOCATION_TPL_KEY = 
"ssl.{0}.keystore.location";
+  public static final String HADOOP_SSL_KEYSTORE_PASSWORD_TPL_KEY = 
"ssl.{0}.keystore.password";
+  public static final String HADOOP_SSL_KEYSTORE_TYPE_TPL_KEY = 
"ssl.{0}.keystore.type";
+  public static final String HADOOP_SSL_KEYSTORE_KEYPASSWORD_TPL_KEY =
+  "ssl.{0}.keystore.keypassword";
+  public static final String HADOOP_SSL_TRUSTSTORE_LOCATION_TPL_KEY = 
"ssl.{0}.truststore.location";
+  public static final String HADOOP_SSL_TRUSTSTORE_PASSWORD_TPL_KEY = 
"ssl.{0}.truststore.password";
+  public static final String HADOOP_SSL_TRUSTSTORE_TYPE_TPL_KEY = 
"ssl.{0}.truststore.type";
+
+  public SSLConfig(DrillConfig config, Configuration hadoopConfig, 
SSLFactory.Mode mode)
+  throws DrillException {
+
+this.config = config;
+httpsEnabled =
+config.hasPath(ExecConstants.HTTP_ENABLE_SSL) && 
config.getBoolean(ExecConstants.HTTP_ENABLE_SSL);
+// For testing we will mock up a hadoop configuration, however for 
regular use, we find the actual hadoop config.
+boolean enableHadoopConfig = 
config.getBoolean(ExecConstants.SSL_USE_HADOOP_CONF);
+if (enableHadoopConfig && this instanceof SSLConfigServer) {
+  if (hadoopConfig == null) {
+this.hadoopConfig = new Configuration(); // get hadoop 
configuration
+  } else {
+this.hadoopConfig = hadoopConfig;
+  }
+  String hadoopSSLConfigFile =
+  
this.hadoopConfig.get(resolveHadoopPropertyName(HADOOP_SSL_CONF_TPL_KEY, mode));
+  logger.debug("Using Hadoop configuration for SSL");
+  logger.debug("Hadoop SSL configuration file: {}", 
hadoopSSLConfigFile);
+